OSASINFRA-4437: openstack: add additional fields to support multiple VIPs - #2976
OSASINFRA-4437: openstack: add additional fields to support multiple VIPs#2976winiciusallan wants to merge 1 commit into
Conversation
In order to support multiple VIPs during an OpenShift installation on top of OpenStack, we need to propagate these VIPs in the Infrastructure CR, so API consumers can get these valeus to perform some operation.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@winiciusallan: This pull request references OSASINFRA-4437 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Hello @winiciusallan! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/XXL
📝 WalkthroughWalkthroughOpenStack platform spec and status types now include additional API-server and ingress IP lists. The lists support up to 10 entries for user-managed load balancers across separate networks. Infrastructure and ControllerConfig CRD schemas expose these fields with atomic list semantics, IP validation for spec entries, and IP formatting for status entries. 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
PR Summary by QodoOpenStack: add additional API/Ingress VIP fields to Infrastructure CR
AI Description
Diagram
High-Level Assessment
Files changed (60)
|
Code Review by Qodo
1. Additional*IPs omit behavior undocumented
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
| // +kubebuilder:validation:MaxItems=10 | ||
| // +listType=atomic | ||
| // +optional | ||
| AdditionalAPIServerIPs []IP `json:"additionalAPIServerIPs,omitempty"` |
There was a problem hiding this comment.
1. Additional*ips missing featuregate marker 📘 Rule violation § Compliance
New stable config/v1 API fields were added without a +openshift:enable:FeatureGate=<Name> marker immediately above them. This violates the requirement to gate new fields in stable APIs to preserve API compatibility expectations.
Agent Prompt
## Issue description
New stable v1 API fields were added without a `+openshift:enable:FeatureGate=<FeatureGateName>` marker directly above the field declarations.
## Issue Context
These fields are in `config/v1` (stable) API structs and must be gated per policy.
## Fix Focus Areas
- config/v1/types_infrastructure.go[1197-1217]
- config/v1/types_infrastructure.go[1273-1295]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // +optional | ||
| AdditionalAPIServerIPs []IP `json:"additionalAPIServerIPs,omitempty"` |
There was a problem hiding this comment.
2. Additional*ips omit behavior undocumented 📜 Skill insight ✧ Quality
The new optional fields AdditionalAPIServerIPs and AdditionalIngressIPs do not document what happens when they are omitted. This makes API behavior unclear for clients and violates the required omission-behavior documentation rule.
Agent Prompt
## Issue description
New optional API fields were added but their doc comments do not explain behavior when the fields are omitted.
## Issue Context
For optional fields (`+optional` and/or pointer/omitempty), documentation must explicitly state what happens when the field is not provided (e.g., "When omitted...").
## Fix Focus Areas
- config/v1/types_infrastructure.go[1197-1217]
- config/v1/types_infrastructure.go[1273-1295]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // simultaneously. Each entry must be a valid IP address. This field is | ||
| // only valid when loadBalancer.type is set to UserManaged. | ||
| // |
There was a problem hiding this comment.
3. Missing xvalidation for loadbalancer.type 📜 Skill insight ≡ Correctness
The new OpenStack additional IP fields are documented as "only valid when loadBalancer.type is set to UserManaged" but there is no corresponding CEL/XValidation rule enforcing that cross-field relationship. This allows invalid combinations into the API and can expose inconsistent objects to consumers, contradicting the documented contract.
Agent Prompt
## Issue description
Field documentation introduces a cross-field constraint: `AdditionalAPIServerIPs` / `AdditionalIngressIPs` are only valid when `loadBalancer.type` is `UserManaged`, but there is no `+kubebuilder:validation:XValidation` / CEL rule enforcing this relationship in the API schema.
## Issue Context
- PR Compliance ID 1511 requires that documented cross-field relationships be enforced with XValidation.
- The new fields currently have only size/list annotations, and the generated CRD schema has no `x-kubernetes-validations` that ties them to `loadBalancer.type`.
- A similar `loadBalancer.type`-dependent invariant is already enforced for `dnsRecordsType` via CEL/XValidation, so there is precedent for codifying these invariants.
- XValidation must be attached at a struct scope that can “see” all referenced fields (potentially the root `Infrastructure` type and/or `OpenStackPlatformStatus` for status-only enforcement).
- After adding validations, regenerate CRDs/openapi artifacts so the rules appear in the generated manifests.
## Fix Focus Areas
- config/v1/types_infrastructure.go[1161-1217]
- config/v1/types_infrastructure.go[1229-1333]
- config/v1/types_infrastructure.go[1273-1310]
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yaml[2186-2234]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // +kubebuilder:validation:MaxItems=10 | ||
| // +listType=atomic | ||
| // +optional | ||
| AdditionalAPIServerIPs []IP `json:"additionalAPIServerIPs,omitempty"` |
There was a problem hiding this comment.
4. maxitems=10 not documented 📜 Skill insight ✧ Quality
The new list fields include +kubebuilder:validation:MaxItems=10 but the field comments do not mention this item-count constraint. This violates the requirement to document kubebuilder constraint markers in field comments.
Agent Prompt
## Issue description
Fields with kubebuilder constraint markers must document those constraints in the human-readable comment text.
## Issue Context
`AdditionalAPIServerIPs` and `AdditionalIngressIPs` have `+kubebuilder:validation:MaxItems=10`, but their doc comments do not state the maximum number of items.
## Fix Focus Areas
- config/v1/types_infrastructure.go[1197-1217]
- config/v1/types_infrastructure.go[1273-1295]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/v1/types_infrastructure.go`:
- Around line 1273-1296: Add CEL validation for OpenStackPlatformStatus so
AdditionalAPIServerIPs and AdditionalIngressIPs are accepted only when the
sibling loadBalancer.type is UserManaged, following the existing DNSRecordsType
XValidation pattern. Apply the rule to both status fields or their containing
struct while preserving current IP and item-count validations.
- Around line 1197-1218: Add an Infrastructure-level CEL validation rule
covering AdditionalAPIServerIPs and AdditionalIngressIPs that requires
status.platformStatus.openstack.loadBalancer.type to be UserManaged, while
permitting objects whose status or OpenStack load balancer status is not yet
present. Ensure the generated CRD enforces this prerequisite for both fields
rather than validating only IP format and list size.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml`:
- Around line 501-538: Enforce the loadBalancer.type UserManaged precondition
for additionalAPIServerIPs and additionalIngressIPs by adding the corresponding
CEL validation to both fields in every listed schema location:
payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml
ranges 501-538 and 2358-2383;
payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
ranges 487-524 and 2325-2350; and
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
ranges 780-817 and 2631-2656. Reject either list when loadBalancer.type is not
UserManaged, while preserving the existing IP validations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0119c317-ff74-4eda-b18c-26bb81df1781
⛔ Files ignored due to path filters (45)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/BGPBasedVIPManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/MutableTopology.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereHostVMGroupZonal.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSEuropeanSovereignCloudInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/BGPBasedVIPManagement.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/MutableTopology.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereHostVMGroupZonal.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (15)
config/v1/types_infrastructure.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Hypershift-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
| // additionalAPIServerIPs is a list of additional IP addresses to contact | ||
| // the Kubernetes API server on separate networks. These are used when a | ||
| // user-managed load balancer exposes the API server on multiple networks | ||
| // simultaneously. Each entry must be a valid IP address. This field is | ||
| // only valid when loadBalancer.type is set to UserManaged. | ||
| // | ||
| // +kubebuilder:validation:MaxItems=10 | ||
| // +listType=atomic | ||
| // +optional | ||
| AdditionalAPIServerIPs []IP `json:"additionalAPIServerIPs,omitempty"` | ||
|
|
||
| // additionalIngressIPs is a list of additional IP addresses that route to | ||
| // the default ingress controller on separate networks. These are used when | ||
| // a user-managed load balancer exposes the ingress controller on multiple | ||
| // networks simultaneously. Each entry must be a valid IP address. This | ||
| // field is only valid when loadBalancer.type is set to UserManaged. | ||
| // | ||
| // +kubebuilder:validation:MaxItems=10 | ||
| // +listType=atomic | ||
| // +optional | ||
| AdditionalIngressIPs []IP `json:"additionalIngressIPs,omitempty"` | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target type and nearby declarations ---'
rg -n -C 8 'AdditionalAPIServerIPs|AdditionalIngressIPs|type OpenStackPlatformSpec|type OpenStackPlatformStatus|LoadBalancer|DNSRecordsType' config/v1/types_infrastructure.go
printf '%s\n' '--- all related validation rules and field references ---'
rg -n -C 5 'additionalAPIServerIPs|additionalIngressIPs|loadBalancer\.type|platformSpec\.openstack|platformStatus\.openstack|controlPlaneTopology must match' .
printf '%s\n' '--- Infrastructure and ControllerConfig declarations ---'
rg -n -C 12 'type Infrastructure struct|type ControllerConfig struct|x-kubernetes-validations' config/v1Repository: openshift/api
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target source matches only ---'
rg -n -C 12 'AdditionalAPIServerIPs|AdditionalIngressIPs|type OpenStackPlatformSpec|type OpenStackPlatformStatus|LoadBalancer|DNSRecordsType' config/v1/types_infrastructure.go
printf '%s\n' '--- Infrastructure and ControllerConfig source declarations ---'
rg -n -C 15 'type Infrastructure struct|type ControllerConfig struct|platformSpec|platformStatus|controlPlaneTopology must match' config/v1 --glob '*.go'
printf '%s\n' '--- current change summary ---'
git diff --stat -- config/v1/types_infrastructure.go
git diff -- config/v1/types_infrastructure.go | sed -n '1,220p'Repository: openshift/api
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact change ---'
git diff --unified=8 -- config/v1/types_infrastructure.go
printf '%s\n' '--- non-generated consumers and validators ---'
rg -n -C 6 'AdditionalAPIServerIPs|AdditionalIngressIPs|additionalAPIServerIPs|additionalIngressIPs' \
--glob '!config/v1/zz_generated.crd-manifests/**' \
--glob '!**/vendor/**' .
printf '%s\n' '--- relevant generated schema fragments ---'
python3 - <<'PY'
from pathlib import Path
for p in Path("config/v1/zz_generated.crd-manifests").glob("*.yaml"):
text = p.read_text(errors="replace")
hits = []
for needle in ("additionalAPIServerIPs:", "additionalIngressIPs:"):
start = 0
while True:
i = text.find(needle, start)
if i < 0:
break
line = text.count("\n", 0, i) + 1
block = text[i:i+900].splitlines()[:18]
hits.append((line, needle, block))
start = i + len(needle)
if hits:
print(f"FILE {p}")
for line, needle, block in hits:
print(f"LINE {line} {needle}")
print("\n".join(block))
PYRepository: openshift/api
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target diff additions and removals ---'
git diff --unified=0 -- config/v1/types_infrastructure.go \
| rg '^(diff|@@|[+-][^+-])' | sed -n '1,180p'
printf '%s\n' '--- consumers in Go and validation markers ---'
rg -n -C 5 'AdditionalAPIServerIPs|AdditionalIngressIPs|additionalAPIServerIPs|additionalIngressIPs' \
config machineconfiguration --glob '*.go' --glob '!zz_generated/**' --glob '!vendor/**'
printf '%s\n' '--- ControllerConfig source shape ---'
rg -n -C 14 'type ControllerConfig struct|Infrastructure|platformSpec|platformStatus|controlPlaneTopology' \
machineconfiguration --glob '*.go' --glob '!zz_generated/**'
printf '%s\n' '--- Infrastructure CRD files ---'
find config/v1/zz_generated.crd-manifests config/v1/zz_generated.featuregated-crd-manifests \
-type f -iname '*infrastructures*' -print | head -20Repository: openshift/api
Length of output: 23321
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
source = Path("config/v1/types_infrastructure.go").read_text()
def type_body(name):
m = re.search(r"(?m)^type " + re.escape(name) + r"\s+struct\s*\{", source)
if not m:
raise SystemExit(f"missing {name}")
start = m.end()
depth = 1
i = start
while depth and i < len(source):
if source[i] == "{":
depth += 1
elif source[i] == "}":
depth -= 1
i += 1
return source[start:i-1]
for name in ("OpenStackPlatformSpec", "OpenStackPlatformStatus"):
body = type_body(name)
print(f"{name}:")
for field in ("LoadBalancer", "AdditionalAPIServerIPs", "AdditionalIngressIPs"):
print(f" {field}: {'yes' if re.search(r'(?m)^\\s*' + field + r'\\b', body) else 'no'}")
print(" loadBalancer.type validation markers:",
"yes" if "loadBalancer.type" in body else "no")
print("Infrastructure root spec/status validation markers:")
infra_head = source[source.index("type Infrastructure struct")-1000:source.index("type Infrastructure struct")]
for needle in ("self.spec.controlPlaneTopology", "self.status.controlPlaneTopology"):
print(f" {needle}: {'yes' if needle in infra_head else 'no'}")
print("Source validation markers mentioning additional fields:")
for line_no, line in enumerate(source.splitlines(), 1):
if ("additionalAPIServerIPs" in line or "additionalIngressIPs" in line) and "XValidation" in line:
print(f" {line_no}: {line.strip()}")
PY
printf '%s\n' '--- generated validation expressions in the default Infrastructure CRD ---'
rg -n -C 3 \
'additionalAPIServerIPs|additionalIngressIPs|spec\.controlPlaneTopology must match|self\.spec\.controlPlaneTopology|self\.status\.controlPlaneTopology' \
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlRepository: openshift/api
Length of output: 428
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
source = Path("config/v1/types_infrastructure.go").read_text()
def type_body(name):
m = re.search(r"(?m)^type " + re.escape(name) + r"\s+struct\s*\{", source)
if not m:
raise SystemExit(f"missing {name}")
start = m.end()
depth = 1
i = start
while depth and i < len(source):
if source[i] == "{":
depth += 1
elif source[i] == "}":
depth -= 1
i += 1
return source[start:i-1]
for name in ("OpenStackPlatformSpec", "OpenStackPlatformStatus"):
body = type_body(name)
print(f"{name}:")
for field in ("LoadBalancer", "AdditionalAPIServerIPs", "AdditionalIngressIPs"):
present = bool(re.search(r"(?m)^\s*" + field + r"\b", body))
print(f" {field}: {'yes' if present else 'no'}")
print(" loadBalancer.type validation markers:",
"yes" if "loadBalancer.type" in body else "no")
print("Infrastructure root spec/status validation markers:")
infra_pos = source.index("type Infrastructure struct")
infra_head = source[max(0, infra_pos - 1200):infra_pos]
for needle in ("self.spec.controlPlaneTopology", "self.status.controlPlaneTopology"):
print(f" {needle}: {'yes' if needle in infra_head else 'no'}")
print("Source validation markers mentioning additional fields:")
for line_no, line in enumerate(source.splitlines(), 1):
if ("additionalAPIServerIPs" in line or "additionalIngressIPs" in line) and "XValidation" in line:
print(f" {line_no}: {line.strip()}")
PY
printf '%s\n' '--- generated validation expressions in the default Infrastructure CRD ---'
rg -n -C 3 \
'additionalAPIServerIPs|additionalIngressIPs|spec\.controlPlaneTopology must match|self\.spec\.controlPlaneTopology|self\.status\.controlPlaneTopology' \
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlRepository: openshift/api
Length of output: 3352
Enforce the loadBalancer.type prerequisite for the additional OpenStack IP fields.
OpenStackPlatformSpec has no LoadBalancer; it exists only in OpenStackPlatformStatus. The generated CRD validates only IP format and list size. It accepts these fields when loadBalancer.type is not UserManaged.
Add an Infrastructure-level CEL rule that checks both fields against status.platformStatus.openstack.loadBalancer.type, while allowing the initial object before status exists. Otherwise, document the component that enforces this prerequisite.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/v1/types_infrastructure.go` around lines 1197 - 1218, Add an
Infrastructure-level CEL validation rule covering AdditionalAPIServerIPs and
AdditionalIngressIPs that requires
status.platformStatus.openstack.loadBalancer.type to be UserManaged, while
permitting objects whose status or OpenStack load balancer status is not yet
present. Ensure the generated CRD enforces this prerequisite for both fields
rather than validating only IP format and list size.
| // additionalAPIServerIPs is a list of additional IP addresses to contact | ||
| // the Kubernetes API server on separate networks. These are used when a | ||
| // user-managed load balancer exposes the API server on multiple networks | ||
| // simultaneously. Each entry must be a valid IP address. This field is | ||
| // only valid when loadBalancer.type is set to UserManaged. | ||
| // | ||
| // +kubebuilder:validation:Format=ip | ||
| // +kubebuilder:validation:MaxItems=10 | ||
| // +listType=atomic | ||
| // +optional | ||
| AdditionalAPIServerIPs []string `json:"additionalAPIServerIPs,omitempty"` | ||
|
|
||
| // additionalIngressIPs is a list of additional IP addresses that route to | ||
| // the default ingress controller on separate networks. These are used when | ||
| // a user-managed load balancer exposes the ingress controller on multiple | ||
| // networks simultaneously. Each entry must be a valid IP address. This | ||
| // field is only valid when loadBalancer.type is set to UserManaged. | ||
| // | ||
| // +kubebuilder:validation:Format=ip | ||
| // +kubebuilder:validation:MaxItems=10 | ||
| // +listType=atomic | ||
| // +optional | ||
| AdditionalIngressIPs []string `json:"additionalIngressIPs,omitempty"` | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add CEL enforcement for the "loadBalancer.type" constraint on the status fields.
Unlike the spec fields, loadBalancer is a real sibling field on OpenStackPlatformStatus (line 1309). The doc comments for AdditionalAPIServerIPs and AdditionalIngressIPs here state the field "is only valid when loadBalancer.type is set to UserManaged," but no x-kubernetes-validations rule enforces it, even though the same-struct pattern is already used for DNSRecordsType (dnsRecordsType may only be set to External when loadBalancer.type is UserManaged).
Add a similar +kubebuilder:validation:XValidation rule on AdditionalAPIServerIPs/AdditionalIngressIPs (or on the struct) to prevent silently-ignored values when loadBalancer.type is not UserManaged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/v1/types_infrastructure.go` around lines 1273 - 1296, Add CEL
validation for OpenStackPlatformStatus so AdditionalAPIServerIPs and
AdditionalIngressIPs are accepted only when the sibling loadBalancer.type is
UserManaged, following the existing DNSRecordsType XValidation pattern. Apply
the rule to both status fields or their containing struct while preserving
current IP and item-count validations.
| additionalAPIServerIPs: | ||
| description: |- | ||
| additionalAPIServerIPs is a list of additional IP addresses to contact | ||
| the Kubernetes API server on separate networks. These are used when a | ||
| user-managed load balancer exposes the API server on multiple networks | ||
| simultaneously. Each entry must be a valid IP address. This field is | ||
| only valid when loadBalancer.type is set to UserManaged. | ||
| items: | ||
| description: IP is an IP address (for example, "10.0.0.0" | ||
| or "fd00::"). | ||
| maxLength: 39 | ||
| minLength: 1 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: value must be a valid IP address | ||
| rule: isIP(self) | ||
| maxItems: 10 | ||
| type: array | ||
| x-kubernetes-list-type: atomic | ||
| additionalIngressIPs: | ||
| description: |- | ||
| additionalIngressIPs is a list of additional IP addresses that route to | ||
| the default ingress controller on separate networks. These are used when | ||
| a user-managed load balancer exposes the ingress controller on multiple | ||
| networks simultaneously. Each entry must be a valid IP address. This | ||
| field is only valid when loadBalancer.type is set to UserManaged. | ||
| items: | ||
| description: IP is an IP address (for example, "10.0.0.0" | ||
| or "fd00::"). | ||
| maxLength: 39 | ||
| minLength: 1 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: value must be a valid IP address | ||
| rule: isIP(self) | ||
| maxItems: 10 | ||
| type: array | ||
| x-kubernetes-list-type: atomic |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce the UserManaged precondition in CEL.
The field descriptions require loadBalancer.type to be UserManaged. No OpenStack CEL rule enforces this requirement. The API server can accept additional endpoint IPs with OpenShiftManagedDefault. This differs from the existing dnsRecordsType constraint.
Add a validation rule that rejects either additional list unless loadBalancer.type is UserManaged. Apply it to both the OpenStack spec and status schemas.
payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml#L501-L538: add the OpenStack spec CEL condition.payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml#L2358-L2383: add the OpenStack status CEL condition.payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml#L487-L524: add the OpenStack spec CEL condition.payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml#L2325-L2350: add the OpenStack status CEL condition.payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml#L780-L817: add the embedded Infrastructure spec CEL condition.payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml#L2631-L2656: add the embedded Infrastructure status CEL condition.
📍 Affects 3 files
payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml#L501-L538(this comment)payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml#L2358-L2383payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml#L487-L524payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml#L2325-L2350payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml#L780-L817payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml#L2631-L2656
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml`
around lines 501 - 538, Enforce the loadBalancer.type UserManaged precondition
for additionalAPIServerIPs and additionalIngressIPs by adding the corresponding
CEL validation to both fields in every listed schema location:
payload-manifests/crds/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml
ranges 501-538 and 2358-2383;
payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
ranges 487-524 and 2325-2350; and
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
ranges 780-817 and 2631-2656. Reject either list when loadBalancer.type is not
UserManaged, while preserving the existing IP validations.
|
@winiciusallan: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@winiciusallan Is there an enhancement proposal associated with this work? Adding new fields to an API requires a feature gate. In this particular scenario, I'd also push for the validation change approach to be feature gated to ensure that this change goes through the standard feature promotion process. Without more background knowledge here, this seems like a change we would want to make sure doesn't have a negative impact, or require changes, to core platform components. |
3 participants
In order to support multiple VIPs during an OpenShift installation on top of OpenStack, we need to propagate these VIPs in the Infrastructure CR, so API consumers can get these valeus to perform some operation.
To avoid removing the CEL validation on the existing fields (
APIServerInternalIPsandIngressVIPs) and breaking compatibility, I decided to add two new fields to handle these "additional" VIPs.