MutatingAdmissionWebhook for relocating image references #37

glyn · 2019-09-25T14:37:19Z

Based on https://github.com/k14s/k8s-resource-protection, thanks to
@cppforlife.

Fixes https://github.com/pivotal/image-relocation/issues/36

codecov · 2019-09-25T14:39:22Z

Codecov Report

Merging #37 into master will increase coverage by 5.66%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
+ Coverage   68.12%   73.79%   +5.66%     
==========================================
  Files           8        9       +1     
  Lines         342      416      +74     
==========================================
+ Hits          233      307      +74     
  Misses         95       95              
  Partials       14       14

Impacted Files	Coverage Δ
pkg/multimap/composite.go	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28a4e87...66043b0. Read the comment docs.

glyn · 2019-09-30T13:56:14Z

TODO: add namespaceSelector so that the webhook doesn't apply to itself. Without this, the following can occur if the webhook is created before the underlying service:

Warning  FailedCreate  36s (x15 over 118s)  replicaset-controller  Error creating: Internal error occurred: failed calling webhook "image-relocating-webhook.image-relocation.pivotal.io": Post https://ir-webhook.image-relocation.svc:443/image-relocation?timeout=30s: no endpoints available for service "ir-webhook"

(Thanks @scothis who spotted this.)

glyn · 2019-10-02T11:20:01Z

TODO: add namespaceSelector so that the webhook doesn't apply to itself. Without this, the following can occur if the webhook is created before the underlying service:
Warning  FailedCreate  36s (x15 over 118s)  replicaset-controller  Error creating: Internal error occurred: failed calling webhook "image-relocating-webhook.image-relocation.pivotal.io": Post https://ir-webhook.image-relocation.svc:443/image-relocation?timeout=30s: no endpoints available for service "ir-webhook"
(Thanks @scothis who spotted this.)

Done.

scothis

The CRD semantics will take a couple more iterations to get right. There are a number of corner cases that don't appear to have consistent defined semantics.

A simpler alternative, would be a single ConfigMap that is mounted in the webhook. This would sidestep reconciling the CRD resources to a single map.

Other questions/comments are inline. Happy to voice on anything that may be confusing.

scothis · 2019-10-03T06:10:31Z

config/image-relocation-webhook/400-crds.yaml

+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+  name: imagemaps.webhook.image-relocation.pivotal.io


webhook is an implementation detail. - should generally be avoided in api groups.

Suggested change

name: imagemaps.webhook.image-relocation.pivotal.io

name: imagemaps.imagerelocation.pivotal.io

Removed, but kept the - for readability.

scothis · 2019-10-03T06:10:54Z

config/image-relocation-webhook/200-rbac.yaml

+metadata:
+  name: ir-cluster-role
+rules:
+- apiGroups: ["*"]


can we lock this down to the exact permissions needed?

Sure, but I'll need to investigate.

Added a FIXME comment for now.

scothis · 2019-10-03T06:12:03Z

config/image-relocation-webhook/400-crds.yaml

+  names:
+    plural: imagemaps
+    singular: imagemap
+    kind: ImageMap


Cluster scoped resources, should be use Cluster as a prefix

Suggested change

kind: ImageMap

kind: ClusterImageMap

config/samples/webhook_v1alpha1_imagemap.yaml

scothis · 2019-10-03T06:18:24Z

pkg/api/v1alpha1/imagemap_types.go

+	Map map[string]string `json:"map,omitempty"`
+}
+
+func (in *ImageMapSpec) DeepCopyInto(out *ImageMapSpec) {


DeepCopy methods are typically generated and put into a separate file (zz_generated.deepcopy.go) so they can be updated without impacting the main source.

I put the generated methods in such a file but had to add these manually.

scothis · 2019-10-03T06:19:25Z

pkg/api/v1alpha1/imagemap_types.go

+
+// ImageMapStatus defines the observed state of ImageMap
+type ImageMapStatus struct {
+	Map map[string]string `json:"map,omitempty"`


are these values any different than the spec?

No. Is that a problem?

pkg/controller/imagemap_controller.go

pkg/multimap/composite.go

scothis · 2019-10-03T06:36:24Z

pkg/relocatingwebhook/image_reference_relocator.go

+			return admission.Errored(http.StatusBadRequest, fmt.Errorf("decoding pod: %s", err))
+		}
+	} else {
+		err := i.client.Get(ctx, types.NamespacedName{Namespace: req.Namespace, Name: req.Name}, &pod)


how can you get a resource from the API Server that hasn't been created yet? I know this isn't original code, I'm just noticing it now

I'm not sure, but I was nervous about ripping this code out. I'll check with @cppforlife.

@cppforlife

Based on https://github.com/k14s/k8s-resource-protection, thanks to @cppforlife.

Fixes https://github.com/pivotal/image-relocation/issues/36

glyn · 2019-10-03T11:30:52Z

The CRD semantics will take a couple more iterations to get right. There are a number of corner cases that don't appear to have consistent defined semantics.

I don't think you detailed the corner cases inline. Please could you elaborate?

A simpler alternative, would be a single ConfigMap that is mounted in the webhook. This would sidestep reconciling the CRD resources to a single map.

This doesn't feel particularly scalable. For instance, if more than one bundle was relocated by different people, they'd have to coordinate to merge the relocation mappings into the single ConfigMap. Plus they'd need to handle any inconsistencies manually.

Other questions/comments are inline. Happy to voice on anything that may be confusing.

Thanks!

glyn · 2019-10-17T15:55:15Z

Closing as the plan is to put the webhook in its own repository (https://github.com/pivotal/kubernetes-image-mapper), starting with vmware-archive/kubernetes-image-mapper#2.

Based on the spike in vmware-archive/image-relocation#37. Deleted controllers/suite_test.go until we are ready to add an integration test.

glyn marked this pull request as ready for review October 2, 2019 11:21

glyn self-assigned this Oct 2, 2019

glyn requested a review from scothis October 2, 2019 11:26

scothis reviewed Oct 3, 2019

View reviewed changes

glyn added 3 commits October 3, 2019 11:12

MutatingAdmissionWebhook for relocating image references

4865d4f

Based on https://github.com/k14s/k8s-resource-protection, thanks to @cppforlife.

Thread safe access/management of relocation mappings

7443a18

Add controller to manage image maps

30ea71c

Fixes https://github.com/pivotal/image-relocation/issues/36

glyn added 2 commits October 3, 2019 15:34

Address review comments

f75716d

FIXMEs

66043b0

glyn closed this Oct 17, 2019

glyn mentioned this pull request Nov 6, 2019

Implement a MutatingAdmissionWebhook to apply relocation mappings to pods vmware-archive/kubernetes-image-mapper#3

Open

glyn added a commit to vmware-archive/kubernetes-image-mapper that referenced this pull request Nov 20, 2019

Custom code and README

2754620

Based on the spike in vmware-archive/image-relocation#37. Deleted controllers/suite_test.go until we are ready to add an integration test.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MutatingAdmissionWebhook for relocating image references #37

MutatingAdmissionWebhook for relocating image references #37

glyn commented Sep 25, 2019

codecov bot commented Sep 25, 2019 •

edited

Loading

glyn commented Sep 30, 2019 •

edited

Loading

glyn commented Oct 2, 2019

scothis left a comment

scothis Oct 3, 2019

glyn Oct 3, 2019

scothis Oct 3, 2019

glyn Oct 3, 2019

glyn Oct 3, 2019

scothis Oct 3, 2019

glyn Oct 3, 2019

scothis Oct 3, 2019

glyn Oct 3, 2019

scothis Oct 3, 2019

glyn Oct 3, 2019

scothis Oct 3, 2019

glyn Oct 3, 2019

glyn commented Oct 3, 2019

glyn commented Oct 17, 2019

	name: imagemaps.webhook.image-relocation.pivotal.io
	name: imagemaps.imagerelocation.pivotal.io

MutatingAdmissionWebhook for relocating image references #37

MutatingAdmissionWebhook for relocating image references #37

Conversation

glyn commented Sep 25, 2019

codecov bot commented Sep 25, 2019 • edited Loading

Codecov Report

glyn commented Sep 30, 2019 • edited Loading

glyn commented Oct 2, 2019

scothis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyn commented Oct 3, 2019

glyn commented Oct 17, 2019

codecov bot commented Sep 25, 2019 •

edited

Loading

glyn commented Sep 30, 2019 •

edited

Loading