CORENET-6390: API changes and feature gate for OVN-Kubernetes no-overlay mode #2537
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @ricky-rav! Some important instructions when contributing to openshift/api: |
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
size/M
|
[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 |
|
@ricky-rav Is this ready for an initial round of API review? |
No no, it was just a draft for myself to keep track of the API changes in an enhancement I'm writing. |
ricky-rav
force-pushed
the
no_overlay_api
branch
from
da2a4f2 to
9bb3788
Compare
openshift-ci
bot
added
size/XXL
ricky-rav
changed the title
ricky-rav
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ricky-rav: This pull request references CORENET-6390 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 story to target the "4.21.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. |
ricky-rav
force-pushed
the
no_overlay_api
branch
2 times, most recently
from
905c086 to
00fcda8
Compare
📝 WalkthroughWalkthroughAdds the NoOverlayMode feature gate and registers FeatureGateNoOverlayMode. Introduces API enums and types: TransportOption, SNATOption, RoutingOption, BGPTopology, NoOverlayOptions, and BGPManagedConfig. Extends OVNKubernetesConfig with DefaultNetworkTransport, DefaultNetworkNoOverlayOptions, and BGPManagedConfig plus validation and immutability rules. Generates OpenAPI, Swagger, deepcopy artifacts, a CRD test manifest, updates payload feature-gate manifests, and adds a row to features.md. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-merge-robot
added
the
needs-rebase
ricky-rav
force-pushed
the
no_overlay_api
branch
2 times, most recently
from
8192eb2 to
d973281
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift/api#2537 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
ricky-rav
force-pushed
the
no_overlay_api
branch
from
1bc31a2 to
2b63885
Compare
openshift/api#2537 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
openshift/api#2537 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
ricky-rav
force-pushed
the
no_overlay_api
branch
2 times, most recently
from
90914d8 to
73d3699
Compare
|
/test all |
ricky-rav
force-pushed
the
no_overlay_api
branch
from
73d3699 to
f57d3ab
Compare
|
/test all |
ricky-rav
force-pushed
the
no_overlay_api
branch
from
a8bd5b5 to
6d4e2d4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @openapi/openapi.json:
- Around line 33535-33539: The schema sets an invalid empty default for
defaultNetworkNoOverlayOptions which references
com.github.openshift.api.operator.v1.NoOverlayOptions (which requires
outboundSNAT and routing); remove the "default": {} entry from the
defaultNetworkNoOverlayOptions definition so the field is truly optional (or
alternatively replace it with a schema-valid object containing outboundSNAT and
routing), updating the OpenAPI fragment that defines
defaultNetworkNoOverlayOptions accordingly.
- Around line 28607-28624: The BGPManagedConfig schema is missing validation:
update the "com.github.openshift.api.operator.v1.BGPManagedConfig" properties so
that "asNumber" includes "minimum": 1, "maximum": 4294967295 and "default":
64512, and "bgpTopology" includes an "enum": ["FullMesh"] to enforce the
documented allowed value; ensure you add these keys to the existing property
objects for asNumber and bgpTopology (referencing the asNumber and bgpTopology
property entries).
- Around line 33540-33543: The schema for the property defaultNetworkTransport
currently allows any string even though the description limits values to
"NoOverlay" and "Geneve"; add an enum constraint to the defaultNetworkTransport
schema (alongside its "type": "string") with values ["NoOverlay","Geneve"] so
validation enforces only those two options while leaving the existing
description intact.
- Around line 33530-33534: Remove the invalid "default": {} on the
bgpManagedConfig property so it no longer supplies an empty object that fails
schema validation against
definitions/com.github.openshift.api.operator.v1.BGPManagedConfig; locate the
bgpManagedConfig entry in the OpenAPI JSON and delete the "default" key (leaving
the description and $ref intact) so omission correctly represents "user does not
configure BGP".
- Around line 32996-33013: The NoOverlayOptions schema is missing enum
constraints for its string fields; update the properties for outboundSNAT and
routing within the NoOverlayOptions object to include the respective enum
arrays: add "enum": ["Enabled","Disabled"] to the outboundSNAT property and
"enum": ["Managed","Unmanaged"] to the routing property so the schema enforces
the allowed values described in their descriptions.
🧹 Nitpick comments (1)
operator/v1/types_network.go (1)
502-508: New NoOverlay/BGP types are well‑designed; consider an optional FRR guard
TransportOption,SNATOption,RoutingOption,BGPTopology,NoOverlayOptions, andBGPManagedConfigare all consistent with the documented behavior and the Swagger/OpenAPI surface (enums, defaults, and BGP topology immutability).One optional enhancement to consider: add a higher‑level (probably NetworkSpec‑scoped) FeatureGateAwareXValidation that requires
AdditionalRoutingCapabilities.providersto includeFRRwhenever you rely on managed BGP routing (e.g., default network in no‑overlay mode withrouting == "Managed"). That would surface misconfiguration at the API boundary rather than leaving all such errors to CNO, and would mirror the existing guard you have forrouteAdvertisements.Also applies to: 940-1013
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (6)
operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/networks.operator.openshift.io/NoOverlayMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (17)
features.mdfeatures/features.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonoperator/v1/tests/networks.operator.openshift.io/NoOverlayMode.yamloperator/v1/types_network.gooperator/v1/zz_generated.deepcopy.gooperator/v1/zz_generated.featuregated-crd-manifests.yamloperator/v1/zz_generated.swagger_doc_generated.gopayload-manifests/featuregates/featureGate-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- features/features.go
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml
- operator/v1/zz_generated.deepcopy.go
- payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
- operator/v1/tests/networks.operator.openshift.io/NoOverlayMode.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml
- features.md
- operator/v1/zz_generated.featuregated-crd-manifests.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
operator/v1/zz_generated.swagger_doc_generated.go (1)
operator/v1/types_network.go (2)
BGPManagedConfig(994-1014)NoOverlayOptions(973-991)
operator/v1/types_network.go (1)
operator/v1/zz_generated.swagger_doc_generated.go (2)
NoOverlayOptions(1915-1917)BGPManagedConfig(1678-1680)
🔇 Additional comments (13)
payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml (1)
38-40: LGTM - NoOverlayMode feature gate addition is correctly implemented across all profiles.The addition of NoOverlayMode to the disabled feature gates list in the SelfManagedHA-DevPreviewNoUpgrade profile is properly structured and maintains alphabetical ordering. Verification confirms that all feature gate manifests have been consistently updated:
- NoOverlayMode is correctly placed in the disabled list for DevPreviewNoUpgrade, Default, and OKD profiles
- NoOverlayMode is correctly placed in the enabled list for TechPreviewNoUpgrade variant
This aligns with the expected rollout strategy across all profile variants (both SelfManagedHA and Hypershift).
openapi/generated_openapi/zz_generated.openapi.go (5)
976-976: LGTM: Schema registrations.The new schema registrations for BGPManagedConfig and NoOverlayOptions are correctly added to the GetOpenAPIDefinitions map.
Also applies to: 1115-1115
57769-57769: LGTM: Dependencies correctly updated.The Dependencies list correctly includes the new BGPManagedConfig and NoOverlayOptions schemas.
56727-56753: The NoOverlayOptions source types already have correct enum validation tags.The source types in
operator/v1/types_network.goalready include the proper kubebuilder validation annotations:
outboundSNAThas+kubebuilder:validation:Enum=Enabled;Disabledroutinghas+kubebuilder:validation:Enum=Managed;UnmanagedHowever, the generated OpenAPI schema at lines 56734-56747 is missing the corresponding
Enumconstraints in the spec. The fields only specifyType: []string{"string"}without enum values.Regenerate the OpenAPI schema to ensure the kubebuilder validation tags are translated into
Enumfields in the generated spec.Likely an incorrect or invalid review comment.
49341-49367: Generated OpenAPI schema is missing validation constraints for BGPManagedConfig fields.The source Go types in
operator/v1/types_network.goalready contain proper kubebuilder validation markers:
asNumber:+kubebuilder:validation:Minimum=1and+kubebuilder:validation:Maximum=4294967295bgpTopology:+kubebuilder:validation:Enum=FullMeshHowever, the generated OpenAPI schema at lines 49341-49367 does not include these constraints:
asNumberlacksMinimumandMaximumfieldsbgpTopologylacks anEnumfieldThe constraints need to be reflected in the generated schema. Verify that the schema generation process properly translates kubebuilder validation markers to OpenAPI constraint fields. If the generation is working correctly, regenerate this file using the appropriate code generation commands.
57744-57764: Enum constraint and conditional requirements are already properly implemented in the source code.The source code at
operator/v1/types_network.goalready includes:
+kubebuilder:validation:Enum=NoOverlay;GeneveforDefaultNetworkTransport- Feature-gated XValidation rules enforcing the conditional requirements:
defaultNetworkNoOverlayOptionsis required whendefaultNetworkTransportis "NoOverlay"bgpManagedConfigis required whendefaultNetworkNoOverlayOptions.Routingis "Managed"- Comprehensive test coverage in
operator/v1/tests/networks.operator.openshift.io/NoOverlayMode.yamlHowever, the generated OpenAPI schema does not reflect the Enum constraint from the
TransportOptiontype alias, which appears to be a code generation limitation with how kubebuilder handles Enum validation on string type aliases.Likely an incorrect or invalid review comment.
operator/v1/zz_generated.swagger_doc_generated.go (3)
1672-1680: BGPManagedConfig Swagger docs match the type semanticsThe asNumber and bgpTopology descriptions align with the validation (range, default, and immutability) and the Go type docs; nothing to change here.
1909-1917: NoOverlayOptions Swagger documentation is consistent with the APIThe descriptions for outboundSNAT and routing correctly reflect the allowed enum values, requiredness, and behavioral notes from
NoOverlayOptionsintypes_network.go.
1919-1936: OVNKubernetesConfig doc entries correctly describe new no‑overlay/BGP fieldsThe strings for
defaultNetworkTransport,defaultNetworkNoOverlayOptions, andbgpManagedConfigmatch the struct comments and CEL validation rules (including when each is required and current defaults), so the generated Swagger surface looks coherent.operator/v1/types_network.go (2)
400-405: Maintainer note about DefaultNetworkTransport default is appropriateThe comment clearly explains why a schema-level default isn’t set yet and how to update it when NoOverlayMode graduates to GA; it documents intent without affecting current behavior.
408-412: NoOverlayMode cross‑field validation and immutability look correctThe FeatureGateAwareXValidation rules and field‑level XValidation on
DefaultNetworkTransport/DefaultNetworkNoOverlayOptionscorrectly enforce:
defaultNetworkNoOverlayOptionsis present whendefaultNetworkTransport == "NoOverlay".bgpManagedConfigis present whendefaultNetworkNoOverlayOptions.routing == "Managed".defaultNetworkTransportanddefaultNetworkNoOverlayOptionscannot be removed after being set, anddefaultNetworkTransportis immutable once non‑empty.This matches the field comments and keeps the TechPreview NoOverlay API well‑constrained.
Also applies to: 482-500
openapi/openapi.json (2)
33530-33543: Runtime validation for conditional requirements is properly implemented.The documented conditional requirements are enforced through feature-gated CEL validation rules in
operator/v1/types_network.go(lines 408-409):
defaultNetworkNoOverlayOptionsis validated as required whendefaultNetworkTransportis'NoOverlay'bgpManagedConfigis validated as required whendefaultNetworkNoOverlayOptions.routingis'Managed'Both rules use OpenShift's
FeatureGateAwareXValidationframework with theNoOverlayModefeature gate and provide clear error messages for violations. No further action needed.
10377-10377: Clarify intent of unrelated policyType description update.This description update for the
policyTypefield concerns signature policy verification (sigstore/Fulcio/Rekor), not OVN-Kubernetes networking. Verify whether this change is part of your PR scope or a merge artifact.
openshift/api#2537 openshift/client-go#349 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
openshift/api#2537 openshift/client-go#349 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
openshift/api#2537 openshift/client-go#349 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
ricky-rav
force-pushed
the
no_overlay_api
branch
from
6d4e2d4 to
dd22f3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @openapi/openapi.json:
- Around line 28599-28602: The schema for the property bgpTopology is missing an
enum constraint despite the description limiting values to "FullMesh"; update
the bgpTopology schema (property name: bgpTopology) to include an enum:
["FullMesh"] while keeping type: "string" and the existing description so
OpenAPI validation only accepts the allowed value.
- Around line 32988-32991: The "routing" property schema is missing an enum
constraint even though the description restricts values to "Managed" and
"Unmanaged"; update the OpenAPI schema for the routing field (the "routing"
property definition in openapi/openapi.json) to include an "enum" array with the
two allowed strings ["Managed","Unmanaged"] so validation rejects other values,
and leave immutability enforcement to controller logic as noted in the
description.
- Around line 33520-33523: The schema for the property defaultNetworkTransport
lacks an enum constraint; update the OpenAPI schema for the JSON property
"defaultNetworkTransport" to include an "enum" array with the two allowed values
["NoOverlay", "Geneve"] while keeping the existing description and type
unchanged; ensure the enum is added alongside "type": "string" for the property
named defaultNetworkTransport so validation will reject other values (keep
conditional requirements enforced elsewhere via CEL/admission webhook as noted).
- Around line 32984-32987: The outboundSNAT property schema is missing an enum
constraint despite the description limiting values to "Enabled" and "Disabled";
update the outboundSNAT definition in openapi.json (the property named
"outboundSNAT") to include "enum": ["Enabled", "Disabled"] so validation will
reject other strings, and ensure the description remains intact; if there are
shared schemas or other places referencing outboundSNAT, add or align the same
enum constraint there as well.
- Around line 33510-33519: The OpenAPI schema sets "default": {} for the
optional object refs bgpManagedConfig and defaultNetworkNoOverlayOptions which
contradicts their "when omitted" semantics and can mask missing required nested
fields (e.g., bgpTopology in
com.github.openshift.api.operator.v1.BGPManagedConfig); remove the "default": {}
entries for the properties named bgpManagedConfig and
defaultNetworkNoOverlayOptions in the openapi JSON so they remain truly
omittable while leaving the "$ref" and "description" intact.
🧹 Nitpick comments (2)
openapi/openapi.json (1)
28594-28598: Consider adding default value to the schema.The description mentions
asNumberdefaults to 64512, but the schema doesn't include adefaultproperty. Adding it improves OpenAPI client generation and documentation.📝 Proposed enhancement
"asNumber": { "description": "asNumber is the 2-byte or 4-byte Autonomous System Number (ASN) to be used in the generated FRR configuration. Valid values are 1 to 4294967295. When omitted, this defaults to 64512.", "type": "integer", - "format": "int64" + "format": "int64", + "default": 64512 }operator/v1/types_network.go (1)
400-405: FeatureGate-aware validation rules for NoOverlayMode are coherent
- The maintainer note around DefaultNetworkTransport defaulting and the four
FeatureGateAwareXValidationrules on OVNKubernetesConfig correctly model:
- Requirement of
defaultNetworkNoOverlayOptionswhendefaultNetworkTransport == "NoOverlay".- Requirement of
bgpManagedConfigwhendefaultNetworkNoOverlayOptions.routing == "Managed".- Preventing removal of
defaultNetworkTransportanddefaultNetworkNoOverlayOptionsonce set (while allowing initial set from empty).- Given that the same
NoOverlayModefeature-gate is also used on the new fields, these validations won’t interfere when the gate is disabled.One subtle, likely intentional behavior:
bgpManagedConfigmay be present even if the default network’s routing isUnmanaged(e.g., to support CUDNs withrouting="Managed"). If you instead want to forbidbgpManagedConfigwhen there is no Managed no-overlay use at all, a symmetric CEL guard would be needed; as-is, the API is more permissive but functionally safe.Also applies to: 408-412
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (6)
operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/networks.operator.openshift.io/NoOverlayMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (17)
features.mdfeatures/features.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonoperator/v1/tests/networks.operator.openshift.io/NoOverlayMode.yamloperator/v1/types_network.gooperator/v1/zz_generated.deepcopy.gooperator/v1/zz_generated.featuregated-crd-manifests.yamloperator/v1/zz_generated.swagger_doc_generated.gopayload-manifests/featuregates/featureGate-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
🚧 Files skipped from review as they are similar to previous changes (10)
- payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml
- operator/v1/zz_generated.featuregated-crd-manifests.yaml
- operator/v1/zz_generated.deepcopy.go
- payload-manifests/featuregates/featureGate-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml
- operator/v1/tests/networks.operator.openshift.io/NoOverlayMode.yaml
- features.md
- openapi/generated_openapi/zz_generated.openapi.go
🧰 Additional context used
🧬 Code graph analysis (3)
operator/v1/types_network.go (1)
operator/v1/zz_generated.swagger_doc_generated.go (2)
NoOverlayOptions(1915-1917)BGPManagedConfig(1678-1680)
features/features.go (1)
config/v1/types_feature.go (1)
TechPreviewNoUpgrade(45-45)
operator/v1/zz_generated.swagger_doc_generated.go (1)
operator/v1/types_network.go (2)
BGPManagedConfig(996-1014)NoOverlayOptions(973-993)
🔇 Additional comments (7)
features/features.go (1)
198-204: LGTM! Feature gate implementation follows established patterns.The feature gate declaration is correctly structured and includes all required metadata:
- Appropriate Jira component and contact person
- Links to the enhancement PR
- Enabled only in TechPreviewNoUpgrade, which is suitable for a new feature
- Consistent with other feature gates in the file
payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml (1)
277-279: Enable NoOverlayMode for SelfManagedHA TechPreview looks correctNoOverlayMode is appropriately added to the enabled list for the TechPreviewNoUpgrade feature set; ordering among peers is fine and has no semantic impact.
payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml (1)
186-188: NoOverlayMode correctly defaulted to disabled in SelfManagedHA-DefaultListing NoOverlayMode in the disabled block for the Default feature set matches expected lifecycle for a TechPreview gate.
payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml (1)
280-282: Hypershift TechPreview enabling of NoOverlayMode is consistentAdding NoOverlayMode to the enabled list for the Hypershift TechPreviewNoUpgrade profile aligns with the SelfManagedHA TechPreview behavior.
operator/v1/zz_generated.swagger_doc_generated.go (1)
1672-1680: Swagger docs correctly wired to new NoOverlay/BGP types and OVN fieldsThe new SwaggerDoc maps for BGPManagedConfig and NoOverlayOptions, plus the extended OVNKubernetesConfig map entries for defaultNetworkTransport, defaultNetworkNoOverlayOptions, and bgpManagedConfig, all match the Go type definitions and comments in operator/v1/types_network.go. JSON field names and semantics (requiredness, immutability notes, and Managed/Unmanaged/BGP behavior) are consistent.
Also applies to: 1909-1917, 1919-1935
operator/v1/types_network.go (2)
482-508: New OVNKubernetesConfig fields model no-overlay and BGP state with appropriate immutability
DefaultNetworkTransport:
- Enum-restricted to
NoOverlay/Geneveand immutable after first non-empty set (oldSelf == '' || self == oldSelf), plus an object-level guard preventing removal once non-empty. This gives a one-shot choice of overlay vs no-overlay, which matches the intended semantics.DefaultNetworkNoOverlayOptions:
- Uses
NoOverlayOptions(with requiredoutboundSNATandrouting) andomitzeroto avoid emitting an all-zero struct.- Separate rule ensures it can’t be removed once present, while allowing changes to
OutboundSNATand keepingRoutingimmutable via the nested XValidation.BGPManagedConfig:
- Global config used when routing is
Managed. The field is optional, but required in the default-network-managed case via the earlier XValidation.- The field-level XValidation
!oldSelf.hasValue() || self == oldSelf.value()withoptionalOldSelf=truemakes the config immutable once first set, including preventing later removal, which is consistent with BGP topology stability expectations.The combination of
,omitzero,omitemptyand the CELhas(...)checks is correct here because valid instances of these structs must have non-zero content due to their own required fields.
940-971: Option types and NoOverlay/BGP config structs are well-typed and validate the documented constraints
- Introducing
TransportOption,SNATOption,RoutingOption, andBGPTopologyplus the associated constants keeps the public API strongly typed and self-documenting.NoOverlayOptions:
- Restricts
outboundSNATtoEnabled/DisabledandroutingtoManaged/Unmanagedwith enum validations.- Makes
routingexplicitly immutable (self == oldSelf), which aligns with the comment and with the higher-level invariants around BGP vs user-managed routing.BGPManagedConfig:
- ASN range 1–4294967295 with a default of 64512 is correctly enforced via
Minimum,Maximum, anddefault.bgpTopologyis required and enum-limited toFullMesh, matching the constantBGPTopologyFullMesh.Overall this block is consistent in style with existing network API types and gives controllers enough structure to reason about no-overlay mode and managed BGP configuration.
Also applies to: 972-994, 995-1014
| "bgpTopology": { | ||
| "description": "bgpTopology defines the BGP topology to be used. Allowed values are \"FullMesh\". When set to \"FullMesh\", every node deploys a BGP router, forming a BGP full mesh. This field is required when BGPManagedConfig is specified.", | ||
| "type": "string" | ||
| } |
There was a problem hiding this comment.
Add enum constraint for bgpTopology.
The description specifies that bgpTopology only allows "FullMesh", but the schema lacks an enum constraint. This allows invalid values to pass OpenAPI validation.
🛡️ Proposed fix to add enum constraint
"bgpTopology": {
"description": "bgpTopology defines the BGP topology to be used. Allowed values are \"FullMesh\". When set to \"FullMesh\", every node deploys a BGP router, forming a BGP full mesh. This field is required when BGPManagedConfig is specified.",
- "type": "string"
+ "type": "string",
+ "enum": ["FullMesh"]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "bgpTopology": { | |
| "description": "bgpTopology defines the BGP topology to be used. Allowed values are \"FullMesh\". When set to \"FullMesh\", every node deploys a BGP router, forming a BGP full mesh. This field is required when BGPManagedConfig is specified.", | |
| "type": "string" | |
| } | |
| "bgpTopology": { | |
| "description": "bgpTopology defines the BGP topology to be used. Allowed values are \"FullMesh\". When set to \"FullMesh\", every node deploys a BGP router, forming a BGP full mesh. This field is required when BGPManagedConfig is specified.", | |
| "type": "string", | |
| "enum": ["FullMesh"] | |
| } |
🤖 Prompt for AI Agents
In @openapi/openapi.json around lines 28599 - 28602, The schema for the property
bgpTopology is missing an enum constraint despite the description limiting
values to "FullMesh"; update the bgpTopology schema (property name: bgpTopology)
to include an enum: ["FullMesh"] while keeping type: "string" and the existing
description so OpenAPI validation only accepts the allowed value.
| "outboundSNAT": { | ||
| "description": "outboundSNAT defines the SNAT behavior for outbound traffic from pods. Allowed values are \"Enabled\" and \"Disabled\". When set to \"Enabled\", SNAT is performed on outbound traffic from pods. When set to \"Disabled\", SNAT is not performed and pod IPs are preserved in outbound traffic. This field is required when the network operates in no-overlay mode.", | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
Add enum constraint for outboundSNAT.
The description specifies allowed values "Enabled" and "Disabled", but the schema lacks an enum constraint, allowing invalid values to pass validation.
🛡️ Proposed fix to add enum constraint
"outboundSNAT": {
"description": "outboundSNAT defines the SNAT behavior for outbound traffic from pods. Allowed values are \"Enabled\" and \"Disabled\". When set to \"Enabled\", SNAT is performed on outbound traffic from pods. When set to \"Disabled\", SNAT is not performed and pod IPs are preserved in outbound traffic. This field is required when the network operates in no-overlay mode.",
- "type": "string"
+ "type": "string",
+ "enum": ["Enabled", "Disabled"]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "outboundSNAT": { | |
| "description": "outboundSNAT defines the SNAT behavior for outbound traffic from pods. Allowed values are \"Enabled\" and \"Disabled\". When set to \"Enabled\", SNAT is performed on outbound traffic from pods. When set to \"Disabled\", SNAT is not performed and pod IPs are preserved in outbound traffic. This field is required when the network operates in no-overlay mode.", | |
| "type": "string" | |
| }, | |
| "outboundSNAT": { | |
| "description": "outboundSNAT defines the SNAT behavior for outbound traffic from pods. Allowed values are \"Enabled\" and \"Disabled\". When set to \"Enabled\", SNAT is performed on outbound traffic from pods. When set to \"Disabled\", SNAT is not performed and pod IPs are preserved in outbound traffic. This field is required when the network operates in no-overlay mode.", | |
| "type": "string", | |
| "enum": ["Enabled", "Disabled"] | |
| }, |
🤖 Prompt for AI Agents
In @openapi/openapi.json around lines 32984 - 32987, The outboundSNAT property
schema is missing an enum constraint despite the description limiting values to
"Enabled" and "Disabled"; update the outboundSNAT definition in openapi.json
(the property named "outboundSNAT") to include "enum": ["Enabled", "Disabled"]
so validation will reject other strings, and ensure the description remains
intact; if there are shared schemas or other places referencing outboundSNAT,
add or align the same enum constraint there as well.
| "routing": { | ||
| "description": "routing specifies whether the pod network routing is managed by OVN-Kubernetes or users. Allowed values are \"Managed\" and \"Unmanaged\". When set to \"Managed\", OVN-Kubernetes manages the pod network routing configuration through BGP. When set to \"Unmanaged\", users are responsible for configuring the pod network routing. This field is required when the network operates in no-overlay mode. This field is immutable once set.", | ||
| "type": "string" | ||
| } |
There was a problem hiding this comment.
Add enum constraint for routing.
The description specifies allowed values "Managed" and "Unmanaged", but the schema lacks an enum constraint, allowing invalid values to pass validation.
Note: The immutability constraint mentioned in the description must be enforced by controller validation logic, as OpenAPI schemas cannot express immutability.
🛡️ Proposed fix to add enum constraint
"routing": {
"description": "routing specifies whether the pod network routing is managed by OVN-Kubernetes or users. Allowed values are \"Managed\" and \"Unmanaged\". When set to \"Managed\", OVN-Kubernetes manages the pod network routing configuration through BGP. When set to \"Unmanaged\", users are responsible for configuring the pod network routing. This field is required when the network operates in no-overlay mode. This field is immutable once set.",
- "type": "string"
+ "type": "string",
+ "enum": ["Managed", "Unmanaged"]
}🤖 Prompt for AI Agents
In @openapi/openapi.json around lines 32988 - 32991, The "routing" property
schema is missing an enum constraint even though the description restricts
values to "Managed" and "Unmanaged"; update the OpenAPI schema for the routing
field (the "routing" property definition in openapi/openapi.json) to include an
"enum" array with the two allowed strings ["Managed","Unmanaged"] so validation
rejects other values, and leave immutability enforcement to controller logic as
noted in the description.
| "bgpManagedConfig": { | ||
| "description": "bgpManagedConfig configures the BGP properties for networks (default network or CUDNs) in no-overlay mode that specify routing=\"Managed\" in their NoOverlayOptions. It is required when DefaultNetworkNoOverlayOptions.Routing is set to \"Managed\". When omitted, this means the user does not configure BGP for managed routing.", | ||
| "default": {}, | ||
| "$ref": "#/definitions/com.github.openshift.api.operator.v1.BGPManagedConfig" | ||
| }, | ||
| "defaultNetworkNoOverlayOptions": { | ||
| "description": "defaultNetworkNoOverlayOptions contains configuration for no-overlay mode for the default network. It is required when DefaultNetworkTransport is \"NoOverlay\". When omitted, this means the user does not configure no-overlay mode options.", | ||
| "default": {}, | ||
| "$ref": "#/definitions/com.github.openshift.api.operator.v1.NoOverlayOptions" | ||
| }, |
There was a problem hiding this comment.
Remove improper default values for optional object references.
Both bgpManagedConfig and defaultNetworkNoOverlayOptions have "default": {}, which conflicts with their "when omitted" semantics described in the documentation. These fields should be fully optional (omittable), not defaulting to empty objects.
The descriptions state:
- "When omitted, this means the user does not configure BGP for managed routing."
- "When omitted, this means the user does not configure no-overlay mode options."
An empty object {} is not the same as an omitted field. If these fields are present with empty values, it may trigger validation errors (e.g., required nested fields in BGPManagedConfig like bgpTopology).
🔧 Proposed fix to remove default values
"bgpManagedConfig": {
"description": "bgpManagedConfig configures the BGP properties for networks (default network or CUDNs) in no-overlay mode that specify routing=\"Managed\" in their NoOverlayOptions. It is required when DefaultNetworkNoOverlayOptions.Routing is set to \"Managed\". When omitted, this means the user does not configure BGP for managed routing.",
- "default": {},
"$ref": "#/definitions/com.github.openshift.api.operator.v1.BGPManagedConfig"
},
"defaultNetworkNoOverlayOptions": {
"description": "defaultNetworkNoOverlayOptions contains configuration for no-overlay mode for the default network. It is required when DefaultNetworkTransport is \"NoOverlay\". When omitted, this means the user does not configure no-overlay mode options.",
- "default": {},
"$ref": "#/definitions/com.github.openshift.api.operator.v1.NoOverlayOptions"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "bgpManagedConfig": { | |
| "description": "bgpManagedConfig configures the BGP properties for networks (default network or CUDNs) in no-overlay mode that specify routing=\"Managed\" in their NoOverlayOptions. It is required when DefaultNetworkNoOverlayOptions.Routing is set to \"Managed\". When omitted, this means the user does not configure BGP for managed routing.", | |
| "default": {}, | |
| "$ref": "#/definitions/com.github.openshift.api.operator.v1.BGPManagedConfig" | |
| }, | |
| "defaultNetworkNoOverlayOptions": { | |
| "description": "defaultNetworkNoOverlayOptions contains configuration for no-overlay mode for the default network. It is required when DefaultNetworkTransport is \"NoOverlay\". When omitted, this means the user does not configure no-overlay mode options.", | |
| "default": {}, | |
| "$ref": "#/definitions/com.github.openshift.api.operator.v1.NoOverlayOptions" | |
| }, | |
| "bgpManagedConfig": { | |
| "description": "bgpManagedConfig configures the BGP properties for networks (default network or CUDNs) in no-overlay mode that specify routing=\"Managed\" in their NoOverlayOptions. It is required when DefaultNetworkNoOverlayOptions.Routing is set to \"Managed\". When omitted, this means the user does not configure BGP for managed routing.", | |
| "$ref": "#/definitions/com.github.openshift.api.operator.v1.BGPManagedConfig" | |
| }, | |
| "defaultNetworkNoOverlayOptions": { | |
| "description": "defaultNetworkNoOverlayOptions contains configuration for no-overlay mode for the default network. It is required when DefaultNetworkTransport is \"NoOverlay\". When omitted, this means the user does not configure no-overlay mode options.", | |
| "$ref": "#/definitions/com.github.openshift.api.operator.v1.NoOverlayOptions" | |
| }, |
🤖 Prompt for AI Agents
In @openapi/openapi.json around lines 33510 - 33519, The OpenAPI schema sets
"default": {} for the optional object refs bgpManagedConfig and
defaultNetworkNoOverlayOptions which contradicts their "when omitted" semantics
and can mask missing required nested fields (e.g., bgpTopology in
com.github.openshift.api.operator.v1.BGPManagedConfig); remove the "default": {}
entries for the properties named bgpManagedConfig and
defaultNetworkNoOverlayOptions in the openapi JSON so they remain truly
omittable while leaving the "$ref" and "description" intact.
| "defaultNetworkTransport": { | ||
| "description": "defaultNetworkTransport describes the transport protocol for east-west traffic for the default network. Allowed values are \"NoOverlay\" and \"Geneve\". When set to \"NoOverlay\", the default network operates in no-overlay mode. When set to \"Geneve\", the default network uses Geneve overlay. When omitted, this means the user has no opinion and the platform chooses a reasonable default which is subject to change over time. The current default is \"Geneve\".", | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
Add enum constraint for defaultNetworkTransport.
The description specifies allowed values "NoOverlay" and "Geneve", but the schema lacks an enum constraint, allowing invalid values to pass validation.
Note: The conditional requirements described in the parent object's documentation (e.g., "defaultNetworkNoOverlayOptions is required when defaultNetworkTransport is NoOverlay") cannot be expressed in OpenAPI schemas and must be enforced through CEL validation rules or admission webhooks.
🛡️ Proposed fix to add enum constraint
"defaultNetworkTransport": {
"description": "defaultNetworkTransport describes the transport protocol for east-west traffic for the default network. Allowed values are \"NoOverlay\" and \"Geneve\". When set to \"NoOverlay\", the default network operates in no-overlay mode. When set to \"Geneve\", the default network uses Geneve overlay. When omitted, this means the user has no opinion and the platform chooses a reasonable default which is subject to change over time. The current default is \"Geneve\".",
- "type": "string"
+ "type": "string",
+ "enum": ["NoOverlay", "Geneve"]
}🤖 Prompt for AI Agents
In @openapi/openapi.json around lines 33520 - 33523, The schema for the property
defaultNetworkTransport lacks an enum constraint; update the OpenAPI schema for
the JSON property "defaultNetworkTransport" to include an "enum" array with the
two allowed values ["NoOverlay", "Geneve"] while keeping the existing
description and type unchanged; ensure the enum is added alongside "type":
"string" for the property named defaultNetworkTransport so validation will
reject other values (keep conditional requirements enforced elsewhere via
CEL/admission webhook as noted).
openshift/api#2537 openshift/client-go#349 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
ricky-rav
force-pushed
the
no_overlay_api
branch
2 times, most recently
from
4c69d69 to
b3e455e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@operator/v1/types_network.go`:
- Around line 408-412: Add inverse validation rules to OVNKubernetesConfig to
forbid defaultNetworkNoOverlayOptions and bgpManagedConfig unless
defaultNetworkTransport == 'NoOverlay': add FeatureGateAwareXValidation rules
that require !has(self.defaultNetworkNoOverlayOptions) ||
self.defaultNetworkTransport == 'NoOverlay' with an appropriate message, and
!has(self.bgpManagedConfig) || self.defaultNetworkTransport == 'NoOverlay' (or
tie bgpManagedConfig presence to defaultNetworkNoOverlayOptions.routing ==
'Managed' and transport == 'NoOverlay') so those fields are rejected when
transport is Geneve or unset; reference the OVNKubernetesConfig type and the
fields defaultNetworkTransport, defaultNetworkNoOverlayOptions, and
bgpManagedConfig when adding the rules.
♻️ Duplicate comments (3)
openapi/openapi.json (3)
28644-28661: Generated schema acknowledged.This BGPManagedConfig schema is auto-generated from
operator/v1/types_network.go. The schema structure correctly reflects the requiredbgpTopologyfield and optionalasNumberwith documented ASN range.Previous review comments about adding enum and min/max constraints were addressed by the author clarifying that any such changes need to be made in the source Go types file, not in this generated output.
33033-33050: Generated schema acknowledged.This NoOverlayOptions schema is auto-generated. The structure correctly requires both
outboundSNATandroutingfields, with allowed values documented in descriptions.Previous comments about enum constraints were addressed—any validation enhancements belong in the source Go types.
33567-33580: Generated schema acknowledged.These OVNKubernetesConfig extensions are auto-generated. Previous comments about
"default": {}values for object references and missing enum constraints fordefaultNetworkTransportwere addressed by the author—any changes need to be made in the source Go types file (operator/v1/types_network.go).
🧹 Nitpick comments (3)
operator/v1/tests/networks.operator.openshift.io/NoOverlayMode.yaml (1)
8-117: Add a “forbidden unless transport=NoOverlay” negative test to prevent ambiguous configsRight now the suite asserts “required when NoOverlay,” but it doesn’t assert the inverse (“must not be set when Geneve”). If the API is intended to reject
defaultNetworkNoOverlayOptions(andbgpManagedConfig) unlessdefaultNetworkTransport: NoOverlay, add a test case for that. This will prevent silently-ignored/contradictory configs.Proposed test addition
tests: onCreate: + - name: Should fail if NoOverlayOptions are set when transport is Geneve + initial: | + apiVersion: operator.openshift.io/v1 + kind: Network + spec: + defaultNetwork: + ovnKubernetesConfig: + defaultNetworkTransport: Geneve + defaultNetworkNoOverlayOptions: + outboundSNAT: Enabled + routing: Unmanaged + expectedError: "defaultNetworkNoOverlayOptions is forbidden when defaultNetworkTransport is not NoOverlay"operator/v1/zz_generated.swagger_doc_generated.go (1)
1672-1680: Swagger docs: consider reflectingbgpManagedConfigimmutability in the source comments
OVNKubernetesConfig.bgpManagedConfigis enforced immutable inoperator/v1/types_network.go(Line 506-508), but the docstring here doesn’t hint at that. Since this file is generated, the fix should be updating the field comment intypes_network.goand re-running generation.operator/v1/types_network.go (1)
482-492: Consider makingdefaultNetworkTransportimmutability rule robust to “unset → set later” edge-casesThe current rule (Line 490) relies on
oldSelf == ''as the “unset” sentinel. If the field is omitted (not an explicit empty string), CEL optional semantics can be subtle across Kubernetes versions. Adding a focused test for “create with field omitted, then set it” (and/or switching tooptionalOldSelf=true+hasValue()style) would make this safer.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (6)
operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/networks.operator.openshift.io/NoOverlayMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (17)
features.mdfeatures/features.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonoperator/v1/tests/networks.operator.openshift.io/NoOverlayMode.yamloperator/v1/types_network.gooperator/v1/zz_generated.deepcopy.gooperator/v1/zz_generated.featuregated-crd-manifests.yamloperator/v1/zz_generated.swagger_doc_generated.gopayload-manifests/featuregates/featureGate-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- features/features.go
- payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml
- operator/v1/zz_generated.deepcopy.go
- payload-manifests/featuregates/featureGate-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml
- operator/v1/zz_generated.featuregated-crd-manifests.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
openapi/generated_openapi/zz_generated.openapi.go (1)
config/v1/types_feature.go (1)
Default(41-41)
operator/v1/zz_generated.swagger_doc_generated.go (1)
operator/v1/types_network.go (2)
BGPManagedConfig(998-1016)NoOverlayOptions(973-995)
🔇 Additional comments (10)
payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml (1)
38-40: LGTM!The
NoOverlayModefeature gate is correctly added to the disabled list, maintaining alphabetical ordering and consistent with the feature being enabled only in TechPreviewNoUpgrade profiles.payload-manifests/featuregates/featureGate-Hypershift-OKD.yaml (1)
200-202: LGTM!The
NoOverlayModefeature gate is correctly added to the disabled list for the OKD profile, maintaining proper alphabetical ordering and consistency with the feature gate definition.features.md (1)
17-17: LGTM!The
NoOverlayModefeature gate documentation is correctly added, showing enablement only in TechPreviewNoUpgrade profiles for both Hypershift and SelfManagedHA. This is consistent with the corresponding YAML manifest changes where the feature is added to disabled lists in Default, DevPreviewNoUpgrade, and OKD profiles.openapi/generated_openapi/zz_generated.openapi.go (4)
979-979: LGTM!The new type registrations for
BGPManagedConfigandNoOverlayOptionsare correctly placed in alphabetical order and properly reference the schema functions defined later in this generated file.Also applies to: 1118-1118
49545-49571: LGTM!The
BGPManagedConfigschema is correctly generated:
asNumberusesint64format appropriate for 4-byte ASN valuesbgpTopologyis marked as required- Descriptions accurately document the allowed values and defaults
Value range validation for
asNumberand enum constraints forbgpTopologyare expected to be enforced via validation markers in the source Go types.
56931-56957: LGTM!The
NoOverlayOptionsschema is correctly generated with bothoutboundSNATandroutingmarked as required. The descriptions clearly document the allowed values and mutability constraints (SNAT is mutable post-install, routing is immutable once set).
57948-57975: LGTM!The
OVNKubernetesConfigschema extensions are correctly generated:
- Three new fields added with appropriate descriptions documenting conditional requirements
Reftypes properly reference the new schemas with standardDefault: map[string]interface{}{}- Dependencies array correctly updated to include
BGPManagedConfigandNoOverlayOptionsThe conditional validation logic (e.g.,
defaultNetworkNoOverlayOptionsrequired when transport is "NoOverlay") is expected to be enforced via CEL validation rules in the source Go types.operator/v1/zz_generated.swagger_doc_generated.go (1)
1909-1936: Docs look consistent for NoOverlayOptions + new OVNKubernetesConfig fieldsThe descriptions for
defaultNetworkTransport,defaultNetworkNoOverlayOptions, andbgpManagedConfigread coherently and match the high-level intent. Any semantic tweaks (e.g., forbidding options when transport!=NoOverlay) should be reflected by updatingtypes_network.gocomments and regenerating.operator/v1/types_network.go (1)
400-405: Maintainer note is clear and usefulCaptures why the default isn’t expressed in-schema yet and what to do at graduation.
operator/v1/tests/networks.operator.openshift.io/NoOverlayMode.yaml (1)
138-174: Verify alignment between test assertions and intended bgpManagedConfig day-2 mutabilityThe onUpdate test (lines 198-224) should match the intended behavior: if day-2 ASN changes are allowed, the test must assert
bgpManagedConfig.asNumberis mutable (or onlybgpTopologyis immutable); if day-2 changes are disallowed, the current immutability assertion is correct. Cross-reference the test with the schema inoperator/v1/types_network.goand PR discussion history to confirm alignment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
openshift/api#2537 openshift/client-go#349 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
openshift/api#2537 openshift/client-go#349 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
ricky-rav
force-pushed
the
no_overlay_api
branch
from
b3e455e to
162b45b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openapi/generated_openapi/zz_generated.openapi.go`:
- Around line 56931-56954: The generated OpenAPI definition in
schema_openshift_api_operator_v1_NoOverlayOptions is missing Enum constraints
for the outboundSNAT and routing properties; update the OpenAPIDefinition so
each spec.SchemaProps for "outboundSNAT" and "routing" includes Enum:
[]interface{}{"Enabled","Disabled"} and Enum:
[]interface{}{"Managed","Unmanaged"} respectively (i.e., add the Enum entries to
the SchemaProps in the OpenAPIDefinition returned by
schema_openshift_api_operator_v1_NoOverlayOptions) and then regenerate the
OpenAPI artifacts so the OpenAPIDefinition/SchemaProps reflect the kubebuilder
+kubebuilder:validation:Enum annotations.
- Around line 57948-57967: The OpenAPI output is missing the Enum for
defaultNetworkTransport and is incorrectly emitting empty-object defaults for
complex optional fields; update the generation so that the schema for
"defaultNetworkTransport" includes Enum: []string{"NoOverlay","Geneve"} (match
the +kubebuilder:validation:Enum tag) and remove the Default:
map[string]interface{}{} entries for "defaultNetworkNoOverlayOptions" and
"bgpManagedConfig" so they remain optional with no default; ensure the refs
still point to NoOverlayOptions and BGPManagedConfig (Ref:
ref("github.com/openshift/api/operator/v1.NoOverlayOptions") and
ref("github.com/openshift/api/operator/v1.BGPManagedConfig")) and adjust the
OpenAPI generator or struct tags so required subfields inside those types are
validated only when the object is present.
♻️ Duplicate comments (4)
operator/v1/tests/networks.operator.openshift.io/NoOverlayMode.yaml (1)
240-265: bgpManagedConfig immutability expectation conflicts with current validation.This update only changes
asNumber, but current API validation only makesbgpTopologyimmutable;asNumberremains mutable. Either enforce full immutability in the API or relax this test accordingly.openapi/openapi.json (3)
28702-28719: BGPManagedConfig type definition appears correctly generated.This type is auto-generated from the Go source types. The structure with required
bgpTopologyand optionalasNumberfields matches the documented API design. Any validation constraints (enum values, numeric ranges) would need to be added via CEL validation rules or kubebuilder markers in the source Go types.
33091-33108: NoOverlayOptions type definition appears correctly generated.This type is auto-generated from Go source types. The structure with both
outboundSNATandroutingas required fields aligns with the no-overlay mode requirements described in the documentation.
33625-33638: OVNKubernetesConfig extensions for no-overlay mode look correctly generated.The new fields (
bgpManagedConfig,defaultNetworkNoOverlayOptions,defaultNetworkTransport) are properly integrated into the OVNKubernetesConfig schema. The"default": {}pattern is a known behavior from kubebuilder's OpenAPI generation for optional struct pointer fields.
🧹 Nitpick comments (1)
operator/v1/types_network.go (1)
408-411: Consider adding inverse validation to forbid no-overlay fields when transport is not NoOverlay.The current rules require
defaultNetworkNoOverlayOptionswhen transport isNoOverlay, but don't forbid these fields when transport isGeneveor omitted. This could allow contradictory configurations wheredefaultNetworkTransport: Geneveis set alongsidedefaultNetworkNoOverlayOptionsorbgpManagedConfig.Proposed additional validations
// +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(self.defaultNetworkTransport) || self.defaultNetworkTransport != 'NoOverlay' || has(self.defaultNetworkNoOverlayOptions)",message="defaultNetworkNoOverlayOptions is required when defaultNetworkTransport is NoOverlay" +// +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(self.defaultNetworkNoOverlayOptions) || (has(self.defaultNetworkTransport) && self.defaultNetworkTransport == 'NoOverlay')",message="defaultNetworkNoOverlayOptions is forbidden when defaultNetworkTransport is not NoOverlay" +// +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(self.bgpManagedConfig) || (has(self.defaultNetworkTransport) && self.defaultNetworkTransport == 'NoOverlay')",message="bgpManagedConfig is forbidden when defaultNetworkTransport is not NoOverlay" // +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(self.defaultNetworkNoOverlayOptions) || self.defaultNetworkNoOverlayOptions.routing != 'Managed' || has(self.bgpManagedConfig)",message="bgpManagedConfig is required when defaultNetworkNoOverlayOptions.routing is Managed"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (6)
operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/networks.operator.openshift.io/NoOverlayMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (17)
features.mdfeatures/features.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonoperator/v1/tests/networks.operator.openshift.io/NoOverlayMode.yamloperator/v1/types_network.gooperator/v1/zz_generated.deepcopy.gooperator/v1/zz_generated.featuregated-crd-manifests.yamloperator/v1/zz_generated.swagger_doc_generated.gopayload-manifests/featuregates/featureGate-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- operator/v1/zz_generated.featuregated-crd-manifests.yaml
- payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
- features.md
- payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
🧰 Additional context used
🧬 Code graph analysis (4)
operator/v1/zz_generated.deepcopy.go (2)
operator/v1/types_network.go (2)
BGPManagedConfig(1000-1018)NoOverlayOptions(975-997)operator/v1/zz_generated.swagger_doc_generated.go (2)
BGPManagedConfig(1678-1680)NoOverlayOptions(1915-1917)
features/features.go (1)
config/v1/types_feature.go (1)
TechPreviewNoUpgrade(45-45)
operator/v1/zz_generated.swagger_doc_generated.go (1)
operator/v1/types_network.go (2)
BGPManagedConfig(1000-1018)NoOverlayOptions(975-997)
operator/v1/types_network.go (1)
operator/v1/zz_generated.swagger_doc_generated.go (2)
NoOverlayOptions(1915-1917)BGPManagedConfig(1678-1680)
🔇 Additional comments (33)
payload-manifests/featuregates/featureGate-Hypershift-Default.yaml (1)
198-200: NoOverlayMode disabled entry looks consistent.
Keeps the default Hypershift profile explicit about the gate state.features/features.go (1)
198-204: Confirm TechPreview enablement scope is intentional.
enableIn(configv1.TechPreviewNoUpgrade)applies across cluster profiles; if NoOverlayMode should be limited to specific profiles, please confirm or switch toenableForClusterProfile.payload-manifests/featuregates/featureGate-Hypershift-OKD.yaml (1)
200-202: NoOverlayMode disabled entry looks consistent for OKD.payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml (1)
50-52: NoOverlayMode disabled entry looks consistent for DevPreviewNoUpgrade.operator/v1/zz_generated.deepcopy.go (3)
393-407: BGPManagedConfig deepcopy additions look good.
3684-3698: NoOverlayOptions deepcopy additions look good.
4193-4194: Verify copy/optionality semantics for new OVNKubernetesConfig fields.
If these fields are pointers, they should be deep-copied with nil checks; if value types, confirm they’re intended to be always present even when no-overlay is off.openapi/generated_openapi/zz_generated.openapi.go (3)
976-980: Schema registration looks correct.
1115-1119: Schema registration looks correct.
49545-49569: Add explicit schema constraints forBGPManagedConfig.The schema description declares an ASN range (1 to 4294967295) and allowed
bgpTopologyvalues ("FullMesh"), but the generated OpenAPI schema does not enforce them. Please verify that kubebuilder validation annotations (Minimum/Maximum forasNumber, enum forbgpTopology) exist in the source type definition, and if missing, add them and regenerate the OpenAPI.operator/v1/tests/networks.operator.openshift.io/NoOverlayMode.yaml (11)
1-6: Feature gate wiring looks good.
8-27: Geneve default-path create test is solid.
28-36: Negative test for missing NoOverlay options is clear.
37-63: Unmanaged no-overlay create test is clear.
64-74: Install-time outboundSNAT constraint test is clear.
75-86: Managed routing requires BGP config test is good.
87-117: Managed routing happy-path test looks good.
119-137: defaultNetworkTransport immutability test is appropriate.
138-174: Day‑2 outboundSNAT update test is clear.
175-197: Routing immutability update test is clear.
198-239: Day‑2 enabling of bgpManagedConfig is covered.operator/v1/zz_generated.swagger_doc_generated.go (3)
1672-1680: Swagger doc for BGPManagedConfig looks fine.
1909-1917: Swagger doc for NoOverlayOptions looks fine.
1919-1936: OVNKubernetesConfig doc updates look consistent.openapi/openapi.json (4)
4575-4587: New conditional update risk types look well-structured.The new
AcceptRisktype and associated fields (conditionalUpdateRisks,riskNames,conditions) follow standard Kubernetes API patterns with appropriate list type semantics (map with unique keys, set for unique strings).Also applies to: 5875-5886, 6094-6102, 6129-6140
6351-6351: TLS profile description enhancements look good.The updated descriptions provide clearer documentation with YAML examples and cipher suite details for the various TLS security profiles. These are documentation improvements that enhance API usability.
Also applies to: 8537-8537, 8815-8815, 9745-9745, 11323-11323, 11334-11353
11639-11650: Update type enhancements for risk acceptance look correct.The new
acceptRisksfield correctly uses a map list type keyed bynameto ensure uniqueness, consistent with theAcceptRisktype definition.Also applies to: 11685-11685
28977-28977: Console capability descriptions correctly updated.The descriptions now clearly enumerate the available capability values (LightspeedButton, GettingStartedBanner, GuidedTour).
Also applies to: 29626-29626
operator/v1/types_network.go (5)
400-405: LGTM: Helpful maintainer note for GA graduation.This comment clearly documents the upgrade path and rationale for not including the default in the schema during TechPreview.
482-510: LGTM: Well-structured feature-gated fields with appropriate immutability constraints.
DefaultNetworkTransport: Correctly enforces set-once semantics at installation time.DefaultNetworkNoOverlayOptions: Field removal prevention at struct level (line 411) complements the feature gate.BGPManagedConfig: The XValidation at line 508 allows setting on day 2 but makes the entire struct immutable oncebgpTopologyis populated, which provides a sensible "configure once" pattern.The
omitzero,omitemptyJSON tags are appropriate for optional feature-gated structs.
941-972: LGTM: Type definitions follow established patterns.The typed string enums and their corresponding constants are consistent with existing patterns in this file (e.g.,
IPsecMode,SDNMode,RouteAdvertisementsEnablement).
974-997: LGTM: NoOverlayOptions validation rules are well-designed.
OutboundSNAT: The installation-time constraint requiring "Enabled" (line 983) with subsequent mutability is a practical pattern allowing initial conservatism with day-2 tuning flexibility.Routing: Full immutability viaself == oldSelf(line 994) is appropriate given topology changes would be disruptive.Both fields use the standard kubebuilder pattern of
+requiredfor OpenAPI schema generation combined withomitemptyfor JSON serialization.
999-1018: LGTM: BGPManagedConfig provides sensible defaults and constraints.
ASNumber: The range 1-4294967295 covers both 2-byte and 4-byte ASNs, and default 64512 is appropriately in the private ASN range (64512-65534).BGPTopology: Currently constrained toFullMeshwith room to extend via the enum.Note that per the design discussion, immutability is enforced at the field level (line 508) rather than within this struct, making the entire
BGPManagedConfigimmutable oncebgpTopologyis populated.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
openshift/api#2537 openshift/client-go#349 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
ricky-rav
force-pushed
the
no_overlay_api
branch
from
162b45b to
1622757
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@openapi/openapi.json`:
- Around line 5875-5886: The schema for the conditionalUpdateRisks array lacks
the documented 500-item cap; update the source Go type that defines the
conditionalUpdateRisks field (the slice of ConditionalUpdateRisk referenced by
spec.desiredUpdate) to add the Kubernetes validation tag for max items (e.g. the
kubebuilder/validation tag that sets MaxItems=500) so OpenAPI generation emits
"maxItems": 500 for conditionalUpdateRisks; after adding the tag, regenerate the
OpenAPI/CRD to verify the maxItems appears in openapi.json.
- Around line 6094-6102: The riskNames array schema lacks the size and
entry-length constraints described; update the "riskNames" definition to include
"maxItems": 500 on the array and add "maxLength": 256 to the array "items" (the
string schema) so each entry is capped at 256 characters; keep the existing
"x-kubernetes-list-type": "set" for uniqueness and ensure you modify the
"riskNames" object and its "items" schema (referencing the "riskNames" symbol
and its "items" child) to enforce these limits.
- Around line 6129-6140: The "conditions" array schema lacks a maxItems
constraint even though the docs say it must not contain more than one entry; add
a maxItems: 1 constraint for the conditions field (the array that references
io.k8s.apimachinery.pkg.apis.meta.v1.Condition) by adding the appropriate
validation annotation in the Go source (e.g. add a kubebuilder/api validation
tag like "+kubebuilder:validation:MaxItems:=1" or the equivalent OpenAPI
generation tag immediately above the Conditions field definition) so the
generated openapi.json includes "maxItems": 1 for the "conditions" property.
- Around line 4575-4586: The OpenAPI schema for AcceptRisk currently documents
constraints for AcceptRisk.name but doesn't enforce them; update the Go struct
definition for type AcceptRisk by adding validation tags (e.g.,
`validate:"min=1,max=256"` or the project's preferred kube/openapi tags) on the
Name field so the generated OpenAPI includes minLength: 1 and maxLength: 256,
then run the OpenAPI generation step to regenerate openapi.json so the schema
for AcceptRisk.name is updated accordingly.
- Around line 11639-11649: The openapi schema for the array field "acceptRisks"
is missing the documented 1000-item cap; update the source Go struct for the
field (the AcceptRisks slice referenced by the definition
com.github.openshift.api.config.v1.AcceptRisk / the AcceptRisks field) to add
the Kubernetes validation tag to enforce the limit (e.g. add a
+kubebuilder:validation:MaxItems=1000 comment above the AcceptRisks field) so
the generated OpenAPI JSON includes "maxItems": 1000 for acceptRisks.
♻️ Duplicate comments (6)
openapi/generated_openapi/zz_generated.openapi.go (2)
56931-56957: Enum constraints still missing fromNoOverlayOptionsschema.The
outboundSNATandroutingfields are still missing theirEnumconstraints as noted in the previous review.
57948-57968: Missing enum and problematic defaults still present.The issues flagged in the previous review remain:
defaultNetworkTransportmissingEnumconstraint for"NoOverlay"/"Geneve"defaultNetworkNoOverlayOptionsandbgpManagedConfighaveDefault: map[string]interface{}{}which creates invalid default states since both types have required subfieldsoperator/v1/types_network.go (1)
408-413: Add inverse validation to forbid no-overlay fields unless transport is NoOverlay.Right now,
defaultNetworkNoOverlayOptionsandbgpManagedConfigcan be set even whendefaultNetworkTransportis omitted or Geneve, which allows contradictory-but-accepted configs (Line 408–413). Please add the inverse constraints.Proposed validation additions
// +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(self.defaultNetworkTransport) || self.defaultNetworkTransport != 'NoOverlay' || has(self.defaultNetworkNoOverlayOptions)",message="defaultNetworkNoOverlayOptions is required when defaultNetworkTransport is NoOverlay" +// +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(self.defaultNetworkNoOverlayOptions) || (has(self.defaultNetworkTransport) && self.defaultNetworkTransport == 'NoOverlay')",message="defaultNetworkNoOverlayOptions is forbidden when defaultNetworkTransport is not NoOverlay" // +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(self.defaultNetworkNoOverlayOptions) || self.defaultNetworkNoOverlayOptions.routing != 'Managed' || has(self.bgpManagedConfig)",message="bgpManagedConfig is required when defaultNetworkNoOverlayOptions.routing is Managed" +// +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(self.bgpManagedConfig) || (has(self.defaultNetworkTransport) && self.defaultNetworkTransport == 'NoOverlay')",message="bgpManagedConfig is forbidden when defaultNetworkTransport is not NoOverlay"openapi/openapi.json (3)
28702-28717: Duplicate: missing enum/min/max validation inBGPManagedConfig.
33091-33106: Duplicate: missing enums forNoOverlayOptionsfields.
33625-33638: Duplicate: default{}and missing enum inOVNKubernetesConfig.
🧹 Nitpick comments (1)
openapi/openapi.json (1)
11334-11352: Validate or clarify allowed values forTLSProfile.type.Line 11350 describes a closed set of values, but the schema allows any string. If this is not intentional for forward compatibility, consider adding an enum (including
""if “unspecified” is valid). If it is intentional, consider clarifying the description.
| "com.github.openshift.api.config.v1.AcceptRisk": { | ||
| "description": "AcceptRisk represents a risk that is considered acceptable.", | ||
| "type": "object", | ||
| "required": [ | ||
| "name" | ||
| ], | ||
| "properties": { | ||
| "name": { | ||
| "description": "name is the name of the acceptable risk. It must be a non-empty string and must not exceed 256 characters.", | ||
| "type": "string" | ||
| } | ||
| } |
There was a problem hiding this comment.
Add length constraints for AcceptRisk.name.
Line 4583 states the name must be non-empty and ≤256 chars, but the schema does not enforce this. Add min/max length validation in the source Go type and regenerate the OpenAPI.
🔧 Suggested schema shape (apply via Go validation tags)
"name": {
"description": "name is the name of the acceptable risk. It must be a non-empty string and must not exceed 256 characters.",
- "type": "string"
+ "type": "string",
+ "minLength": 1,
+ "maxLength": 256
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "com.github.openshift.api.config.v1.AcceptRisk": { | |
| "description": "AcceptRisk represents a risk that is considered acceptable.", | |
| "type": "object", | |
| "required": [ | |
| "name" | |
| ], | |
| "properties": { | |
| "name": { | |
| "description": "name is the name of the acceptable risk. It must be a non-empty string and must not exceed 256 characters.", | |
| "type": "string" | |
| } | |
| } | |
| "com.github.openshift.api.config.v1.AcceptRisk": { | |
| "description": "AcceptRisk represents a risk that is considered acceptable.", | |
| "type": "object", | |
| "required": [ | |
| "name" | |
| ], | |
| "properties": { | |
| "name": { | |
| "description": "name is the name of the acceptable risk. It must be a non-empty string and must not exceed 256 characters.", | |
| "type": "string", | |
| "minLength": 1, | |
| "maxLength": 256 | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@openapi/openapi.json` around lines 4575 - 4586, The OpenAPI schema for
AcceptRisk currently documents constraints for AcceptRisk.name but doesn't
enforce them; update the Go struct definition for type AcceptRisk by adding
validation tags (e.g., `validate:"min=1,max=256"` or the project's preferred
kube/openapi tags) on the Name field so the generated OpenAPI includes
minLength: 1 and maxLength: 256, then run the OpenAPI generation step to
regenerate openapi.json so the schema for AcceptRisk.name is updated
accordingly.
| "conditionalUpdateRisks": { | ||
| "description": "conditionalUpdateRisks contains the list of risks associated with conditionalUpdates. When performing a conditional update, all its associated risks will be compared with the set of accepted risks in the spec.desiredUpdate.acceptRisks field. If all risks for a conditional update are included in the spec.desiredUpdate.acceptRisks set, the conditional update can proceed, otherwise it is blocked. The risk names in the list must be unique. conditionalUpdateRisks must not contain more than 500 entries.", | ||
| "type": "array", | ||
| "items": { | ||
| "default": {}, | ||
| "$ref": "#/definitions/com.github.openshift.api.config.v1.ConditionalUpdateRisk" | ||
| }, | ||
| "x-kubernetes-list-map-keys": [ | ||
| "name" | ||
| ], | ||
| "x-kubernetes-list-type": "map" | ||
| }, |
There was a problem hiding this comment.
Enforce the documented 500-item cap for conditionalUpdateRisks.
Line 5876 says the list must not contain more than 500 entries, but there’s no maxItems. Add the validation in the source type so it appears in OpenAPI.
🔧 Suggested schema shape (apply via Go validation tags)
"conditionalUpdateRisks": {
"description": "conditionalUpdateRisks contains the list of risks associated with conditionalUpdates. ... conditionalUpdateRisks must not contain more than 500 entries.",
"type": "array",
+ "maxItems": 500,
"items": {
"default": {},
"$ref": "#/definitions/com.github.openshift.api.config.v1.ConditionalUpdateRisk"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "conditionalUpdateRisks": { | |
| "description": "conditionalUpdateRisks contains the list of risks associated with conditionalUpdates. When performing a conditional update, all its associated risks will be compared with the set of accepted risks in the spec.desiredUpdate.acceptRisks field. If all risks for a conditional update are included in the spec.desiredUpdate.acceptRisks set, the conditional update can proceed, otherwise it is blocked. The risk names in the list must be unique. conditionalUpdateRisks must not contain more than 500 entries.", | |
| "type": "array", | |
| "items": { | |
| "default": {}, | |
| "$ref": "#/definitions/com.github.openshift.api.config.v1.ConditionalUpdateRisk" | |
| }, | |
| "x-kubernetes-list-map-keys": [ | |
| "name" | |
| ], | |
| "x-kubernetes-list-type": "map" | |
| }, | |
| "conditionalUpdateRisks": { | |
| "description": "conditionalUpdateRisks contains the list of risks associated with conditionalUpdates. When performing a conditional update, all its associated risks will be compared with the set of accepted risks in the spec.desiredUpdate.acceptRisks field. If all risks for a conditional update are included in the spec.desiredUpdate.acceptRisks set, the conditional update can proceed, otherwise it is blocked. The risk names in the list must be unique. conditionalUpdateRisks must not contain more than 500 entries.", | |
| "type": "array", | |
| "maxItems": 500, | |
| "items": { | |
| "default": {}, | |
| "$ref": "#/definitions/com.github.openshift.api.config.v1.ConditionalUpdateRisk" | |
| }, | |
| "x-kubernetes-list-map-keys": [ | |
| "name" | |
| ], | |
| "x-kubernetes-list-type": "map" | |
| }, |
🤖 Prompt for AI Agents
In `@openapi/openapi.json` around lines 5875 - 5886, The schema for the
conditionalUpdateRisks array lacks the documented 500-item cap; update the
source Go type that defines the conditionalUpdateRisks field (the slice of
ConditionalUpdateRisk referenced by spec.desiredUpdate) to add the Kubernetes
validation tag for max items (e.g. the kubebuilder/validation tag that sets
MaxItems=500) so OpenAPI generation emits "maxItems": 500 for
conditionalUpdateRisks; after adding the tag, regenerate the OpenAPI/CRD to
verify the maxItems appears in openapi.json.
| "riskNames": { | ||
| "description": "riskNames represents the set of the names of conditionalUpdateRisks that are relevant to this update for some clusters. The Applies condition of each conditionalUpdateRisks entry declares if that risk applies to this cluster. A conditional update is accepted only if each of its risks either does not apply to the cluster or is considered acceptable by the cluster administrator. The latter means that the risk names are included in value of the spec.desiredUpdate.acceptRisks field. Entries must be unique and must not exceed 256 characters. riskNames must not contain more than 500 entries.", | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string", | ||
| "default": "" | ||
| }, | ||
| "x-kubernetes-list-type": "set" | ||
| }, |
There was a problem hiding this comment.
Constrain riskNames entries to match the description.
Line 6095 says entries must be unique, ≤256 chars, and the list must not exceed 500 items. Uniqueness is handled by x-kubernetes-list-type: set, but length and size caps are missing.
🔧 Suggested schema shape (apply via Go validation tags)
"riskNames": {
"description": "riskNames represents the set ... Entries must be unique and must not exceed 256 characters. riskNames must not contain more than 500 entries.",
"type": "array",
+ "maxItems": 500,
"items": {
"type": "string",
- "default": ""
+ "default": "",
+ "minLength": 1,
+ "maxLength": 256
},
"x-kubernetes-list-type": "set"
},🤖 Prompt for AI Agents
In `@openapi/openapi.json` around lines 6094 - 6102, The riskNames array schema
lacks the size and entry-length constraints described; update the "riskNames"
definition to include "maxItems": 500 on the array and add "maxLength": 256 to
the array "items" (the string schema) so each entry is capped at 256 characters;
keep the existing "x-kubernetes-list-type": "set" for uniqueness and ensure you
modify the "riskNames" object and its "items" schema (referencing the
"riskNames" symbol and its "items" child) to enforce these limits.
| "conditions": { | ||
| "description": "conditions represents the observations of the conditional update risk's current status. Known types are: * Applies, for whether the risk applies to the current cluster. The condition's types in the list must be unique. conditions must not contain more than one entry.", | ||
| "type": "array", | ||
| "items": { | ||
| "default": {}, | ||
| "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Condition" | ||
| }, | ||
| "x-kubernetes-list-map-keys": [ | ||
| "type" | ||
| ], | ||
| "x-kubernetes-list-type": "map" | ||
| }, |
There was a problem hiding this comment.
Limit conditions to a single entry as documented.
Line 6130 says the list must not contain more than one entry, but no maxItems is present. Add it in the source schema.
🔧 Suggested schema shape (apply via Go validation tags)
"conditions": {
"description": "conditions represents the observations ... conditions must not contain more than one entry.",
"type": "array",
+ "maxItems": 1,
"items": {
"default": {},
"$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Condition"
},🤖 Prompt for AI Agents
In `@openapi/openapi.json` around lines 6129 - 6140, The "conditions" array schema
lacks a maxItems constraint even though the docs say it must not contain more
than one entry; add a maxItems: 1 constraint for the conditions field (the array
that references io.k8s.apimachinery.pkg.apis.meta.v1.Condition) by adding the
appropriate validation annotation in the Go source (e.g. add a kubebuilder/api
validation tag like "+kubebuilder:validation:MaxItems:=1" or the equivalent
OpenAPI generation tag immediately above the Conditions field definition) so the
generated openapi.json includes "maxItems": 1 for the "conditions" property.
| "acceptRisks": { | ||
| "description": "acceptRisks is an optional set of names of conditional update risks that are considered acceptable. A conditional update is performed only if all of its risks are acceptable. This list may contain entries that apply to current, previous or future updates. The entries therefore may not map directly to a risk in .status.conditionalUpdateRisks. acceptRisks must not contain more than 1000 entries. Entries in this list must be unique.", | ||
| "type": "array", | ||
| "items": { | ||
| "default": {}, | ||
| "$ref": "#/definitions/com.github.openshift.api.config.v1.AcceptRisk" | ||
| }, | ||
| "x-kubernetes-list-map-keys": [ | ||
| "name" | ||
| ], | ||
| "x-kubernetes-list-type": "map" |
There was a problem hiding this comment.
Enforce the documented 1000-item cap for acceptRisks.
Line 11640 says the list must not exceed 1000 entries, but no maxItems is present. Add it via source validation.
🔧 Suggested schema shape (apply via Go validation tags)
"acceptRisks": {
"description": "acceptRisks is an optional set ... acceptRisks must not contain more than 1000 entries.",
"type": "array",
+ "maxItems": 1000,
"items": {
"default": {},
"$ref": "#/definitions/com.github.openshift.api.config.v1.AcceptRisk"
},🤖 Prompt for AI Agents
In `@openapi/openapi.json` around lines 11639 - 11649, The openapi schema for the
array field "acceptRisks" is missing the documented 1000-item cap; update the
source Go struct for the field (the AcceptRisks slice referenced by the
definition com.github.openshift.api.config.v1.AcceptRisk / the AcceptRisks
field) to add the Kubernetes validation tag to enforce the limit (e.g. add a
+kubebuilder:validation:MaxItems=1000 comment above the AcceptRisks field) so
the generated OpenAPI JSON includes "maxItems": 1000 for acceptRisks.
openshift/api#2537 openshift/client-go#349 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
openshift/api#2537 openshift/client-go#349 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
ricky-rav
force-pushed
the
no_overlay_api
branch
from
1622757 to
9bc28f5
Compare
openshift/api#2537 openshift/client-go#349 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openapi/generated_openapi/zz_generated.openapi.go`:
- Around line 49545-49567: The generated OpenAPI definition function
schema_openshift_api_operator_v1_BGPManagedConfig is missing the validation
constraints and default from the source type: add the minimum (1), maximum
(4294967295) and default (64512) to the asNumber SchemaProps and add the enum
["FullMesh"] to the bgpTopology SchemaProps so the OpenAPI includes these
constraints; to do this, fix the OpenAPI generator config or regenerate the
schema from the operator/v1/types_network.go sources (ensuring the
+kubebuilder:validation:Minimum, Maximum, default and Enum tags are picked up)
so schema_openshift_api_operator_v1_BGPManagedConfig contains
minimum/maximum/default for asNumber and enum for bgpTopology.
In `@operator/v1/types_network.go`:
- Around line 406-412: Add a validation rule to ovnKubernetesConfig that forbids
bgpManagedConfig unless defaultNetworkTransport == 'NoOverlay'; specifically,
add a FeatureGateAwareXValidation (featureGate=NoOverlayMode) rule referencing
ovnKubernetesConfig that ensures either bgpManagedConfig is absent or
defaultNetworkTransport is 'NoOverlay' (e.g., rule like
"!has(self.bgpManagedConfig) || (has(self.defaultNetworkTransport) &&
self.defaultNetworkTransport == 'NoOverlay')"), referencing the bgpManagedConfig
and defaultNetworkTransport fields so bgpManagedConfig cannot be set for
non-NoOverlay transports.
♻️ Duplicate comments (6)
openapi/openapi.json (4)
4575-4587: Generated schema definitions for conditional update risks look structurally correct.The new
AcceptRisktype and related arrays (conditionalUpdateRisks,riskNames,conditions,acceptRisks) are properly structured with appropriate list types (mapwith keys,set).Previous review comments flagged missing validation constraints (e.g.,
maxItems,maxLength) that are documented in descriptions but not enforced in the schema. Since this is generated code, those constraints would need to be added via validation tags in the source Go types (config/v1/types_cluster_version.goor similar).Also applies to: 5875-5886, 6094-6102, 6129-6140, 11639-11650
28702-28719: BGPManagedConfig schema structure is correct.The schema correctly marks
bgpTopologyas required and definesasNumberas an optional integer with int64 format (sufficient for the documented 1-4294967295 range).Missing enum constraint for
bgpTopologyand min/max constraints forasNumberwere previously discussed — these would need to be added in the source Go type via validation markers.
33091-33108: NoOverlayOptions schema structure is correct.The schema correctly marks both
outboundSNATandroutingas required fields, aligning with the documented requirement that these are mandatory when operating in no-overlay mode.Missing enum constraints were previously discussed — these would need to be added in the source Go type.
33625-33638: OVNKubernetesConfig extended with no-overlay mode fields.The three new fields (
bgpManagedConfig,defaultNetworkNoOverlayOptions,defaultNetworkTransport) properly extend OVNKubernetesConfig to support the no-overlay mode feature.The
"default": {}values on object references and missing enum constraint ondefaultNetworkTransportwere previously discussed — the source Go types control this behavior.openapi/generated_openapi/zz_generated.openapi.go (2)
56931-56954: Missing enum constraints foroutboundSNATandrouting.The schema should enforce allowed values for these fields. If the source already has
+kubebuilder:validation:Enum, regenerate OpenAPI so theEnumentries appear here.✅ Expected OpenAPI shape
"outboundSNAT": { SchemaProps: spec.SchemaProps{ Description: "...", Type: []string{"string"}, Format: "", + Enum: []interface{}{"Enabled", "Disabled"}, }, }, "routing": { SchemaProps: spec.SchemaProps{ Description: "...", Type: []string{"string"}, Format: "", + Enum: []interface{}{"Managed", "Unmanaged"}, }, },
57948-57967: Add enum fordefaultNetworkTransportand remove empty-object defaults.
defaultNetworkTransportlacks the enum constraint, and the empty-object defaults fordefaultNetworkNoOverlayOptions/bgpManagedConfigcreate invalid default states for optional objects with required subfields. Regenerate the schema so enums are enforced and optional complex fields have no default.✅ Expected OpenAPI shape
"defaultNetworkTransport": { SchemaProps: spec.SchemaProps{ Description: "...", Type: []string{"string"}, Format: "", + Enum: []interface{}{"NoOverlay", "Geneve"}, }, }, "defaultNetworkNoOverlayOptions": { SchemaProps: spec.SchemaProps{ Description: "...", - Default: map[string]interface{}{}, Ref: ref("github.com/openshift/api/operator/v1.NoOverlayOptions"), }, }, "bgpManagedConfig": { SchemaProps: spec.SchemaProps{ Description: "...", - Default: map[string]interface{}{}, Ref: ref("github.com/openshift/api/operator/v1.BGPManagedConfig"), }, },
🧹 Nitpick comments (1)
operator/v1/tests/networks.operator.openshift.io/NoOverlayMode.yaml (1)
37-48: Consider adding a test for bgpManagedConfig with Geneve transport.There's a test validating that
defaultNetworkNoOverlayOptionsis forbidden with Geneve transport, but no corresponding test forbgpManagedConfigwith Geneve transport. This would help ensure the API rejects semantically invalid configurations where BGP config is set without NoOverlay mode.Example test case to add:
- name: Should fail if bgpManagedConfig is set with Geneve transport initial: | apiVersion: operator.openshift.io/v1 kind: Network spec: defaultNetwork: ovnKubernetesConfig: defaultNetworkTransport: Geneve bgpManagedConfig: bgpTopology: FullMesh expectedError: "bgpManagedConfig is forbidden when defaultNetworkTransport is not NoOverlay"
operator/v1/types_network.go
Outdated
| // ovnKubernetesConfig contains the configuration parameters for networks | ||
| // using the ovn-kubernetes network project | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="has(self.defaultNetworkNoOverlayOptions) == (has(self.defaultNetworkTransport) && self.defaultNetworkTransport == 'NoOverlay')",message="defaultNetworkNoOverlayOptions must be set if and only if defaultNetworkTransport is NoOverlay" | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(self.defaultNetworkNoOverlayOptions) || self.defaultNetworkNoOverlayOptions.routing != 'Managed' || has(self.bgpManagedConfig)",message="bgpManagedConfig is required when defaultNetworkNoOverlayOptions.routing is Managed" | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(oldSelf.defaultNetworkTransport) || oldSelf.defaultNetworkTransport == '' || has(self.defaultNetworkTransport)",message="defaultNetworkTransport cannot be removed once set to a non-empty value" | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(oldSelf.defaultNetworkNoOverlayOptions) || has(self.defaultNetworkNoOverlayOptions)",message="defaultNetworkNoOverlayOptions cannot be removed once set" | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(oldSelf.bgpManagedConfig) || oldSelf.bgpManagedConfig.bgpTopology == '' || has(self.bgpManagedConfig)",message="bgpManagedConfig cannot be removed once configured" |
There was a problem hiding this comment.
Missing validation: bgpManagedConfig should be forbidden when transport is not NoOverlay.
Line 408 correctly enforces a bi-conditional for defaultNetworkNoOverlayOptions (required iff transport is NoOverlay). However, there's no corresponding constraint for bgpManagedConfig.
Currently, this configuration would be accepted:
ovnKubernetesConfig:
defaultNetworkTransport: Geneve
bgpManagedConfig:
bgpTopology: FullMeshThis is semantically invalid—BGP configuration only applies to no-overlay mode.
🔧 Proposed fix: Add validation to forbid bgpManagedConfig when transport is not NoOverlay
// +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="has(self.defaultNetworkNoOverlayOptions) == (has(self.defaultNetworkTransport) && self.defaultNetworkTransport == 'NoOverlay')",message="defaultNetworkNoOverlayOptions must be set if and only if defaultNetworkTransport is NoOverlay"
+// +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(self.bgpManagedConfig) || (has(self.defaultNetworkTransport) && self.defaultNetworkTransport == 'NoOverlay')",message="bgpManagedConfig is forbidden when defaultNetworkTransport is not NoOverlay"
// +openshift:validation:FeatureGateAwareXValidation:featureGate=NoOverlayMode,rule="!has(self.defaultNetworkNoOverlayOptions) || self.defaultNetworkNoOverlayOptions.routing != 'Managed' || has(self.bgpManagedConfig)",message="bgpManagedConfig is required when defaultNetworkNoOverlayOptions.routing is Managed"🤖 Prompt for AI Agents
In `@operator/v1/types_network.go` around lines 406 - 412, Add a validation rule
to ovnKubernetesConfig that forbids bgpManagedConfig unless
defaultNetworkTransport == 'NoOverlay'; specifically, add a
FeatureGateAwareXValidation (featureGate=NoOverlayMode) rule referencing
ovnKubernetesConfig that ensures either bgpManagedConfig is absent or
defaultNetworkTransport is 'NoOverlay' (e.g., rule like
"!has(self.bgpManagedConfig) || (has(self.defaultNetworkTransport) &&
self.defaultNetworkTransport == 'NoOverlay')"), referencing the bgpManagedConfig
and defaultNetworkTransport fields so bgpManagedConfig cannot be set for
non-NoOverlay transports.
This commit introduces API changes to support OVN-Kubernetes no-overlay mode for the default network, where the cluster default network operates without overlay encapsulation (Geneve). API changes (operator.openshift.io/v1): - Added defaultNetworkTransport field to OVNKubernetesConfig (NoOverlay or Geneve) - Added defaultNetworkNoOverlayOptions with outboundSNAT and routing configuration - Added bgpManagedConfig for BGP configuration when routing is managed - All no-overlay fields are immutable after cluster creation Added feature gate (TechPreviewNoUpgrade) for NoOverlayMode within the Networking/ovn-kubernetes component. Ran these commands to update autogenerated source files: $ hack/update-codegen-crds.sh $ hack/update-swagger-docs.sh $ hack/update-payload-featuregates.sh Openshift enhancement for this feature: openshift/enhancements#1859 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
ricky-rav
force-pushed
the
no_overlay_api
branch
from
9bc28f5 to
8e31b8d
Compare
openshift/api#2537 openshift/client-go#349 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
openshift/api#2537 openshift/client-go#349 Signed-off-by: Riccardo Ravaioli <rravaiol@redhat.com>
|
@ricky-rav: all tests passed! 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. |
5 participants
Related openshift enhancement: openshift/enhancements#1859