fix: use unique namespace suffix in cilium connectivity check verifier - #6438
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Cilium connectivity-check verifier to avoid namespace/SCC name collisions when the verifier is run multiple times within a single E2E test run (e.g., per-nodepool invocation), by generating a unique namespace name per invocation and mutating the embedded SCC manifest at creation time.
Changes:
- Generate a unique namespace name per verifier run using a 6-character random suffix (
cilium-connectivity-check-xxxxxx). - Add an optional variadic “mutator” hook to
createArbitraryResourceso callers can patch loaded YAML objects prior to creation. - Use the mutator hook in the Cilium verifier to rewrite the SCC
metadata.nameandusersserviceaccount reference to the dynamic namespace.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
test/util/verifiers/helper.go |
Adds an optional mutator hook to allow patching unstructured resources before creating them. |
test/util/verifiers/cilium.go |
Creates a unique namespace per run and rewrites the SCC manifest to match it to prevent collisions. |
| if obj.GetKind() != "SecurityContextConstraints" { | ||
| return nil | ||
| } | ||
| obj.SetName(namespaceName) |
There was a problem hiding this comment.
Add a suffix instead of replacing the name entirely.
There was a problem hiding this comment.
Good catch — updated. The mutator now appends the suffix to the existing SCC name (obj.GetName() + "-" + suffix) instead of replacing it with namespaceName.
AI-generated. Review for accuracy.
ac07e55 to
42dfd01
Compare
|
/lgtm |
|
/retest-required AI-generated. Review for accuracy. |
|
/lgtm |
| obj.SetName(fmt.Sprintf("%s-%s", obj.GetName(), suffix)) | ||
| return unstructured.SetNestedStringSlice( | ||
| obj.Object, | ||
| []string{fmt.Sprintf("system:serviceaccount:%s:default", namespaceName)}, |
There was a problem hiding this comment.
Use the original value from the yaml, and update just the name of the namespace instead of hard coding it.
There was a problem hiding this comment.
Good point — updated. The mutator now reads the original users entries via unstructured.NestedStringSlice and rewrites only the namespace portion of each system:serviceaccount:<ns>:<sa-name> reference, preserving the service account names and any non-service-account entries from the YAML.
AI-generated. Review for accuracy.
The cilium connectivity-check verifier hardcoded the namespace name "cilium-connectivity-check". cluster_create_feature_aggregation.go invokes VerifyCiliumConnectivityChecks once per nodepool. Namespace deletion is asynchronous, so the second invocation could fail with "object is being deleted: namespaces already exists" while the first namespace was still terminating. The cluster-scoped SCC (name + service-account reference) was hardcoded to the same name and collided too. Generate a random suffix so each invocation uses a distinct cilium-connectivity-check-<suffix> namespace, and rewrite the SCC name and its users service-account reference to match via a mutator applied before resource creation. Deferred cleanup already tracks the dynamic names. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
42dfd01 to
92606ab
Compare
|
/retest-required AI-generated. Review for accuracy. |
1 similar comment
|
/retest-required AI-generated. Review for accuracy. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, mbukatov, miquelsi, redhat-chai-bot 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 |
|
/retest-required AI-generated. Review for accuracy. |
1 similar comment
|
/retest-required AI-generated. Review for accuracy. |
|
Addressed in #6460. This PR can be closed |
Problem
The
VerifyCiliumConnectivityChecksverifier intest/util/verifiers/cilium.gohardcodes the namespace name ascilium-connectivity-check. When the verifier is invoked multiple times in the same test (as incluster_create_feature_aggregation.gowhich calls it twice, once per nodepool), the second invocation fails because the namespace from the first invocation is still inTerminatingstate:The cluster-scoped
SecurityContextConstraintsresource also has the name andusersfield hardcoded to reference the same namespace, causing a double collision.Fix
rand.String(6)fromk8s.io/apimachinery/pkg/util/rand, producing namespaces likecilium-connectivity-check-abc123.createArbitraryResourceinhelper.go(backward-compatible variadic parameter) so the SCC name andusersservice account path can be patched at load time to reference the dynamic namespace.sccNamesandnamespaceNamedynamically.Testing
go build ./...passesgo vet ./...passesAI-generated. Review for accuracy.
David Eads (@deads2k) requested in Slack thread