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.

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

bgpv1: Use kube-system namespace by default for MD5 secret #29478

Merged

Conversation

YutaroHayakawa
Copy link
Member

Currently, BGP Control Plane uses cilium-bgp-secrets namespace by default to fetch MD5 password authentication. Also, if not specified, it creates a new namespace by default.

However, the rest of the cilium-related secrets such as cilium-ca, hubble certificates, or clustermesh certificates are in kube-system namespace. So, having a new namespace is inconsistent with rest of the system, and also inconvenient since users need to manage one more namespace.

This commit addresses the issue by changing the default value of bgpControlPlane.secretNamespace.name to kube-system and bgpControlPlane.secretNamespace.create to false. Users can always change the namespace if they wish to have an isolation.

I put release-note/minor because this BGP MD5 feature is only released in v1.15-pre releases.

bgpv1: Use kube-system namespace by default for MD5 secret

@YutaroHayakawa YutaroHayakawa added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/bgp labels Nov 29, 2023
@YutaroHayakawa YutaroHayakawa requested review from a team and rastislavs and removed request for a team November 29, 2023 10:10
@YutaroHayakawa YutaroHayakawa marked this pull request as ready for review November 29, 2023 13:55
@YutaroHayakawa YutaroHayakawa requested review from a team as code owners November 29, 2023 13:55
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.

In terms of Helm structure, the change looks good to me. I'll defer the discussion of what namespace to use to others. - it seems we're not consistent across different components (BGP vs Hubble/ClusterMesh vs Ingress/GatewayAPI)

@YutaroHayakawa
Copy link
Member Author

/test

@YutaroHayakawa YutaroHayakawa force-pushed the bgp-md5-password-namespace-default branch from 2d65875 to 19b2a1f Compare November 29, 2023 15:01
@YutaroHayakawa
Copy link
Member Author

Rebased on top of the main to pull-in a2694fc

Copy link
Contributor

@rastislavs rastislavs left a comment

Choose a reason for hiding this comment

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

LGTM

@YutaroHayakawa
Copy link
Member Author

Hmm, Quay looks under the weather... Let me retry.

Currently, BGP Control Plane uses cilium-bgp-secrets namespace by
default to fetch MD5 password authentication. Also, if not specified, it
creates a new namespace by default.

However, the rest of the cilium-related secrets such as cilium-ca,
hubble certificates, or clustermesh certificates are in kube-system
namespace. So, having a new namespace is inconsistent with rest of the
system, and also inconvenient since users need to manage one more
namespace.

This commit addresses the issue by changing the default value of
`bgpControlPlane.secretNamespace.name` to `kube-system` and
`bgpControlPlane.secretNamespace.create` to `false`. Users can always
change the namespace if they wish to have an isolation.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa YutaroHayakawa force-pushed the bgp-md5-password-namespace-default branch from 19b2a1f to 6b69064 Compare November 29, 2023 15:39
@YutaroHayakawa
Copy link
Member Author

Forgot to update cmdref 🤦 Force pushed.

@YutaroHayakawa
Copy link
Member Author

/test

Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM, nice work.

For everyone's info: the cilium-secrets namespace is a bit of a different use case, in that it's a place where Secrets relevant to Ingress or Gateway API get synced to, which means that the Agent doesn't need whole-cluster Secret read access (which is way bigger than what it needs).

So, that namespace, although named generically, really needs to only be used for automatically-synced Secrets (which is only used in Ingress and Gateway API at the moment).

@YutaroHayakawa
Copy link
Member Author

ci-e2e-upgrade: #29484
ci-clustermesh: #29499
ci-ginkgo: Docker rate limit
ci-ipsec-e2e: Docker rate limit
ci-runtime: Quay 502 error

@YutaroHayakawa
Copy link
Member Author

ci-clustermesh: #28622

@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 Nov 30, 2023
@youngnick youngnick added this pull request to the merge queue Nov 30, 2023
Merged via the queue into cilium:main with commit d199a07 Nov 30, 2023
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bgp 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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants