Webhook proposal

[theishshah]

Description of the change:

Markdown proposal for including webhook support from controller runtime into the operator-sdk

Motivation for the change:

Implementation in progress, want to get feedback on the design/proposal

ref: #1218 #1217

[joelanford]

@theishshah Have you had a chance to think about some of the improvements we discussed offline? If I recall, I think we talked about:

• Supporting a webhook per API
• Designing the webhook support such that the user's main.go file does not need to be changed to add new webhook servers.

Also, would you mind adding more detail to this proposal doc?

• Will there be a new SDK subcommand to scaffold webhooks and if so, what flags will it have. Descriptions of all of those would be nice to have documented here for discussion (and will make it easy to drop into the real docs when the proposal is approved)
• Can you embed example project layout changes and scaffold snippets for the major places we're proposing to change? Basically, when someone runs the above subcommand, what will we create, and how will it hook into the user's main.go?

[lilic]

Agree with @joelanford on the last two points ^

[lilic]

While I like having a working example, can you include the code parts roughly in this proposal?

[lilic]

Agree with @joelanford on the last two points ^

[jianzhangbjz]

/cc many customers look forward to this feature. :)

[lilic]

Can we specify the version here, e.g.:

Suggested change

	The upcoming stable version of controller runtime has support for running a webhook server and having webhooks to mutate or validate pods. The mutation webhook can change various attributes of a pod, and the validation webhook can read pod attributes and allow/deny a pod to run based on this information.
	The `v0.2.0` of controller runtime has support for running a webhook server and having webhooks to mutate or validate pods. The mutation webhook can change various attributes of a pod, and the validation webhook can read pod attributes and allow/deny a pod to run based on this information.

[lilic]

Could we have these files live in the users webhook package. e.g. pkg/webhook/validationwebhook.go and so on. This will make it easier to migrate to kubebuilder as they have only one main.go file that lives in the root of the project.

[lilic]

As this is the proposal, not sure the out of date part belongs here. You can just leave a comment in the PR for that note.

[lilic]

These flags are new flags for operator-sdk if I understand correctly but as part of what command?

[lilic]

regardless of whether or not a user opts in

Can you elaborate on this, when will this take place, during which step of the generation?

[AlexNPavel]

Suggested change

	osdk-webhook.go will be responsible for containing the logic to start up a webhook server, including a `webhookServer` struct to contain the necesary information to start the server.
	`osdk-webhook.go` will be responsible for containing the logic to start up a webhook server, including a `webhookServer` struct to contain the necesary information to start the server.

[AlexNPavel]

Suggested change

The webhook initilization logic function will be a function which returns successfully _without_ starting the server if the user does not enable webhook support via the command line (planned command is `$ operator-sdk generate webhooks`, more detail is outlined in the next section). If a user chooses to generate webhooks, the osdk-webhook.go file will be overwritten with a new file to contain a server init function with full functionality. Additionally it will generate the server information struct populated with the deafult values. Controller runtime defaults for these values are `Port: 9876` and `CertDir: "/tmp/cert"`.

The webhook initialization logic function will be a function which returns successfully _without_ starting the server if the user does not enable webhook support via the command line (planned command is `$ operator-sdk generate webhooks`, more detail is outlined in the next section). If a user chooses to generate webhooks, the osdk-webhook.go file will be overwritten with a new file to contain a server init function with full functionality. Additionally it will generate the server information struct populated with the default values. Controller runtime defaults for these values are `Port: 9876` and `CertDir: "/tmp/cert"`.

[AlexNPavel]

Suggested change

	In addition to the above file being used to start up the server, an additional files (`mutationswebhook.go` and `validationwebhooks.go`) will generated when the user chooses to include webhooks. These are the files which have the necesary boilerplate for a user to implement the `Handle` function in order to run custom logic on validation and admission of pods.
	In addition to the above file being used to start up the server, additional files (`mutationswebhook.go` and `validationwebhooks.go`) will generated when a user chooses to include webhooks. These are the files which have the necessary boilerplate for a user to implement the `Handle` function in order to run custom logic on validation and admission of pods.

[AlexNPavel]

Suggested change

	My suggested method for interacting with this feature is to have a command in the osdk which can be run after generating the base operator. The new command `$ operator-sdk generate webhook` will write the files cmd/manager/osdk-webhook.go, cmd/manager/mutationwebhook.go, and cmd/manager/validationwebhook.go
	My suggested method for interacting with this feature is to have a command in the osdk which can be run after generating the base operator. The new command `$ operator-sdk generate webhook` will write the files `cmd/manager/osdk-webhook.go`, `cmd/manager/mutationwebhook.go`, and `cmd/manager/validationwebhook.go`.

[joelanford]

@theishshah For operators with multiple APIs, we need to be able to support multiple webhooks. Consider the following scenario:

1. User runs operator-sdk new my-operator

   Right now, this scaffolds out a main.go and some boilerplate in pkg/apis and pkg/controllers for future calls to operator-sdk add api and operator-sdk add controller, respectively.

2. User runs operator-sdk up local

   This will compile and run successfully but nothing happens since we have no controllers or APIs

3. User runs:

   • operator-sdk add api --api-verison=example.com/v1alpha1 --kind MyDatabase
   • operator-sdk add api --api-verison=example.com/v1alpha1 --kind MyWebApp
   • operator-sdk add controller --api-verison=example.com/v1alpha1 --kind MyDatabase
   • operator-sdk add controller --api-verison=example.com/v1alpha1 --kind MyWebApp

   None of these commands modify anything in cmd/manager. Instead the scaffolded files register themselves via the functions created previously by operator-sdk new in pkg/apis and pkg/controller. Now, when running operator-sdk up local, these APIs and controllers are picked up and CRs are reconciled.

4. Now the user wants to add some webhooks. Going along with this pattern, that probably means something like a operator-sdk add webhook command that takes:

   • --api-version and --kind so that we know where to create the file(s) that define the webhooks for a specific type.
   • --webhook-type <validating|mutating|conversion> so that we know which types of webhooks to create (maybe we have one file per type?)

5. The user runs the following to create two webhooks:

   • operator-sdk add webhook --api-verison=example.com/v1alpha1 --kind MyDatabase --webhook-type=mutating
   • operator-sdk add webhook --api-verison=example.com/v1alpha1 --kind MyWebApp --webhook-type=validating

   These, perhaps, would end up creating:

   • pkg/apis/example/v1alpha1/mydatabase_mutating_webhook.go
   • pkg/apis/example/v1alpha1/mywebapp_validating_webhook.go

As discussed offline, it would be really helpful to include links in the proposal that contain examples, types, functions, etc. provided by controller-runtime that we intend to use. That would give reviewers more context about the proposal.

[joelanford]

We need to make sure we have a smooth user experience that makes it easy to add webhooks for individual types without modifying things in cmd/manager. Based on our offline discussion, that looks doable given what's currently provided by controller-runtime.

I'd like to see this proposal updated to include a mini user-guide that follows along the scenario I described with more details about what files are scaffolded at each point in the process, and what would go into those files to make the initial webhook setup simple and to make it easy for users to customize various aspects of the webhooks.

Lastly, we also need to consider the TLS integration with webhook servers (e.g. how to request, use, and rotate certs). This might be something we do as a separate proposal since it is also relevant to metrics servers.

[joelanford]

Suggested change

	The upcoming stable version of controller runtime has support for running a webhook server and having webhooks to mutate or validate pods. The mutation webhook can change various attributes of a pod, and the validation webhook can read pod attributes and allow/deny a pod to run based on this information.
	The upcoming stable version of controller runtime has support for running a webhook server and having webhooks to mutate or validate Kubernetes resources. The mutation webhook can change various attributes of a resource, and the validation webhook can read resource attributes and allow/deny changes based on this information.

[joelanford]

@theishshah For operators with multiple APIs, we need to be able to support multiple webhooks. Consider the following scenario:

1. User runs operator-sdk new my-operator

   Right now, this scaffolds out a main.go and some boilerplate in pkg/apis and pkg/controllers for future calls to operator-sdk add api and operator-sdk add controller, respectively.

2. User runs operator-sdk up local

   This will compile and run successfully but nothing happens since we have no controllers or APIs

3. User runs:

   • operator-sdk add api --api-verison=example.com/v1alpha1 --kind MyDatabase
   • operator-sdk add api --api-verison=example.com/v1alpha1 --kind MyWebApp
   • operator-sdk add controller --api-verison=example.com/v1alpha1 --kind MyDatabase
   • operator-sdk add controller --api-verison=example.com/v1alpha1 --kind MyWebApp

   None of these commands modify anything in cmd/manager. Instead the scaffolded files register themselves via the functions created previously by operator-sdk new in pkg/apis and pkg/controller. Now, when running operator-sdk up local, these APIs and controllers are picked up and CRs are reconciled.

4. Now the user wants to add some webhooks. Going along with this pattern, that probably means something like a operator-sdk add webhook command that takes:

   • --api-version and --kind so that we know where to create the file(s) that define the webhooks for a specific type.
   • --webhook-type <validating|mutating|conversion> so that we know which types of webhooks to create (maybe we have one file per type?)

5. The user runs the following to create two webhooks:

   • operator-sdk add webhook --api-verison=example.com/v1alpha1 --kind MyDatabase --webhook-type=mutating
   • operator-sdk add webhook --api-verison=example.com/v1alpha1 --kind MyWebApp --webhook-type=validating

   These, perhaps, would end up creating:

   • pkg/apis/example/v1alpha1/mydatabase_mutating_webhook.go
   • pkg/apis/example/v1alpha1/mywebapp_validating_webhook.go

As discussed offline, it would be really helpful to include links in the proposal that contain examples, types, functions, etc. provided by controller-runtime that we intend to use. That would give reviewers more context about the proposal.

[theishshah]

Here is an example of the planned implementation on the alpha version of controller runtime (out of date, currently being re-implemented with new/updated design).

Additionally here are reference resources being used:
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/builder/webhook.go#L50:30

[robbie-demuth]

I'm still reading up on the process for proposed changes to the SDK, but is there a ballpark as to when this feature would be available? I've looked through the planned implementation and it looks like controller-runtime is getting closer to its v0.2.0 release. Kubebuilder supports scaffolding webhook implementations as well as generating the corresponding Kubernetes manifests via controller-gen, but mixing Kubebuilder with the Operator SDK is a bit wonky

[robbie-demuth]

Any updates on this, @theishshah?

[camilamacedo86]

Could we change it to use the template? WDYT?

[joelanford]

@theishshah Given that we plan to integrate Kubebuilder and Operator SDK, I think this proposal is obsolete, as we'll inherit Kubebuilder's support for webhooks. If you agree, let's close this.

[robbie-demuth]

For what it's worth, I used Kubebuilder to scaffold the Go implementations for the webhooks in my Operator SDK project and controller-gen to autogenerate the Kubernetes manifests. It's working quite nicely now that v0.11.0 is using v0.2.0+ of controller-runtime. All of that being said, I think integrating the two frameworks would be a huge plus

[wreed4]

@joelanford would you say the recommended practice at this point is to upgrade to v0.11.0 and do what @robbie-demuth said? Or should I follow the existing documentation surrounding webhooks at this point?