OCPBUGS-104452: frr-k8s: sync CRD - #3108
Conversation
Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@oribon: This pull request references Jira Issue OCPBUGS-104452, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci-robot
added
the
jira/valid-bug
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
Summary by CodeRabbit
WalkthroughThe FRRConfiguration CRD adds router-level EVPN settings, neighbor address-family and ChangesFRR BGP schema
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors)
✅ Passed checks (21 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: oribon 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 |
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
🧹 Nitpick comments (1)
bindata/network/frr-k8s/001-crd.yaml (1)
139-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport the duplicated enum and the description typo upstream.
advertiseVNIsdeclares the same enum twice insideallOf. This is a controller-gen artifact from duplicated kubebuilder markers on both the type alias and the field. It is functionally harmless but it duplicates validation work on every request.Line 150 also contains a typo: "Avertise all VNIs". This text is user-visible through
oc explain.This file is synced from openshift/frr, so fix both in the upstream API types and re-sync.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindata/network/frr-k8s/001-crd.yaml` around lines 139 - 152, Remove the duplicated enum validation contributing to advertiseVNIs in the upstream API type, retaining a single Disabled/All enum definition, and correct the description text from “Avertise all VNIs” to “Advertise all VNIs”; then re-sync bindata/network/frr-k8s/001-crd.yaml from the upstream API definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindata/network/frr-k8s/001-crd.yaml`:
- Around line 186-201: Update the 4-byte ASN Route Target validation rule in the
CRD to enforce the global administrator upper bound of 4294967295, replacing the
incorrect lower-bound condition and preserving the existing format checks. Apply
the same validation change in the corresponding openshift/frr source and
regenerate or sync the FRR-K8s CRD.
---
Nitpick comments:
In `@bindata/network/frr-k8s/001-crd.yaml`:
- Around line 139-152: Remove the duplicated enum validation contributing to
advertiseVNIs in the upstream API type, retaining a single Disabled/All enum
definition, and correct the description text from “Avertise all VNIs” to
“Advertise all VNIs”; then re-sync bindata/network/frr-k8s/001-crd.yaml from the
upstream API definitions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a36b318-73ef-45d5-944f-03c7893b0914
📒 Files selected for processing (1)
bindata/network/frr-k8s/001-crd.yaml
| - message: RT with 4-byte ASN global administrator | ||
| must have format GHJK:MN where GHJK <= 4294967295 | ||
| and MN <= 65535 | ||
| rule: self.split(':').size() != 2 || self.split(':')[0].contains('.') | ||
| || !self.split(':')[0].matches('[0-9]+') | ||
| || !self.split(':')[1].matches('[0-9]+') | ||
| || uint(self.split(':')[0]) <= 65535u || | ||
| uint(self.split(':')[1]) <= 65535u | ||
| - message: RT with 2-byte ASN global administrator | ||
| must have format EF:OPQR where EF <= 65535 | ||
| and OPQR <= 4294967295 | ||
| rule: self.split(':').size() != 2 || self.split(':')[0].contains('.') | ||
| || !self.split(':')[0].matches('[0-9]+') | ||
| || !self.split(':')[1].matches('[0-9]+') | ||
| || uint(self.split(':')[0]) > 65535u || | ||
| uint(self.split(':')[1]) <= 4294967295u |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the gap with a CEL evaluation of the 4-byte ASN rule.
pip install --quiet cel-python 2>/dev/null
python - <<'PY'
import celpy
env = celpy.Environment()
rule = ("self.split(':').size() != 2 || self.split(':')[0].contains('.') "
"|| !self.split(':')[0].matches('[0-9]+') || !self.split(':')[1].matches('[0-9]+') "
"|| uint(self.split(':')[0]) <= 65535u || uint(self.split(':')[1]) <= 65535u")
prog = env.program(env.compile(rule))
for v in ["65000:100", "192.0.2.1:100", "99999999999:100", "4294967296:100"]:
print(v, prog.evaluate({"self": celpy.celtypes.StringType(v)}))
PYRepository: openshift/cluster-network-operator
Length of output: 290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find FRR-K8s CRD files:"
fd -a '001-crd\.yaml$|00.*crd\.yaml$' . | sed 's#^\./##' | rg 'frr-k8s|FRR|frr' || true
echo
echo "Relevant rule lines:"
if [ -f bindata/network/frr-k8s/001-crd.yaml ]; then
sed -n '160,215p' bindata/network/frr-k8s/001-crd.yaml | cat -n
fi
echo
echo "Search for FRR RT/RD/l3vni CEL rule definitions:"
rg -n "global administrator|RouteTarget|routeTargets|l3vni|importRTs|RD|rd|matches\\(.*\\)'|contains\\('\\.')" bindata/network/frr-k8s . || true
echo
echo "Search exact rule strings:"
rg -n "self\\.split\\(':')\\.size\\(\\) != 2|GHJK|OPQR|4-byte ASN|2-byte ASN" bindata/network/frr-k8s . || trueRepository: openshift/cluster-network-operator
Length of output: 5581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Validate Python/CEL setup:"
python3 - <<'PY'
try:
import celpy, celpy.celtypes
print("celpy available", celpy.__version__ if hasattr(celpy, '__version__') else "")
except Exception as e:
print("celpy unavailable:", type(e).__name__, e)
PY
echo
echo "Evaluate validation rules with a read-only CEL probe:"
python3 - <<'PY'
try:
import celpy, celpy.celtypes
except Exception:
exit(77)
env = celpy.Environment()
compile = env.compile
evaluate = lambda rule, value: env.program(compile(rule)).evaluate({"self": celpy.celtypes.StringType(value)}).value()
cases = ["65000:100", "192.0.2.1:100", "99999999999:100", "4294967296:100", "4294967295:65536", "4294967296:65536"]
rules = {
"colon_size": "self.split(':').size() == 2",
"global_non_ipv4_numeric": "self.split(':').size() != 2 || (isIP(self.split(':')[0]) or self.split(':')[0].matches('[0-9]+'))",
"local_numeric": "self.split(':').size() != 2 or self.split(':')[1].matches('[0-9]+')",
"ipv4_local_bound": "self.split(':').size() != 2 or !self.split(':')[0].contains('.') or (self.split(':')[1].matches('[0-9]+') and uint(self.split(':')[1]) <= 65535u)",
"4byte_rule": "self.split(':').size() != 2 or self.split(':')[0].contains('.') or !self.split(':')[0].matches('[0-9]+') or !self.split(':')[1].matches('[0-9]+') or uint(self.split(':')[0]) <= 65535u or uint(self.split(':')[1]) <= 65535u",
"2byte_rule": "self.split(':').size() != 2 or self.split(':')[0].contains('.') or !self.split(':')[0].matches('[0-9]+') or !self.split(':')[1].matches('[0-9]+') or uint(self.split(':')[0]) > 65535u or uint(self.split(':')[1]) <= 4294967295u",
}
print("value\tcolon_size\tglobal_non_ipv4_numeric\tlocal_numeric\tipv4_local_bound\t4byte_rule\t2byte_rule")
for v in cases:
print(v, "\t".join(str(evaluate(rules[k], v)) for k in rules))
PYRepository: openshift/cluster-network-operator
Length of output: 319
🌐 Web query:
OpenShift CNO FRR-K8s 001-crd.yaml RouteTarget CEL validation rules
💡 Result:
Based on current OpenShift and OVN-Kubernetes technical documentation, CEL (Common Expression Language) validation rules for RouteTarget are specifically associated with the OVN-Kubernetes project's UserDefinedNetwork (and related EVPN) CRDs, not the OpenShift Cluster Network Operator's (CNO) frr-k8s 001-crd.yaml [1][2][3]. Recent updates have improved the robustness of these RouteTarget validations [1][2]. Specifically, developers identified an issue where unanchored regex patterns in CEL rules allowed invalid strings (e.g., those with substring matches) to pass validation [1]. To fix this, the CEL validation rules were updated to use anchored regex, ensuring the entire field value conforms to the required format [1]. Technical details regarding the RouteTarget CEL validation: - Purpose: Ensures that RouteTarget fields (used in EVPN/VRF configurations) follow the required ASN:VNI or similar format [4][3]. - Regex Anchoring: The rules utilize ^ and $ anchors in matches() calls to enforce that the entire string is evaluated [1]. - Leading Zeros: Recommendations and updates have been proposed to tighten the regex (e.g., ^[1-9][0-9]*:[1-9][0-9]*$) to prevent non-canonical representations such as leading zeros (e.g., 00001:00001) [3]. - Implementation Location: These validations are defined within the ovn-kubernetes codebase, specifically within files like go-controller/pkg/crd/userdefinednetwork/v1/evpn.go [1][2][3]. If you are looking at 001-crd.yaml within the cluster-network-operator repository for frr-k8s, note that this file primarily defines resources like FRRConfiguration and FRRK8sConfiguration [5][6]. These CNO-managed CRDs focus on BGP neighbor configurations (e.g., ASN, localASN, and address family settings) rather than OVN-Kubernetes EVPN RouteTargets [5][6][7].
Citations:
- 1: ovn-kubernetes/ovn-kubernetes@cd9082b
- 2: http://github.com/openshift/ovn-kubernetes/pull/3114
- 3: Add EVPN and VTEP API ovn-kubernetes/ovn-kubernetes#5779
- 4: https://ovn-kubernetes.io/okeps/okep-5088-evpn/
- 5: OCPBUGS-99074: Align frr-k8s 5.0 CRDs #3070
- 6: OCPBUGS-82973: Frrk8s tls manifests #2963
- 7: 2a1d615...bc62016
🌐 Web query:
FRR-K8s 99999999999:100 route target 4-byte ASN maximum 4294967295 validation
💡 Result:
In the context of FRR-K8s (the Kubernetes-native BGP implementation based on FRRouting), configuration parameters such as Autonomous System Numbers (ASN) are validated to support the 4-byte ASN range [1][2][3]. The maximum valid value for a 4-byte ASN is 4294967295 (UINT32_MAX) [1][2][3]. Regarding the specific configuration string 99999999999💯 1. ASN Validation: The value 99999999999 exceeds the maximum allowed 4-byte ASN of 4294967295 [1][3][4]. Consequently, any attempt to use 99999999999 as an ASN in a route target or neighbor configuration will fail validation in current FRR-K8s and FRR versions [4]. 2. FRR Technical Context: FRRouting and associated tools have implemented updates to ensure proper parsing and validation of 4-byte ASNs [4]. Earlier implementations often relied on 2-byte ASN limits (65535) or incorrectly handled 4-byte values, leading to overflow errors or malformed configuration [4][5]. Modern FRR configurations and Kubernetes Custom Resource Definitions (CRDs) for FRR-K8s explicitly define the upper bound for ASN fields as 4294967295 [1][2][3]. 3. Extended Community Parsing: Specifically for Extended Communities (such as Route Targets), FRR has been updated to use 64-bit integers during parsing to correctly distinguish between valid 4-byte ASNs and values that exceed the 32-bit limit, preventing configuration errors [4]. Attempts to input values exceeding 4294967295 will be rejected by the parser as malformed [4]. In summary, the value 99999999999 is invalid for use as an ASN in FRR-K8s because it is greater than the maximum 4-byte ASN (4294967295) [1][4].
Citations:
- 1: https://github.com/openshift/frr/blob/210c63f7ec61bd3377796518a755d617d8f6ca04/API-DOCS.md
- 2: OCPBUGS-99074: Align frr-k8s 5.0 CRDs #3070
- 3: openshift/frr@c018203
- 4: bgpd: fix ecommunity parsing for AS4 FRRouting/frr#13181
- 5: https://docs.frrouting.org/en/latest/bgp.html
Enforce the 4-byte global administrator upper bound in Route Target validation.
The 4-byte ASN branch still rejects values with uint(self.split(':')[0]) <= 65535u; values above 65535 fall through without checking <= 4294967295. This lets inputs like 99999999999:100 pass the CRD CEL validation while maxLength: 21 allows 11-digit administrators. Add the 4-byte global bound to the FRR-K8s CRD and sync it from openshift/frr.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindata/network/frr-k8s/001-crd.yaml` around lines 186 - 201, Update the
4-byte ASN Route Target validation rule in the CRD to enforce the global
administrator upper bound of 4294967295, replacing the incorrect lower-bound
condition and preserving the existing format checks. Apply the same validation
change in the corresponding openshift/frr source and regenerate or sync the
FRR-K8s CRD.
|
/verified by ci |
openshift-ci-robot
added
the
verified
|
@oribon: 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. |
|
/test frrk8s-e2e |
1 similar comment
|
/test frrk8s-e2e |
|
@oribon: 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. |
|
/test frrk8s-e2e |
|
/test e2e-metal-ipi-ovn-dualstack-bgp |
2 participants
sync with openshift/frr#135