Add webhook with cert-manager recipe #2100

anyasabo · 2019-11-01T21:29:04Z

Related to #1769

Enables the webhook server if it's toggled in the config. It is not on by default because it looks for certificates in a specific location, so if they're not there it will be sad.
Adds docs on how to install the webhook with cert-manager
Updates the hasMaster validator to use a more specific error message

The update to use cert-manager v0.11 was only merged a week ago in kubebuilder so a lot of the docs there still need some work. I also added comments in the yaml of some of the weird things I noticed in Kubebuilder about how it expects webhooks to function.

The other unfortunate part is that kubebuilder docs very much expect you to be working with their deployments + kustomize configuration, which we have not migrated to yet.

To test this you may need to run make operator-image and then update the image field with the newly built image. Once this is in master I think we can update the example to a snapshot build.

anyasabo · 2019-11-01T22:31:58Z

Added some upstream docs PRs that should hopefully make things a little easier for people in the future:
kubernetes-sigs/controller-tools#353
kubernetes-sigs/kubebuilder#1147

pebrc

Some initial comments, will try it out on my installation as well.

pebrc · 2019-11-13T10:19:57Z

config/recipes/cert-manager/webhook.asciidoc

+  namespace: elastic-system
+spec:
+  selfSigned: {}
+...


Any particular reason why you put document end markers ("...") between the documents?

I just got in the habit of it, I can take it out if there's a reason I'm missing

pebrc · 2019-11-13T10:38:27Z

cmd/manager/main.go

@@ -122,6 +120,11 @@ func init() {
 		certificates.DefaultRotateBefore,
 		"Duration representing how long before expiration TLS certificates should be reissued",
 	)
+	Cmd.Flags().Bool(
+		EnableWebhooksFlag,


This somewhat duplicates what we used to express with the webhook role. I am fine with moving away from the operator roles concept but I think we should not have both the role and the explicit flag at the same time.

Ah that is an oversight on my part, I forgot about the roles. I'll change this, we'll just want to change the default to not-webhook now I think. And change it back when we have automatic webhook configuration

pebrc · 2019-11-13T10:46:10Z

cmd/manager/main.go

-	// 	os.Exit(1)
-	// }
+	if viper.GetBool(EnableWebhooksFlag) {
+		if err = (&estype.Elasticsearch{}).SetupWebhookWithManager(mgr); err != nil {


This only validates Elasticsearch resources but we used to have validations on (license) Secrets as well. Just flicking through the docs I think it should be possible to re-introduce them, but it might be worth revisiting the decision to validate secrets because it can potentially have a big impact on the stability of the k8s installation. Maybe worth re-introducing a license CRD?

pebrc · 2019-11-13T10:46:22Z

config/recipes/cert-manager/webhook.asciidoc

+[id="{p}-webhook"]
+== Validating webhook
+
+A validating webhook provides additional validation of Elasticsearch resources beyond what can be specified in OpenAPI specs. This allows advanced validation to help prevent Elasticsearch resources that are invalid and will not function correctly from being admitted by the Kubernetes API server, which can aid troubleshooting. To enable the validating webhook, you must first install cert-manager v0.11+ as described in their instructions link:from being admitted by the Kubernetes API server[here].


link to cert-manager doc is missing, probably a copy/paste mistake

barkbay

First review, still need to do some additional tests

barkbay · 2019-11-13T08:57:17Z

config/recipes/cert-manager/webhook.asciidoc

+[id="{p}-webhook"]
+== Validating webhook
+
+A validating webhook provides additional validation of Elasticsearch resources beyond what can be specified in OpenAPI specs. This allows advanced validation to help prevent Elasticsearch resources that are invalid and will not function correctly from being admitted by the Kubernetes API server, which can aid troubleshooting. To enable the validating webhook, you must first install cert-manager v0.11+ as described in their instructions link:from being admitted by the Kubernetes API server[here].


The link at the end of the sentence if not set

barkbay · 2019-11-13T09:14:50Z

config/recipes/cert-manager/webhook.asciidoc

+
+[source,yaml]
+----
+cat $$<<$$EOF | kubectl apply -f -


$$<<$$EOF is rendered "as is" with github, not sure why

barkbay · 2019-11-13T09:30:01Z

config/recipes/cert-manager/webhook.asciidoc

+      path: /validate-elasticsearch-k8s-elastic-co-v1beta1-elasticsearch
+  failurePolicy: Ignore
+  name: elastic-es-validation.k8s.elastic.co
+  timeoutSeconds: 5


I get this error when applying the ValidationWebhookConfiguration on GKE v1.13.12-gke.8 :
error: error validating "STDIN": error validating data: ValidationError(ValidatingWebhookConfiguration.webhooks[0]): unknown field "timeoutSeconds" in io.k8s.api.admissionregistration.v1beta1.Webhook; if you choose to ignore these errors, turn validation off with --validate=false

TBH I don't understand why... 😕

It's fine on GKE 1.14, so unless someone else understand the error I have on GKE 1.13 I would remove timeoutSeconds and keep the default (which is 30 seconds)

The field simply did not exist prior to 1.14 see kubernetes/api@c33fb2b and kubernetes/kubernetes#74562

barkbay · 2019-11-13T10:42:50Z

config/recipes/cert-manager/webhook.asciidoc

@@ -0,0 +1,168 @@
+[id="{p}-webhook"]


Should we not add this file in our current documentation ? Not sure I would look into the recipes directory if I was looking for information about how to configure the webhook with the cert manager.

I'm okay with moving it there. I think I had it here with the idea that this would be a rarely used option once we have automatic configuration enabled

barkbay · 2019-11-13T15:05:13Z

config/recipes/cert-manager/webhook.asciidoc

+
+=== Troubleshooting
+
+You can change the `failurePolicy` of the webhook configuration to `Fail`, which will cause creations to error out if there is an error contacting the webhook.


I don't remember why we have a default failurePolicy set to Ignore, I have some mixed feelings about this as I have been able to inject a incorrect manifest while testing certificate renewal: certificate was not up to date in the operator, got this message in ECK: http: TLS handshake error from 10.64.1.1:38142: remote error: tls: bad certificate but the resource was created anyway.

As a side note this part is not specific to the cert-manager so I guess we should eventually include it in a more generic documentation.

We used to have Fail but that caused a lot of issues with users having trouble with the webhooks. We decided because of these issues to loosen the policy to strike a balance between giving a better experience through immediate validation and not blocking users (especially because we were also validating k8s secrets in < 1.0)

anyasabo added 5 commits November 1, 2019 15:53

WIP

a372bbf

Add cert-manager and webhook instructions

1f92b0a

Add webhook config doc

bfdfd14

Add webhook install doc

da96e23

Update webhook manifest

06e0b27

pebrc added the >enhancement Enhancement of existing functionality label Nov 8, 2019

pebrc reviewed Nov 13, 2019

View reviewed changes

barkbay requested changes Nov 13, 2019

View reviewed changes

barkbay reviewed Nov 13, 2019

View reviewed changes

barkbay self-assigned this Nov 20, 2019

barkbay mentioned this pull request Nov 22, 2019

Add some documentation about the webhook #2158

Merged

anyasabo closed this Nov 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add webhook with cert-manager recipe #2100

Add webhook with cert-manager recipe #2100

anyasabo commented Nov 1, 2019

anyasabo commented Nov 1, 2019

pebrc left a comment

pebrc Nov 13, 2019

anyasabo Nov 13, 2019

pebrc Nov 13, 2019

anyasabo Nov 13, 2019

pebrc Nov 13, 2019

pebrc Nov 13, 2019

barkbay left a comment

barkbay Nov 13, 2019

barkbay Nov 13, 2019

barkbay Nov 13, 2019

pebrc Nov 13, 2019 •

edited

Loading

barkbay Nov 13, 2019

anyasabo Nov 13, 2019

barkbay Nov 13, 2019

pebrc Nov 13, 2019


		=== Troubleshooting

		You can change the `failurePolicy` of the webhook configuration to `Fail`, which will cause creations to error out if there is an error contacting the webhook.

Add webhook with cert-manager recipe #2100

Add webhook with cert-manager recipe #2100

Conversation

anyasabo commented Nov 1, 2019

anyasabo commented Nov 1, 2019

pebrc 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

barkbay 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

pebrc Nov 13, 2019 • edited Loading

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

pebrc Nov 13, 2019 •

edited

Loading