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.

Sign up for GitHub

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

Jump to bottom

helm: default to cronJob as tls.auto.method #25620

Closed
wants to merge 4 commits into from
Closed

helm: default to cronJob as tls.auto.method #25620

wants to merge 4 commits into from

Conversation

kaworu
Copy link
Member

@kaworu kaworu commented May 23, 2023

For both Hubble and Clustermesh. Deprecate the previous default helm method on the way.

See first task of #25345.

@kaworu kaworu added kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. area/helm Impacts helm charts and user deployment experience labels May 23, 2023
@kaworu kaworu requested review from a team as code owners May 23, 2023 13:08
@kaworu kaworu mentioned this pull request May 23, 2023
Open
5 tasks
@kaworu kaworu force-pushed the pr/kaworu/tls-default-to-certgen branch from e6ae6e2 to 0f5ec27 Compare May 23, 2023 13:34
rolinh
rolinh approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for tackling this!

gandro
gandro approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

I'm fine with the change as is, but if we make cronJob the default, we likely also need to figure out how helm uninstall is supposed to work in cronJob mode before the feature freeze in a couple of weeks: cilium/cilium-cli#1648

@giorio94
Copy link
Member

I haven't yet looked through this PR, but FYI #25261 should be merged soon, and it will remove the latest occurrences of hubble.tls.ca.

giorio94
giorio94 approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks! I just left a couple of nits inline. I guess that you should be able to drop the first commit once #25261 gets merged.

install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved
install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved
install/kubernetes/cilium/values.yaml.tmpl Show resolved Hide resolved
zacharysarah
zacharysarah approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

A couple of nits, otherwise LGTM. Approving with the understanding that changes must be made prior to merge.

install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved
install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved
@michi-covalent
Copy link
Contributor

is this test failure related to this pull request? 👀 https://github.com/cilium/cilium/actions/runs/5057980055/jobs/9077443138?pr=25620

michi-covalent
michi-covalent approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

let's harmonize 🧘

@kaworu kaworu force-pushed the pr/kaworu/tls-default-to-certgen branch from 0f5ec27 to dc5abc4 Compare May 24, 2023 16:35
@kaworu kaworu requested a review from a team as a code owner May 24, 2023 16:46
@kaworu
Copy link
Member Author

kaworu commented May 24, 2023 •
edited

@gandro @giorio94 thanks! Addressed all the nits/typos and added a couple more cleanup commits, please take another look.

I'm fine with the change as is, but if we make cronJob the default, we likely also need to figure out how helm uninstall is supposed to work in cronJob mode before the feature freeze in a couple of weeks: cilium/cilium-cli#1648

is this test failure related to this pull request? eyes https://github.com/cilium/cilium/actions/runs/5057980055/jobs/9077443138?pr=25620

@gandro @michi-covalent yes so my understanding is that the cilium-cli assumes the helm method (see here) and consequently is not compatible with certgen. The best way forward might be to "teach" certgen to the cilium-cli so that we can drop the helm method eventually, but as @gandro pointed out the problem of generated resource ownership described at cilium/cilium-cli#1648 needs to be tackled (both in the context of the cilium-cli and helm if I understand correctly). Thoughts?

@kaworu kaworu marked this pull request as draft May 24, 2023 16:55
@kaworu
Copy link
Member Author

kaworu commented May 24, 2023

Converted to draft until the cilium-cli compatibility issue is sorted out.

@kaworu kaworu removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Nov 16, 2023
@kaworu kaworu force-pushed the pr/kaworu/tls-default-to-certgen branch from 0b691bb to 6940095 Compare November 16, 2023 14:19
@michi-covalent
Copy link
Contributor

/test

@michi-covalent
Copy link
Contributor

@kaworu are you trying to get this in before v1.15 feature freeze? 👀 it's 11/30.

@kaworu
Copy link
Member Author

kaworu commented Nov 22, 2023

@kaworu are you trying to get this in before v1.15 feature freeze? 👀 it's 11/30.

As @gandro mentionned the test failures are legit, and I don't think I'll have cycles to look into them before Nov 30th sadly.

@michi-covalent
Copy link
Contributor

very sad indeed but i understand 🚀🙏

@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Dec 23, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 6, 2024

This pull request has not seen any activity since it was marked stale.
Closing.

@github-actions github-actions bot closed this Jan 6, 2024
@kaworu kaworu reopened this Jan 8, 2024
@github-actions github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 9, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 9, 2024

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Feb 9, 2024
@github-actions GitHub Actions
Copy link

This pull request has not seen any activity since it was marked stale.
Closing.

@github-actions github-actions bot closed this Feb 24, 2024
@asauber asauber reopened this Feb 24, 2024
@github-actions github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Feb 25, 2024
@joestringer joestringer added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Feb 26, 2024
@joestringer
Copy link
Member

@kaworu @asauber is there a plan to move this PR forward? Otherwise if the PR is linked from the parent issue #25345 then we can always find the PR again, so we may as well close this.

@joestringer joestringer marked this pull request as draft February 26, 2024 18:26
@kaworu
helm: deprecate the helm auto tls method
5e092ef 
This method was introduced as a quick way to get TLS setup for Hubble
and clustermesh before other method were implemented. Because of its
inherent limitations (not gitops friendly, forcing certificate
re-generation on Helm ugprades) the default is going to be changed to
the cronJob method, and thus the helm method is now deprecated and
planned for removal in v1.16.

Signed-off-by: Alexandre Perrin <alex@isovalent.com>
@kaworu
helm: use cronJob as the default auto tls method
cfba9df 
The previous commit deprecated the default helm method, and this commit
promote the cronJob method as the new default.

Signed-off-by: Alexandre Perrin <alex@isovalent.com>
@kaworu
doc: remove stale reference to hubble-ca-cert
9074234 
The hubble-ca-cert ConfigMap was removed by 4635ffa.

Signed-off-by: Alexandre Perrin <alex@isovalent.com>
@kaworu
test: remove hubble-ca-secret from the cleanup list
73a00a5 
All other references to this secret were removed in
fcc927f.

Signed-off-by: Alexandre Perrin <alex@isovalent.com>
@kaworu kaworu force-pushed the pr/kaworu/tls-default-to-certgen branch from 6940095 to 73a00a5 Compare February 28, 2024 16:37
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Mar 30, 2024
@github-actions GitHub Actions
Copy link

This pull request has not seen any activity since it was marked stale.
Closing.

@github-actions github-actions bot closed this Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@joamaki joamaki joamaki approved these changes

@rolinh rolinh rolinh approved these changes

@zacharysarah zacharysarah zacharysarah approved these changes

@michi-covalent michi-covalent michi-covalent approved these changes

@giorio94 giorio94 giorio94 approved these changes

@viktor-kurchenko viktor-kurchenko viktor-kurchenko approved these changes

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@kaworu @giorio94 @michi-covalent @asauber @gandro @joestringer @joamaki @rolinh @zacharysarah @viktor-kurchenko