Avoid updating webhooks service account unless required. #300
Conversation
amisevsk
changed the title
Since webhooks are required for conversion, set them to always enabled and remove any related checks from the codebase. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Each time the webhooks serviceAccount is updated with nil .secrets, a new secret is generated to fill the `secrets` field. This results in every restart of the DevWorkspace operator creating a new secret on the cluster. Since the only fields we set on the spec ServiceAccount are labels, we only need to update when the labels differ from the cluster. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
| return err | ||
| if needsUpdate(serviceAccount, existingCfg) { | ||
| serviceAccount.ResourceVersion = existingCfg.ResourceVersion | ||
| err = client.Update(ctx, serviceAccount) |
There was a problem hiding this comment.
Does it mean that after we update labels on SA, new token will be created? Can we avoid it with patching or it's by design for k8s?
There was a problem hiding this comment.
The .secrets field in a SA object gets populated with a reference to the secret that should be used. When we apply the spec SA as an update, it wipes out that field and k8s fills it again... with a new secret.
We could potentially work around this by copying the current secrets field over to the spec before updating, but I'm not sure if that causes any issues in other ways, so I opted to avoid updating unless necessary (which realistically, should not be needed 99% of the time)
| if config.ControllerCfg.GetWebhooksEnabled() != "true" { | ||
| return "", nil | ||
| } | ||
| func (r *DevWorkspaceReconciler) validateCreatorLabel(workspace *devworkspace.DevWorkspace) (msg string, err error) { |
There was a problem hiding this comment.
Does it make sense to disable that check for non-restricted-access workspaces?
There was a problem hiding this comment.
Probably, but all workspaces get a creator label now; the check itself is doing something kind of different from before. The only way we would have a DW without the label would be if it was created before webhooks, in which case it may make sense to fail the workspace anyways since it might have missed conversion, etc.
| @@ -156,17 +156,6 @@ func (r *DevWorkspaceReconciler) Reconcile(req ctrl.Request) (reconcileResult ct | |||
| return reconcile.Result{Requeue: true}, err | |||
| } | |||
|
|
|||
| restrictedAccess := clusterWorkspace.Annotations[config.WorkspaceRestrictedAccessAnnotation] | |||
There was a problem hiding this comment.
would it be a good idea to check if webhooks really exists on the cluster here? Or when they absent workspace can't be reconciled due validateCreatorLabel check?
I think the second.
There was a problem hiding this comment.
I think it's probably safer to do some check that webhooks are enabled (i.e. actually checking that webhooks exist on the cluster), but I would separate that out into another issue.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, sleshchenko The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test v5-devworkspaces-operator-e2e |
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
|
New changes are detected. LGTM label has been removed. |
|
/test v5-devworkspaces-operator-e2e |
What does this PR do?
.secretsfield and causes k8s to create a new secret for the SA.What issues does this PR fix or reference?
Fixes #199
Is it tested? How?
make restarta few times; after first run, log should containWebhook server service account up to dateinstead ofUpdated webhook server service accountand only one secret nameddevworkspace-controller-serviceaccount-token-xxxxxshould exist.