CORENET-6610: Add EVPN APIs#2881
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@kyrtapz: This pull request references CORENET-6610 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.22.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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds EVPN support gated by OVN_EVPN_ENABLE: new cluster-scoped VTEP CRD, EVPN fields and cross-field validations in ClusterUserDefinedNetwork and topology, conditional RBAC for vteps, feature-gate wiring and tests, ovnkube startup flag injection, keepCRDs update, and two dependency bumps. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bindata/network/ovn-kubernetes/common/001-crd.yaml`:
- Around line 3851-3864: The RT validation for 4-byte ASN global administrator
in both the ipVRF and macVRF EVPN routeTarget rule blocks erroneously only
checks that uint(self.split(':')[0]) <= 65535u; update the boolean expression in
the first rule to also enforce the documented upper bound by requiring
uint(self.split(':')[0]) <= 4294967295u (i.e., add this upper-bound check
alongside the existing checks or replace the 65535 limit with 4294967295 where
appropriate) so the rule rejects values greater than 4294967295; apply the same
change to both the ipVRF and macVRF routeTarget validation blocks.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (71)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.ci-operator.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/.coderabbit.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/.golangci.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/AGENTS.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/Dockerfile.ocpis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/install.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_image_policy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_feature.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_image_policy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_node.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_scheduling.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_image_policy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_image_policy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/console/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/console/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/util.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/install.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machine.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types_machineconfignode.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_internalreleaseimage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_osimagestream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_pinnedimageset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_console.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_12_etcd_01_etcds-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_console_01_consoles.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (5)
bindata/network/ovn-kubernetes/common/001-crd.yamlbindata/network/ovn-kubernetes/common/002-rbac-node.yamlbindata/network/ovn-kubernetes/common/004-rbac-control-plane.yamlgo.modpkg/network/ovn_kubernetes.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/network/ovn_kubernetes.gogo.modbindata/network/ovn-kubernetes/common/004-rbac-control-plane.yamlbindata/network/ovn-kubernetes/common/002-rbac-node.yamlbindata/network/ovn-kubernetes/common/001-crd.yaml
🔇 Additional comments (7)
go.mod (1)
105-105: LGTM!The dependency update to
github.com/openshift/apiis required to bring in the new EVPN-related API types (VTEP CRD and EVPN configuration fields) used by this PR.bindata/network/ovn-kubernetes/common/002-rbac-node.yaml (1)
212-221: LGTM!The conditional RBAC rule follows the established pattern for feature-gated permissions. Granting read-only access (list/get/watch) to
vtepsfor nodes is appropriate and follows the principle of least privilege.bindata/network/ovn-kubernetes/common/004-rbac-control-plane.yaml (1)
255-271: LGTM!The RBAC rules for the control plane appropriately include:
- Read access (list/get/watch) to
vteps- Write access (patch/update) to
vteps/statusfor status reconciliationThis follows the same pattern as other feature-gated CRDs like
routeadvertisements.pkg/network/ovn_kubernetes.go (1)
344-344: LGTM!The feature gate integration follows the established pattern used by other OVN feature gates in this file. The
OVN_EVPN_ENABLEdata key will control conditional rendering of EVPN-related CRDs and RBAC rules in the templates.FeatureGateEVPNis properly defined in the vendoredgithub.com/openshift/api/featuresdependency.bindata/network/ovn-kubernetes/common/001-crd.yaml (3)
4504-4518: Transport enum addition looks good.Clear values and description fit the EVPN feature gate behavior.
4536-4558: EVPN topology/transport guardrails are well covered.The validation rules align EVPN fields with Layer2/Layer3 primary semantics.
5272-5424: VTEP CRD schema and validations look solid.The CIDR masking checks and mode defaulting are clear.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
kyrtapz
force-pushed
the
evpn_api
branch
3 times, most recently
from
27d1b8e to
6a4690e
Compare
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
@kyrtapz: This pull request references CORENET-6610 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.22.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. |
| - Layer3 | ||
| - Localnet | ||
| type: string | ||
| {{- if .OVN_EVPN_ENABLE }} |
There was a problem hiding this comment.
@pliurh FYI: This will have to be changed in the future to accommodate no-overlay as well.
|
/retest-required |
|
/test ? |
|
@kyrtapz: This pull request references CORENET-6610 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.22.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. |
|
@kyrtapz: This pull request references CORENET-6610 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.22.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. |
kyrtapz
force-pushed
the
evpn_api
branch
3 times, most recently
from
2c1a7fd to
534c8cc
Compare
|
|
||
| evpn_enable_flag= | ||
| # OVN_EVPN_ENABLE represents a feature gate which is static; OVN_ROUTE_ADVERTISEMENTS_ENABLE is runtime-togglable and we need it for EVPN to work. | ||
| if [[ "{{.OVN_EVPN_ENABLE}}" == "true" && "{{.OVN_ROUTE_ADVERTISEMENTS_ENABLE}}" == "true" ]]; then |
There was a problem hiding this comment.
Why not handle the logic just once in ovn_kubernetes.go?
Also check for local gateway mode as well?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bindata/network/ovn-kubernetes/common/001-crd.yaml`:
- Around line 3792-3793: The YAMLlint error is caused by raw Go template guards
like "{{- if .OVN_EVPN_ENABLE_API }}" and matching "{{- end }}" appearing as
plain YAML; update the template to comment out or neutralize each guard so
YAMLlint can parse the file (e.g., replace raw guards with commented versions)
for every occurrence including the guards around "evpn" (the "{{- if
.OVN_EVPN_ENABLE_API }}" / "{{- end }}" pair) and the other listed guard
locations; ensure you keep the original guard text commented so the template
logic is preserved for tooling that processes the template, or alternatively add
this file to YAMLlint exclusions if commenting is not acceptable.
go get github.com/openshift/api@latest go get github.com/openshift/client-go@latest go mod tidy go mod vendor Signed-off-by: Patryk Diak <pdiak@redhat.com>
Use the EVPN featuregate. Only enable the feature in ovn-kubernetes when route advertisements are enabled. Signed-off-by: Patryk Diak <pdiak@redhat.com>
|
/test e2e-gcp-ovn-techpreview |
| // to avoid removing API fields and potentially losing data from existing custom resources. | ||
| // OVN_EVPN_ENABLE depends on both the feature gate and route advertisements being enabled, | ||
| // and controls deployment of EVPN runtime components (VTEP CRD, RBAC, FRR containers). | ||
| data.Data["OVN_EVPN_ENABLE_API"] = featureGates.Enabled(apifeatures.FeatureGateEVPN) |
There was a problem hiding this comment.
If you have the chance to make another push, consider something along the lines of OVN_EVPN_ENABLE_CUDN_API
Don't bother if you don't need to make another push or you are happy enough with current name
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcaamano, kyrtapz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
This PR only affects techpreview |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
openshift-ci-robot
added
the
verified
|
@kyrtapz: This PR has been marked as verified by 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. |
|
/retest |
|
/retest-required |
1 similar comment
|
/retest-required |
|
The failures are almost certainly unrelated to the changes since they only affect TP clusters. |
|
https://redhat-internal.slack.com/archives/C01CQA76KMX/p1771336302007649 /override ci/prow/hypershift-e2e-aks |
|
@kyrtapz: Overrode contexts on behalf of kyrtapz: ci/prow/e2e-aws-ovn-hypershift-conformance, ci/prow/hypershift-e2e-aks 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 kubernetes-sigs/prow repository. |
|
@kyrtapz: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
Bot
merged commit f28e91e
into
openshift:master
5 participants
NOTE: EVPN requires route advertisements to function, enabling or disabling it dynamically changes the CUDN CRD schema (adding/removing EVPN fields), which can invalidate existing CUDN resources that use EVPN transport.We have decided to make the API static, we only disable EVPN flag in ovn-k based on the route advertisements being enabled.
Needs to wait for openshift/ovn-kubernetes#2953 (introduces the feature flag to ovn-k).