Resolved a race condition where provisioners could skip having defaults if applied before karpenter-webhook came online #465
Conversation
…ts if applied before karpenter-webhook came online
| @@ -46,8 +46,8 @@ spec: | |||
| containerPort: 8081 | |||
| livenessProbe: | |||
| httpGet: | |||
| path: /healthz | |||
| port: 8081 | |||
| scheme: HTTPS | |||
There was a problem hiding this comment.
A couple of quick questions that may just be my lack of understanding of liveness probes:
- Do we still need a path here?
- Does putting the port to the same port as the webhook container mean it only pings healthy when that container is healthy?
- Do we still need the health-probe port at 8081?
There was a problem hiding this comment.
Do we still need a path here?
Knative pkg sets up probe handlers here: https://github.com/knative/pkg/blob/b558677ab03404ed118ba3e179a7438fe2d8509a/network/handlers/drain.go#L83 that do not require a path and share the port with the webhook. It's actually pretty cool how it works. I just missed this the first time around and it was a good opportunity to simplify and remove our custom webhook.
Does putting the port to the same port as the webhook container mean it only pings healthy when that container is healthy?
Yeah more or less. See the code above.
Do we still need the health-probe port at 8081?
Good catch! No! Removed.
| // Controllers and webhook | ||
| sharedmain.MainWithConfig( | ||
| webhook.WithOptions(injection.WithNamespaceScope(signals.NewContext(), system.Namespace()), webhook.Options{ | ||
| Port: options.Port, | ||
| ServiceName: "karpenter-webhook", | ||
| SecretName: "karpenter-webhook-cert", | ||
| }), | ||
| "Karpenter Webhooks", | ||
| "karpenter.webhooks", |
There was a problem hiding this comment.
Looks like this component is for logging purposes. Since we name the webhooks karpenter-webhook, maybe we could do Karpenter-Webhooks? WDYT?
There was a problem hiding this comment.
It's corresponds to a configmap, which means that the name must be dns compliant. This was causing noisy errors in the log and logging fallback behavior.
Issue #, if available: #463
Description of changes:
The main change is that we now statically populate webhooks.yaml instead of allowing the webhook controller to inject the resources into the webhook. This ensures that the webhook configuration prevents creation of provisioners even if the webhook controller fails to come online (e.g. auth issues)
rules: - apiGroups: - provisioning.karpenter.sh apiVersions: - v1alpha1 resources: - provisioners provisioners/status operations: - CREATE - UPDATE - DELETEBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.