✨ Move webhook generator #136

mengqiy · 2019-02-01T18:41:55Z

This PR

add webhook subcommand as part of controller-gen all
move the generator code from controller-runtime to controller-tools and make some necessary changes.
move some file to make CT not depend on CR from dep perspective.

Updating the annotation will be done in a separate PR, since it requires more than just updating the annotation.
Annotation format is at https://github.com/jetstack/cert-manager/blob/ccd2dd853321c3ac568736b0d66ab9d3ad494d7b/pkg/controller/cainjector/controller.go#L40

mengqiy · 2019-02-07T02:45:29Z

PTAL

droot

I took a look and have a few comments. I will take another look sometime today, but want to send in the early feedback.

droot · 2019-02-07T17:10:50Z

pkg/webhook/admission.go

+	// name is the name of the webhook
+	name string
+	// t is the webhook type, i.e. mutating, validating
+	t webhookType


Not sure if typ is better than using t or not.
It's a partial word which reads strange.

Why you think typ is better?

Oh.. just wanted it be more descriptive from readability perspective. Suggested typ because type is probably reserved word.

droot · 2019-02-07T17:22:42Z

pkg/webhook/admission.go

+	// This optional.
+	namespaceSelector *metav1.LabelSelector
+
+	once sync.Once


this can be taken out now.

droot · 2019-02-07T19:22:25Z

pkg/webhook/generator.go

+			ObjectMeta: metav1.ObjectMeta{
+				Name: o.mutatingWebhookConfigName,
+				Annotations: map[string]string{
+					"admissionwebhook.alpha.kubebuilder.io/ca-secret-name": "webhook-cert",


Two comments:

We should add a comment explaining (or linking to an issue/doc) why this annotation so readers understand it.

annotation name "alpha" needs to come before admissionwebhooks ? alpha.admissionwebhooks.kubebuilder.io/ca-secret-name

"webhook-cert" is hardcoded ?

I now uses alpha.admissionwebhook.kubebuilder.io/ca-secret-name.

"webhook-cert" is hardcoded ?

fixed.

droot · 2019-02-07T19:45:13Z

pkg/webhook/generator_test.go

+      - name: cert
+        secret:
+          defaultMode: 420
+          secretName: webhook-secret


Two concerns:

Looks like input for CA injections (secret-name, service name etc.) is split across the objects. It might make the cert controller implementation difficult. I am wondering if we can combine all these and apply it only on field. Annotation value could be then in JSON ?

webhook-secret is hardcoded. We should think of a way to override it (by default ?)

Looks like input for CA injections (secret-name, service name etc.) is split across the objects. It might make the cert controller implementation difficult.

For generating cert, everything it needs is in the service object.
For syncing CA, everything it needs is in the WebhookConfiguration object.
It should not make the controller hard to implement. Instead, it should be quite easy to implement it :)

webhook-secret is hardcoded.

This one is not.

Got it. You are right, those are decoupled annotations, so it should be simpler :)

droot · 2019-02-07T19:47:49Z

pkg/webhook/writer.go

+	return nil
+}
+
+func (o *WriterOptions) writeObjectsToDisk(objects ...runtime.Object) error {


This can probably go in a util pkg.

I wonder if we should have "k8s/ioutil" pkg which implements k8s object reader/writer :)

droot

Looks good. I have one comment about how well it will work with kustomize ?

droot · 2019-02-12T21:11:06Z

pkg/webhook/generator_test.go

+      - name: cert
+        secret:
+          defaultMode: 420
+          secretName: webhook-secret


Got it. You are right, those are decoupled annotations, so it should be simpler :)

droot · 2019-02-12T21:12:18Z

pkg/webhook/generator_test.go

+kind: MutatingWebhookConfiguration
+metadata:
+  annotations:
+    alpha.admissionwebhook.kubebuilder.io/ca-secret-name: test-system/webhook-secret


I am wondering how will these value in these annotations will play with kustomization ?

mengqiy · 2019-02-12T21:24:18Z

how well it will work with kustomize ?

Kustomize by default doesn't know how to update the secret name in the annotation.
@droot we had a brief discussion last week. The outcome is that we don't use kustomize to manage the serving cert secret. We hand off this work to cert-manager. If the secret is not ready, let the pod wait for the secret.

mengqiy · 2019-02-13T20:03:40Z

Cherry-picker @DirectXMan12's commit which cleanup the vendor dir.
Resolved the conflicts caused by #142.

droot · 2019-02-13T20:29:37Z

The outcome is that we don't use kustomize to manage the serving cert secret. We hand off this work to cert-manager. If the secret is not ready, let the pod wait for the secret.

Sounds good.

mengqiy · 2019-02-14T21:05:21Z

@droot @DirectXMan12 We need to decide what format the annotations should look like.

I'm currently using
alpha.admissionwebhook.cert-manager.io: "true"
and
alpha.service.cert-manager.io/serving-cert-secret-name: some-secret-name
in my code, since I don't remember what Solly were using in the PoC controller code.

Besides the annotation format, other code is ready for another round of review.

mengqiy · 2019-02-21T01:43:50Z

PTAL
No big changes.
Rebased to address merge conflicts.

This removes the dependency on CR by marking the testData directory as ignored (it's moved so that we can make `go vet` ignore it). It also cleans up vendor by removing unecessary requires and adding prune-unused (we only need to keep around the dependencies in KB and possibly CR).

mengqiy · 2019-02-27T21:47:28Z

Rebased.
Commit Clean up vendor, remove dep on CR cause merge conflicts, due to moving some packages around.

k8s-ci-robot · 2019-02-27T21:59:07Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: mengqiy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 1, 2019

k8s-ci-robot requested review from droot and pwittrock February 1, 2019 18:42

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 1, 2019

mengqiy force-pushed the webhookgenerator branch from 216d2a9 to f3c7b09 Compare February 1, 2019 18:47

mengqiy requested a review from DirectXMan12 February 1, 2019 18:57

mengqiy force-pushed the webhookgenerator branch 4 times, most recently from 7403182 to a019e3a Compare February 5, 2019 21:04

mengqiy changed the title ~~[WIP] ✨ Move webhook generator~~ ✨ Move webhook generator Feb 5, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 5, 2019

mengqiy force-pushed the webhookgenerator branch 3 times, most recently from d1faaf1 to 2970c77 Compare February 6, 2019 21:59

droot reviewed Feb 7, 2019

View reviewed changes

mengqiy added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 8, 2019

mengqiy force-pushed the webhookgenerator branch 2 times, most recently from b57747a to 6a27543 Compare February 9, 2019 03:14

droot reviewed Feb 12, 2019

View reviewed changes

mengqiy force-pushed the webhookgenerator branch 3 times, most recently from 2a73501 to 54f2f6a Compare February 14, 2019 21:03

mengqiy force-pushed the webhookgenerator branch from 54f2f6a to d1e8d55 Compare February 14, 2019 22:21

mengqiy force-pushed the webhookgenerator branch 3 times, most recently from 23c8932 to 5d22b6c Compare February 21, 2019 01:42

Mengqi Yu and others added 3 commits February 27, 2019 13:44

🏃 update vendor

044a0e2

✨ move webhook generator from CR to CT

cb5b319

mengqiy force-pushed the webhookgenerator branch from 5d22b6c to cb5b319 Compare February 27, 2019 21:45

mengqiy removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2019

droot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 27, 2019

k8s-ci-robot merged commit 5752ac2 into kubernetes-sigs:master Feb 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Move webhook generator #136

✨ Move webhook generator #136

mengqiy commented Feb 1, 2019 •

edited

Loading

mengqiy commented Feb 7, 2019

droot left a comment

droot Feb 7, 2019

mengqiy Feb 8, 2019

droot Feb 12, 2019

droot Feb 7, 2019

droot Feb 7, 2019

mengqiy Feb 8, 2019

droot Feb 7, 2019

mengqiy Feb 8, 2019

droot Feb 12, 2019

droot Feb 7, 2019

droot left a comment

droot Feb 12, 2019

droot Feb 12, 2019

mengqiy commented Feb 12, 2019

mengqiy commented Feb 13, 2019

droot commented Feb 13, 2019

mengqiy commented Feb 14, 2019

mengqiy commented Feb 21, 2019

mengqiy commented Feb 27, 2019

k8s-ci-robot commented Feb 27, 2019

✨ Move webhook generator #136

✨ Move webhook generator #136

Conversation

mengqiy commented Feb 1, 2019 • edited Loading

mengqiy commented Feb 7, 2019

droot 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

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy commented Feb 12, 2019

mengqiy commented Feb 13, 2019

droot commented Feb 13, 2019

mengqiy commented Feb 14, 2019

mengqiy commented Feb 21, 2019

mengqiy commented Feb 27, 2019

k8s-ci-robot commented Feb 27, 2019

mengqiy commented Feb 1, 2019 •

edited

Loading