Repository: metallb/metallb
Pull request #1264 (2022-03-15 12:58:39)

Store BGP passwords in secrets
provides a feature to store bgp peer passwords in secrets instead of plain text.


fedepaol: @sabinaaledort mind reviewing this?
sabinaaledort: 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)
fedepaol: Re: using namespaced secrets, I filed https://github.com/metallb/metallb/pull/1287 today, which seems to make the trick.
I'd wait that pr to get in (that in turn depends on https://github.com/metallb/metallb/pull/1275), the changes will happen mostly at manifest level and a few lines in k8s.go.
fedepaol: @pperiyasamy https://github.com/metallb/metallb/pull/1287 was merged, you can now make the secrets namescoped too
pperiyasamy: > @pperiyasamy #1287 was merged, you can now make the secrets namescoped too

Thanks @fedepaol for the PR https://github.com/metallb/metallb/pull/1287, now rebased it accordingly.
fedepaol: LGTM, thanks @pperiyasamy !

Patch:
diff --git a/api/v1beta2/bgppeer_types.go b/api/v1beta2/bgppeer_types.go
index 5364f59e..8954f442 100644
--- a/api/v1beta2/bgppeer_types.go
+++ b/api/v1beta2/bgppeer_types.go
@@ -17,6 +17,7 @@ limitations under the License.
 package v1beta2
 
 import (
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -68,6 +69,10 @@ type BGPPeerSpec struct {
 	// +optional
 	Password string `json:"password,omitempty" yaml:"password,omitempty"`
 
+	// passwordSecret is name of the authentication secret for BGP Peer
+	// +optional
+	PasswordSecret v1.SecretReference `json:"passwordSecret,omitempty" yaml:"passwordSecret,omitempty"`
+
 	BFDProfile string `json:"bfdProfile,omitempty" yaml:"bfdprofile,omitempty"`
 
 	// EBGP peer is multi-hops away
diff --git a/api/v1beta2/groupversion_info.go b/api/v1beta2/groupversion_info.go
index 85f30b3d..f7db7d7f 100644
--- a/api/v1beta2/groupversion_info.go
+++ b/api/v1beta2/groupversion_info.go
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-// Package v1beta1 contains API Schema definitions for the metallb v1beta1 API group.
+// Package v1beta2 contains API Schema definitions for the metallb v1beta1 API group.
 
 // +kubebuilder:object:generate=true
 // +groupName=metallb.io
diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go
index f0617692..59698298 100644
--- a/api/v1beta2/zz_generated.deepcopy.go
+++ b/api/v1beta2/zz_generated.deepcopy.go
@@ -83,6 +83,7 @@ func (in *BGPPeerSpec) DeepCopyInto(out *BGPPeerSpec) {
 			(*in)[i].DeepCopyInto(&(*out)[i])
 		}
 	}
+	out.PasswordSecret = in.PasswordSecret
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new BGPPeerSpec.
diff --git a/charts/metallb/templates/crds.yaml b/charts/metallb/templates/crds.yaml
index 07a55b23..e835fd00 100644
--- a/charts/metallb/templates/crds.yaml
+++ b/charts/metallb/templates/crds.yaml
@@ -593,6 +593,17 @@ spec:
                 description: Authentication password for routers enforcing TCP MD5
                   authenticated sessions
                 type: string
+              passwordSecret:
+                description: Authentication secret reference for routers enforcing
+                   TCP MD5 authenticated sessions
+                properties:
+                  name:
+                    description: Name of the secret object
+                    type: string
+                  namespace:
+                    description: Namespace on which secret object resides
+                    type: string
+                type: object
               peerASN:
                 description: AS number to expect from the remote end of the session.
                 format: int32
diff --git a/charts/metallb/templates/rbac.yaml b/charts/metallb/templates/rbac.yaml
index 66f4efef..48b9bf94 100644
--- a/charts/metallb/templates/rbac.yaml
+++ b/charts/metallb/templates/rbac.yaml
@@ -54,6 +54,9 @@ rules:
 - apiGroups: [""]
   resources: ["pods"]
   verbs: ["list"]
+- apiGroups: [""]
+  resources: ["secrets"]
+  verbs: ["get", "list", "watch"]
 - apiGroups: ["metallb.io"]
   resources: ["addresspools"]
   verbs: ["get", "list", "watch"]
@@ -82,7 +85,7 @@ metadata:
 rules:
 - apiGroups: [""]
   resources: ["secrets"]
-  verbs: ["create"]
+  verbs: ["create", "get", "list", "watch"]
 - apiGroups: [""]
   resources: ["secrets"]
   resourceNames: [{{ include "metallb.secretName" . | quote }}]
diff --git a/config/crd/bases/metallb.io_bgppeers.yaml b/config/crd/bases/metallb.io_bgppeers.yaml
index 55fd6d98..f9fa3749 100644
--- a/config/crd/bases/metallb.io_bgppeers.yaml
+++ b/config/crd/bases/metallb.io_bgppeers.yaml
@@ -211,6 +211,19 @@ spec:
                 description: Authentication password for routers enforcing TCP MD5
                   authenticated sessions
                 type: string
+              passwordSecret:
+                description: passwordSecret is name of the authentication secret for
+                  BGP Peer
+                properties:
+                  name:
+                    description: Name is unique within a namespace to reference a
+                      secret resource.
+                    type: string
+                  namespace:
+                    description: Namespace defines the space within which the secret
+                      name must be unique.
+                    type: string
+                type: object
               peerASN:
                 description: AS number to expect from the remote end of the session.
                 format: int32
diff --git a/config/manifests/metallb-frr.yaml b/config/manifests/metallb-frr.yaml
index 9556fb7d..333fe9fa 100644
--- a/config/manifests/metallb-frr.yaml
+++ b/config/manifests/metallb-frr.yaml
@@ -577,6 +577,19 @@ spec:
                 description: Authentication password for routers enforcing TCP MD5
                   authenticated sessions
                 type: string
+              passwordSecret:
+                description: passwordSecret is name of the authentication secret for
+                  BGP Peer
+                properties:
+                  name:
+                    description: Name is unique within a namespace to reference a
+                      secret resource.
+                    type: string
+                  namespace:
+                    description: Namespace defines the space within which the secret
+                      name must be unique.
+                    type: string
+                type: object
               peerASN:
                 description: AS number to expect from the remote end of the session.
                 format: int32
@@ -858,6 +871,9 @@ rules:
   - secrets
   verbs:
   - create
+  - get
+  - list
+  - watch
 - apiGroups:
   - ""
   resourceNames:
@@ -939,6 +955,14 @@ rules:
   - pods
   verbs:
   - list
+- apiGroups:
+  - ""
+  resources:
+  - secrets
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - metallb.io
   resources:
diff --git a/config/manifests/metallb-native.yaml b/config/manifests/metallb-native.yaml
index 46e4a1ed..0f62cd25 100644
--- a/config/manifests/metallb-native.yaml
+++ b/config/manifests/metallb-native.yaml
@@ -577,6 +577,19 @@ spec:
                 description: Authentication password for routers enforcing TCP MD5
                   authenticated sessions
                 type: string
+              passwordSecret:
+                description: passwordSecret is name of the authentication secret for
+                  BGP Peer
+                properties:
+                  name:
+                    description: Name is unique within a namespace to reference a
+                      secret resource.
+                    type: string
+                  namespace:
+                    description: Namespace defines the space within which the secret
+                      name must be unique.
+                    type: string
+                type: object
               peerASN:
                 description: AS number to expect from the remote end of the session.
                 format: int32
@@ -858,6 +871,9 @@ rules:
   - secrets
   verbs:
   - create
+  - get
+  - list
+  - watch
 - apiGroups:
   - ""
   resourceNames:
@@ -939,6 +955,14 @@ rules:
   - pods
   verbs:
   - list
+- apiGroups:
+  - ""
+  resources:
+  - secrets
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - metallb.io
   resources:
diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
index 3599ff7f..a49d816f 100644
--- a/config/rbac/role.yaml
+++ b/config/rbac/role.yaml
@@ -88,6 +88,14 @@ rules:
       - pods
     verbs:
       - list
+  - apiGroups:
+      - ""
+    resources:
+      - secrets
+    verbs:
+      - get
+      - list
+      - watch
   - apiGroups:
       - metallb.io
     resources:
@@ -151,6 +159,9 @@ rules:
       - secrets
     verbs:
       - create
+      - get
+      - list
+      - watch
   - apiGroups:
       - ""
     resources:
diff --git a/internal/config/config.go b/internal/config/config.go
index 0760b6d5..c940059a 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -27,6 +27,7 @@ import (
 	"github.com/pkg/errors"
 	metallbv1beta1 "go.universe.tf/metallb/api/v1beta1"
 	metallbv1beta2 "go.universe.tf/metallb/api/v1beta2"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"k8s.io/apimachinery/pkg/labels"
@@ -39,6 +40,7 @@ type ClusterResources struct {
 	BGPAdvs            []metallbv1beta1.BGPAdvertisement
 	L2Advs             []metallbv1beta1.L2Advertisement
 	LegacyAddressPools []metallbv1beta1.AddressPool
+	PasswordSecrets    map[string]corev1.Secret
 }
 
 // Config is a parsed MetalLB configuration.
@@ -174,7 +176,7 @@ func For(resources ClusterResources, validate Validate) (*Config, error) {
 	}
 
 	for i, p := range resources.Peers {
-		peer, err := peerFromCR(p)
+		peer, err := peerFromCR(p, resources.PasswordSecrets)
 		if err != nil {
 			return nil, fmt.Errorf("parsing peer #%d: %s", i+1, err)
 		}
@@ -311,7 +313,7 @@ func For(resources ClusterResources, validate Validate) (*Config, error) {
 	return cfg, nil
 }
 
-func peerFromCR(p metallbv1beta2.BGPPeer) (*Peer, error) {
+func peerFromCR(p metallbv1beta2.BGPPeer, passwordSecrets map[string]corev1.Secret) (*Peer, error) {
 	if p.Spec.MyASN == 0 {
 		return nil, errors.New("missing local ASN")
 	}
@@ -370,9 +372,9 @@ func peerFromCR(p metallbv1beta2.BGPPeer) (*Peer, error) {
 		nodeSels = []labels.Selector{labels.Everything()}
 	}
 
-	var password string
-	if p.Spec.Password != "" {
-		password = p.Spec.Password
+	password, err := passwordForPeer(p, passwordSecrets)
+	if err != nil {
+		return nil, err
 	}
 
 	return &Peer{
@@ -391,6 +393,32 @@ func peerFromCR(p metallbv1beta2.BGPPeer) (*Peer, error) {
 	}, nil
 }
 
+func passwordForPeer(p metallbv1beta2.BGPPeer, passwordSecrets map[string]corev1.Secret) (string, error) {
+	if p.Spec.Password != "" && p.Spec.PasswordSecret.Name != "" {
+		return "", fmt.Errorf("can not have both password and secret ref set in peer config %q/%q", p.Namespace,
+			p.Name)
+	}
+	var password string
+	if p.Spec.Password != "" {
+		password = p.Spec.Password
+	} else if p.Spec.PasswordSecret.Name != "" {
+		secret, ok := passwordSecrets[p.Spec.PasswordSecret.Name]
+		if !ok {
+			return "", fmt.Errorf("secret ref not found for peer config %q/%q", p.Namespace, p.Name)
+		}
+		if secret.Type != corev1.SecretTypeBasicAuth {
+			return "", fmt.Errorf("secret type mismatch on %q/%q, type %q is expected ", secret.Namespace,
+				secret.Name, corev1.SecretTypeBasicAuth)
+		}
+		srcPass, ok := secret.Data["password"]
+		if !ok {
+			return "", fmt.Errorf("password not specified in the secret %q/%q", secret.Namespace, secret.Name)
+		}
+		password = string(srcPass)
+	}
+	return password, nil
+}
+
 func addressPoolFromCR(p metallbv1beta1.IPPool, bgpCommunities map[string]uint32) (*Pool, error) {
 	if p.Name == "" {
 		return nil, errors.New("missing pool name")
diff --git a/internal/k8s/controllers/config_controller.go b/internal/k8s/controllers/config_controller.go
index 2f1fe3e7..215c46e5 100644
--- a/internal/k8s/controllers/config_controller.go
+++ b/internal/k8s/controllers/config_controller.go
@@ -25,6 +25,7 @@ import (
 	metallbv1beta1 "go.universe.tf/metallb/api/v1beta1"
 	metallbv1beta2 "go.universe.tf/metallb/api/v1beta2"
 	"go.universe.tf/metallb/internal/config"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -82,18 +83,24 @@ func (r *ConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		return ctrl.Result{}, err
 	}
 
-	metallbCRs := config.ClusterResources{
+	secrets, err := r.getSecrets(ctx)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	resources := config.ClusterResources{
 		Pools:              ipPools.Items,
 		Peers:              bgpPeers.Items,
 		BFDProfiles:        bfdProfiles.Items,
 		L2Advs:             l2Advertisements.Items,
 		BGPAdvs:            bgpAdvertisements.Items,
 		LegacyAddressPools: addressPools.Items,
+		PasswordSecrets:    secrets,
 	}
 
-	level.Debug(r.Logger).Log("controller", "ConfigReconciler", "metallb CRs", spew.Sdump(metallbCRs))
+	level.Debug(r.Logger).Log("controller", "ConfigReconciler", "metallb CRs and Secrets", spew.Sdump(resources))
 
-	cfg, err := config.For(metallbCRs, r.ValidateConfig)
+	cfg, err := config.For(resources, r.ValidateConfig)
 	if err != nil {
 		level.Error(r.Logger).Log("controller", "ConfigReconciler", "error", "failed to parse the configuration", "error", err)
 		return ctrl.Result{}, nil
@@ -104,13 +111,13 @@ func (r *ConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	res := r.Handler(r.Logger, cfg)
 	switch res {
 	case SyncStateError:
-		level.Error(r.Logger).Log("controller", "ConfigReconciler", "metallb CRs", spew.Sdump(metallbCRs), "event", "reload failed, retry")
+		level.Error(r.Logger).Log("controller", "ConfigReconciler", "metallb CRs and Secrets", spew.Sdump(resources), "event", "reload failed, retry")
 		return ctrl.Result{}, retryError
 	case SyncStateReprocessAll:
 		level.Info(r.Logger).Log("controller", "ConfigReconciler", "event", "force service reload")
 		r.ForceReload()
 	case SyncStateErrorNoRetry:
-		level.Error(r.Logger).Log("controller", "ConfigReconciler", "metallb CRs", spew.Sdump(metallbCRs), "event", "reload failed, no retry")
+		level.Error(r.Logger).Log("controller", "ConfigReconciler", "metallb CRs and Secrets", spew.Sdump(resources), "event", "reload failed, no retry")
 		return ctrl.Result{}, nil
 	}
 
@@ -126,5 +133,19 @@ func (r *ConfigReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Watches(&source.Kind{Type: &metallbv1beta1.L2Advertisement{}}, &handler.EnqueueRequestForObject{}).
 		Watches(&source.Kind{Type: &metallbv1beta1.BFDProfile{}}, &handler.EnqueueRequestForObject{}).
 		Watches(&source.Kind{Type: &metallbv1beta1.AddressPool{}}, &handler.EnqueueRequestForObject{}).
+		Watches(&source.Kind{Type: &corev1.Secret{}}, &handler.EnqueueRequestForObject{}).
 		Complete(r)
 }
+
+func (r *ConfigReconciler) getSecrets(ctx context.Context) (map[string]corev1.Secret, error) {
+	var secrets corev1.SecretList
+	if err := r.List(ctx, &secrets, client.InNamespace(r.Namespace)); err != nil {
+		level.Error(r.Logger).Log("controller", "ConfigReconciler", "error", "failed to get secrets", "error", err)
+		return nil, err
+	}
+	secretsMap := make(map[string]corev1.Secret)
+	for _, secret := range secrets.Items {
+		secretsMap[secret.Name] = secret
+	}
+	return secretsMap, nil
+}
diff --git a/internal/k8s/k8s.go b/internal/k8s/k8s.go
index 738962fe..450e9512 100644
--- a/internal/k8s/k8s.go
+++ b/internal/k8s/k8s.go
@@ -125,6 +125,7 @@ func New(cfg *Config) (*Client, error) {
 				&metallbv1beta1.IPPool{}:           namespaceSelector,
 				&metallbv1beta1.L2Advertisement{}:  namespaceSelector,
 				&metallbv1beta2.BGPPeer{}:          namespaceSelector,
+				&corev1.Secret{}:                   namespaceSelector,
 			},
 		}),
 	})


Test patch:
diff --git a/e2etest/bgptests/bgp.go b/e2etest/bgptests/bgp.go
index 4c0527ef..df5cce6e 100644
--- a/e2etest/bgptests/bgp.go
+++ b/e2etest/bgptests/bgp.go
@@ -492,6 +492,31 @@ var _ = ginkgo.Describe("BGP", func() {
 		table.Entry("IPV4", ipfamily.IPv4),
 		table.Entry("IPV6", ipfamily.IPv6))
 
+	table.DescribeTable("validate external containers are paired with nodes", func(ipFamily ipfamily.Family) {
+		ginkgo.By("configure peer")
+
+		resources := metallbconfig.ClusterResources{
+			Peers: metallb.PeersForContainers(FRRContainers, ipFamily, func(p *metallbv1beta2.BGPPeer) {
+				p.Spec.PasswordSecret = corev1.SecretReference{Name: metallb.GetBGPPeerSecretName(p.Spec.ASN, p.Spec.Port)}
+				p.Spec.Password = ""
+			}),
+			PasswordSecrets: metallb.BGPPeerSecretReferences(FRRContainers),
+		}
+		err := ConfigUpdater.Update(resources)
+		framework.ExpectNoError(err)
+
+		for _, c := range FRRContainers {
+			err = frrcontainer.PairWithNodes(cs, c, ipFamily)
+			framework.ExpectNoError(err)
+		}
+
+		for _, c := range FRRContainers {
+			validateFRRPeeredWithNodes(cs, c, ipFamily)
+		}
+	},
+		table.Entry("IPV4 with Secret Ref set for BGPPeer CR", ipfamily.IPv4),
+		table.Entry("IPV6 with Secret Ref set for BGPPeer CR", ipfamily.IPv6))
+
 	ginkgo.Context("BFD", func() {
 		table.DescribeTable("should work with the given bfd profile", func(bfd metallbv1beta1.BFDProfile, pairingFamily ipfamily.Family, poolAddresses []string, tweak testservice.Tweak) {
 			resources := metallbconfig.ClusterResources{
diff --git a/e2etest/pkg/config/update.go b/e2etest/pkg/config/update.go
index 3d1468cc..1634382c 100644
--- a/e2etest/pkg/config/update.go
+++ b/e2etest/pkg/config/update.go
@@ -8,6 +8,7 @@ import (
 	metallbv1beta1 "go.universe.tf/metallb/api/v1beta1"
 	metallbv1beta2 "go.universe.tf/metallb/api/v1beta2"
 	"go.universe.tf/metallb/internal/config"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -36,6 +37,10 @@ func UpdaterForCRs(r *rest.Config, ns string) (*beta1Updater, error) {
 		return nil, err
 	}
 
+	if err := corev1.AddToScheme(myScheme); err != nil {
+		return nil, err
+	}
+
 	cl, err := client.New(r, client.Options{
 		Scheme: myScheme,
 	})
@@ -62,6 +67,11 @@ func (o beta1Updater) Update(r config.ClusterResources) error {
 		key = key + 1
 	}
 
+	for _, secret := range r.PasswordSecrets {
+		objects[key] = secret.DeepCopy()
+		key = key + 1
+	}
+
 	for _, peer := range r.Peers {
 		objects[key] = peer.DeepCopy()
 		key = key + 1
diff --git a/e2etest/pkg/metallb/config.go b/e2etest/pkg/metallb/config.go
index d0104145..f63e4271 100644
--- a/e2etest/pkg/metallb/config.go
+++ b/e2etest/pkg/metallb/config.go
@@ -10,6 +10,7 @@ import (
 	metallbv1beta2 "go.universe.tf/metallb/api/v1beta2"
 	frrcontainer "go.universe.tf/metallb/e2etest/pkg/frr/container"
 	"go.universe.tf/metallb/internal/ipfamily"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -26,7 +27,7 @@ func init() {
 }
 
 // PeersForContainers returns the metallb config peers related to the given containers.
-func PeersForContainers(containers []*frrcontainer.FRR, ipFamily ipfamily.Family) []metallbv1beta2.BGPPeer {
+func PeersForContainers(containers []*frrcontainer.FRR, ipFamily ipfamily.Family, tweak ...func(p *metallbv1beta2.BGPPeer)) []metallbv1beta2.BGPPeer {
 	var peers []metallbv1beta2.BGPPeer
 
 	for i, c := range containers {
@@ -40,7 +41,7 @@ func PeersForContainers(containers []*frrcontainer.FRR, ipFamily ipfamily.Family
 			ebgpMultihop = true
 		}
 		for i, address := range addresses {
-			peers = append(peers, metallbv1beta2.BGPPeer{
+			peer := metallbv1beta2.BGPPeer{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: c.Name + fmt.Sprint(i), // Otherwise the peers will override
 				},
@@ -52,8 +53,11 @@ func PeersForContainers(containers []*frrcontainer.FRR, ipFamily ipfamily.Family
 					Password:     c.RouterConfig.Password,
 					HoldTime:     metav1.Duration{Duration: holdTime},
 					EBGPMultiHop: ebgpMultihop,
-				},
-			})
+				}}
+			for _, f := range tweak {
+				f(&peer)
+			}
+			peers = append(peers, peer)
 		}
 	}
 	return peers
@@ -74,3 +78,22 @@ func WithRouterID(peers []metallbv1beta2.BGPPeer, routerID string) []metallbv1be
 	}
 	return peers
 }
+
+func BGPPeerSecretReferences(containers []*frrcontainer.FRR) map[string]corev1.Secret {
+	secretMap := make(map[string]corev1.Secret)
+	for _, c := range containers {
+		name := GetBGPPeerSecretName(c.RouterConfig.ASN, c.RouterConfig.BGPPort)
+		secretMap[name] = corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: name,
+			},
+			Type: corev1.SecretTypeBasicAuth,
+			Data: map[string][]byte{"password": []byte(c.RouterConfig.Password)},
+		}
+	}
+	return secretMap
+}
+
+func GetBGPPeerSecretName(asn uint32, port uint16) string {
+	return fmt.Sprintf("bgppeer-%d-%d-secret", asn, port)
+}
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 823aa903..08db4fa5 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -11,6 +11,7 @@ import (
 	"go.universe.tf/metallb/api/v1beta1"
 	"go.universe.tf/metallb/api/v1beta2"
 	"go.universe.tf/metallb/internal/pointer"
+	corev1 "k8s.io/api/core/v1"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 )
@@ -965,6 +966,115 @@ func TestParse(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc: "BGP Peer with both password and secret ref set",
+			crs: ClusterResources{
+				Peers: []v1beta2.BGPPeer{
+					{
+						Spec: v1beta2.BGPPeerSpec{
+							MyASN:    42,
+							ASN:      42,
+							Address:  "1.2.3.4",
+							Password: "nopass",
+							PasswordSecret: corev1.SecretReference{Name: "nosecret",
+								Namespace: "metallb-system"},
+						},
+					},
+				},
+			},
+		},
+		{
+			desc: "BGP Peer with invalid secret type",
+			crs: ClusterResources{
+				Peers: []v1beta2.BGPPeer{
+					{
+						Spec: v1beta2.BGPPeerSpec{
+							MyASN:   42,
+							ASN:     42,
+							Address: "1.2.3.4",
+							PasswordSecret: corev1.SecretReference{Name: "bgpsecret",
+								Namespace: "metallb-system"},
+						},
+					},
+				},
+				PasswordSecrets: map[string]corev1.Secret{
+					"bgpsecret": {Type: corev1.SecretTypeOpaque, ObjectMeta: v1.ObjectMeta{Name: "bgpsecret", Namespace: "metallb-system"}},
+				},
+			},
+		},
+		{
+			desc: "BGP Peer without password set in the secret",
+			crs: ClusterResources{
+				Peers: []v1beta2.BGPPeer{
+					{
+						Spec: v1beta2.BGPPeerSpec{
+							MyASN:   42,
+							ASN:     42,
+							Address: "1.2.3.4",
+							PasswordSecret: corev1.SecretReference{Name: "bgpsecret",
+								Namespace: "metallb-system"},
+						},
+					},
+				},
+				PasswordSecrets: map[string]corev1.Secret{
+					"bgpsecret": {Type: corev1.SecretTypeBasicAuth, ObjectMeta: v1.ObjectMeta{Name: "bgpsecret", Namespace: "metallb-system"}},
+				},
+			},
+		},
+		{
+			desc: "BGP Peer with a valid secret",
+			crs: ClusterResources{
+				Peers: []v1beta2.BGPPeer{
+					{
+						Spec: v1beta2.BGPPeerSpec{
+							MyASN:   42,
+							ASN:     42,
+							Port:    179,
+							Address: "1.2.3.4",
+							PasswordSecret: corev1.SecretReference{Name: "bgpsecret",
+								Namespace: "metallb-system"},
+						},
+					},
+				},
+				PasswordSecrets: map[string]corev1.Secret{
+					"bgpsecret": {Type: corev1.SecretTypeBasicAuth, ObjectMeta: v1.ObjectMeta{Name: "bgpsecret", Namespace: "metallb-system"},
+						Data: map[string][]byte{"password": []byte([]byte("nopass"))}},
+				},
+			},
+			want: &Config{
+				Peers: []*Peer{
+					{
+						MyASN:         42,
+						ASN:           42,
+						Addr:          net.ParseIP("1.2.3.4"),
+						Port:          179,
+						HoldTime:      90 * time.Second,
+						KeepaliveTime: 30 * time.Second,
+						NodeSelectors: []labels.Selector{labels.Everything()},
+						BFDProfile:    "",
+						Password:      "nopass",
+					},
+				},
+				Pools:       map[string]*Pool{},
+				BFDProfiles: map[string]*BFDProfile{},
+			},
+		},
+		{
+			desc: "BGP Peer with unavailable secret ref",
+			crs: ClusterResources{
+				Peers: []v1beta2.BGPPeer{
+					{
+						Spec: v1beta2.BGPPeerSpec{
+							MyASN:   42,
+							ASN:     42,
+							Address: "1.2.3.4",
+							PasswordSecret: corev1.SecretReference{Name: "nosecret",
+								Namespace: "metallb-system"},
+						},
+					},
+				},
+			},
+		},
 		{
 			desc: "Peer with non existing BFD Profile",
 			crs: ClusterResources{