Skip to content
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

Prevent Service Account annotation conflicts on OpenShift 4.16 #10314

Merged

Conversation

scholzj
Copy link
Member

@scholzj scholzj commented Jul 7, 2024

Type of change

  • Bugfix

Description

It looks like we have on OCP 4.16 a conflict in the reconciliation of Service Accounts.

OpenShift 4.16 will create an image pull secret, add it to the image pull secrets list, and attach an annotation openshift.io/internal-registry-pull-secret-ref to the Service Account with the name of the image pull secret. If Strimzi removes this annotation by patching the resource in the next reconciliation, OpenShift will create another Secret, add it to the image pull secret list and add back the annotation. This way, every reconciliation will for every Service Account make OCP generate a new Secret and in a little while, the namespace is full of these Secrets.

This PR recovers the annotation from the original resource when patching it and reads it to the desired Service Account to avoid removing the annotation and triggering OCP to create a new Secret. This is not a great solution, but until we have Server Side Apply support, this is the best we can do.

Checklist

  • Write tests
  • Make sure all tests pass
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally

@scholzj scholzj added this to the 0.43.0 milestone Jul 7, 2024
@scholzj scholzj requested a review from ppatierno July 7, 2024 21:45
@scholzj
Copy link
Member Author

scholzj commented Jul 7, 2024

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@fvaleri fvaleri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Failed tests seem to be unrelated. Do we want to log an issue for getting rid of this workaround once the Server Side Apply is merged?

@scholzj scholzj force-pushed the prevent-SA-annotation-conflicts-with-OCP-4.16 branch from b1fadb0 to 49cc5f2 Compare July 8, 2024 13:07
@k-wall
Copy link
Contributor

k-wall commented Jul 9, 2024

I wanted to satisfy myself that this fix was sufficient (Is this the only SA annotation that needs special casing, should it be a pattern?) The PR that made the change appears to be openshift/openshift-controller-manager#288.

It looks to me like the change made is sufficient. lgtm.

@scholzj scholzj merged commit cf290db into strimzi:main Jul 10, 2024
13 checks passed
@scholzj scholzj deleted the prevent-SA-annotation-conflicts-with-OCP-4.16 branch July 10, 2024 13:01
mstruk pushed a commit to mstruk/strimzi-kafka-operator that referenced this pull request Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants