OPNET-781: Add BGP VIP management support #10718
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| package manifests | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "path" | ||
|
|
||
| "github.com/pkg/errors" | ||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "sigs.k8s.io/yaml" | ||
|
|
||
| "github.com/openshift/installer/pkg/asset" | ||
| "github.com/openshift/installer/pkg/asset/installconfig" | ||
| "github.com/openshift/installer/pkg/types/baremetal" | ||
| ) | ||
|
|
||
| var ( | ||
| bgpVIPConfigMapFileName = path.Join(manifestDir, "bgp-vip-config.yaml") | ||
| ) | ||
|
|
||
| const ( | ||
| bgpVIPConfigMapName = "bgp-vip-config" | ||
| bgpVIPConfigMapNamespace = "openshift-network-operator" | ||
| bgpVIPConfigMapDataKey = "config.json" | ||
| ) | ||
|
|
||
| // bgpVIPConfigJSON is the JSON structure stored in the ConfigMap. Its field | ||
| // names match baremetal-runtimecfg's FRRPeerMapping so the same JSON can be | ||
| // written verbatim to /etc/kubernetes/static-pod-resources/frr-k8s/frr-peers.json. | ||
| type bgpVIPConfigJSON struct { | ||
| LocalASN int64 `json:"localASN"` | ||
| DefaultPeers []baremetal.BGPPeerConfig `json:"defaultPeers"` | ||
| Communities []string `json:"communities,omitempty"` | ||
| APIVIPs []string `json:"apiVIPs"` | ||
| IngressVIPs []string `json:"ingressVIPs"` | ||
| HostOverrides map[string][]baremetal.BGPPeerConfig `json:"hostOverrides,omitempty"` | ||
| } | ||
|
|
||
| // BGPVIPConfigMap generates the bgp-vip-config ConfigMap for CNO. | ||
| type BGPVIPConfigMap struct { | ||
| ConfigMap *corev1.ConfigMap | ||
| File *asset.File | ||
| } | ||
|
|
||
| var _ asset.WritableAsset = (*BGPVIPConfigMap)(nil) | ||
|
|
||
| // Name returns a human friendly name for the asset. | ||
| func (*BGPVIPConfigMap) Name() string { | ||
| return "BGP VIP Config ConfigMap" | ||
| } | ||
|
|
||
| // Dependencies returns all of the dependencies directly needed to generate | ||
| // the asset. | ||
| func (*BGPVIPConfigMap) Dependencies() []asset.Asset { | ||
| return []asset.Asset{ | ||
| &installconfig.InstallConfig{}, | ||
| } | ||
| } | ||
|
|
||
| // Generate generates the BGP VIP Config ConfigMap. | ||
| func (b *BGPVIPConfigMap) Generate(_ context.Context, dependencies asset.Parents) error { | ||
| installConfig := &installconfig.InstallConfig{} | ||
| dependencies.Get(installConfig) | ||
|
|
||
| // Only generate for baremetal platform with BGPVIPConfig set. | ||
| if installConfig.Config.Platform.Name() != baremetal.Name || | ||
| installConfig.Config.Platform.BareMetal == nil || | ||
| installConfig.Config.Platform.BareMetal.BGPVIPConfig == nil { | ||
| return nil | ||
| } | ||
|
|
||
| bm := installConfig.Config.Platform.BareMetal | ||
| bgpConfig := bm.BGPVIPConfig | ||
|
|
||
| configData := bgpVIPConfigJSON{ | ||
| LocalASN: bgpConfig.LocalASN, | ||
| DefaultPeers: bgpConfig.Peers, | ||
| Communities: bgpConfig.Communities, | ||
| APIVIPs: bm.APIVIPs, | ||
| IngressVIPs: bm.IngressVIPs, | ||
| } | ||
|
|
||
| // Collect per-host BGP peer overrides. | ||
| hostOverrides := make(map[string][]baremetal.BGPPeerConfig) | ||
| for _, host := range bm.Hosts { | ||
| if host != nil && len(host.BGPPeers) > 0 { | ||
| hostOverrides[host.Name] = host.BGPPeers | ||
| } | ||
| } | ||
| if len(hostOverrides) > 0 { | ||
| configData.HostOverrides = hostOverrides | ||
| } | ||
|
|
||
| jsonBytes, err := json.Marshal(configData) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to marshal BGP VIP config JSON") | ||
| } | ||
|
|
||
| cm := &corev1.ConfigMap{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| APIVersion: corev1.SchemeGroupVersion.String(), | ||
| Kind: "ConfigMap", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: bgpVIPConfigMapNamespace, | ||
| Name: bgpVIPConfigMapName, | ||
| }, | ||
| Data: map[string]string{ | ||
| bgpVIPConfigMapDataKey: string(jsonBytes), | ||
|
Comment on lines
+76
to
+110
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift Do not render BGP credentials into a ConfigMap.
As per coding guidelines, flag MD5 usage. 🤖 Prompt for AI AgentsSource: Coding guidelines
Contributor
Author
There was a problem hiding this comment. Known DevPreview limitation, deliberate for now: the password is BGP TCP-MD5 session authentication (RFC 2385) — the hash is mandated by the BGP protocol option itself, not a choice of ours — and the whole config flows installer → ConfigMap → MCO/CNO as one schema. Moving it to a Secret requires the full chain (installer generates the Secret, MCO bootstrap render and CNO FRRConfiguration consume passwordSecret references) and is planned alongside the TechPreview structured API; the CNO side documents the same limitation (cluster-network-operator#3047, docs/bgp_vip_management.md). Splitting just the installer side now would leave mixed sources without removing the exposure. This reply was written with AI assistance. Please verify before acting on it. There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Relevant installer symbols ==='
ast-grep outline pkg/types/baremetal/platform.go --items all --match 'BGPPeerConfig|BGPVIPConfig|Host|Platform' --view expanded
ast-grep outline pkg/asset/manifests/bgpvipconfig.go --items all --match 'bgpVIPConfigJSON|BGPVIPConfigMap' --view expanded
printf '%s\n' '=== BGP fields and serialization ==='
rg -n -A18 -B4 'type BGPPeerConfig|type BGPVIPConfig|BGPPeers|BGPVIPConfig|json\.Marshal|hostOverrides' \
pkg/types/baremetal/platform.go pkg/asset/manifests/bgpvipconfig.go pkg/asset/manifests/bgpvipconfig_test.go
printf '%s\n' '=== In-repository BGP VIP documentation/references ==='
rg -n -i -C3 'BGPBasedVIPManagement|BGP VIP|bgpVIPConfig|passwordSecret|TCP-MD5|RFC 2385|bgp_vip_management' \
README.md docs pkg data cmd 2>/dev/null || true
printf '%s\n' '=== Referenced CNO item metadata ==='
gh api repos/openshift/cluster-network-operator/issues/3047 \
--jq '{number, title, state, body, html_url, labels: [.labels[].name]}' || true
printf '%s\n' '=== Referenced CNO item comments containing credential/schema terms ==='
gh api --paginate repos/openshift/cluster-network-operator/issues/3047/comments \
--jq '.[] | select((.body | ascii_downcase) | test("password|secret|md5|bgp-vip|frrconfiguration")) | {user: .user.login, created_at, body, html_url}' || trueLength of output: 104813
I also correct the MD5 wording: installer does not implement MD5; this is the BGP TCP-MD5 shared secret required by the protocol. Given the DevPreview scope and coupled consumer changes, I won’t require a partial installer-only change. The plaintext ConfigMap exposure remains a known pre-GA security limitation and should stay explicitly documented and tracked for the Secret-backed structured API work. Would you like a dedicated follow-up issue created for that end-to-end migration? ✏️ Learnings added
|
||
| }, | ||
| } | ||
|
|
||
| cmData, err := yaml.Marshal(cm) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to create %s manifest", b.Name()) | ||
| } | ||
| b.ConfigMap = cm | ||
| b.File = &asset.File{ | ||
| Filename: bgpVIPConfigMapFileName, | ||
| Data: cmData, | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // Files returns the files generated by the asset. | ||
| func (b *BGPVIPConfigMap) Files() []*asset.File { | ||
| if b.File != nil { | ||
| return []*asset.File{b.File} | ||
| } | ||
| return []*asset.File{} | ||
| } | ||
|
|
||
| // Load loads the already-rendered files back from disk. | ||
| func (b *BGPVIPConfigMap) Load(f asset.FileFetcher) (bool, error) { | ||
| return false, nil | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package manifests | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
|
|
||
| "github.com/openshift/installer/pkg/asset" | ||
| "github.com/openshift/installer/pkg/asset/installconfig" | ||
| "github.com/openshift/installer/pkg/types" | ||
| "github.com/openshift/installer/pkg/types/baremetal" | ||
| ) | ||
|
|
||
| func bgpInstallConfig() *installconfig.InstallConfig { | ||
| return installconfig.MakeAsset(&types.InstallConfig{ | ||
| Platform: types.Platform{ | ||
| BareMetal: &baremetal.Platform{ | ||
| APIVIPs: []string{"192.168.111.5"}, | ||
| IngressVIPs: []string{"192.168.111.4"}, | ||
| BGPVIPConfig: &baremetal.BGPVIPConfig{ | ||
| LocalASN: 64512, | ||
| Peers: []baremetal.BGPPeerConfig{ | ||
| {PeerAddress: "192.168.111.1", PeerASN: 64513}, | ||
| }, | ||
| }, | ||
| Hosts: []*baremetal.Host{ | ||
| { | ||
| Name: "master-0", | ||
| BGPPeers: []baremetal.BGPPeerConfig{ | ||
| {PeerAddress: "192.168.1.1", PeerASN: 64513}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| // The config.json schema must match baremetal-runtimecfg's FRRPeerMapping: | ||
| // top-level "defaultPeers" (not "peers") and hostOverrides as a flat | ||
| // map[hostname][]peer (not map[hostname]{"peers":[...]}). | ||
| func TestBGPVIPConfigMapSchemaMatchesRuntimecfg(t *testing.T) { | ||
| parents := asset.Parents{} | ||
| parents.Add(bgpInstallConfig()) | ||
|
|
||
| cmAsset := &BGPVIPConfigMap{} | ||
| if !assert.NoError(t, cmAsset.Generate(context.Background(), parents)) { | ||
| return | ||
| } | ||
| if !assert.NotNil(t, cmAsset.ConfigMap) { | ||
| return | ||
| } | ||
|
|
||
| var got map[string]json.RawMessage | ||
| if !assert.NoError(t, json.Unmarshal([]byte(cmAsset.ConfigMap.Data["config.json"]), &got)) { | ||
| return | ||
| } | ||
|
|
||
| assert.Contains(t, got, "defaultPeers") | ||
| assert.NotContains(t, got, "peers") | ||
|
|
||
| var overrides map[string][]baremetal.BGPPeerConfig | ||
| if !assert.NoError(t, json.Unmarshal(got["hostOverrides"], &overrides)) { | ||
| return | ||
| } | ||
| if !assert.Len(t, overrides["master-0"], 1) { | ||
| return | ||
| } | ||
| assert.Equal(t, "192.168.1.1", overrides["master-0"][0].PeerAddress) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Known DevPreview limitation, deliberate for now: the password is BGP TCP-MD5 session authentication (RFC 2385) — the hash is mandated by the BGP protocol option itself, not a choice of ours — and the whole config flows installer → ConfigMap → MCO/CNO as one schema. Moving it to a Secret requires the full chain (installer generates the Secret, MCO bootstrap render and CNO FRRConfiguration consume passwordSecret references) and is planned alongside the TechPreview structured API; the CNO side documents the same limitation (cluster-network-operator#3047, docs/bgp_vip_management.md). Splitting just the installer side now would leave mixed sources without removing the exposure.
This reply was written with AI assistance. Please verify before acting on it.