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

bgpv1: Add BGP peer password #23759

Merged
merged 1 commit into from Oct 11, 2023
Merged

bgpv1: Add BGP peer password #23759

merged 1 commit into from Oct 11, 2023

Conversation

dgl
Copy link
Contributor

@dgl dgl commented Feb 15, 2023 •
edited

Add 'authSecret' to the BGP peering policies CRD.

This makes it possible to provide passwords for BGP peers through a Secret and CiliumBGPPeeringPolicy.

Support BGP passwords in the Go BGP implementation.

@dgl dgl requested review from a team as code owners February 15, 2023 00:01
@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 Feb 15, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Feb 15, 2023
dylandreimerink
dylandreimerink previously requested changes Feb 16, 2023
View reviewed changes
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

First of all, thank you very much for this contribution.

While this is a sorely needed feature, I think this implementation is not secure enough. Having the password in plain text in the policy seems irresponsible. I would suggest updating this PR so the peering policy holds the name+key of a secret. Then retrieve this secret from the API server when adding the neighbor.

We already have facilities for getting secrets from the API server which are used in other locations in the code base:

cilium/pkg/crypto/certificatemanager/certificate_manager.go

Line 42 in e8b9edb

func (m *Manager) GetSecrets(ctx context.Context, secret *api.Secret, ns string) (string, map[string][]byte, error) {

@youngnick
Copy link
Contributor

@dgl this one's been sitting for a while, are you still interested in working on it?

@dgl
Copy link
Contributor Author

dgl commented Mar 21, 2023

Yes, I'm looking at adding secret fetch support, should have something soon.

@dgl dgl requested review from a team as code owners April 6, 2023 07:24
@maintainer-s-little-helper
Copy link

Commit 57971eba4a762260f5ea34678146f30e44c4c4c9 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 6, 2023
@dgl
Copy link
Contributor Author

dgl commented Apr 6, 2023

I've pushed an update with the ability to read secrets. There's currently no integration test for BGP, so I'm not sure how best to test this in an automated way, any feedback welcome. It also needs some docs.

@pchaigno pchaigno added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 6, 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 Apr 6, 2023
@pchaigno pchaigno added release-note/major This PR introduces major new functionality to Cilium. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Apr 6, 2023
@mikejoh mikejoh mentioned this pull request Apr 10, 2023
Closed
@tommyp1ckles
Copy link
Contributor

K8s changes make sense to me, but im a bit unfamiliar with the BGP feature.

@dylandreimerink any thoughts on the CRD schema changes?

tommyp1ckles
tommyp1ckles reviewed Apr 25, 2023
View reviewed changes
pkg/bgpv1/cell.go Show resolved Hide resolved
pkg/bgpv1/gobgp/server.go Outdated Show resolved Hide resolved
pkg/bgpv1/agent/controller.go Outdated Show resolved Hide resolved
pkg/bgpv1/agent/controller.go Outdated Show resolved Hide resolved
ti-mo
ti-mo reviewed Oct 2, 2023
View reviewed changes
pkg/option/config.go Outdated Show resolved Hide resolved
danehans
danehans reviewed Oct 2, 2023
View reviewed changes
Copy link
Contributor

@danehans danehans left a comment

Choose a reason for hiding this comment

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

@dgl thank you for working on this feature. When you have a moment, PTAL at my review comments.

pkg/k8s/apis/cilium.io/v2alpha1/bgpp_types.go Outdated Show resolved Hide resolved
pkg/option/config.go Outdated Show resolved Hide resolved
YutaroHayakawa
YutaroHayakawa approved these changes Oct 3, 2023
View reviewed changes
Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

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

Now LGTM 👍 Thanks for your contribution!

danehans
danehans reviewed Oct 4, 2023
View reviewed changes
pkg/bgpv1/types/bgp.go Outdated Show resolved Hide resolved
@dgl dgl force-pushed the bgp-auth branch 2 times, most recently from 3aeb41c to 797ac15 Compare October 6, 2023 06:47
@danehans
Copy link
Contributor

danehans commented Oct 6, 2023

/test

@dgl
Copy link
Contributor Author

dgl commented Oct 9, 2023 •
edited

/test

1 similar comment
@YutaroHayakawa
Copy link
Member

/test

rastislavs
rastislavs approved these changes Oct 9, 2023
View reviewed changes
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 👍 Thanks!

@YutaroHayakawa
Copy link
Member

YutaroHayakawa commented Oct 10, 2023 •
edited

This needs another rebase to deal with CI failure: #28462 😞

@YutaroHayakawa YutaroHayakawa added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. and removed dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. labels Oct 10, 2023
@dgl
bgpv1: Add BGP peer password support
633b5f1 
Add 'authSecretRef' to the BGP peering policies CRD which specifies a
secret which contains a password.

This also adds support for creating the namespace for BGP secrets to the
Helm chart.

Signed-off-by: David Leadbeater <dgl@dgl.cx>
@YutaroHayakawa
Copy link
Member

/test

@YutaroHayakawa
Copy link
Member

YutaroHayakawa commented Oct 11, 2023 •
edited

Conformance K8s Kind: New flake #28510
Cilium IPsec upgrade: #28088
Travis CI: Likely to be a flake. Couldn't reproduce in local #28511

@YutaroHayakawa
Copy link
Member

YutaroHayakawa commented Oct 11, 2023 •
edited

The IPSec upgrading test is failing consistently at the main branch, and the fix is going on with #28088.

https://github.com/cilium/cilium/actions/workflows/tests-ipsec-upgrade.yaml

I don't see the scenario that this PR affects IPSec, and at least for some cases, it is succeeding, so it's safe to ignore the failure for the 5.10 case.

Reviews are in and the rest of the CIs are passing. Making this ready to merge.

@dgl

@YutaroHayakawa YutaroHayakawa added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. labels Oct 11, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 11, 2023
@squeed squeed merged commit f50d292 into cilium:main Oct 11, 2023
60 of 61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danehans danehans danehans left review comments

@ti-mo ti-mo ti-mo left review comments

@kaworu kaworu kaworu approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@asauber asauber asauber approved these changes

@lambdanis lambdanis lambdanis approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

@YutaroHayakawa YutaroHayakawa YutaroHayakawa approved these changes

@rastislavs rastislavs rastislavs approved these changes

@dylandreimerink dylandreimerink dylandreimerink left review comments

@aspsk aspsk Awaiting requested review from aspsk aspsk was automatically assigned from cilium/sig-lb

Assignees
No one assigned
Labels
area/bgp kind/community-contribution This was a contribution made by a community member. release-note/major This PR introduces major new functionality to Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet