Allow custom annotations on service accounts #3106
Conversation
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
| @@ -194,7 +194,6 @@ func mutateConfigMap(existing, desired *corev1.ConfigMap) { | |||
| } | |||
|
|
|||
| func mutateServiceAccount(existing, desired *corev1.ServiceAccount) { | |||
| existing.Annotations = desired.Annotations | |||
There was a problem hiding this comment.
doesn't this mean that if a user were to add an annotation that is expected to propagate to their serviceaccount, we would no longer do this? Also, if we remove an annotation it will no longer be removed? should we do some type of merge here?
There was a problem hiding this comment.
doesn't this mean that if a user were to add an annotation that is expected to propagate to their serviceaccount, we would no longer do this?
This PR actually makes sure that the user-added annotations on SA are not reverted by the operator. The mutate.go by default keeps user-defined annotations but for some kinds those annotations are removed (e.g. SA) and this PR relaxes that for SA.
There was a problem hiding this comment.
yes, but what about annotations added to the desired SA? i.e. the current logic allows us to update the existing SA with the desired new annotations
There was a problem hiding this comment.
The logic in this PR makes sure that the desired annotations (set by the operator) are enforced and the user-defined annotations are not removed.
There was a problem hiding this comment.
can you add a test for the serviceaccount annotation updating to verify that in here?
| @@ -198,6 +204,12 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) { | |||
| "app.kubernetes.io/managed-by": "opentelemetry-operator", | |||
| "app.kubernetes.io/part-of": "opentelemetry", | |||
| }) | |||
|
|
|||
| sa := &v1.ServiceAccount{} | |||
There was a problem hiding this comment.
can you update the collector we update here to add an annotation as well to confirm that the SA gets that new annotation as well?
i.e. we apply addedMetadataDeployment initially and then deploymentExtraPorts, we should add another annotation to deploymentExtraPorts to verify that new annotations propagate to the existing SA
There was a problem hiding this comment.
I think the test already covers what was changed in the PR.
There was a problem hiding this comment.
not entirely – my worry is that on updates we will no longer be able to add new annotations to service accounts because we no longer take the desired SA annotations and put them on the existing SA. this is good for external systems that are adding annotations as they will not be overridden, but would result in an odd user experience for ourselves where users can not add new annotations to their existing SA via the otelcol CR.
There was a problem hiding this comment.
The single line this PR removes ensures that user-defined annotations are added as well.
There was a problem hiding this comment.
I have added an extra annotation to deploymentExtraPorts and it is propagated to the SA after the update
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Description:
Allow custom annotations on service accounts. OpenShift 4.16 adds an annotation (
openshift.io/internal-registry-pull-secret-ref: simplest-collector-dockercfg-jwq66) to SA which then configures pull secret.See https://docs.openshift.com/container-platform/4.16/release_notes/ocp-4-16-release-notes.html (
Legacy service account API token secrets are no longer generated for each service account)Link to tracking Issue(s):
Testing:
Documentation: