-
Notifications
You must be signed in to change notification settings - Fork 33
VWC generation moved to operator code #660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
cfe56e0
to
71521c3
Compare
Continuation of discussion:
|
ab8e911
to
cc45e42
Compare
// EnsureValidatingWebhook creates or updates the ValidatingWebhookConfiguration | ||
// that used to be templated by Helm. It is a best-effort reconciliation and | ||
// returns an error only when we cannot make the desired state match the spec. | ||
func EnsureValidatingWebhook( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be done using the renderAndSyncResource
pattern we follow in the controllers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VWC is cluster-scoped. Kubernetes doesn't allow setting a namespaced owner as controller of a cluster-scoped object. The current helper calls SetControllerReference, which would fail in that case.
if err = controllerutil.SetControllerReference(nimService, resource, r.GetScheme()); err != nil {
logger.Error(err, "failed to set owner", conditionType, namespacedName)
statusError := r.updater.SetConditionsFailed(ctx, nimService, reason, err.Error())
if statusError != nil {
logger.Error(statusError, "failed to update status", "nimservice", nimService.Name)
}
return err
}
To use this exact pattern for the cluster scoped vwc we'd have to either:
- Add a variant that skips SetControllerReference (and handle cleanup via explicit delete/finalizer), or
- Manage the webhook from an operator-owned, cluster-scoped controller rather than from a namespaced CR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add code to call EnsureValidatingWebhook
fb6af6d
to
4d07047
Compare
Signed-off-by: Aryan <[email protected]>
318a6e3
to
7aa0c63
Compare
Fixed. main.go how calls code to EnsureValidatingWebhook |
Helm
The operator now generates validatingwebhookconfiguration in the operator code itself (in cmd/main.go). Also detects orchestrator type in main.go. validatingwebhookconfiguration removed from Helm charts.
internal/webhook/apps/v1alpha1/configuration.go contains this generation.
In
deployments/helm/k8s-nim-operator/templates/manager-rbac.yaml
added create and update to RBAC.OLM
Validatingwebhookconfiguration (called webhookdefinitions in OLM) removed from CSV and moved to the operator code (same as above). The non-Helm specific changes (changes not in deployments are common to OLM as well (including new ENV variables).
To work around this: