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: Added support for existing Cilium SPIRE NS #29032

Merged
merged 1 commit into from Dec 4, 2023
Merged

helm: Added support for existing Cilium SPIRE NS #29032

merged 1 commit into from Dec 4, 2023

Conversation

PhilipSchmid
Copy link
Contributor

Added additional existingNamespace bool flag to configure if the SPIRE namespace already exists or not. If it's an existing one, we should not include our Cilium SPIRE namespace template as this causes the Helm installation to fail. Helm then complains about Namespace "xxx" in namespace "" exists and cannot be imported into the current release.

Please also backport this PR to 1.14.

Please ensure your pull request adheres to the following guidelines:

  • 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.
  • 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.
  • Thanks for contributing!
helm: Added support for existing Cilium SPIRE NS

@PhilipSchmid PhilipSchmid requested review from a team as code owners November 7, 2023 13:27
@maintainer-s-little-helper 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 Nov 7, 2023
@nebril nebril added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Nov 7, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.4 Nov 7, 2023
@nebril nebril added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Nov 7, 2023
@maintainer-s-little-helper 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 Nov 7, 2023
@nebril
Copy link
Member

nebril commented Nov 7, 2023

/test

meyskens
meyskens approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@meyskens meyskens left a comment

Choose a reason for hiding this comment

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

LGTM

@thorn3r thorn3r added this to Needs backport from main in 1.14.5 Nov 9, 2023
@thorn3r thorn3r removed this from Needs backport from main in 1.14.4 Nov 9, 2023
@meyskens
Copy link
Member

You need to run make -C Documentation update-helm-values still 😉

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/support_existing_spire_ns branch from 95230b5 to 61e36d7 Compare November 10, 2023 10:22
@PhilipSchmid
Copy link
Contributor Author

@meyskens Thanks for the input! I just run it and amended the Documentation/helm-values.rst changes.

@meyskens
Copy link
Member

/test

@PhilipSchmid
Copy link
Contributor Author

Is there anything missing for this to be merged?

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/support_existing_spire_ns branch from 61e36d7 to 855ddaa Compare November 30, 2023 11:30
@meyskens
Copy link
Member

meyskens commented Dec 4, 2023

@PhilipSchmid CI is failing on the Helm chart: https://github.com/cilium/cilium/actions/runs/7045839909/job/19176386249?pr=29032#step:3:79

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/support_existing_spire_ns branch from 855ddaa to 9ce6ae5 Compare December 4, 2023 09:49
@PhilipSchmid
helm: Added support for existing Cilium SPIRE NS
4d6c4ba 
Added additional `existingNamespace` bool flag to configure if the SPIRE
`namespace` already exists or not. If it's an existing one, we should not
include our Cilium SPIRE namespace template as this causes the Helm
installation to fail. Helm then complains about `Namespace "xxx" in
namespace "" exists and cannot be imported into the current release`.

Signed-off-by: Philip Schmid <philip.schmid@isovalent.com>
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/support_existing_spire_ns branch from 9ce6ae5 to 4d6c4ba Compare December 4, 2023 09:50
@PhilipSchmid
Copy link
Contributor Author

@meyskens Right, thanks, it failed because I forgot to re-run make -C install/kubernetes after I rebased it the last time 🤦‍♂️.
🤞 that it now runs through.

@meyskens
Copy link
Member

meyskens commented Dec 4, 2023

/test

@aanm aanm added the dont-merge/blocked Another PR must be merged before this one. label Dec 4, 2023
@aanm aanm added dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs and removed dont-merge/blocked Another PR must be merged before this one. labels Dec 4, 2023
@joestringer joestringer removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Dec 4, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 4, 2023
@joestringer
Copy link
Member

This one just missed the cutoff but all reviews are in + CI passed. It looks trivially correct so makes sense to get in before the first v1.15 RC.

@joestringer joestringer added this pull request to the merge queue Dec 4, 2023
@joestringer
Copy link
Member

We typically wouldn't backport something like this since it's adding support for a new mode. IIRC external SPIRE was out of scope for v1.14, and it's also a beta feature. Dropping backport labels. This should be part of the next v1.15 release candidate.

@joestringer joestringer removed the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Dec 4, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from main in 1.14.5 Dec 4, 2023
@joestringer joestringer added the affects/v1.14 This issue affects v1.14 branch label Dec 4, 2023
Merged via the queue into cilium:main with commit 57a6089 Dec 4, 2023
61 checks passed
@PhilipSchmid PhilipSchmid deleted the pr/philip/support_existing_spire_ns branch December 5, 2023 08:01
@joestringer joestringer mentioned this pull request Dec 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nebril nebril nebril approved these changes

@meyskens meyskens meyskens approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

Assignees
No one assigned
Labels
affects/v1.14 This issue affects v1.14 branch ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@PhilipSchmid @nebril @meyskens @joestringer @tommyp1ckles @aanm