Repository navigation
OPNET-595: Promote BGPBasedVIPManagement to TechPreviewNoUpgrade #3165
+12,657
−24
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
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
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
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 |
|---|---|---|
|
|
@@ -4,7 +4,6 @@ metadata: | |
| annotations: | ||
| api-approved.openshift.io: https://github.com/openshift/api/pull/1453 | ||
| api.openshift.io/merged-by-featuregates: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| release.openshift.io/feature-set: TechPreviewNoUpgrade | ||
| labels: | ||
|
|
@@ -63,6 +62,18 @@ spec: | |
| description: baseOSExtensionsContainerImage is the matching extensions | ||
| container for the new-format container | ||
| type: string | ||
| bgpVIPPeersJSON: | ||
|
Member
There was a problem hiding this comment. MCO writes the plaintext peer passwords into this field, and cluster-reader can read ControllerConfig. That exposes the passwords without access to the Secrets. Can MCO move them to a Secret-backed transport before this goes to Tech Preview?
Contributor
There was a problem hiding this comment. Yes, this change merges (openshift/machine-config-operator#6750) as we speak |
||
| description: |- | ||
| bgpVIPPeersJSON carries the BGP VIP peer configuration (the config.json | ||
| payload of the bgp-vip-config ConfigMap) for rendering the frr-k8s | ||
| static pod peer file on control plane nodes. Only set when BGP-based | ||
| VIP management is enabled. | ||
| When omitted, BGP-based VIP management is not configured and no | ||
| frr-k8s peer file is rendered. | ||
| When set, the value must be between 1 and 65536 characters long. | ||
| maxLength: 65536 | ||
|
Member
There was a problem hiding this comment. BGPVIPConfig accepts more hostOverrides and peers than fit in this field. When that happens, the whole ControllerConfig update fails after admission. Can we align the BGPVIPConfig bounds with this limit so the API rejects it up front?
Contributor
There was a problem hiding this comment. Yes and no. It's not a trivial math. You have 64K limit on the CR size. You can consume this limit with gazillion of small host overrides or a handful of monstrous ones. I am happy to implement a suggestion but I don't have an idea. Just setting the limit to something conservatively small like 5 is defeating the purpose. |
||
| minLength: 1 | ||
| type: string | ||
| cloudProviderCAData: | ||
| description: cloudProviderCAData specifies the cloud provider CA data | ||
| format: byte | ||
|
|
@@ -2100,6 +2111,25 @@ spec: | |
| datacenter DNS, a DNS service is hosted as a static pod to serve those hostnames | ||
| to the nodes in the cluster. | ||
| type: string | ||
| vipManagement: | ||
| description: |- | ||
| vipManagement indicates which VIP management mechanism is active | ||
| on this cluster. | ||
| Allowed values are `Keepalived`, `BGP`, and omitted. | ||
| Once set to a non-empty value, this field is immutable. | ||
| When set to `BGP`, kube-vip (Routing Table Mode) and frr-k8s are | ||
| deployed as static pods to advertise VIPs via BGP, replacing the | ||
| default keepalived/VRRP mechanism. | ||
| When set to `Keepalived`, the default keepalived-based VIP | ||
| management is used. | ||
| When omitted, the default keepalived-based VIP management is used. | ||
| enum: | ||
| - Keepalived | ||
| - BGP | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: vipManagement is immutable once set | ||
| rule: oldSelf == '' || self == oldSelf | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: dnsRecordsType may only be set to External | ||
|
|
||
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.
The enhancement in openshift/enhancements#1982 is still open, and we expect it to merge before a gate moves into TechPreviewNoUpgrade. Its Tech Preview criteria such as the tagged origin tests are also still open. Can we hold this until both are done?
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 have jobs (https://prow.ci.openshift.org/?job=*bgp-vip*) running for this feature already for quite some time as periodic on main (e.g. https://prow.ci.openshift.org/job-history/gs/test-platform-results-public/logs/periodic-ci-openshift-release-main-nightly-5.1-e2e-metal-ipi-bgp-vip).
Tests are not yet tagged purely because not enough understanding of the framework (Sippy etc.)
I have moved them to e2e origin in openshift/origin#31893 + openshift/release#88704 and it start collecting data soon, but in any case we will not get 14 days of tags on them.
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.
Enhancement is in.
Jobs started to populate https://sippy-auth.dptools.openshift.org/sippy-ng/feature_gates/5.1/BGPBasedVIPManagement already. We have metal ipv4 and metal dual-stack. Other "missing" variants are not relevant for this feature.