-
Notifications
You must be signed in to change notification settings - Fork 882
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
Store BGP passwords in secrets #1264
Conversation
65faa9b
to
e989afc
Compare
@sabinaaledort mind reviewing this? |
internal/config/config.go
Outdated
} | ||
password = string(dstPass) | ||
} else { | ||
return nil, fmt.Errorf("invalid secret specified in the secret %q", p.Spec.SecretRef) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we give more info in the err msg? maybe mentioning the type mismatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, done.
api/v1beta1/bgppeer_types.go
Outdated
@@ -81,6 +82,10 @@ type BGPPeerSpec struct { | |||
// +optional | |||
Password string `json:"password,omitempty" yaml:"password,omitempty"` | |||
|
|||
// secretRef is name of the authentication secret for BGP Peer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secretRef is the name or a reference? i'm still not sure how it's going to look like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed it to use corev1.SecretReference
and added few unit tests. hope that explains it clearly.
looks good. the only thing i'm not sure about is how this field is going to look like. maybe worth adding an example or a unit test? (or maybe it's just me that don't get it) |
internal/config/config.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
if secret.Type == corev1.SecretTypeOpaque || secret.Type == corev1.SecretTypeBasicAuth { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If basicauth works, I'd stick with just it. It's easier to document instead of needing to explain how the secret must look like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
manifests/metallb-frr.yaml
Outdated
@@ -577,6 +577,7 @@ rules: | |||
- services | |||
- endpoints | |||
- nodes | |||
- secrets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means the speaker will be able to read the secrets of all the namespaces, which is too much.
I'd move it to the Role section, so we limit ourselves to our namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried with having secrets in Role, but below error occurred from reconciler while listing secrets from metallb-system namespace, so still holding it in cluster role.
E0317 23:21:27.475103 100 reflector.go:138] pkg/mod/k8s.io/client-go@v0.20.2/tools/cache/reflector.go:167: Failed to watch *v1.Secret: failed to list *v1.Secret: secrets is forbidden: User "system:serviceaccount:metallb-system:speaker" cannot list resource "secrets" in API group "" at the cluster scope
Any idea what can be done to avoid this ?
api/v1beta1/bgppeer_types.go
Outdated
@@ -81,6 +82,10 @@ type BGPPeerSpec struct { | |||
// +optional | |||
Password string `json:"password,omitempty" yaml:"password,omitempty"` | |||
|
|||
// secretRef is name of the authentication secret for BGP Peer | |||
// +optional | |||
SecretRef *v1.LocalObjectReference `json:"secretRef,omitempty" yaml:"secretRef,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found out that there's a corev1.SecretReference which is maybe better https://pkg.go.dev/k8s.io/api/core/v1#SecretReference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'd add this in the v1beta2 version (which #1245 is adding)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good find! done.
api/v1beta1/bgppeer_types.go
Outdated
@@ -81,6 +82,10 @@ type BGPPeerSpec struct { | |||
// +optional | |||
Password string `json:"password,omitempty" yaml:"password,omitempty"` | |||
|
|||
// secretRef is name of the authentication secret for BGP Peer | |||
// +optional | |||
SecretRef *v1.LocalObjectReference `json:"secretRef,omitempty" yaml:"secretRef,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rename this as PasswordSecret, meaning the secret that holds the password.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
internal/config/config.go
Outdated
@@ -169,7 +175,7 @@ func validateHoldTime(ht time.Duration) error { | |||
} | |||
|
|||
// Parse loads and validates a Config from bs. | |||
func For(resources ClusterResources, validate Validate) (*Config, error) { | |||
func For(ctx context.Context, cli client.Client, resources ClusterResources, validate Validate) (*Config, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is pure, meaning it has no side effects and the result does not depend on the system state.
It's important that stays this way, because otherwise we'd need to inject a mock of the client to test it.
So, we need to collect the secrets upfront and pass them here, in a way or another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, done.
internal/config/config.go
Outdated
@@ -319,9 +325,35 @@ func peerFromCR(p metallbv1beta1.BGPPeer) (*Peer, error) { | |||
} | |||
} | |||
|
|||
if p.Spec.Password != "" && p.Spec.SecretRef != nil { | |||
return nil, fmt.Errorf("can not have both password and secret ref set in the peer config") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return the peer name, it's gonna be easier for the user to understand which bgppeer is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
internal/config/config.go
Outdated
srcPass []byte | ||
ok bool | ||
) | ||
if srcPass, ok = secret.Data["password"]; !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd wrap the parsing in a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
e2etest/pkg/config/update.go
Outdated
peer, err := o.peerToCR(p) | ||
secretName := fmt.Sprintf("bgppeer-%d-%d-secret", p.ASN, p.Port) | ||
existing := &corev1.Secret{} | ||
err := o.Client.Get(context.TODO(), types.NamespacedName{Name: secretName, Namespace: o.namespace}, existing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to ensure both work, so I'd add a specific test that just fills the secret and checks the session is established (as opposed to changing all the tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, added specific e2e tests to cover this.
internal/config/config.go
Outdated
@@ -319,9 +325,35 @@ func peerFromCR(p metallbv1beta1.BGPPeer) (*Peer, error) { | |||
} | |||
} | |||
|
|||
if p.Spec.Password != "" && p.Spec.SecretRef != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a unit test that verifies this fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, added relevant unit tests to cover this change.
e989afc
to
10950ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for great design inputs ! rebased it with new changes.
api/v1beta1/bgppeer_types.go
Outdated
@@ -81,6 +82,10 @@ type BGPPeerSpec struct { | |||
// +optional | |||
Password string `json:"password,omitempty" yaml:"password,omitempty"` | |||
|
|||
// secretRef is name of the authentication secret for BGP Peer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed it to use corev1.SecretReference
and added few unit tests. hope that explains it clearly.
api/v1beta1/bgppeer_types.go
Outdated
@@ -81,6 +82,10 @@ type BGPPeerSpec struct { | |||
// +optional | |||
Password string `json:"password,omitempty" yaml:"password,omitempty"` | |||
|
|||
// secretRef is name of the authentication secret for BGP Peer | |||
// +optional | |||
SecretRef *v1.LocalObjectReference `json:"secretRef,omitempty" yaml:"secretRef,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good find! done.
api/v1beta1/bgppeer_types.go
Outdated
@@ -81,6 +82,10 @@ type BGPPeerSpec struct { | |||
// +optional | |||
Password string `json:"password,omitempty" yaml:"password,omitempty"` | |||
|
|||
// secretRef is name of the authentication secret for BGP Peer | |||
// +optional | |||
SecretRef *v1.LocalObjectReference `json:"secretRef,omitempty" yaml:"secretRef,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
e2etest/pkg/config/update.go
Outdated
peer, err := o.peerToCR(p) | ||
secretName := fmt.Sprintf("bgppeer-%d-%d-secret", p.ASN, p.Port) | ||
existing := &corev1.Secret{} | ||
err := o.Client.Get(context.TODO(), types.NamespacedName{Name: secretName, Namespace: o.namespace}, existing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, added specific e2e tests to cover this.
internal/config/config.go
Outdated
@@ -169,7 +175,7 @@ func validateHoldTime(ht time.Duration) error { | |||
} | |||
|
|||
// Parse loads and validates a Config from bs. | |||
func For(resources ClusterResources, validate Validate) (*Config, error) { | |||
func For(ctx context.Context, cli client.Client, resources ClusterResources, validate Validate) (*Config, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, done.
internal/config/config.go
Outdated
@@ -319,9 +325,35 @@ func peerFromCR(p metallbv1beta1.BGPPeer) (*Peer, error) { | |||
} | |||
} | |||
|
|||
if p.Spec.Password != "" && p.Spec.SecretRef != nil { | |||
return nil, fmt.Errorf("can not have both password and secret ref set in the peer config") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
internal/config/config.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
if secret.Type == corev1.SecretTypeOpaque || secret.Type == corev1.SecretTypeBasicAuth { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
internal/config/config.go
Outdated
srcPass []byte | ||
ok bool | ||
) | ||
if srcPass, ok = secret.Data["password"]; !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
internal/config/config.go
Outdated
} | ||
password = string(dstPass) | ||
} else { | ||
return nil, fmt.Errorf("invalid secret specified in the secret %q", p.Spec.SecretRef) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, done.
manifests/metallb-frr.yaml
Outdated
@@ -577,6 +577,7 @@ rules: | |||
- services | |||
- endpoints | |||
- nodes | |||
- secrets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried with having secrets in Role, but below error occurred from reconciler while listing secrets from metallb-system namespace, so still holding it in cluster role.
E0317 23:21:27.475103 100 reflector.go:138] pkg/mod/k8s.io/client-go@v0.20.2/tools/cache/reflector.go:167: Failed to watch *v1.Secret: failed to list *v1.Secret: secrets is forbidden: User "system:serviceaccount:metallb-system:speaker" cannot list resource "secrets" in API group "" at the cluster scope
Any idea what can be done to avoid this ?
271e8e0
to
bc3ebf9
Compare
e2etest/pkg/metallb/config.go
Outdated
} | ||
|
||
// PeersForContainers2 returns the metallb config peers related to the given containers. | ||
func PeersForContainers2(containers []*frrcontainer.FRR, ipFamily ipfamily.Family, useSecret bool) []config.Peer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion - maybe a more meaningful function name here?
46584e8
to
5643582
Compare
internal/config/config.go
Outdated
} else if p.Spec.PasswordSecret.Name != "" { | ||
var secret corev1.Secret | ||
var ok bool | ||
if secret, ok = passwordSecrets[p.Spec.PasswordSecret.Name]; !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have the := form and checking if !ok on the next line
secret, ok: = ...
if !ok {
}
internal/config/config.go
Outdated
@@ -370,9 +372,23 @@ func peerFromCR(p metallbv1beta2.BGPPeer) (*Peer, error) { | |||
nodeSels = []labels.Selector{labels.Everything()} | |||
} | |||
|
|||
if p.Spec.Password != "" && p.Spec.PasswordSecret.Name != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about wrapping this all in a passwordForPeer
function ?
internal/config/config.go
Outdated
srcPass []byte | ||
ok bool | ||
) | ||
if srcPass, ok = secret.Data["password"]; !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same:
pass, ok := ...
if !ok {
}
return ctrl.Result{}, err | ||
} | ||
|
||
metallbCRsAndSecrets := config.ClusterResources{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just call this "resources"
Pools: ipPools.Items, | ||
Peers: bgpPeers.Items, | ||
BFDProfiles: bfdProfiles.Items, | ||
L2Advs: l2Advertisements.Items, | ||
BGPAdvs: bgpAdvertisements.Items, | ||
LegacyAddressPools: addressPools.Items, | ||
} | ||
metallbCRsAndSecrets.PasswordSecrets = make(map[string]corev1.Secret) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd wrap this (and the List above) in a func to put in the bottom of the file, and then use the result when you do the literal initializer above.
e2etest/pkg/metallb/config.go
Outdated
@@ -74,3 +85,22 @@ func WithRouterID(peers []metallbv1beta2.BGPPeer, routerID string) []metallbv1be | |||
} | |||
return peers | |||
} | |||
|
|||
func BGPPeerSecretRefernces(containers []*frrcontainer.FRR) map[string]corev1.Secret { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: BGPPeerSercretReferences
e2etest/pkg/metallb/config.go
Outdated
} | ||
|
||
// PeersForContainersWithSecret returns the metallb config peers related to the given containers. | ||
func PeersForContainersWithSecret(containers []*frrcontainer.FRR, ipFamily ipfamily.Family, useSecret bool) []metallbv1beta2.BGPPeer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use a vararg with a modifier function, pretty much like we do in
metallb/e2etest/pkg/service/create.go
Line 20 in 10a0249
func CreateWithBackend(cs clientset.Interface, namespace string, jigName string, tweak ...func(svc *corev1.Service)) (*corev1.Service, *e2eservice.TestJig) { |
and expose a WithSecret
func that does the modification.
5643582
to
1e364f5
Compare
} | ||
var password string | ||
if p.Spec.Password != "" { | ||
password = p.Spec.Password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just return here
@@ -74,3 +78,22 @@ func WithRouterID(peers []metallbv1beta2.BGPPeer, routerID string) []metallbv1be | |||
} | |||
return peers | |||
} | |||
|
|||
func BGPPeerSecretReferences(containers []*frrcontainer.FRR) map[string]corev1.Secret { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can return a slice, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be still be a map to align with PasswordSecrets in ClusterResources, I chose map over slice as its mainly used for a secret lookup for secret ref in bgp peer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
@pperiyasamy #1287 was merged, you can now make the secrets namescoped too |
1e364f5
to
3e70452
Compare
This makes BGPPeer refer to k8s secret object of type kubernetes.io/basic-auth contains only with password. It enables BGP peer password stored as a secret instead of plain text. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
This consumes newly introduced PasswordSecret field in the speaker when provided and it also honors existing password field. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
This validates BGP pairing with router container when BGPPeer CR uses secret ref for storing BGP peer password. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
Since there is no need for secrets to be in cluster role, this commit moves that into namespaced scope. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
3e70452
to
782d134
Compare
Thanks @fedepaol for the PR #1287, now rebased it accordingly. |
LGTM, thanks @pperiyasamy ! |
In the event that one uses a secret ref:
|
Hi @junkiebev , thanks for the feedback! You are right, the documentation is lacking details about this change, as it slipped under the other changes of the last release. |
provides a feature to store bgp peer passwords in secrets instead of plain text.