Do not propagate managed-by annotation to Pods #8846
Conversation
tekton-robot
added
kind/bug
tekton-robot
added
the
size/S
|
The following is the coverage report on the affected files.
|
Let's make sure we are not propagating `app.kubernetes.io/managed-by` annotation to Pods. Doing it can lead to weird behavior, for example with applied with Helm, or if some admission controller is looking for pipeline's Pods and do not get it if the `TaskRun` or `PipelineRun` was created with an `app.kubernetes.io/managed-by`. Signed-off-by: Vincent Demeester <[email protected]>
vdemeester
force-pushed
the
8827-no-managedby-on-pods
branch
from
d23e33c to
07bcc83
Compare
|
The following is the coverage report on the affected files.
|
twoGiants
left a comment
twoGiants
left a comment
There was a problem hiding this comment.
Looks good 😸 👍
I found one typo and have a question/idea about the location of the change. It can be left as it is but maybe there is a better place, see my comment below.
| // KubectlLastAppliedAnnotationKey is the key used by kubectl to store its last applied configuration (using kubectl apply) | ||
| KubectlLastAppliedAnnotationKey = "kubectl.kubernetes.io/last-applied-configuration" | ||
|
|
||
| // KubernetesLastAppliedAnnotationKey is the key used by tools to tell who is managing an object |
There was a problem hiding this comment.
Small typo: // KubernetesLastAppliedAnnotationKey => // KubernetesManagedByAnnotationKey
| labels[pipeline.TaskRunLabelKey] = s.Name | ||
| labels[pipeline.TaskRunUIDLabelKey] = string(s.UID) | ||
| // Enforce app.kubernetes.io/managed-by to be the value configured | ||
| labels[tknreconciler.KubernetesManagedByAnnotationKey] = defaultManagedByLabelValue |
There was a problem hiding this comment.
Just thinking out loud: could this live in the pipelinerun reconciler when the TaskRun is created with it's labels and annotations? For example here in getTaskRunLabels line 1408. Then those labels should be correctly propagated down to the pod right?
And the same for excluding the annotation. You could enforce it here in the getTaskrunAnnotations line 1358.
Then no change to pod would be necessary and the source of truth remains the place where the labels and annotations are set in the first place.
Wdyt?
There was a problem hiding this comment.
I am not sure, for me no matter what is there on TaskRun and PipelineRun, we really do not want this particular label to be propagated. So the quickest path to it was to do it when we create the Pod.
There was a problem hiding this comment.
You could also add a test which checks that the Annotation was not set.
And because this PR resolves a bug we could add a comment with the issue reference to the added tests mentioning that those test checks against a resolved bug from #8827.
| }, | ||
| Annotations: podAnnotations, | ||
| Labels: makeLabels(taskRun), | ||
| Labels: makeLabels(taskRun, defaultManagedByLabelValue), |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: twoGiants, waveywaves 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 |
tekton-robot
added
the
approved
|
/lgtm |
4 participants
Changes
Closes #8827
Let's make sure we are not propagating
app.kubernetes.io/managed-byannotation to Pods. Doing it can lead to weird behavior, for example
with applied with Helm, or if some admission controller is looking for
pipeline's Pods and do not get it if the
TaskRunorPipelineRunwas created with an
app.kubernetes.io/managed-by.Signed-off-by: Vincent Demeester [email protected]
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes