restructure certwriter and certprovisioner packages #113

mengqiy · 2018-08-20T07:09:10Z

This PR is ready for 1st round review.
Will add the tests very soon.

godoc:
http://35.232.176.188/pkg/sigs.k8s.io/controller-runtime/pkg/webhook/

droot

Review is still in progress. Have only reviewed till cert/doc.go.

I was wondering if there is a way to split this review. Long reviews are really discouraging for the reviewers :)

droot · 2018-08-20T22:39:43Z

pkg/webhook/bootstrap.go

+		s.CertDir = path.Join("k8s-webhook-server", "cert")
+	}
+	if s.KVMap == nil {
+		s.KVMap = map[string]interface{}{}
 	}
 	s.setBootstrappingDefault()


almost missed this because simply assumed all stuff is being defaulted inline here (no func call).
May be from readability perspective, we can group the inline defaults above in separate method and invoke the method here ?

droot · 2018-08-20T22:41:26Z

pkg/webhook/bootstrap.go

@@ -54,7 +56,10 @@ func (s *Server) setDefault() {
 		s.Port = 443
 	}


Do the defaults for s.Host and s.Service should be moved here from the setBootstrappingDefault method ?

s.Host and s.Service is for bootstrapping the AdmissionWebhookConfiguration and Service.

droot · 2018-08-20T22:42:30Z

pkg/webhook/bootstrap.go

+		s.CertGenerator = &generator.SelfSignedCertGenerator{}
+	}
+	if s.Writer == nil {
+		s.Writer = os.Stdout
 	}


I am a bit confused about what belongs to boostrapping and what belongs at the server level ? Looking at the attributes looks like all of them can be done inline in the setDefaults ?

Actually they were in the same defaulting function and I didn't plan to distinguish them. But the linter keeps complaining about high cyclomatic complexity. Then I have to split this defauting function :/

droot · 2018-08-20T22:49:15Z

pkg/webhook/bootstrap.go

+	// Provision the cert by creating new one or refreshing existing one.
+	s.certProvisioner.Provision(false)
+	// Inject the cert into MutatingWebhookConfiguration, ValidatingWebhookConfiguration and ConversionWebhook
+	s.certProvisioner.Inject(s.mutatingWebhookConfig, s.validatingWebhookConfig)


Re-reading the code, I have a few naming suggestion for Inject:
ApplyConfig ?
ConfigureWebhook ?
Configure ?

So it will read like:
Provision the certs and then the configure or apply the config for webhooks.

droot · 2018-08-20T22:53:25Z

pkg/webhook/bootstrap.go

+	cc, err := s.clientConfig()
+
+	s.certProvisioner = &cert.Provisioner{
+		ClientConfig:  cc,


wondering if this should be a parameter to the Inject method ?

Do we envision certProvisioner's Inject being called with different configs ? (by other programs)

If we do that, then certProvisioner and certWrtier initialization can be moved to setDefault probably ? and bootstrap just becomes:

Generate the configuration

Apply the configuration

droot · 2018-08-20T23:14:43Z

pkg/webhook/bootstrap.go

 // TODO: use the helper in #98 when it merges.
-func createOrReplace(c client.Client, obj runtime.Object, fn mutateFn) error {
+func createOrReplaceHelper(c client.Client, obj runtime.Object, fn mutateFn) error {


hmm....looks like this could be useful in general. We might be able to move these under controller-runtime under some clientutil pkg.

This may be a temporary thing.
#98 will land sometime.

droot · 2018-08-20T23:15:41Z

pkg/webhook/bootstrap.go

-func (w *webhookClientConfig) withPath(path string) error {
-	if w.URL != nil {
-		u, err := url.Parse(*w.URL)
+func (s *Server) clientConfigWithPath(path string) (*admissionregistration.WebhookClientConfig, error) {


comment explaining path will be helpful here.

droot · 2018-08-20T23:21:55Z

pkg/webhook/bootstrap.go

 	}

 	if len(mutatingWebhooks) > 0 {
 		return &admissionregistration.MutatingWebhookConfiguration{
 			TypeMeta: metav1.TypeMeta{
-				APIVersion: "admissionregistration.k8s.io/v1beta1",
+				APIVersion: fmt.Sprintf("%s/%s", admissionregistration.GroupName, "v1beta1"),


I think http pkg has some utility methods to manipulate the URL paths, would suggest using those.

Hmm... Why handling apiVersion with URL manipulation util in http pkg?

droot · 2018-08-20T23:34:24Z

pkg/webhook/bootstrap.go

-	if w.Service != nil {
-		w.Service.Path = &path
+	if cc.Service != nil {
+		cc.Service.Path = &path
 	}
 	return nil
 }


hmm..... may be we need ccWithPath type with methods it ?

Made some changes here.

droot · 2018-08-20T23:35:18Z

pkg/webhook/cert/doc.go

-Build the Provisioner
+	provisioner := Provisioner{
+		CertWriter: admission.NewSecretCertWriter(admission.SecretCertWriterOptions{...}),
+		ClientConfig: cc,


I commented earlier if we can make it a parameter to the Inject method ?

droot

Took another look, have some more comments.

droot · 2018-08-21T01:27:40Z

pkg/webhook/server.go

 	return s.bootstrap(true)
 }

 // RefreshCert refreshes the certificate using Server's Provisioner if the certificate is expiring.
-func (s *Server) RefreshCert() error {
+func (s *Server) RefreshCert() (bool, error) {
 	objs := []runtime.Object{s.mutatingWebhookConfig, s.validatingWebhookConfig}


we can have a method s.webhookConfigTarget() or s.webhookConfigObjects() (I am seeing this in multiple places) ?

droot · 2018-08-21T01:31:24Z

pkg/webhook/server.go

-			err = s.RefreshCert()
+			if !changed {
+				continue
+			}


we should add a log statement indicating server is shutting down to refresh the certs.

droot · 2018-08-21T01:33:26Z

pkg/webhook/server.go

@@ -189,21 +200,27 @@ func (s *Server) Start(stop <-chan struct{}) error {
 		errCh <- err


wondering where writer is coming from ?

writer is package name here.

droot · 2018-08-21T01:34:03Z

pkg/webhook/server.go

 		sMux:          http.NewServeMux(),
 		registry:      map[string]Webhook{},
+		ServerOptions: ops,


ops --> options

ops reads as operations :)

droot · 2018-08-21T19:21:35Z

pkg/webhook/cert/provisioner.go

+// Provision provisions certificates for for the WebhookClientConfig.
+// It ensures the cert and CA are valid and not expiring.
+// It updates the CABundle in the webhookClientConfig if necessary.
+func (cp *Provisioner) Provision(dryrun bool) (bool, error) {


I am wondering if we should combine Provision and Inject methods into one by introducing parameters to Provision method because ideally, that should be one operation. provision and inject cc in the objects ?

droot · 2018-08-21T19:22:54Z

pkg/webhook/cert/provisioner.go

+		}
+	}
+	return cp.CertWriter.Inject(objs...)
+}


How do I ensure that caller have called Provision before Inject ? or is it okay ?

droot · 2018-08-21T19:27:40Z

pkg/webhook/server.go

@@ -84,9 +89,15 @@ type BootstrapOptions struct {
 	// If neither Service nor Host is unspecified, Host will be defaulted to "localhost".
 	Host *string

-	// Provisioner provisions certificates for the admission webhook server.
+	// fid godoc Provisioner provisions certificates for the admission webhook server.


typo: fid godoc

Liujingfang1 · 2018-08-21T18:34:32Z

pkg/webhook/bootstrap.go

+		s.CertGenerator = &generator.SelfSignedCertGenerator{}
+	}
+	if s.Writer == nil {
+		s.Writer = os.Stdout
 	}
 }

 // bootstrap returns the configuration of admissionWebhookConfiguration in yaml format if dryrun is true.


This comment hasn't been updated.

Liujingfang1 · 2018-08-22T16:27:37Z

pkg/webhook/bootstrap.go

-	if err != nil {
-		return nil, err
+
+	cc, err := s.clientConfig()


Maybe you can have another function like getConfigs to get all the configs needed in bootstrap: mutatinWebhookConfig, validatingWHConfig and clientConfig

clientConfig is not a runtime.Object. It's a field in both mutatinWebhookConfig and validatingWHConfig.

Liujingfang1 · 2018-08-22T16:35:55Z

pkg/webhook/bootstrap.go

@@ -353,11 +381,11 @@ func (s *Server) admissionWebhook(path string, wh *admission.Webhook) (*admissio
 	if err != nil {
 		return nil, err
 	}
-	err = cc.withPath(path)
+	err = setPath(cc, path)


Can you using clientConfigWithPath instead of clientConfig and setPath?

Liujingfang1 · 2018-08-22T16:45:07Z

pkg/webhook/cert/writer/fs.go

+	// certGenerator generates the certificates.
+	certGenerator generator.CertGenerator
+	// path is the directory that the certificate and private key and CA certificate will be written.
+	path string


Can you use FSCertWriterOptions instead of path and certGenerator? It has better readability

mengqiy · 2018-08-24T18:16:53Z

Addressed comments and rebased on #115. PTAL

mengqiy · 2018-08-24T18:27:17Z

Travis is failing due to an linter error.
It seems there is a bug with structcheck linter that it doesn't handle inlined fields correctly.

EDIT: stuctcheck linter doesn't support embeded fields, it is a known issue

droot

Looks good to me.

droot · 2018-08-27T17:00:31Z

pkg/webhook/bootstrap.go

+		s.CertGenerator = &generator.SelfSignedCertGenerator{}
+	}
+	if s.Writer == nil {
+		s.Writer = os.Stdout
 	}


droot · 2018-08-27T17:02:06Z

pkg/webhook/bootstrap.go

 // Otherwise, it creates the the admissionWebhookConfiguration objects and service if any.
-// It also provisions the certificate for your admission server.
+// It also provisions the certificate for the admission server.
 func (s *Server) bootstrap(dryrun bool) error {


May be we should rename this function to installWebhookConfig to make it more explicit. Also, can dryrun be a server option ?

droot · 2018-08-27T17:09:10Z

pkg/webhook/internal/cert/provisioner.go

-	once sync.Once
+	// Objects are the objects that will use the ClientConfig above.
+	Objects []runtime.Object
+	// Dryrun controls if the objects are sent to the API server or output in the io.Writer


s/output in/written to

mengqiy · 2018-08-29T23:18:47Z

Rebased on #123
And cleanup tests and some stale files.

…nown-limitations

mengqiy · 2018-08-29T23:56:30Z

Cherry-picked #126

droot · 2018-08-30T16:50:07Z

Looks good to me. You can squash the commits, then we can merge it.

mengqiy · 2018-08-30T17:05:21Z

@droot Done

droot · 2018-08-30T20:35:19Z

/lgtm

k8s-ci-robot · 2018-08-30T20:36:26Z

[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

Add basic test for kubebuilder create config --crds

mengqiy requested review from DirectXMan12, droot and Liujingfang1 August 20, 2018 07:09

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 20, 2018

droot reviewed Aug 20, 2018

View reviewed changes

droot reviewed Aug 21, 2018

View reviewed changes

Liujingfang1 reviewed Aug 22, 2018

View reviewed changes

mengqiy force-pushed the prepare_for_conv_wh branch 2 times, most recently from 8395cd6 to 2b2a4ee Compare August 24, 2018 18:14

mengqiy force-pushed the prepare_for_conv_wh branch from 2b2a4ee to e3ce169 Compare August 24, 2018 21:44

droot approved these changes Aug 27, 2018

View reviewed changes

mengqiy force-pushed the prepare_for_conv_wh branch from 0e45f79 to 2f1180d Compare August 29, 2018 23:16

Mengqi Yu added 2 commits August 29, 2018 16:55

gas linter has been renamed to gosec

ad5a1b4

disable structcheck linter due to https://github.com/opennota/check#k…

3f46d4a

…nown-limitations

mengqiy force-pushed the prepare_for_conv_wh branch from 2f1180d to 7fdb8b1 Compare August 29, 2018 23:55

mengqiy force-pushed the prepare_for_conv_wh branch from 7fdb8b1 to 617592c Compare August 30, 2018 17:04

restructure certwriter and certprovisioner packages

f297678

mengqiy force-pushed the prepare_for_conv_wh branch from 617592c to f297678 Compare August 30, 2018 18:00

k8s-ci-robot assigned droot Aug 30, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2018

mengqiy added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2018

k8s-ci-robot merged commit 069efa2 into kubernetes-sigs:admissionwebhook Aug 30, 2018

mengqiy deleted the prepare_for_conv_wh branch August 30, 2018 20:38

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #113 from pmorie/basic-config-crd-test

0535d0a

Add basic test for kubebuilder create config --crds

		@@ -54,7 +56,10 @@ func (s *Server) setDefault() {
		s.Port = 443
		}

		@@ -189,21 +200,27 @@ func (s *Server) Start(stop <-chan struct{}) error {
		errCh <- err

restructure certwriter and certprovisioner packages #113

restructure certwriter and certprovisioner packages #113

Uh oh!

Conversation

mengqiy commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengqiy commented Aug 24, 2018

Uh oh!

mengqiy commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mengqiy commented Aug 20, 2018 •

edited

Loading

mengqiy commented Aug 24, 2018 •

edited

Loading