ztunnel/helm: move ztunnel daemonset management from operator to helm#43763
ztunnel/helm: move ztunnel daemonset management from operator to helm#43763ldelossa merged 1 commit intocilium:mainfrom
Conversation
50a408d to
035ea33
Compare
rgo3
left a comment
There was a problem hiding this comment.
Did you accidentally remove the changes to the values.yaml? I thought I've seen it on your first push 🤔
Also this allows very fine grained configuration of ztunnel. AFAIK we wanted to keep configurability to a minimum for now so that users can't really "bring their own ztunnel" cc @ldelossa
035ea33 to
2e32b53
Compare
|
@rgo3 It is fixed now, don't know what happened there 😅, needed to update the values.yaml.tmpl file anyway. For the customization concerns, I think we need to have at least the image and the env vars configurable so that it is easier for testing/releasing. |
2e32b53 to
1051eaf
Compare
|
LGTM, but I will defer to @rgo3 for the amount of configurability in the PR as I don't have enough context for this. |
rgo3
left a comment
There was a problem hiding this comment.
I'm by no means experienced with helm charts, but looking at our other helm deployments this seems to do roughly the same things. I've pointed out some config nobs where I'm not sure if it makes sense to make them configurable given the cilium agent itself hardcodes these values. If you can make a case for them to stay I'm not going to object, but please see my comments for details.
Thanks for working on this @nddq
8f6efa8 to
2b1b453
Compare
|
/test |
3a309cb to
9801114
Compare
|
/test |
Head branch was pushed to by a user without write access
9801114 to
d78c17d
Compare
Move the ztunnel daemonset from being managed by a controller in the operator to being managed declaratively via Helm templates. This aligns ztunnel with other components like envoy and node-init that are already managed via Helm. Changes: - Add new Helm templates for ztunnel (daemonset, secret, serviceaccount) - Add ztunnel configuration under encryption section in values.yaml - Add ztunnel serviceAccount configuration in values.yaml - Remove ztunnel daemonset controller from operator (controller.go) - Remove embedded ztunnel-daemonset.yaml from operator - Keep ztunnel config cell for enable-ztunnel flag The ztunnel resources are now conditionally deployed when: - encryption.enabled=true - encryption.type=ztunnel Signed-off-by: Quang Nguyen <nguyenquang@microsoft.com>
d78c17d to
733441e
Compare
rgo3
left a comment
There was a problem hiding this comment.
Did you manually request the re-review or does this somehow happen after a force-push now @nddq? From the cilium slack I take it the latest change was just a rebase to get CI green and usually rebasing shouldn't trigger another round of getting all reviews. I'm a bit confused why I need to approve it for the 3rd time.
|
@rgo3 yeah it was a force push for a rebase, but GitHub showed the prompt that I can re-request for a review, so I assumed that a reapproval is needed |
|
@nddq yeah no need for us to re-review on a simple force push to rebase against HEAD. :) this is also the third time I'm approving this PR lol |
|
/test |
Move the ztunnel daemonset from being managed by a controller in the operator to being managed declaratively via Helm templates. This aligns ztunnel with other components like envoy and node-init that are already managed via Helm.
Changes:
The ztunnel resources are now conditionally deployed when:
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Fixes: <commit-id>tag, thenplease add the commit author[s] as reviewer[s] to this issue.
Fixes: #issue-number