Conversation
Both reconcilers requeued healthy resources after a bare 5 * time.Minute literal, duplicated, uncommented and unnamed. That number is not arbitrary: it bounds how long the cluster can sit with drifted AWS state, a route server peer deleted or SourceDestCheck re-enabled by someone, before we notice and put it back. It is worth naming. It is also worth being able to change. Diagnosing anything that depends on a reconcile costs five minutes per attempt, which is long enough that you stop investigating and start guessing. So name it DefaultResyncInterval, give each reconciler a ResyncInterval field that overrides it, and expose --resync-interval. The default is unchanged, and the existing assertions that pin 5m still pass, so nothing moves for anyone who does not ask for it.
DescribeRouteServerPeers keeps returning peers long after AWS has
deleted them, in state "deleted". listAllPeers filtered only on endpoint
ID, so those tombstones counted as working peers. Delete a CUDNBgpConfig
and recreate it, which is what an e2e run does, and the operator finds
the old peers still listed, creates nothing, and reports:
AWSResourcesReconciled True Route Server peers and source/dest check
reconciled
with no peers in existence and every BGP session in Connect. Observed on
a live cluster: seven consecutive reconciles over three and a half
minutes, three nodes, zero peers created, phase Ready throughout. It also
made us call DeleteRouteServerPeer on peers AWS had already deleted,
which fails with IncorrectState.
Decide liveness with an allowlist rather than by excluding the states we
happen to know about, so a state AWS adds later is treated as gone and we
recreate, instead of being counted as a working peer and silently
believed. "pending" is alive: peers sit there for minutes after creation
and must not be duplicated on the next resync.
The same pass also stops asking AWS the same question repeatedly.
DescribeRouteServerPeers accepts no endpoint filter, so every call
returns every peer in the region regardless; calling it once per
endpoint, twice over, made 2N identical full-region calls per reconcile,
twelve on a three-AZ cluster. These rate buckets are per-account, so on a
shared development account that amplification is the contended resource.
One call now, bucketed by endpoint.
That rewrite lands in the same lines as the liveness fix, hence one
commit rather than a split that would describe a state this code was
never in.
Iteration is sorted throughout. endpointsByAZ is a map, and Go randomises
map iteration, so peers were created and deleted in a different order on
every reconcile and no failure could be reproduced.
The three existing peer fixtures gain State: available. They model peers
that really exist, and omitting the field now reads as "gone".
The suite could not be re-run after any failure. It creates a CUDNBgpConfig, a CUDNBgpRouting and a namespace with Create, which is not idempotent, and had no AfterSuite, so anything a failed run left behind made every later run fail on AlreadyExists until someone cleared it by hand. Interrupting a run had the same effect, and interrupting runs is what you do while iterating. Four changes, all of the same kind: stop assuming the cluster is already in the state you want. Generate the namespace name. The CUDN selects namespaces by the cluster-udn label, never by name, so naming it after the network was convention rather than requirement, and it was the whole source of the collision. A stranded prod-x4k2m also reads as test wreckage in a way a stranded prod does not. Add AfterSuite, deleting the routing CR, then the config CR, then the namespace, tolerating NotFound. Order matters: the config CR's finalizer blocks its own deletion while a routing CR exists, which E2E-AWS-05 asserts deliberately. E2E_SKIP_CLEANUP keeps the wreckage when you want to look at it, which is how the bug in the previous commit was found. Retry the creates. Cleanup completing is not the same as the cluster having caught up: the config CR's finalizer is only removed on the next reconcile, so it lingers in Terminating and a run started in that window died instantly with "object is being deleted". Wait for a peer to be available before deleting one. E2E-AWS-03 took peers[0] and deleted it, but AWS rejects DeleteRouteServerPeer on a pending peer, and E2E-AWS-01 is satisfied by peers merely existing, so the spec was racing the operator it was testing. It also filters out dead peers, mirroring the operator, and sorts before choosing: allManagedPeers walks a map, so peers[0] was a uniformly random peer, a different node in a different AZ on every run. Finally, take the reconcile budget from E2E_RESYNC_INTERVAL rather than assuming the operator's old default. The suite hard-coded six minutes per self-healing assertion and printed "next 5m cycle" whatever the operator was actually using.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
createOrUpdate did Get then Update with no comparison, so every reconcile rewrote its object whether or not anything had changed. Both controllers watch what they write, so each write came back immediately as an event and caused another reconcile, which wrote again. On a live cluster the routing controller settled into roughly two reconciles a second, indefinitely, rewriting the ClusterUserDefinedNetwork every time. Nothing converged because nothing was meant to: the write was its own trigger. Measured over ninety seconds with the default five minute resync, so the requeue interval had nothing to do with it. Compare the spec, and the labels we set, against what is already there, and return without writing when they match. Labels added by anyone else are ignored rather than treated as drift, since reacting to them would reopen the same loop from the other side. Same cluster, same CRs, before and after: 213 reconciles in 114 seconds becomes 1, and the CUDN's resourceVersion stops moving.
frobware
marked this pull request as ready for review
August 12, 2026 11:17
frobware
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
WalkthroughThe change adds configurable reconciler resynchronization, avoids unchanged FRR writes, and makes AWS route-server peer reconciliation liveness-aware and deterministic. AWS E2E tests now support configurable intervals, generated namespaces, retry-based creation, ordered cleanup, and stable peer selection. ChangesController resync and conditional updates
AWS route-server reconciliation
AWS E2E lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RouteServerReconciler
participant AWSRouteServerAPI
participant ManagedPeers
RouteServerReconciler->>AWSRouteServerAPI: DescribeRouteServerPeers once
AWSRouteServerAPI-->>RouteServerReconciler: Return peer states and tags
RouteServerReconciler->>ManagedPeers: Filter live managed peers and group by endpoint
RouteServerReconciler->>AWSRouteServerAPI: Delete stale peers and create missing peers in sorted order
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 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 `@cmd/main.go`:
- Around line 86-88: Validate resyncInterval immediately after flag.Parse() in
cmd/main.go and reject zero or negative values before controller startup. Update
the ResyncInterval comments in internal/controller/cudnbgpconfig_controller.go
lines 67-75 and internal/controller/cudnbgprouting_controller.go lines 50-58
from “non-zero” to “positive”; no other changes are required at those sites.
In `@internal/controller/frr.go`:
- Around line 279-282: Update specEqual in internal/controller/frr.go:279-282 to
return NestedMap errors, and propagate them through createOrUpdate so malformed
specs are not overwritten. In
internal/controller/cudnbgprouting_controller_test.go:106 and 393-395, make
ignored Reconcile errors fail the tests; at 422-424 and 464-466, assert
SetNestedMap errors; and at 482, assert the NestedString error.
In `@internal/platform/aws/route_server.go`:
- Around line 95-107: Update listPeersByEndpoint and listAllPeers to iterate
through every DescribeRouteServerPeers page using the SDK paginator or a shared
pagination helper, propagating each returned NextToken until exhausted and
aggregating all peers before reconciliation. Preserve the existing filtering and
endpoint grouping behavior, and add a two-page mock test verifying token
propagation and combined results.
In `@test/e2e/aws/aws_e2e_suite_test.go`:
- Around line 222-230: Update createEventually to use the configured
reconcileTimeout for Eventually instead of the hardcoded three-minute timeout,
ensuring it is at least resyncInterval so finalizer removal and creation retries
can complete after a suite restart.
- Around line 182-193: Update the deleteAndWait cleanup helper to fail the
Ginkgo test when k8sClient.Delete returns a non-NotFound error instead of
logging and returning. Propagate the deletion error through the helper using the
suite’s established Ginkgo failure mechanism, while preserving successful
handling of already-absent resources and the existing wait for deletion.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 76a973cb-6e3b-41f3-ba36-fc25832f4aa6
📒 Files selected for processing (10)
cmd/main.gointernal/controller/constants.gointernal/controller/cudnbgpconfig_controller.gointernal/controller/cudnbgprouting_controller.gointernal/controller/cudnbgprouting_controller_test.gointernal/controller/frr.gointernal/platform/aws/aws_test.gointernal/platform/aws/route_server.gotest/e2e/aws/aws_e2e_suite_test.gotest/e2e/aws/aws_e2e_test.go
Comment on lines
+86
to
+88
| flag.DurationVar(&resyncInterval, "resync-interval", controller.DefaultResyncInterval, | ||
| "How long to wait before re-examining a healthy resource. Lower values detect "+ | ||
| "drifted AWS state sooner at the cost of more API calls.") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant symbols and call sites ---'
rg -n -C 4 \
'resyncInterval|ResyncInterval|DefaultResyncInterval|resync-interval|flag\.Parse|Parse\(\)' \
cmd/main.go internal/controller/cudnbgpconfig_controller.go \
internal/controller/cudnbgprouting_controller.go internal/controller/frr.go \
internal/controller/cudnbgprouting_controller_test.go
printf '%s\n' '--- file outlines ---'
ast-grep outline cmd/main.go
ast-grep outline internal/controller/cudnbgpconfig_controller.go
ast-grep outline internal/controller/cudnbgprouting_controller.goRepository: openshift/bgp-cloud-connector
Length of output: 10946
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- startup flow and default ---'
sed -n '40,125p' cmd/main.go
rg -n -C 3 'DefaultResyncInterval' --glob '*.go'
printf '%s\n' '--- related tests and documentation ---'
rg -n -C 4 'resync|ResyncInterval|resync-interval' --glob '*_test.go' --glob '*.md' --glob '*.yaml' --glob '*.yml' .
printf '%s\n' '--- Go tool availability ---'
command -v go || true
go version 2>/dev/null || trueRepository: openshift/bgp-cloud-connector
Length of output: 3961
🏁 Script executed:
#!/bin/bash
set -eu
# Read-only standalone probe of standard-library flag.DurationVar semantics.
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/probe.go" <<'EOF'
package main
import (
"flag"
"fmt"
"time"
)
func main() {
for _, arg := range []string{"0s", "-1s", "1s", "not-a-duration"} {
fs := flag.NewFlagSet("probe", flag.ContinueOnError)
fs.SetOutput(nil)
var d time.Duration
fs.DurationVar(&d, "resync-interval", 5*time.Minute, "")
err := fs.Parse([]string{"-resync-interval", arg})
fmt.Printf("%q: value=%s error=%v\n", arg, d, err)
}
}
EOF
go run "$tmpdir/probe.go"Repository: openshift/bgp-cloud-connector
Length of output: 488
🏁 Script executed:
#!/bin/bash
set -eu
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/probe.go" <<'EOF'
package main
import (
"flag"
"fmt"
"time"
)
func main() {
for _, arg := range []string{"0s", "-1s", "1s", "not-a-duration"} {
fs := flag.NewFlagSet("probe", flag.ContinueOnError)
fs.SetOutput(nil)
var d time.Duration
fs.DurationVar(&d, "resync-interval", 5*time.Minute, "")
err := fs.Parse([]string{"-resync-interval", arg})
fmt.Printf("%q: value=%s error=%v\n", arg, d, err)
}
}
EOF
go run "$tmpdir/probe.go"Repository: openshift/bgp-cloud-connector
Length of output: 488
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- locate default declaration ---'
rg -n -C 5 --glob '*.go' 'DefaultResyncInterval|time\.Minute|RequeueAfter' .
printf '%s\n' '--- all reconciler construction sites ---'
rg -n -C 5 --glob '*.go' \
'CUDNBgpConfigReconciler\s*\{|CUDNBgpRoutingReconciler\s*\{|ResyncInterval:'
printf '%s\n' '--- all resync documentation ---'
rg -n -i -C 4 --glob '*.go' --glob '*.md' --glob '*.yaml' --glob '*.yml' \
'resync.?interval|resync|requeueafter' . || true
printf '%s\n' '--- remaining main startup flow ---'
sed -n '125,235p' cmd/main.goRepository: openshift/bgp-cloud-connector
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
paths = [
Path("internal/controller/cudnbgpconfig_controller.go"),
Path("internal/controller/cudnbgprouting_controller.go"),
]
for path in paths:
text = path.read_text()
match = re.search(
r"func \(r \*[^)]*\) resyncInterval\(\) time\.Duration \{\s*"
r"if r\.ResyncInterval > 0 \{\s*"
r"return r\.ResyncInterval\s*"
r"\}\s*"
r"return DefaultResyncInterval",
text,
re.S,
)
print(f"{path}: resolver_pattern={'present' if match else 'absent'}")
if match:
for value in ("-1ns", "0s", "1ns", "5m0s"):
# The source condition is > 0; this models the resolver's branch.
nanoseconds = {
"-1ns": -1,
"0s": 0,
"1ns": 1,
"5m0s": 300_000_000_000,
}[value]
result = value if nanoseconds > 0 else "DefaultResyncInterval"
print(f" {value} -> {result}")
PYRepository: openshift/bgp-cloud-connector
Length of output: 496
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
paths = [
Path("internal/controller/cudnbgpconfig_controller.go"),
Path("internal/controller/cudnbgprouting_controller.go"),
]
for path in paths:
text = path.read_text()
match = re.search(
r"func \(r \*[^)]*\) resyncInterval\(\) time\.Duration \{\s*"
r"if r\.ResyncInterval > 0 \{\s*"
r"return r\.ResyncInterval\s*"
r"\}\s*"
r"return DefaultResyncInterval",
text,
re.S,
)
print(f"{path}: resolver_pattern={'present' if match else 'absent'}")
if match:
for value in ("-1ns", "0s", "1ns", "5m0s"):
nanoseconds = {
"-1ns": -1,
"0s": 0,
"1ns": 1,
"5m0s": 300_000_000_000,
}[value]
result = value if nanoseconds > 0 else "DefaultResyncInterval"
print(f" {value} -> {result}")
PYRepository: openshift/bgp-cloud-connector
Length of output: 496
Reject non-positive --resync-interval values.
flag.DurationVar accepts 0s and negative durations. Both resolvers replace them with DefaultResyncInterval (5 * time.Minute). Validate resyncInterval > 0 after flag.Parse() and update both ResyncInterval comments from “non-zero” to “positive”.
📍 Affects 3 files
cmd/main.go#L86-L88(this comment)internal/controller/cudnbgpconfig_controller.go#L67-L75internal/controller/cudnbgprouting_controller.go#L50-L58
🤖 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 `@cmd/main.go` around lines 86 - 88, Validate resyncInterval immediately after
flag.Parse() in cmd/main.go and reject zero or negative values before controller
startup. Update the ResyncInterval comments in
internal/controller/cudnbgpconfig_controller.go lines 67-75 and
internal/controller/cudnbgprouting_controller.go lines 50-58 from “non-zero” to
“positive”; no other changes are required at those sites.
Source: Path instructions
Comment on lines
+279
to
+282
| func specEqual(existing, desired *unstructured.Unstructured) bool { | ||
| existingSpec, _, _ := unstructured.NestedMap(existing.Object, "spec") | ||
| desiredSpec, _, _ := unstructured.NestedMap(desired.Object, "spec") | ||
| return equality.Semantic.DeepEqual(existingSpec, desiredSpec) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline internal/controller/frr.go --items all
ast-grep outline internal/controller/cudnbgprouting_controller_test.go --items all
rg -n -C 2 \
'unstructured\.(NestedMap|SetNestedMap|NestedString)|_, _ = r\.Reconcile' \
internal/controller/frr.go \
internal/controller/cudnbgprouting_controller_test.goRepository: openshift/bgp-cloud-connector
Length of output: 5878
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- frr.go ---'
sed -n '240,310p' internal/controller/frr.go
printf '%s\n' '--- affected test sections ---'
sed -n '80,120p' internal/controller/cudnbgprouting_controller_test.go
sed -n '372,495p' internal/controller/cudnbgprouting_controller_test.go
printf '%s\n' '--- helper call sites and dependency versions ---'
rg -n -C 3 'specEqual\(|createOrUpdate\(|NestedMap\(|SetNestedMap\(|NestedString\(' internal/controller
rg -n 'k8s.io/apimachinery|controller-runtime' go.mod go.sumRepository: openshift/bgp-cloud-connector
Length of output: 14770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- local module cache candidates ---'
find "${GOMODCACHE:-/root/go/pkg/mod}" -path '*/k8s.io/apimachinery*' -type f \
\( -name 'helpers.go' -o -name 'unstructured.go' \) 2>/dev/null | head -20 || true
printf '%s\n' '--- all ignored errors in the affected test file ---'
rg -n -C 2 '(^|[^[:alnum:]_])(_|_, _)[[:space:]]*=[[:space:]]*[^;]*(Reconcile|SetNestedMap|NestedString)' \
internal/controller/cudnbgprouting_controller_test.go
printf '%s\n' '--- object construction relevant to the tests ---'
sed -n '1,90p' internal/controller/cudnbgprouting_controller_test.go
sed -n '400,490p' internal/controller/cudnbgprouting_controller_test.goRepository: openshift/bgp-cloud-connector
Length of output: 7202
🌐 Web query:
k8s.io/apimachinery v0.34.0 unstructured NestedMap SetNestedMap NestedString source
💡 Result:
In the k8s.io/apimachinery library (e.g., version 0.34.0), the functions NestedMap, SetNestedMap, and NestedString are helper utilities located in the k8s.io/apimachinery/pkg/apis/meta/v1/unstructured package [1][2]. These functions are designed to generically access or manipulate fields within the nested map structure of an Unstructured Kubernetes object [3][4]. NestedMap This function retrieves a deep copy of a map[string]interface{} value located at a specified nested path [2][5]. Signature: func NestedMap(obj map[string]interface{}, fields...string) (map[string]interface{}, bool, error) Behavior: It returns the map, a boolean indicating if the field was found, and an error if the path contains non-map elements or if the target is not a map[string]interface{} [2][6]. SetNestedMap This function sets the map[string]interface{} value of a nested field [2][5]. Signature: func SetNestedMap(obj map[string]interface{}, value map[string]interface{}, fields...string) error Behavior: It updates the provided object map with the new value at the specified field path [2]. It returns an error if any level of the path cannot be traversed because it is not a map[string]interface{} [2][6]. NestedString This function retrieves the string value of a nested field [2][5]. Signature: func NestedString(obj map[string]interface{}, fields...string) (string, bool, error) Behavior: It returns the string value, a found boolean, and an error if the field is not a string [2][6]. These functions operate directly on the underlying map[string]interface{} (often accessed via the Object field of an Unstructured struct) [3][4]. You can find the source code for these helpers in the k8s.io/apimachinery repository under pkg/apis/meta/v1/unstructured/helpers.go [2][6].
Citations:
- 1: https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured
- 2: https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/unstructured/helpers.go
- 3: https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/unstructured/unstructured.go
- 4: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/apis/meta/v1/unstructured/unstructured.go
- 5: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/apis/meta/v1/unstructured/helpers.go
- 6: https://github.com/kubernetes/apimachinery/blob/v0.35.3/pkg/apis/meta/v1/unstructured/helpers.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
curl -fsSL \
'https://raw.githubusercontent.com/kubernetes/apimachinery/v0.34.0/pkg/apis/meta/v1/unstructured/helpers.go' \
-o "$tmp"
printf '%s\n' '--- exact helper implementations ---'
rg -n -A35 -B3 \
'func (NestedMap|SetNestedMap|NestedString)\b' "$tmp"
printf '%s\n' '--- behavioral probe for the shown object shapes ---'
python3 - <<'PY'
def nested_map(obj, *fields):
cur = obj
for field in fields:
if not isinstance(cur, dict):
return None, False, "cannot traverse non-map"
if field not in cur:
return None, False, None
cur = cur[field]
if not isinstance(cur, dict):
return None, False, "value is not a map"
return dict(cur), True, None
def set_nested_map(obj, value, *fields):
cur = obj
for field in fields[:-1]:
if field not in cur:
cur[field] = {}
elif not isinstance(cur[field], dict):
return "cannot traverse non-map"
cur = cur[field]
cur[fields[-1]] = dict(value)
return None
def nested_string(obj, *fields):
cur = obj
for field in fields:
if not isinstance(cur, dict):
return "", False, "cannot traverse non-map"
if field not in cur:
return "", False, None
cur = cur[field]
if not isinstance(cur, str):
return "", False, "value is not a string"
return cur, True, None
desired = {}
set_error = set_nested_map(desired, {"topology": "Layer2"}, "spec")
changed = {}
set_error_changed = set_nested_map(changed, {"topology": "Layer3"}, "spec")
topology, found, string_error = nested_string(changed, "spec", "topology")
print({"desired_set_error": set_error,
"changed_set_error": set_error_changed,
"nested_string": (topology, found, string_error)})
malformed_existing = {"spec": "not-a-map"}
existing_spec, found, map_error = nested_map(malformed_existing, "spec")
print({"malformed_existing_nested_map": (existing_spec, found, map_error)})
PYRepository: openshift/bgp-cloud-connector
Length of output: 5117
Propagate reconciliation and unstructured-object errors.
Make specEqual return NestedMap errors so createOrUpdate does not overwrite an object with a malformed spec. Fail the tests on ignored Reconcile errors at lines 106 and 393, SetNestedMap errors at lines 422 and 464, and the NestedString error at line 482.
📍 Affects 2 files
internal/controller/frr.go#L279-L282(this comment)internal/controller/cudnbgprouting_controller_test.go#L393-L395internal/controller/cudnbgprouting_controller_test.go#L422-L424internal/controller/cudnbgprouting_controller_test.go#L464-L466internal/controller/cudnbgprouting_controller_test.go#L482-L482
🤖 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 `@internal/controller/frr.go` around lines 279 - 282, Update specEqual in
internal/controller/frr.go:279-282 to return NestedMap errors, and propagate
them through createOrUpdate so malformed specs are not overwritten. In
internal/controller/cudnbgprouting_controller_test.go:106 and 393-395, make
ignored Reconcile errors fail the tests; at 422-424 and 464-466, assert
SetNestedMap errors; and at 482, assert the NestedString error.
Source: Path instructions
Comment on lines
+95
to
+107
| output, err := p.ec2Client.DescribeRouteServerPeers(ctx, &ec2.DescribeRouteServerPeersInput{}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| byEndpoint := make(map[string][]ec2types.RouteServerPeer) | ||
| for _, peer := range output.RouteServerPeers { | ||
| if !peerIsAlive(peer.State) { | ||
| continue | ||
| } | ||
| id := aws.ToString(peer.RouteServerEndpointId) | ||
| byEndpoint[id] = append(byEndpoint[id], peer) | ||
| } | ||
| return byEndpoint, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
curl --fail --silent --show-error \
"https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_DescribeRouteServerPeers.html" \
| grep -E -i -C2 'NextToken|MaxResults'Repository: openshift/bgp-cloud-connector
Length of output: 537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | grep -E '(^|/)route_server\.go$|go\.mod$|go\.sum$' || true
printf '%s\n' '--- route-server symbols and API calls ---'
rg -n -C 4 'DescribeRouteServerPeers|listPeersByEndpoint|listAllPeers|NextToken|peerIsAlive' --glob '*.go' .
printf '%s\n' '--- AWS SDK module versions ---'
rg -n 'aws-sdk-go-v2|service/ec2' go.mod go.sum 2>/dev/null || trueRepository: openshift/bgp-cloud-connector
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
route_file="$(git ls-files | grep 'internal/platform/aws/route_server\.go$' | head -n 1)"
printf '%s\n' "--- $route_file ---"
cat -n "$route_file" | sed -n '1,220p'
printf '%s\n' '--- non-vendor API call sites ---'
rg -n -C 5 --glob '*.go' --glob '!vendor/**' 'DescribeRouteServerPeers|listPeersByEndpoint|listAllPeers'
printf '%s\n' '--- SDK definitions for DescribeRouteServerPeers ---'
sdk_file="$(git ls-files | grep 'vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DescribeRouteServerPeers\.go$' | head -n 1)"
if [ -n "$sdk_file" ]; then
cat -n "$sdk_file" | sed -n '1,220p'
else
rg -l --glob 'vendor/**/*.go' 'type DescribeRouteServerPeersInput struct|type DescribeRouteServerPeersOutput struct' \
| head -n 5
fiRepository: openshift/bgp-cloud-connector
Length of output: 9029
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tracked route-server tests and call sites ---'
git ls-files | grep -E '(^|/)route_server.*\.go$' | while IFS= read -r f; do
printf '\n--- %s ---\n' "$f"
rg -n -C 4 'DescribeRouteServerPeers|listPeersByEndpoint|listAllPeers|NextToken' "$f" || true
done
printf '%s\n' '--- tracked SDK operation definition ---'
sdk_file="$(git ls-files | grep -E '/api_op_DescribeRouteServerPeers\.go$' | head -n 1 || true)"
if [ -n "$sdk_file" ]; then
cat -n "$sdk_file" | sed -n '1,180p'
else
printf '%s\n' 'SDK operation file not found in tracked files'
rg -n -m 12 --glob 'vendor/github.com/aws/aws-sdk-go-v2/service/ec2/**/*.go' \
'type DescribeRouteServerPeers(Input|Output) struct|func \(.*DescribeRouteServerPeers' vendor 2>/dev/null || true
fiRepository: openshift/bgp-cloud-connector
Length of output: 9112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- paginator API ---'
sdk_file="$(git ls-files | grep -E '/api_op_DescribeRouteServerPeers\.go$' | head -n 1)"
cat -n "$sdk_file" | sed -n '170,280p'
printf '%s\n' '--- all listManagedPeers/listAllPeers usages ---'
rg -n -C 5 --glob '*.go' --glob '!vendor/**' \
'listManagedPeers|listAllPeers|listPeersByEndpoint' . || true
printf '%s\n' '--- route-server test files ---'
git ls-files | grep -E '(^|/)internal/platform/aws/.*_test\.go$' | head -n 100Repository: openshift/bgp-cloud-connector
Length of output: 10311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
route = Path("internal/platform/aws/route_server.go").read_text()
sdk = next(Path(".").glob("vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_DescribeRouteServerPeers.go")).read_text()
checks = {
"route_server_has_two_unpaginated_calls": len(re.findall(
r'DescribeRouteServerPeers\(ctx,\s*&ec2\.DescribeRouteServerPeersInput\{\}\)',
route,
)) == 2,
"sdk_input_has_next_token": bool(re.search(
r'type DescribeRouteServerPeersInput struct \{.*?NextToken \*string',
sdk, re.S,
)),
"sdk_output_has_next_token": bool(re.search(
r'type DescribeRouteServerPeersOutput struct \{.*?NextToken \*string',
sdk, re.S,
)),
"sdk_exposes_paginator": "NewDescribeRouteServerPeersPaginator" in sdk,
"list_all_peers_is_used_by_cleanup": "listManagedPeers(ctx, endpointID)" in route,
}
for name, result in checks.items():
print(f"{name}={result}")
assert all(checks.values())
PYRepository: openshift/bgp-cloud-connector
Length of output: 338
Handle all DescribeRouteServerPeers pages.
listPeersByEndpoint and listAllPeers each issue one unfiltered request. If NextToken is returned, reconciliation can create duplicate peers or skip stale-peer deletion. Use the SDK paginator or a shared paginated helper in both methods. Add a two-page mock test that asserts token propagation and result aggregation.
🤖 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 `@internal/platform/aws/route_server.go` around lines 95 - 107, Update
listPeersByEndpoint and listAllPeers to iterate through every
DescribeRouteServerPeers page using the SDK paginator or a shared pagination
helper, propagating each returned NextToken until exhausted and aggregating all
peers before reconciliation. Preserve the existing filtering and endpoint
grouping behavior, and add a two-page mock test verifying token propagation and
combined results.
Comment on lines
+182
to
+193
| deleteAndWait := func(obj client.Object, what string) { | ||
| if err := k8sClient.Delete(ctx, obj); err != nil { | ||
| if !apierrors.IsNotFound(err) { | ||
| GinkgoWriter.Printf("cleanup: deleting %s: %v\n", what, err) | ||
| } | ||
| return | ||
| } | ||
| GinkgoWriter.Printf("cleanup: deleted %s\n", what) | ||
| Eventually(func() bool { | ||
| err := k8sClient.Get(ctx, client.ObjectKeyFromObject(obj), obj) | ||
| return apierrors.IsNotFound(err) | ||
| }, 3*time.Minute, 5*time.Second).Should(BeTrue(), "%s should go away", what) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail cleanup when deletion fails.
Line 187 logs a non-NotFound Delete error and returns. The suite can then pass while the fixed-name resource remains. A later run can fail during creation.
Propagate the error through Ginkgo, or retry deletion until the cleanup timeout expires. As per path instructions, **/*.go: Never ignore error returns.
🤖 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 `@test/e2e/aws/aws_e2e_suite_test.go` around lines 182 - 193, Update the
deleteAndWait cleanup helper to fail the Ginkgo test when k8sClient.Delete
returns a non-NotFound error instead of logging and returning. Propagate the
deletion error through the helper using the suite’s established Ginkgo failure
mechanism, while preserving successful handling of already-absent resources and
the existing wait for deletion.
Source: Path instructions
Comment on lines
+222
to
+230
| func createEventually(ctx context.Context, obj client.Object, what string) { | ||
| GinkgoHelper() | ||
| Eventually(func() error { | ||
| err := k8sClient.Create(ctx, obj) | ||
| if apierrors.IsAlreadyExists(err) { | ||
| GinkgoWriter.Printf("waiting for previous %s to finish deleting\n", what) | ||
| } | ||
| return err | ||
| }, 3*time.Minute, 5*time.Second).Should(Succeed(), "creating %s", what) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use the configured reconciliation timeout.
Line 230 stops creation retries after three minutes. E2E_RESYNC_INTERVAL defaults to five minutes, and this helper documents that finalizer removal can require one resync interval. A restarted suite can time out before the previous CR is removed.
Use reconcileTimeout, or a timeout that is at least resyncInterval.
🤖 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 `@test/e2e/aws/aws_e2e_suite_test.go` around lines 222 - 230, Update
createEventually to use the configured reconcileTimeout for Eventually instead
of the hardcoded three-minute timeout, ensuring it is at least resyncInterval so
finalizer removal and creation retries can complete after a suite restart.
Contributor
|
I think I hit the same issue while trying the e2e, /assign |
Contributor
|
@frobware: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
PR needs rebase. 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. |
frobware
marked this pull request as draft
August 20, 2026 15:00
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
aswinsuryan
added a commit
to aswinsuryan/bgp-cloud-connector
that referenced
this pull request
Aug 21, 2026
createOrUpdate did an unconditional Get-then-Update on every call, and both controllers watch the objects they write, so every write re-triggered their own reconcile - an infinite loop (already found and fixed once, in unmerged upstream PR openshift#13, but never merged to main). It also fanned out further than that: CUDNBgpConfig's reconcile makes live AWS calls, and CUDNBgpRouting's shared RouteAdvertisements object turns one CR's rewrite into a reconcile storm across every CR. createOrUpdate now skips the write when the spec and our own managed labels already match, while leaving any labels we don't own untouched. Comparing the spec exactly doesn't work: FRRConfiguration defaults neighbors[].dualStackAddressFamily, so what comes back from the API server always has a field we never set, and an exact comparison would be false forever, rewriting the object on every single reconcile. The comparison now only checks that the fields we set are present with the value we want, and ignores anything only the server added. The write also replaced metadata wholesale, so anything we don't manage ourselves - a foreign label, or ovn-kubernetes' own finalizer or annotations on a CUDN - was silently dropped the first time a real update happened. Labels and annotations from the existing object are now merged in before the update instead of being overwritten. Adds direct unit tests for createOrUpdate/specEqual/labelsSatisfied and regression tests in both controllers' reconcile tests. The envtest integration suite from the first pass of this work is left out for now and will follow separately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Aswin Suryanarayanan <asuryana@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
The operator can report
phase: Ready, with every conditionTrue, whilst no BGP session exists and no Route Server peers exist at all.DescribeRouteServerPeerskeeps returning peers long after AWS has deleted them, in statedeleted.listAllPeersfiltered only on endpoint ID, so those tombstones counted as live peers, their addresses looked already-peered, and the operator created nothing. It then reported success, because the condition describes what the reconcile function did rather than what is true of the world:Reproducing it needs nothing exotic. Delete a
CUDNBgpConfigand create it again, which is exactly what an e2e run does. The finalizer removes the peers, AWS keeps listing them asdeleted, and the next reconcile believes they are still there.Observed on a live cluster in
us-east-2with three router nodes across three AZs: seven consecutive reconciles over three and a half minutes,nodeCount: 3logged each time, zero peers created,phase: Readythroughout, all three BGP sessions sat inConnect. With the fix, the peers are recreated on the first reconcile after startup and reachavailablewithin ninety seconds.The same defect made us call
DeleteRouteServerPeeron peers AWS had already deleted, which returnsIncorrectState. That is whatE2E-AWS-03had been hitting.Why an allowlist
The obvious fix is to skip peers in
deleted, and it is the wrong one. It leaves us guessing at the set of states that mean gone, and silently counts any state AWS introduces later as a working peer.peerIsAlivetherefore admits onlyavailableandpendingand treats everything else, known or not, as absent. Getting that wrong recreates a peer unnecessarily; getting the denylist wrong leaves BGP down whilst claiming success.pendinghas to be alive. Peers sit there for minutes after creation, and at a short resync interval treating them as gone means creating a second peer for the same address.API amplification
Whilst in that function:
DescribeRouteServerPeersaccepts no endpoint filter, so every call returns every peer in the region regardless of what you asked for. Calling it once per endpoint, twice over, made2Nidentical full-region calls per reconcile, twelve on a three-AZ cluster, measured by a test that counts them. It is one call now, bucketed by endpoint.That matters more than it looks. These rate buckets are per-account (
CreateRouteServerPeerandDeleteRouteServerPeerrefill at 5/s for the whole account), so on a shared development or CI account the contended resource is the API, not the route servers.Iteration is sorted throughout, because
endpointsByAZis a map and Go randomises map iteration: peers were created and deleted in a different order on every reconcile, so no failure could be reproduced.That rewrite lands in the same lines as the liveness fix, so the two are one commit rather than a split describing a state this code was never in.
The reconcile storm
createOrUpdatedidGetthenUpdatewith no comparison, so every reconcile rewrote its object whether or not anything had changed. Both controllers watch what they write, so each write returned immediately as an event and caused another reconcile, which wrote again.On a live cluster the routing controller settled into roughly two reconciles a second, indefinitely, rewriting the
ClusterUserDefinedNetworkevery time. Nothing converged because nothing was meant to: the write was its own trigger.It reproduces on any cluster with FRR enabled and needs no AWS at all, since
config.Spec.AWS != nilgates the cloud work. Apply aCUDNBgpConfigwith staticavailabilityZones, a labelled namespace and aCUDNBgpRouting, then count reconciles.Measured with the default five minute resync, so the requeue interval is not involved.
Worth being explicit about #12, since it addresses the same symptom: it is not the fix for this, and this is not a criticism of it. #12 guards status writes by comparing against a baseline, which is correct and worth having. This is a spec write on a different object, which #12 does not touch, and the rate is unchanged with it applied. The two are complementary and I would expect both to be wanted.
Configurable resync interval
Both reconcilers requeued after a bare
5 * time.Minute, duplicated and uncommented. That number bounds how long the cluster can sit with drifted AWS state before we repair it, so it now has a name and a--resync-intervalflag. The default is unchanged and the existing assertions still pass.This is not cosmetic. Diagnosing the peer bug at five minutes per attempt is why it went unnoticed; at thirty seconds it took two cycles to see.
The e2e suite
E2E-AWS-03, "should recreate the deleted peer within the reconcile window", is the spec written to catch exactly the bug above. It could never have passed, because the suite's ownlistManagedPeerscontains the identical missing state check. So the test for the bug shared the bug.Fixing that exposed three more, all of the same family, all assuming the cluster is already in the state you want:
Createis not idempotent and there was noAfterSuite, so leftovers made every later run fail onAlreadyExistsE2E-AWS-03deletedpeers[0]without waiting for it to beavailable, racing the operator's own creation; and sinceallManagedPeerswalks a map,peers[0]was a uniformly random peer, a different node in a different AZ on every runnext 5m cycleregardless of what the operator was actually doingThe namespace is now generated rather than named after the network. The CUDN selects it by the
cluster-udnlabel, never by name, so the fixed name was convention and was the entire source of the collision.Test plan
Unit tests are red-green: each new test was run against the unfixed code first and fails for the stated reason.
TestReconcilePeers_RecreatesPeerNotAlivecoversdeleted,deleting,failing,failed, an invented future state and the empty string;TestReconcilePeers_DoesNotDuplicateLivePeerpins thatavailableandpendingare not duplicated;TestReconcilePeers_DescribesPeersOncecounts API calls (12 before, 1 after);TestReconcilePeers_CreateOrderIsDeterministicruns the reconcile twenty times and compares the sequence.End to end against a real cluster: OCP 5.0 nightly on AWS
us-east-2, three router nodes across three AZs, one Route Server with an endpoint per AZ, operator run locally against the cluster.E2E-AWS-03andE2E-AWS-05pass here for the first time.E2E-AWS-01also asserts established BGP sessions, so the fix is confirmed all the way through to FRR rather than only at the AWS API.Full run, verbatim
A note on
E2E_PROFILEmake test-e2e-aws <profile>and the suite'sBeforeSuitetake a profile naming a directory undertest/e2e/manifests/, holding theCUDNBgpConfigandCUDNBgpRoutingthe run applies. The profile is therefore the fixture: it decides which cluster, which route servers and which ASNs the run targets.qeabove is a local one, not in this branch. None of the three in tree fit a cluster you build yourself:rosa-bgp-pocpoints at the placeholderrs-0abc123456789abcdinus-east-1, and the twoocp-or18profiles are non-AWS, aimed at a lab with an external BGP peer at a fixed address. So running the AWS suite against any new cluster means hand-writing a profile with that cluster's realrouteServerIDsfirst.That is worth addressing separately, and it is part of why this suite appears not to have been run much: there is no documented path from a fresh cluster to a working profile, and the route server IDs inside one die with the cluster that owns them.
Not addressed
Deliberately out of scope, but found on the way and worth separate issues:
internal/platform/awshas no CI coverage at all.make testruns./internal/controller/... ./api/... ./cmd/..., and CI runs onlymake verify-vendorandmake test.make test-awsexists and is never invoked. Every test in this PR touching the AWS platform would not run in CI. A one-line change to thetesttarget fixes it, and would have caught this bug the moment a test existed for it.make generatedoes not reproduce the committedzz_generated.deepcopy.go. It is a deterministic 35-line reordering with the repo's own pinned controller-gen v0.18.0, so every developer runningmake test,make runormake deploygets a dirty tree.verify-vendorhas no counterpart for generated code, so CI cannot see it.The operator cannot start on a cluster lacking
frrk8s.metallb.ioandRouteAdvertisements. controller-runtime abandons the cache sync after two minutes and the process exits:PatchNetworkOperatorcreates that precondition during reconcile, which cannot help, because reconcile is downstream of a manager that will not start. So every result in this PR required enabling FRR by hand first, with the same patch the operator would have applied itself:Be aware of what that does before pasting it into a cluster you care about. It is a cluster-wide networking change: CNO rolls out OVN-Kubernetes and stands up an
frr-k8sdaemonset on every node. On the cluster used here it completed in about two minutes withco/networknever leavingDegraded=False.It is reversible, which I have since tested rather than assumed. Removing
additionalRoutingCapabilitiesand settingrouteAdvertisements: Disabledcauses CNO to delete thefrr-k8sdaemonset and its namespace and rollovnkube-nodeback across the cluster, again finishingAvailable=True, Progressing=False, Degraded=False. Thefrrk8s.metallb.ioandRouteAdvertisementsCRDs remain afterwards. One observation, on one OCP 5.0 cluster, with no workload depending on the advertised routes.It has to be done once per cluster before the operator will start at all. I have not added a script for it here on purpose: a workaround in
hack/would make this liveable rather than fixed, and the operator watching types it cannot guarantee exist is the actual defect. Deferring the watches until the CRDs appear, or tolerating their absence at startup, would remove the manual step entirely.There is no credential mechanism. No
CredentialsRequest, norole-arnannotation on the ServiceAccount. A deployed operator gets no AWS credentials from anything in this repo;ccoctlwould give roles to the core cluster operators and nothing to this one.(The reconcile storm that was listed here is now fixed by the fourth commit, described above.)
The general point
Every condition this operator sets describes what its reconcile function did, not what became true. That is why it can report
Readywith BGP down, and why the fault was invisible until three sources were compared by hand: the CR status,aws ec2 describe-route-server-peers, andBGPSessionState. The status cannot be used to verify the behaviour, because it is derived from the attempt.Making the conditions assert observed state would turn this operator into something self-diagnosing rather than confidently wrong, and is a bigger change than this PR.
Summary by CodeRabbit
New Features
--resync-intervaloption to control how frequently healthy resources are rechecked, defaulting to five minutes.Bug Fixes