operator: Add an option to configure OVN-Kubernetes GatewayConfig without an uplink specified. #3009
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -642,6 +642,7 @@ const ( | |
| ) | ||
|
|
||
| // GatewayConfig holds node gateway-related parsed config file parameters and command-line overrides | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=OVNKubernetesUplinkMode,rule="!has(self.uplinkMode) || (has(self.routingViaHost) && self.routingViaHost == true)",message="uplinkMode can only be set when routingViaHost is true" | ||
| type GatewayConfig struct { | ||
| // routingViaHost allows pod egress traffic to exit via the ovn-k8s-mp0 management port | ||
| // into the host before sending it out. If this is not set, traffic will always egress directly | ||
|
|
@@ -650,6 +651,16 @@ type GatewayConfig struct { | |
| // +kubebuilder:default:=false | ||
| // +optional | ||
| RoutingViaHost bool `json:"routingViaHost,omitempty"` | ||
| // uplinkMode controls whether the external gateway bridge (br-ex) requires a physical uplink port. | ||
|
Contributor
There was a problem hiding this comment. I forget if the option is only for br-ex or for all Uplinks (example Uplink API+CUDN API https://ovn-kubernetes.io/master/features/user-defined-networks/uplinks/#introduction - there is plans to make the whole CDN bridge an Uplink API defined thing in future and maybe answer is this option would then become a per Uplink thing) In upstream it might not be a well defined thing now and we don't support Uplink API in OCP yet, but its something we need to consider since once we defined the field's purpose as only for br-ex we won't be able to change that scoping, so it would be good for us to think if we only want it for br-ex or keep it vague to accommodate other bridges as well including the APBER Secondary bridge which is also an external gateway bridge today. |
||
| // Allowed values are "Required" and "Optional". | ||
| // When set to "Required", ovn-kubernetes requires an uplink on the gateway bridge. | ||
| // When set to "Optional", ovn-kubernetes allows the gateway bridge to start without an uplink. | ||
| // When omitted, this means no opinion and the platform is left to choose a reasonable default, | ||
| // which is subject to change over time. The current default is "Required". | ||
|
Contributor
There was a problem hiding this comment.
can defaults really change over time - if its from API standing that's considered breaking change right? if we change from required -> optional later on? maybe we have to reword this - from API standpoint the CRD defaulting is "required" and that is not subject to change, the customer can choose their own value to override that default but when we say "choose a reasonable default" that is confusing with the API default
Contributor
There was a problem hiding this comment. also this field is mutable on day2 I suppose? and CNO will roll out the changes? (it comes with conditions apply on if people remove uplinks mid-way on day2 and change this knob that's on them for the downtime)
Contributor
Author
There was a problem hiding this comment. That's a disclaimer that we can do without. It's simply saying that if the field is omitted, and tomorrow we move to a different behavior for some reason than requiring uplinks we are covering our bases. If it is adding confusion for a hypothetical future-proofing, I can remove it. The behavior for Day 2 changes needs to be documented at the very least, if it can't be ensured for correctness functionally by the implementation.
Contributor
There was a problem hiding this comment. For configuration APIs like this, yes we can change default behaviors without warning when we make this explicit statement in the API documentation. It allows us to change our opinion on what the default value should be for the platform as we see fit. Customers can always override that by specifying an explicit opinion on the field.
Contributor
There was a problem hiding this comment.
oh nice, I learnt something new then! |
||
| // This setting only takes effect when routingViaHost is true (local gateway mode). | ||
|
Contributor
There was a problem hiding this comment. might want to enforce this using CEL on the parent struct // +kubebuilder:validation:XValidation:rule="!has(self.uplinkMode) || (has(self.routingViaHost) && self.routingViaHost == true)",message="uplinkMode can only be set when routingViaHost is true"
Contributor
Author
There was a problem hiding this comment. Added CEL validation on parent struct GatewayConfig. |
||
| // +openshift:enable:FeatureGate=OVNKubernetesUplinkMode | ||
| // +optional | ||
| UplinkMode UplinkMode `json:"uplinkMode,omitempty"` | ||
| // ipForwarding controls IP forwarding for all traffic on OVN-Kubernetes managed interfaces (such as br-ex). | ||
| // By default this is set to Restricted, and Kubernetes related traffic is still forwarded appropriately, but other | ||
| // IP traffic will not be routed by the OCP node. If there is a desire to allow the host to forward traffic across | ||
|
|
@@ -900,6 +911,16 @@ const ( | |
| IPsecModeFull IPsecMode = "Full" | ||
| ) | ||
|
|
||
| // +kubebuilder:validation:Enum:="Required";"Optional" | ||
| type UplinkMode string | ||
|
|
||
| var ( | ||
| // UplinkModeRequired requires an uplink on the gateway bridge. | ||
| UplinkModeRequired UplinkMode = "Required" | ||
| // UplinkModeOptional allows the gateway bridge to start without a physical uplink. | ||
| UplinkModeOptional UplinkMode = "Optional" | ||
| ) | ||
|
|
||
| // +kubebuilder:validation:Enum:="";"Enabled";"Disabled" | ||
| type RouteAdvertisementsEnablement string | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this change need to be made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so verify-payload-crds.sh was failing the prow
verifyjob.The script fails with echo receiving SIGPIPE when the
${files}list is large and echo is still piping it to grep, when grep -q finds an early match and exits.Without the fix, the script would incorrectly report a generated payload crd as untracked, although it did find the file. The change gets rid of the piping business entirely.