:warning: split webhook server and manifest generation by mengqiy · Pull Request #300 · kubernetes-sigs/controller-runtime

mengqiy · 2019-01-17T20:46:10Z

Code related to running a webhook server stays in CR repo.
Code related to generating cert are dropped.
Code related to generating non-cert manifests (e.g. webhookConfiguration, service) are currently under pkg/webhookgenerator,which will be moved to the controller-tools repo.

mengqiy · 2019-01-17T20:47:52Z

The PR is not polished yet.
Sent out the draft for getting feedback before @DirectXMan12 disappears next week :)

droot

I took a quick look and have a few questions.

droot · 2019-01-18T18:13:48Z

pkg/internal/webhookgenerator/admission/webhook.go

+	"sigs.k8s.io/controller-runtime/pkg/internal/webhookgenerator/types"
+)
+
+// Webhook represents each individual webhook.


May be we can describe it a bit more:

Webhook contains bits needed for generating a Webhook Configuration/manifest ?

droot · 2019-01-18T18:23:10Z

pkg/internal/webhookgenerator/server.go

+)
+
+// ServerOptions are options for configuring an admission webhook server.
+type ServerOptions struct {


may be we should rename the filename to be also server_options.go ?

droot · 2019-01-18T18:28:03Z

pkg/internal/webhookgenerator/server.go

+	// store it in this directory.
+	// If using SecretCertWriter in Provisioner, the server will provision the certificate in a secret,
+	// the user is responsible to mount the secret to the this location for the server to consume.
+	CertDir string


I think most of these comments need to be re-written from the new purely generation perspective. Currently they read as if webhook server itself is going to use these options to install webhook configuration.

droot · 2019-01-18T18:30:50Z

pkg/internal/webhookgenerator/server.go

+	s.setDefault()
+
+	return s.InstallWebhookManifests()
+}


Same comment as above. Some of these methods Register and Start doesn't seem relevant from purely generation perspective.

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

PTAL

mengqiy · 2019-02-12T00:12:58Z

Pushed a little more changes on top of the earlier commit.
The 2nd commit is intended to be squashed into the 1st one.

DirectXMan12

comments inline

DirectXMan12 · 2019-02-12T01:16:04Z

pkg/webhook/server.go


+// setDefault does defaulting for the Server.
+func (s *Server) setDefault() {
+	if len(s.Name) == 0 {


I think we can remove this name field (I've removed it in #323 IIRC, so we can just wait till that if you want)

Yup.
It was for the name (identifier) of each individual webhook, but it is no longer used anywhere in CR.

DirectXMan12 · 2019-02-12T01:16:25Z

pkg/webhook/server.go

+		s.registry = map[string]http.Handler{}
+	}
+	if s.sMux == nil {
+		s.sMux = http.DefaultServeMux


why is the default mux different from the mux used if you call the constructor?

for that matter, why do we have a constructor if we've got the setDefaults style?

why is the default mux different from the mux used if you call the constructor?

Thanks for catching this.
I agree we should use the same mux. i.e. use http.NewServeMux().

for that matter, why do we have a constructor if we've got the setDefaults style?

Because some users may use the public Server struct directly, we need to ensure it get sane defaulting and works.

DirectXMan12 · 2019-02-12T01:17:40Z

pkg/webhook/server.go

+		s.CertDir = path.Join("k8s-webhook-server", "cert")
+	}
+
+	if s.Client == nil {


we shouldn't be auto-initializing the client like this. We don't actually use this anywhere, so we shouldn't have a client field at all

I realized it too when fixing the injector for webhook. I have dropped it in #316.

DirectXMan12 · 2019-02-12T01:18:52Z

pkg/webhook/server.go


-	// httpServer is the actual server that serves the traffic.
-	httpServer *http.Server
+	// err will be non-nil if there is an error occur during initialization.


this design is weird. We can leave this in for now, but I don't think it makes much sense to leave in long-term

(e.g. multiple registrations will overwrite errors)

It is kind of the same pattern as kubectl resource builder.

e.g. multiple registrations will overwrite errors

You are right. We can probably make it an array of errors i.e. []error.

If you really don't like it, we rethink how to handle it :)

DirectXMan12 · 2019-02-12T03:21:23Z

pkg/webhook/server.go

-			return err
+	for path := range s.registry {
+		// TODO(mengqiy): remove this in PR #316
+		if wh, ok := s.registry[path].(Webhook); ok {


this inject code isn't quite right the way it's written, since someone could write their own webhook impl that needed the info. If webhook isn't an interface, this isn't a problem.

This is fixed in a4ef929#diff-fbc18bb07cdd05391b7081acc1dfe170R209
I tried to avoid putting everything in the same PR, so the code here may look incorrect.

mengqiy · 2019-02-13T19:52:35Z

PTAL

DirectXMan12 · 2019-02-14T18:47:13Z

pkg/webhook/server.go

-		ClientConfig: cc,
-		Objects:      s.webhookConfigurations,
-	})
+	listener, err := tls.Listen("tcp", net.JoinHostPort("", strconv.Itoa(int(s.Port))), cfg)


yay! JoinHostPort 👍

DirectXMan12

/lgtm
/approve

k8s-ci-robot · 2019-02-14T18:47:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, mengqiy

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [DirectXMan12]

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 · 2019-02-14T19:05:06Z

New changes are detected. LGTM label has been removed.

mengqiy · 2019-02-14T19:06:19Z

Squashed the commits, no new code change.
Re-applying the lgtm label per #300 (review)

/hold cancel

mengqiy requested review from DirectXMan12 and droot January 17, 2019 20:46

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 Jan 17, 2019

k8s-ci-robot requested a review from pwittrock January 17, 2019 20:46

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

mengqiy force-pushed the decouple branch 2 times, most recently from 8f03a29 to 4589fd6 Compare January 17, 2019 22:08

droot reviewed Jan 18, 2019

View reviewed changes

mengqiy force-pushed the decouple branch 3 times, most recently from 03c3f1b to b2ea9dc Compare February 4, 2019 23:58

mengqiy changed the title ~~[WIP] split webhook server and manifest generation~~ split webhook server and manifest generation Feb 4, 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 4, 2019

mengqiy removed the request for review from pwittrock February 5, 2019 00:08

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

mengqiy changed the title ~~split webhook server and manifest generation~~ ⚠️ split webhook server and manifest generation Feb 6, 2019

mengqiy force-pushed the decouple branch 4 times, most recently from fda3ed5 to 2ddd02d Compare February 7, 2019 01:35

DirectXMan12 mentioned this pull request Feb 11, 2019

⚠️ webhook redesign for generic case #323

Merged

mengqiy force-pushed the decouple branch from 2ddd02d to d169520 Compare February 12, 2019 00:11

mengqiy mentioned this pull request Feb 12, 2019

🐛 fix webhook injector #316

Closed

DirectXMan12 suggested changes Feb 12, 2019

View reviewed changes

DirectXMan12 reviewed Feb 14, 2019

View reviewed changes

k8s-ci-robot assigned DirectXMan12 Feb 14, 2019

DirectXMan12 approved these changes Feb 14, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2019

Mengqi Yu added 2 commits February 14, 2019 10:54

⚠️ move webhook self installer to CT as generator

81b48be

⚠️fix webhook injector, drop name and client

29adcfc

mengqiy force-pushed the decouple branch from 96d418e to 29adcfc Compare February 14, 2019 19:05

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2019

mengqiy added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2019

k8s-ci-robot merged commit c2f9dae into kubernetes-sigs:master Feb 14, 2019

mengqiy deleted the decouple branch February 15, 2019 16:56

bradhoekstra mentioned this pull request Jul 8, 2019

🐛 Remove unused disableWebhookConfigInstaller flag #515

Merged

mszostok mentioned this pull request Sep 11, 2019

✨Add option to use Lease lock if coordination group is available #600

Closed

alenkacz mentioned this pull request Nov 26, 2019

📖 Remove obsolete cert provisioner docs #698

Merged

thephw mentioned this pull request Apr 15, 2020

ManagerOptions#CertDir default is confusing #900

Open

Conversation

mengqiy commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mengqiy commented Jan 17, 2019

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!

mengqiy commented Feb 7, 2019

Uh oh!

mengqiy commented Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DirectXMan12 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!

mengqiy commented Feb 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 14, 2019

Uh oh!

k8s-ci-robot commented Feb 14, 2019

Uh oh!

mengqiy commented Feb 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mengqiy commented Jan 17, 2019 •

edited

Loading

mengqiy commented Feb 12, 2019 •

edited

Loading