helm: allow annotations to be set for preflight resources #27860

bradwhitfield · 2023-08-31T19:45:25Z

Currently preflight resources, except the serviceaccount, do not allow annotations to be configured at the top level of the resources. This patch adds support for adding annotations.

Adding annotations at the top level of resources (not just in pod templates) will allow for use cases like ArgoCD and helm hooks.

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Are you a user of Cilium? Please add yourself to the Users doc
Thanks for contributing!

helm: allow annotations to be set for preflight resources

gandro · 2023-09-04T08:45:04Z

Thanks for the PR! Please note that Travis is failing because some files have not been regenerated:

HINT: to fix this, run 'make -C Documentation update-helm-values'

gandro

Thanks! PR and motivation look solid overall. There is an issue with CI (see previous comment) that needs to be fixed, but otherwise this looks good to me

bradwhitfield · 2023-09-07T20:03:43Z

Thanks, @gandro! I've been focused on getting the diff command to work in the Preflight Clusterrole Check that I missed that. It occurred to me that maybe a better approach for this PR is to include the annotations at the top levels of all resources. It does make the diff easier and I feel like a more consistent experience. Any reason not to just add annotations to everything?

youngnick

PR LGTM once the CI is sorted, nice work @bradwhitfield.

gandro · 2023-09-11T08:51:58Z

It occurred to me that maybe a better approach for this PR is to include the annotations at the top levels of all resources. It does make the diff easier and I feel like a more consistent experience. Any reason not to just add annotations to everything?

I'm not sure I entirely understand your proposal. Would the idea be to have global annotations value that applies to all resources (preflight and regular deployments?).

I think that's also a fine approach, though there is always a danger that contributors who add new resources might miss that they need to make the annotations customizable, which could lead to inconsistencies over time. Maybe we should have a CI or lint check for that if we decide to annotate all resources.

bradwhitfield · 2023-09-11T18:35:25Z

Would the idea be to have global annotations value that applies to all resources (preflight and regular deployments?).

So this is part of the diff of what I'm currently working on. I've created an annotations parameter for all of the various templates, if that makes sense.

+# -- Annotations to be added to all top-level cilium-agent objects (resources under templates/cilium-agent)
+annotations: {}
+
 # -- Security Context for cilium-agent pods.
 podSecurityContext: {}
 
@@ -965,6 +968,9 @@ hubble:
   # -- Enable Hubble (true by default).
   enabled: true
 
+  # -- Annotations to be added to all top-level hubble objects (resources under templates/hubble)
+  annotations: {}
+
   # -- Buffer size of the channel Hubble uses to receive monitor events. If this
   # value is not set, the queue size is set to the default monitor queue size.
   # eventQueueSize: ""
@@ -1177,6 +1183,9 @@ hubble:
     # -- Additional hubble-relay environment variables.
     extraEnv: []
 
+    # -- Annotations to be added to all top-level hubble-relay objects (resources under templates/hubble-relay)
+    annotations: {}
+
     # -- Annotations to be added to hubble-relay pods
     podAnnotations: {}
 
@@ -1432,6 +1441,9 @@ hubble:
     # -- The number of replicas of Hubble UI to deploy.
     replicas: 1
 
+    # -- Annotations to be added to all top-level hubble-ui objects (resources under templates/hubble-ui)
+    annotations: {}
+

This does increase the scope of this PR quite a bit, so can revisit this later if that's preferred.

gandro · 2023-09-12T08:52:35Z

So this is part of the diff of what I'm currently working on. I've created an annotations parameter for all of the various templates, if that makes sense.

Ah, thanks for the details! Yeah I think that does make sense too. I'm fine both with extending this PR to add top-level values, or creating a new PR for that.

bradwhitfield · 2023-09-15T18:34:27Z

Apologies for the delay. I've pushed what I came up with. While doing this, I had a couple of notes and questions I wanted to bring up. I can make any changes as needed based on feedback.

tls-cronjob/job.yaml files already had annotations you can control. My thinking is that auto tls should have some sort of annotations across all the resources, so I’m curious to get feedback on how I handled those. For tls-provided resources, I just use the appropriate top-level annotations since you wouldn’t need them to be regenerated in the same way as helm or cert-manager might.
There were a few files that I wasn’t sure the best approach for. Pretty much every file not under template/ folder, I left alone. In my mind, they could either be annotated with the same annotations as the cilium-agent, or they could have unique annotation values. I was initially thinking the first option, but again was curious to get thoughts on that.
In some places where it would have rendered and empty annotations before, I made conditionally generate that field. I can undo this if needed.
Do you want it to be consistent wether labels or annotations are first?

gandro · 2023-09-18T10:12:16Z

This is excellent work! I'll leave a few minor nits in inline comments.

* `tls-cronjob/job.yaml` files already had annotations you can control. My thinking is that auto tls should have some sort of annotations across all the resources, so I’m curious to get feedback on how I handled those. For `tls-provided` resources, I just use the appropriate top-level annotations since you wouldn’t need them to be regenerated in the same way as helm or cert-manager might.

The current solution looks good to me, though I think it might be simpler to use the top-level annotations for all TLS resources, regardless if provided or not.

* There were a few files that I wasn’t sure the best approach for. Pretty much every file not under `template/` folder, I left alone. In my mind, they could either be annotated with the same annotations as the cilium-agent, or they could have unique annotation values. I was initially thinking the first option, but again was curious to get thoughts on that.

Yeah, I think we can always add more annotations if needed. From what I can tell, your current choice looks good to me.

* In some places where it would have rendered and empty `annotations` before, I made conditionally generate that field. I can undo this if needed.

Yeah, the if guards look good to me overall. I think that's a good solution.

* Do you want it to be consistent wether labels or annotations are first?

The PR is already pretty large. I think that is better done in a follow-up PR, but I don't think it's a must-have.

gandro · 2023-09-18T09:30:55Z

Documentation/helm-values.rst

@@ -2472,6 +2472,10 @@
     - Affinity for cilium-preflight
     - object
     - ``{"podAffinity":{"requiredDuringSchedulingIgnoredDuringExecution":[{"labelSelector":{"matchLabels":{"k8s-app":"cilium"}},"topologyKey":"kubernetes.io/hostname"}]}}``
+   * - :spelling:ignore:`preflight.annotations`


nit: Please squash this into the previous commit

gandro · 2023-09-18T09:32:33Z

install/kubernetes/cilium/README.md

@@ -60,12 +60,14 @@ contributors across the globe, there is almost always someone available to help.
 | aksbyocni.enabled | bool | `false` | Enable AKS BYOCNI integration. Note that this is incompatible with AKS clusters not created in BYOCNI mode: use Azure integration (`azure.enabled`) instead. |
 | alibabacloud.enabled | bool | `false` | Enable AlibabaCloud ENI integration |
 | annotateK8sNode | bool | `false` | Annotate k8s node upon initialization with Cilium's metadata. |
+| annotations | object | `{}` | Annotations to be added to all top-level cilium-agent objects (resources under templates/cilium-agent) |


Same as above: Please squash into the previous commit. That way we can use "git bisect" to build without errors

So to make sure I'm clear, should I end up with two commits like these, or just one commit like the bottom one?

helm: allow annotations to be set for preflight resources

helm: allow annotations to be set on nearly all resources

edit:

For the PR feedback, should that be in another commit, or would you like it amended to the helm: allow annotations to be set on nearly all resources commit?

I think just having the two commits that you outlined will yield a clean git history, that sounds good to me. No need to put the PR feedback in a separate commit, since GitHub can show us those diffs too (as long as you don't change the commits and rebase at the same time).

install/kubernetes/cilium/templates/cilium-envoy/service.yaml

install/kubernetes/cilium/templates/cilium-nodeinit/serviceaccount.yaml

install/kubernetes/cilium/templates/clustermesh-apiserver/tls-certmanager/admin-secret.yaml

install/kubernetes/cilium/templates/hubble/tls-certmanager/relay-client-secret.yaml

bradwhitfield · 2023-09-20T20:12:55Z

Thank you for all the feedback! I'm just getting back from being out a few and will work on address this tomorrow.

aditighag · 2023-09-21T18:41:07Z

@bradwhitfield Feel free to move it out of draft once you've addressed all the review comments, and it's ready for review.

gandro

Code-wise this looks good to me now, thanks!

Currently preflight resources, except the serviceaccount, do not allow annotations to be configured at the top level of the resources. This patch adds support for adding annotations. Adding annotations at the top level of resources (not just in pod templates) will allow for use cases like ArgoCD and helm hooks. Signed-off-by: Brad Whitfield <bradswhitfield@gmail.com>

Currently most resources don't allow for setting custom annotations at the top level of the resources. This change allows chart consumers to define annotations for these resources breaking them up by resources under "templates" and additional fine-grained annotations for `tls.auto` resources. Adding annotations at the top level of resources (not just in pod templates) will allow for use cases like ArgoCD and helm hooks. Signed-off-by: Brad Whitfield <bradswhitfield@gmail.com>

…eedback Signed-off-by: Brad Whitfield <bradswhitfield@gmail.com>

bradwhitfield · 2023-09-26T21:07:31Z

I think this is ready. I see that the check failed, but I did amend the commit to fix that. It doesn't appear to have retriggered the build if I'm parsing things correctly. Is there a way I can trigger that?

aditighag

Approving for my codeowners

aditighag · 2023-09-26T21:32:50Z

Review was requested from sig-servicemesh, but no one was specifically assigned for reviews. /cc @cilium/sig-servicemesh
I've marked the PR as ready for merge, but if you have any comments, feel free to post them on the PR.

gandro · 2023-09-27T08:17:19Z

Let's run CI as well, before we merge

gandro · 2023-09-27T08:17:24Z

/test

gandro · 2023-09-28T08:18:31Z

All required tests have passed. The failing tests are not blockers:

checkpatch is complaining about an overly long git commit subject, but that's not a hard requirement
ci-aks timed out https://github.com/cilium/cilium/actions/runs/6323246632/job/17170451500
ci-ipsec-uprade https://github.com/cilium/cilium/actions/runs/6323247394/job/17170446827 hit CI Flake: ci-ipsec-upgrade - no-interrupted-connections (after downgrade) #27827

Marking again as ready to merge. There is an outstanding request from sig-servicemesh, but Nick (who is part of that team) has already reviewed.

install/kubernetes/Makefile

coro · 2023-11-01T14:22:30Z

install/kubernetes/cilium/templates/cilium-agent/serviceaccount.yaml

  annotations:
    {{- toYaml .Values.serviceAccounts.cilium.annotations | nindent 4 }}
  {{- end }}
+  {{- if or .Values.serviceAccounts.cilium.annotations .Values.annotations }}
+  annotations:
+    {{- with .Values.annotations }}
+      {{- toYaml . | nindent 4 }}
+    {{- end }}
+    {{- with .Values.serviceAccounts.cilium.annotations }}
+      {{- toYaml . | nindent 4 }}
+    {{- end }}
+  {{- end }}


@bradwhitfield This is causing a duplicate annotations key if .Values.serviceAccounts.cilium.annotations is specified in the Helm release. Would you mind sending in a PR to fix this?

Fixes cilium#27860 Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com>

Fixes #27860 Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com>

Fixes cilium#27860 Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com>

bradwhitfield requested review from a team as code owners August 31, 2023 19:45

bradwhitfield requested review from youngnick and gandro August 31, 2023 19:45

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 31, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Aug 31, 2023

bradwhitfield force-pushed the preflight-daemonset-annotations branch 2 times, most recently from f5b2658 to 370b4e6 Compare September 1, 2023 13:32

gandro added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/helm Impacts helm charts and user deployment experience labels Sep 4, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 4, 2023

gandro approved these changes Sep 4, 2023

View reviewed changes

youngnick approved these changes Sep 11, 2023

View reviewed changes

bradwhitfield force-pushed the preflight-daemonset-annotations branch from 370b4e6 to 5aac862 Compare September 15, 2023 18:20

bradwhitfield requested review from a team as code owners September 15, 2023 18:20

bradwhitfield requested a review from aditighag September 15, 2023 18:20

gandro requested changes Sep 18, 2023

View reviewed changes

bradwhitfield force-pushed the preflight-daemonset-annotations branch from 4bde1ae to cca13be Compare September 20, 2023 13:56

aditighag marked this pull request as draft September 21, 2023 18:40

gandro approved these changes Sep 25, 2023

View reviewed changes

bradwhitfield added 2 commits September 25, 2023 12:14

bradwhitfield force-pushed the preflight-daemonset-annotations branch from 77b6173 to 9e4b20b Compare September 26, 2023 20:55

bradwhitfield marked this pull request as ready for review September 26, 2023 20:55

helm: simplify auto TLS annotations and various cleanup based on PR f…

dea1d18

…eedback Signed-off-by: Brad Whitfield <bradswhitfield@gmail.com>

bradwhitfield force-pushed the preflight-daemonset-annotations branch from 9e4b20b to dea1d18 Compare September 26, 2023 21:00

aditighag approved these changes Sep 26, 2023

View reviewed changes

aditighag added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 26, 2023

gandro removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 27, 2023

gandro added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 28, 2023

lmb merged commit 44698df into cilium:main Sep 28, 2023

maintainer-s-little-helper bot removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Sep 28, 2023

bradwhitfield deleted the preflight-daemonset-annotations branch September 28, 2023 16:51

joestringer reviewed Oct 31, 2023

View reviewed changes

install/kubernetes/Makefile Show resolved Hide resolved

coro reviewed Nov 1, 2023

View reviewed changes

This was referenced Nov 1, 2023

makefile: add back the sed command to update the logo path #28929

Merged

helm: Fix annotation duplication problems #28978

Merged

marseel added a commit to marseel/cilium that referenced this pull request Nov 16, 2023

Fix pre-flight clusterrole check

91de7aa

Fixes cilium#27860 Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com>

marseel mentioned this pull request Nov 16, 2023

Fix pre-flight clusterrole check #29224

Merged

julianwiedmann pushed a commit that referenced this pull request Nov 20, 2023

Fix pre-flight clusterrole check

3959987

Fixes #27860 Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com>

pjablonski123 pushed a commit to pjablonski123/cilium that referenced this pull request Dec 15, 2023

Fix pre-flight clusterrole check

e0d27fe

Fixes cilium#27860 Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com>

helm: allow annotations to be set for preflight resources #27860

helm: allow annotations to be set for preflight resources #27860

Uh oh!

Conversation

bradwhitfield commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gandro commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gandro left a comment

Choose a reason for hiding this comment

Uh oh!

bradwhitfield commented Sep 7, 2023

Uh oh!

youngnick left a comment

Choose a reason for hiding this comment

Uh oh!

gandro commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradwhitfield commented Sep 11, 2023

Uh oh!

gandro commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradwhitfield commented Sep 15, 2023

Uh oh!

gandro commented Sep 18, 2023

Uh oh!

gandro Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

gandro Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

bradwhitfield Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gandro Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bradwhitfield commented Sep 20, 2023

Uh oh!

aditighag commented Sep 21, 2023

Uh oh!

gandro left a comment

Choose a reason for hiding this comment

Uh oh!

bradwhitfield commented Sep 26, 2023

Uh oh!

aditighag left a comment

Choose a reason for hiding this comment

Uh oh!

aditighag commented Sep 26, 2023

Uh oh!

gandro commented Sep 27, 2023

Uh oh!

gandro commented Sep 27, 2023

Uh oh!

gandro commented Sep 28, 2023

Uh oh!

Uh oh!

coro Nov 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bradwhitfield commented Aug 31, 2023 •

edited

Loading

gandro commented Sep 4, 2023 •

edited

Loading

gandro commented Sep 11, 2023 •

edited

Loading

gandro commented Sep 12, 2023 •

edited

Loading

bradwhitfield Sep 20, 2023 •

edited

Loading