ROSAENG-61409 | fix: harden shared VPC e2e teardown and subnet validation#3341
ROSAENG-61409 | fix: harden shared VPC e2e teardown and subnet validation#3341olucasfreitas wants to merge 2 commits into
Conversation
…tion Three CI failures in shared VPC profiles share a common root in the test infrastructure: ROSAENG-61409 — DeleteSecurityGroup UnauthorizedOperation during teardown. CleanupProxyResources now treats authorization errors as non-fatal so the remaining cleanup (VPC chain, roles, hosted zones) is not blocked. ROSAENG-61411 — InvalidSubnetID.NotFound during cluster preparation. After the RAM resource share is created, the subnets may not yet be visible from the primary account. A polling wait (up to 5 min) now validates subnet visibility before cluster creation proceeds. ROSAENG-61408 — OCP-84981 autonode configuration test failure. The hosted zone registration in PrepareHostedZone was swapped: the hypershift.local zone was recorded as IngressHostedZoneID and vice-versa. This caused the cleanup flow to use the wrong zone IDs, contributing to shared VPC environment instability that cascaded into autonode test failures. Signed-off-by: Lucas Freitas <lfreitas@redhat.com> Signed-off-by: lufreita <lufreita@redhat.com>
📝 WalkthroughWalkthroughThe PR adds subnet visibility polling during BYOVPC cluster creation after shared resource preparation, updates hosted zone registration to use an HCP-internal suffix helper, and changes proxy resource cleanup so AWS authorization errors from security-group deletion are logged and ignored while other errors still fail. It also adds helper functions and tests for AWS authorization detection, hosted zone classification, and subnet visibility polling behavior. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olucasfreitas 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.
🧹 Nitpick comments (5)
tests/utils/handler/resources_handler_clean.go (2)
430-434: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the
hypershift.localliteral to a shared constant.The
"hypershift.local"suffix is duplicated here and in the hosted-zone-name construction logic elsewhere (e.g.cluster_handler.go'sfmt.Sprintf("%s.hypershift.local", ...)). Extracting a package constant would prevent drift between the classifier and the name-builder. As per coding guidelines, "Avoid magic numbers; extract named constants when the value matters to behavior or readability."🤖 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 `@tests/utils/handler/resources_handler_clean.go` around lines 430 - 434, The hosted zone suffix is duplicated as a string literal in both the classifier and the zone-name builder, so extract it into a shared package constant and use that constant in hostedZoneIsHCPInternal as well as the hosted-zone construction logic (for example, the code that formats the HCP hostname). This keeps the suffix definition centralized and prevents drift between the functions that build and पहचान the zone.Source: Coding guidelines
418-428: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer
smithy.APIError/errors.Asover substring matching for AWS error classification.
isAWSAuthorizationErrorinspects the formatted error string viastrings.Contains, which is brittle (message text is not a stable contract and could produce false positives/negatives, e.g. if a resource name or unrelated context happens to contain the substring). AWS SDK for Go v2 exposessmithy.APIErrorwith anErrorCode()method specifically for this purpose.♻️ Suggested refactor using smithy.APIError
+import "github.com/aws/smithy-go" + func isAWSAuthorizationError(err error) bool { if err == nil { return false } - msg := err.Error() - return strings.Contains(msg, "UnauthorizedOperation") || - strings.Contains(msg, "AccessDenied") + var apiErr smithy.APIError + if !errors.As(err, &apiErr) { + return false + } + switch apiErr.ErrorCode() { + case "UnauthorizedOperation", "AccessDenied": + return true + default: + return false + } }🤖 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 `@tests/utils/handler/resources_handler_clean.go` around lines 418 - 428, `isAWSAuthorizationError` currently relies on `strings.Contains` against `err.Error()`, which is brittle for AWS error classification. Update this helper to use `errors.As` to detect `smithy.APIError` and check `ErrorCode()` for authorization-related codes like `UnauthorizedOperation` and `AccessDenied`, keeping the nil guard intact and preserving the existing teardown behavior for non-fatal auth failures.tests/utils/handler/resources_handler_prepare.go (3)
1244-1247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract poll interval/timeout as named constants.
10*time.Secondand5*time.Minuteare magic numbers governing retry behavior that directly affects CI reliability/flakiness for this fix. Naming them (e.g.subnetVisibilityPollInterval,subnetVisibilityPollTimeout) makes the tuning intent explicit and centralizes future adjustments.As per coding guidelines, "Avoid magic numbers; extract named constants when the value matters to behavior or readability."
♻️ Proposed fix
+const ( + subnetVisibilityPollInterval = 10 * time.Second + subnetVisibilityPollTimeout = 5 * time.Minute +) + func waitForSubnetsVisible(subnetIDs []string, checker subnetChecker) error { ... return wait.PollUntilContextTimeout( context.TODO(), - 10*time.Second, - 5*time.Minute, + subnetVisibilityPollInterval, + subnetVisibilityPollTimeout, true,🤖 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 `@tests/utils/handler/resources_handler_prepare.go` around lines 1244 - 1247, The retry settings in the polling call are hardcoded magic numbers; extract the `10*time.Second` and `5*time.Minute` values into named constants near `wait.PollUntilContextTimeout` (for example, constants used by the subnet visibility check) and use those symbols in this helper so the behavior is explicit and easier to tune later.Source: Coding guidelines
1235-1266: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
context.TODO()sparingly — accept a context for cancellation.
waitForSubnetsVisiblehardcodescontext.TODO()rather than accepting a caller-suppliedcontext.Context, so the 5-minute poll cannot be cancelled by an overall test/command timeout. Per the security guideline on usingcontext.Contextfor cancellation and timeouts, threading a context parameter through would let callers cap this wait with their own deadline.As per path instructions, "context.Context for cancellation and timeouts" is called out under Go security guidance.
♻️ Proposed refactor to accept a context
-func waitForSubnetsVisible(subnetIDs []string, checker subnetChecker) error { +func waitForSubnetsVisible(ctx context.Context, subnetIDs []string, checker subnetChecker) error { if len(subnetIDs) == 0 { return nil } log.Logger.Infof( "Waiting for %d shared subnets to become visible from the primary account (RAM propagation)...", len(subnetIDs)) return wait.PollUntilContextTimeout( - context.TODO(), + ctx, 10*time.Second, 5*time.Minute, true,🤖 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 `@tests/utils/handler/resources_handler_prepare.go` around lines 1235 - 1266, The wait helper currently hardcodes context.TODO() in waitForSubnetsVisible, which prevents caller-driven cancellation and timeout control. Update waitForSubnetsVisible to accept a context.Context parameter and pass it through to wait.PollUntilContextTimeout, then thread that context from the callers so the subnet visibility wait can be cancelled by the overall test or command deadline. Use the existing waitForSubnetsVisible symbol as the refactor point and keep the retry/error handling logic unchanged.Source: Path instructions
1252-1256: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse structured AWS error matching here.
strings.Contains(descErr.Error(), "InvalidSubnetID.NotFound")is brittle; useawserrors.IsSubnetNotFoundError(descErr)orawserrors.IsErrorCode(descErr, awserrors.SubnetNotFound)so the retry path keys offsmithy.APIError.ErrorCode()instead of the formatted message.🤖 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 `@tests/utils/handler/resources_handler_prepare.go` around lines 1252 - 1256, The subnet retry check in the shared-subnets description path is matching on a formatted error string, which is brittle. Update the logic in the handler that inspects descErr to use structured AWS error matching instead of strings.Contains, ideally awserrors.IsSubnetNotFoundError(descErr) or awserrors.IsErrorCode(descErr, awserrors.SubnetNotFound), so the retry branch keys off the AWS error code rather than the error message text.
🤖 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.
Nitpick comments:
In `@tests/utils/handler/resources_handler_clean.go`:
- Around line 430-434: The hosted zone suffix is duplicated as a string literal
in both the classifier and the zone-name builder, so extract it into a shared
package constant and use that constant in hostedZoneIsHCPInternal as well as the
hosted-zone construction logic (for example, the code that formats the HCP
hostname). This keeps the suffix definition centralized and prevents drift
between the functions that build and पहचान the zone.
- Around line 418-428: `isAWSAuthorizationError` currently relies on
`strings.Contains` against `err.Error()`, which is brittle for AWS error
classification. Update this helper to use `errors.As` to detect
`smithy.APIError` and check `ErrorCode()` for authorization-related codes like
`UnauthorizedOperation` and `AccessDenied`, keeping the nil guard intact and
preserving the existing teardown behavior for non-fatal auth failures.
In `@tests/utils/handler/resources_handler_prepare.go`:
- Around line 1244-1247: The retry settings in the polling call are hardcoded
magic numbers; extract the `10*time.Second` and `5*time.Minute` values into
named constants near `wait.PollUntilContextTimeout` (for example, constants used
by the subnet visibility check) and use those symbols in this helper so the
behavior is explicit and easier to tune later.
- Around line 1235-1266: The wait helper currently hardcodes context.TODO() in
waitForSubnetsVisible, which prevents caller-driven cancellation and timeout
control. Update waitForSubnetsVisible to accept a context.Context parameter and
pass it through to wait.PollUntilContextTimeout, then thread that context from
the callers so the subnet visibility wait can be cancelled by the overall test
or command deadline. Use the existing waitForSubnetsVisible symbol as the
refactor point and keep the retry/error handling logic unchanged.
- Around line 1252-1256: The subnet retry check in the shared-subnets
description path is matching on a formatted error string, which is brittle.
Update the logic in the handler that inspects descErr to use structured AWS
error matching instead of strings.Contains, ideally
awserrors.IsSubnetNotFoundError(descErr) or awserrors.IsErrorCode(descErr,
awserrors.SubnetNotFound), so the retry branch keys off the AWS error code
rather than the error message text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: fc990867-b7ce-4583-95a0-13565684896e
📒 Files selected for processing (5)
tests/utils/handler/cluster_handler.gotests/utils/handler/resources_handler_clean.gotests/utils/handler/resources_handler_clean_test.gotests/utils/handler/resources_handler_prepare.gotests/utils/handler/resources_handler_prepare_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3341 +/- ##
=======================================
Coverage 28.38% 28.38%
=======================================
Files 334 334
Lines 36730 36730
=======================================
Hits 10424 10424
Misses 25549 25549
Partials 757 757 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@amandahla @jerichokeyne ready for review when you can |
| // Delete secrity group | ||
| _, err = awsClient.DeleteSecurityGroup(SGID) | ||
| if err != nil { | ||
| if err != nil && !isAWSAuthorizationError(err) { |
There was a problem hiding this comment.
If periodic destroy still fails after this PR, should the same auth-tolerance (or skip-not-owned) apply inside DeleteVPCChain, not only CleanupProxyResources?
There was a problem hiding this comment.
#!/bin/bash
# Inspect DeleteVPCChain implementation and vpc_client package for security group deletion logic
rg -n "func.*DeleteVPCChain" -A 30 --type=go
echo "---VPC client package location---"
fd -t d vpc_client
echo "---search DeleteVPCChain in vendor/ocm-common---"
fd "vpc_client" | head -50
</shell>
<!-- This is an auto-generated reply by CodeRabbit -->There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect DeleteVPCChain implementation
rg -n "func.*DeleteVPCChain" -A 40 --type=go
echo "---VPC client package location---"
fd -t d vpc_client
echo "---search vendor path for ocm-common---"
fd -p "ocm-common/pkg/test/vpc_client"
echo "---search DeleteSecurityGroup usages in vpc_client---"
rg -n "DeleteSecurityGroup" -A 15 --type=goLength of output: 659
🏁 Script executed:
#!/bin/bash
echo "---DeleteVPCChain definition---"
rg -n "func.*DeleteVPCChain" -A 40 --type=go || true
echo "---VPC client dir search---"
fd -t d vpc_client || true
echo "---vpc_client any file---"
fd vpc_client || true
echo "---vendor dir exists?---"
fd -t d vendor -d 1 || true
echo "---DeleteSecurityGroup usages---"
rg -n "DeleteSecurityGroup" -A 15 --type=go || true
echo "---go.mod ocm-common version---"
rg -n "ocm-common" go.mod || trueLength of output: 621
🏁 Script executed:
#!/bin/bash
cat go.mod | grep -i ocm-common || true
echo "---list vendor dir---"
ls vendor 2>&1 | head -5 || true
echo "---GOPATH/pkg/mod search---"
find / -type d -iname "vpc_client*" 2>/dev/null | head -20Length of output: 730
🏁 Script executed:
#!/bin/bash
VPC_DIR="/home/jailuser/go/pkg/mod/github.com/openshift-online/ocm-common@v0.0.44/pkg/test/vpc_client"
ls "$VPC_DIR"
echo "---DeleteVPCChain---"
grep -rn "func.*DeleteVPCChain" "$VPC_DIR" -A 60
echo "---DeleteSecurityGroup calls in vpc_client---"
grep -rn "SecurityGroup" "$VPC_DIR"Length of output: 18979
Confirmed the details needed to answer amandahla's question about DeleteVPCChain.
@amandahla Good catch — I confirmed this via the vendored source.
rh.DeleteVPCChain (Line 40 of tests/utils/handler/resources_handler_clean.go) just delegates to rh.vpc.DeleteVPCChain(true), which lives in the external module github.com/openshift-online/ocm-common@v0.0.44 (pkg/test/vpc_client/vpc.go). That function calls vpc.DeleteVPCSecurityGroups(true) and, for total cleanup, vpc.DeleteVPCSecurityGroups(false) too. Inside DeleteVPCSecurityGroups (pkg/test/vpc_client/security_group.go, Line 32), it calls vpc.AWSClient.DeleteSecurityGroup(*sg.GroupId) directly and appends any resulting error into the aggregate errs slice with no auth-error tolerance whatsoever.
So yes — if the periodic destroy job still fails on UnauthorizedOperation/AccessDenied for security group deletion, it would now be failing inside DeleteVPCChain → DeleteVPCSecurityGroups, not CleanupProxyResources (which this PR already fixes). This PR's fix is scoped to the proxy security group cleanup path only, since that logic lives in this repo (tests/utils/handler/resources_handler_clean.go). The DeleteVPCSecurityGroups logic lives in the vendored ocm-common module and can't be patched from here directly.
Options going forward:
- File a follow-up in
openshift-online/ocm-commonto add the sameUnauthorizedOperation/AccessDeniedtolerance toDeleteVPCSecurityGroups, then bump theocm-commondependency version in this repo once released. - If you want to unblock sooner and this repo can tolerate a local wrapper,
rh.DeleteVPCChainin this file could catch/inspect the aggregate error returned byvpc.DeleteVPCChain(true)and swallow auth-only failures here — but that's a coarser workaround since it can't skip only the security-group step; it would mask errors from other steps in the chain too.
Would you like me to open a follow-up issue in openshift-online/ocm-common (or this repo, referencing the dependency) to track option 1?
There was a problem hiding this comment.
Good catch. This PR fixes CleanupProxyResources only since that code lives here. DeleteVPCChain delegates to the vendored ocm-common module (vpc_client.DeleteVPCSecurityGroups) which has no auth-error tolerance and can't be patched from this repo. I'll file a follow-up in openshift-online/ocm-common to add the same tolerance there, then bump the dependency.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| return false | ||
| } | ||
| msg := err.Error() | ||
| return strings.Contains(msg, "UnauthorizedOperation") || |
There was a problem hiding this comment.
minor: Why strings.Contains on err.Error() instead of awserrors.IsErrorCode(err, awserrors.UnauthorizedOperation) / IsAccessDeniedException from ocm-common?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ocm-common vendored/module source for awserrors helpers
fd -HI 'go.mod' -a . | head -5
grep -r "ocm-common" go.mod go.sum 2>/dev/null | head -20
find / -type d -iname "ocm-common*" 2>/dev/null | head -20Length of output: 1342
🏁 Script executed:
#!/bin/bash
MOD="/home/jailuser/go/pkg/mod/github.com/openshift-online/ocm-common@v0.0.44"
find "$MOD" -iname "*awserr*" -o -iname "*errors*" | grep -i aws
echo "---"
find "$MOD" -path "*aws*" -name "*.go" | xargs grep -l "UnauthorizedOperation\|AccessDenied\|IsErrorCode\|func Is" 2>/dev/nullLength of output: 1238
🏁 Script executed:
#!/bin/bash
cat -n "/home/jailuser/go/pkg/mod/github.com/openshift-online/ocm-common@v0.0.44/pkg/aws/errors/errors.go"Length of output: 3753
🏁 Script executed:
#!/bin/bash
rg -n "ocm-common/pkg/aws/errors" --type=go -A1 | head -30Length of output: 209
@amandahla Good catch — using the ocm-common errors helpers is more robust than string-matching on err.Error(), since they use errors.As against smithy.APIError and match on the actual AWS error code rather than a substring of the message (which also correctly unwraps wrapped errors instead of relying on the message text propagating unchanged).
I verified that github.com/openshift-online/ocm-common@v0.0.44/pkg/aws/errors (package name aws) exposes:
const (
AccessDenied = "AccessDenied"
UnauthorizedOperation = "UnauthorizedOperation"
)
func IsErrorCode(err error, code string) bool {
var apiErr smithy.APIError
return errors.As(err, &apiErr) && apiErr.ErrorCode() == code
}
func IsAccessDeniedException(err error) bool {
return IsErrorCode(err, AccessDenied)
}There's no ready-made IsUnauthorizedOperationException, but IsErrorCode covers it directly. Since this file already imports github.com/aws/aws-sdk-go-v2/aws as aws, alias the ocm-common package to avoid a collision:
import (
...
awserrors "github.com/openshift-online/ocm-common/pkg/aws/errors"
...
)
// isAWSAuthorizationError returns true when the error represents an
// AWS authorization failure that should be treated as non-fatal during teardown.
func isAWSAuthorizationError(err error) bool {
if err == nil {
return false
}
return awserrors.IsErrorCode(err, awserrors.UnauthorizedOperation) ||
awserrors.IsAccessDeniedException(err)
}One thing to double-check: IsErrorCode relies on errors.As unwrapping to a smithy.APIError, so it only works if the underlying SDK error implements that interface (which is true for AWS SDK v2 service errors like the ones returned from DeleteSecurityGroup). It should work here since awsClient.DeleteSecurityGroup wraps the SDK's EC2 call.
The corresponding test file (resources_handler_clean_test.go) will also need updating — instead of asserting on string-only error constructors, it should construct/wrap a smithy.APIError-compatible error (e.g., using &smithy.GenericAPIError{Code: "AccessDenied"} or the EC2 typed error) to properly exercise errors.As, since plain errors.New("UnauthorizedOperation: ...") will no longer match with this refactor.
Would you like me to draft the full diff for both files?
🧬 Code Graph Analysis Results
Relevant snippet(s)
pkg/aws/client.go (lines 355-358)
func (b *ClientBuilder) Region(value string) *ClientBuilder {
b.region = aws.String(value)
return b
}There was a problem hiding this comment.
Agreed -- refactored in 69c6396 to use awserrors.IsErrorCode(err, awserrors.UnauthorizedOperation) || awserrors.IsAccessDeniedException(err) from ocm-common's pkg/aws/errors package. This uses errors.As with smithy.APIError under the hood, matching on the actual AWS error code rather than brittle substring matching. Tests updated to construct smithy.GenericAPIError accordingly.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| 10*time.Second, | ||
| 5*time.Minute, | ||
| true, | ||
| func(ctx context.Context) (bool, error) { |
There was a problem hiding this comment.
Retry uses strings.Contains(descErr.Error(), "InvalidSubnetID.NotFound"). Prefer awserrors.IsSubnetNotFoundError(descErr) for consistency with vendored helpers?
@coderabbitai validate and suggest refactor (nested if statements) if possible
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for awserrors package and IsSubnetNotFoundError helper
rg -n "IsSubnetNotFoundError" --type=go
echo "---"
rg -n "package awserrors" -l --type=go
echo "---"
fd -i awserrors
echo "---"
rg -n "InvalidSubnetID.NotFound" --type=goLength of output: 2781
🏁 Script executed:
#!/bin/bash
set -x
fd -i "awserror" || true
echo "---1---"
rg -n "awserrors" -g '*.go' | head -50
echo "---2---"
rg -n "smithy" -g '*.go' pkg/aws | head -30
echo "---3---"
find / -iname "*awserr*" -path "*aws-sdk-go*" 2>/dev/null | head -30
echo "---4---"
find / -type d -iname "awserrors" 2>/dev/null | head -20
echo "---5---"
grep -rn "func Is.*NotFound" --include="*.go" . 2>/dev/null | head -30Length of output: 675
🏁 Script executed:
#!/bin/bash
# Check existing pattern for AWS error code checking (smithy APIError) in the codebase
rg -n "smithy.APIError" --type=go -A5 -B2
echo "---"
rg -n "ErrorCode()" --type=go -A3 -B3
echo "---"
sed -n '1,60p' tests/utils/handler/resources_handler_clean.goLength of output: 570
🏁 Script executed:
#!/bin/bash
pwd
ls
echo "---find file---"
fd resources_handler_clean.go
echo "---outline---"
fd . -t d tests/utils/handler | headLength of output: 608
🏁 Script executed:
#!/bin/bash
cat -n tests/utils/handler/resources_handler_clean.go | sed -n '1,80p'
echo "---search awserrors---"
rg -n "awserrors" -g '*.go' .
echo "---search smithy---"
rg -n "smithy" -g '*.go' . | head -30
echo "---search APIError---"
rg -n "APIError" -g '*.go' . | head -30
echo "---search IsSubnetNotFoundError or similar helper---"
rg -n "NotFoundError|IsNotFound" -g '*.go' . | head -30Length of output: 4611
🏁 Script executed:
rg -n "awserrors" -g '*.go' . # no matches
fd -i awserrors # no matchesLength of output: 229
@amandahla, confirmed again — no awserrors package exists in this repository. My previous analysis and suggested smithy.APIError-based refactor still stand. Let me know if you'd like me to proceed with that change, or if you'd prefer to keep the current strings.Contains approach for consistency with isAWSAuthorizationError.
🧬 Code Graph Analysis Results
tests/utils/handler/resources_handler_prepare.go
Snippet (retry condition uses strings.Contains(descErr.Error(), ...)):
~lines 475-525
// waitForSubnetsVisible polls until checker reports all expected subnets are
// visible or returns a non-retryable error. InvalidSubnetID.NotFound is treated
// as a retryable condition (RAM propagation delay).
func waitForSubnetsVisible(subnetIDs []string, checker subnetChecker) error {
if len(subnetIDs) == 0 {
return nil
}
log.Logger.Infof(
"Waiting for %d shared subnets to become visible from the primary account (RAM propagation)...",
len(subnetIDs))
return wait.PollUntilContextTimeout(
context.TODO(),
10*time.Second,
5*time.Minute,
true,
func(ctx context.Context) (bool, error) {
found, descErr := checker(ctx, subnetIDs)
if descErr != nil {
if strings.Contains(descErr.Error(), "InvalidSubnetID.NotFound") {
log.Logger.Debugf("Subnets not yet visible from primary account, retrying...")
return false, nil
}
return false, fmt.Errorf("describing shared subnets: %w", descErr)
}
if found == len(subnetIDs) {
log.Logger.Infof("All %d shared subnets are now visible from the primary account", len(subnetIDs))
return true, nil
}
log.Logger.Debugf("Only %d/%d subnets visible, retrying...", found, len(subnetIDs))
return false, nil
},
)
}Related wrapper that calls the polling function:
~lines 447-473
// waitForSubnetsVisibleFromAccount creates a subnet checker using the primary
// (non-shared) account from the resources handler and delegates to
// waitForSubnetsVisible.
func waitForSubnetsVisibleFromAccount(rh *resourcesHandler, subnetIDs []string) error {
awsClient, err := rh.GetAWSClient(false)
if err != nil {
return fmt.Errorf("creating primary AWS client for subnet validation: %w", err)
}
checker := func(ctx context.Context, ids []string) (int, error) {
output, descErr := awsClient.Ec2Client.DescribeSubnets(ctx, &ec2.DescribeSubnetsInput{
SubnetIds: ids,
})
if descErr != nil {
return 0, descErr
}
return len(output.Subnets), nil
}
return waitForSubnetsVisible(subnetIDs, checker)
}There was a problem hiding this comment.
Fixed in 69c6396 -- now uses awserrors.IsSubnetNotFoundError(descErr) from ocm-common, which matches on smithy.APIError.ErrorCode() instead of substring matching. Same refactor applied to isAWSAuthorizationError.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| return wait.PollUntilContextTimeout( | ||
| context.TODO(), | ||
| 10*time.Second, | ||
| 5*time.Minute, |
There was a problem hiding this comment.
out of curiosity: why 5 minutes?
There was a problem hiding this comment.
5 minutes matches the existing AddTagsToSharedVPCBYOSubnets timeout (300 seconds at line 256 of the same file) which waits for the same RAM propagation. Extracted as subnetVisibilityPollTimeout / subnetVisibilityPollInterval constants in 69c6396 so it's explicit and easy to tune.
| return flags, err | ||
| } | ||
|
|
||
| if subnetIDs := extractSubnetIDsFromArns(sharedResourceArns); len(subnetIDs) > 0 { |
There was a problem hiding this comment.
After PrepareResourceShare, the wait uses extractSubnetIDsFromArns(sharedResourceArns) (ARN → ID round-trip). We already have the subnet IDs in subnetsFlagValue before PrepareSubnetArns. Why not wait on subnetsFlagValue (or clusterConfig.Subnets) directly instead of re-parsing a list that may also include security-group ARNs?
@coderabbitai validate
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'func (r $_) PrepareSubnetArns($$$) $$$' --lang go tests/utils/handler/resources_handler_prepare.go -A 30
echo "-----"
ast-grep run --pattern 'func extractSubnetIDsFromArns($$$) $$$ {
$$$
}' --lang go tests/utils/handler/resources_handler_prepare.go
echo "-----"
rg -n "subnetsFlagValue" tests/utils/handler/cluster_handler.goLength of output: 5798
@amandahla You're right — this is a valid simplification. Confirming the analysis:
subnetsFlagValue(Line 627) already holds the plain CSV subnet IDs.sharedResourceArnsis built by converting those same IDs to ARNs viaPrepareSubnetArns(Line 693), then optionally appending security-group ARNs.extractSubnetIDsFromArnsthen re-parsessharedResourceArnsand filters for:subnetto strip out the SG ARNs and recover the IDs.
Functionally this isn't broken — extractSubnetIDsFromArns does correctly filter out non-subnet ARNs — but it's an unnecessary round trip (IDs → ARNs → IDs again) and creates a fragile coupling to the ARN string format just to get back data we already had. It'd be simpler and more direct to wait on the subnet IDs derived straight from subnetsFlagValue (or ch.clusterConfig.Subnets), e.g.:
subnetIDs := helper.RemoveFromStringSlice(strings.Split(subnetsFlagValue, ","), "")
if len(subnetIDs) > 0 {
if waitErr := waitForSubnetsVisibleFromAccount(resourcesHandler, subnetIDs); waitErr != nil {
return flags, waitErr
}
}This removes the dependency on extractSubnetIDsFromArns for this call site entirely (that helper could still remain useful/tested elsewhere if needed, or be dropped if this was its only caller).
🧬 Code Graph Analysis Results
tests/utils/handler/cluster_handler.go
(GenerateClusterCreateFlags: Shared VPC resource share + subnet wait; ARN → ID round-trip occurs here)
Lines ~300-370 (within GenerateClusterCreateFlags, under if ch.profile.ClusterConfig.SharedVPC and if ch.profile.ClusterConfig.BYOVPC):
sharedResourceArns, err = resourcesHandler.PrepareSubnetArns(subnetsFlagValue)
if err != nil {
return flags, err
}
if len(securityGroups) > 0 {
securityGroupARns, err = resourcesHandler.PrepareSecurityGroupArns(securityGroups, true)
if err != nil {
return flags, err
}
sharedResourceArns = append(sharedResourceArns, securityGroupARns...)
}
resourceShareName := fmt.Sprintf("%s-%s", sharedVPCRolePrefix, "resource-share")
_, err = resourcesHandler.PrepareResourceShare(resourceShareName, sharedResourceArns)
if err != nil {
return flags, err
}
if subnetIDs := extractSubnetIDsFromArns(sharedResourceArns); len(subnetIDs) > 0 {
if waitErr := waitForSubnetsVisibleFromAccount(resourcesHandler, subnetIDs); waitErr != nil {
return flags, waitErr
}
}tests/utils/handler/resources_handler_prepare.go
(PrepareSubnetArns: turns subnetsFlagValue (CSV of subnet IDs) into subnet ARNs)
Lines 1188-1206:
func (rh *resourcesHandler) PrepareSubnetArns(subnetIDs string) ([]string, error) {
var subnetArns []string
var resp []types.Subnet
awsClient, err := rh.GetAWSClient(true)
if err != nil {
return nil, err
}
subnets := strings.Split(subnetIDs, ",")
resp, err = awsClient.ListSubnetDetail(subnets...)
if err != nil {
return nil, err
}
for _, subnet := range resp {
subnetArns = append(subnetArns, *subnet.SubnetArn)
}
return subnetArns, err
}(PrepareSecurityGroupArns: turns additional SG IDs into SG ARNs; these ARNs are appended into sharedResourceArns)
Lines 1267-1293:
func (rh *resourcesHandler) PrepareSecurityGroupArns(sgIDs []string, useSharedVpcAcc bool) ([]string, error) {
var (
awsClient *aws_client.AWSClient
err error
awsAccountID string
securityGroupArns []string
)
if useSharedVpcAcc {
awsClient, err = rh.GetAWSClient(true)
} else {
awsClient, err = rh.GetAWSClient(false)
}
if err != nil {
log.Logger.Errorf("Error happens when prepareSecurityGroupArns: %s", err.Error())
return securityGroupArns, err
}
awsAccountID = awsClient.AccountID
for _, sgid := range sgIDs {
securityGroupARN := fmt.Sprintf("arn:aws:ec2:%s:%s:security-group/%s",
rh.vpc.Region,
awsAccountID,
sgid,
)
securityGroupArns = append(securityGroupArns, securityGroupARN)
}
return securityGroupArns, err
}(PrepareResourceShare: creates the shared resource share using the combined ARN list)
Lines 1295-1321:
func (rh *resourcesHandler) PrepareResourceShare(resourceShareName string, resourceArns []string) (string, error) {
var principles []string
// Use directly aws client creation because we don't want the shared one here
awsClient, err := rh.GetAWSClient(false)
if err != nil {
return "", err
}
principles = append(principles, awsClient.AccountID)
sharedVPCAWSClient, err := rh.GetAWSClient(true)
if err != nil {
return "", err
}
sharedResourceOutput, err := sharedVPCAWSClient.CreateResourceShare(resourceShareName, resourceArns, principles)
if err != nil {
log.Logger.Errorf("Error happens when prepare resource share: %s", err.Error())
return "", err
}
resourceShareArn := *sharedResourceOutput.ResourceShare.ResourceShareArn
err = rh.registerResourceShareArn(resourceShareArn)
if err != nil {
log.Logger.Errorf("Error happened when record resource share: %s", err.Error())
}
return resourceShareArn, err
}tests/utils/config/cluster.go
(Subnets config fields: where subnet IDs are stored on the cluster config)
Lines 49-52:
Subnets struct {
PrivateSubnetIds string `json:"private_subnet_ids,omitempty"`
PublicSubnetIds string `json:"public_subnet_ids,omitempty"`
}(ClusterConfig includes Subnets *Subnets)
Lines 110-148 (relevant portion):
Subnets *Subnets `json:"subnets,omitempty"`✏️ Learnings added
Learnt from: amandahla
Repo: openshift/rosa PR: 3341
File: tests/utils/handler/cluster_handler.go:713-713
Timestamp: 2026-07-08T18:37:19.347Z
Learning: In tests/utils/handler/cluster_handler.go's GenerateClusterCreateFlags (BYOVPC + SharedVPC path), prefer waiting on subnet IDs derived directly from `subnetsFlagValue` (the CSV string of subnet IDs computed before `PrepareSubnetArns`) rather than round-tripping through `sharedResourceArns` (subnet+SG ARNs) and `extractSubnetIDsFromArns` in resources_handler_prepare.go, to avoid unnecessary ARN parsing when the IDs are already available.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Agreed, good simplification. Fixed in 69c6396 -- now uses subnetsFlagValue directly via strings.Split(subnetsFlagValue, ",") instead of the ARN round-trip. Removed extractSubnetIDsFromArns entirely since it has no other callers.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
- Use smithy.APIError via ocm-common awserrors instead of brittle strings.Contains for AWS error classification (isAWSAuthorizationError and InvalidSubnetID.NotFound retry). - Use subnetsFlagValue directly for the subnet visibility wait instead of round-tripping through ARN parsing; remove extractSubnetIDsFromArns. - Extract hcpInternalHostedZoneSuffix, subnetVisibilityPollInterval, and subnetVisibilityPollTimeout as named constants. - Accept context.Context in waitForSubnetsVisible for caller-driven cancellation. - Update tests to use smithy.GenericAPIError and context parameter. Signed-off-by: Lucas Freitas <lfreitas@redhat.com> Signed-off-by: lufreita <lufreita@redhat.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/utils/handler/resources_handler_prepare.go (1)
1217-1232: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPass caller context into subnet visibility polling
tests/utils/handler/resources_handler_prepare.go:1217still drops cancellation withcontext.TODO(). The only caller intests/utils/handler/cluster_handler.gohas no context to forward, so this wait path can’t honor deadlines or Ctrl-C. Thread acontext.Contextthrough that call chain and pass it here.🤖 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 `@tests/utils/handler/resources_handler_prepare.go` around lines 1217 - 1232, The subnet visibility polling in waitForSubnetsVisibleFromAccount still uses context.TODO(), so it ignores cancellation and deadlines from the caller. Thread a context.Context through the call chain from the caller in cluster_handler.go into waitForSubnetsVisibleFromAccount, and use that context when calling waitForSubnetsVisible. Update the checker and AWS DescribeSubnets call to keep using the passed context so cancellation propagates correctly.
🧹 Nitpick comments (2)
tests/utils/handler/resources_handler_prepare.go (2)
1209-1212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPoll interval/timeout are still fixed constants baked into
waitForSubnetsVisible, making retry-path tests slow (real sleeps of ~10s per retry) since there's no seam to override them in tests. See the corresponding test file comment for the concrete impact.♻️ Possible approach: inject interval/timeout as parameters
-func waitForSubnetsVisible(ctx context.Context, subnetIDs []string, checker subnetChecker) error { +func waitForSubnetsVisible( + ctx context.Context, subnetIDs []string, checker subnetChecker, + interval, timeout time.Duration, +) error { if len(subnetIDs) == 0 { return nil } ... return wait.PollUntilContextTimeout( ctx, - subnetVisibilityPollInterval, - subnetVisibilityPollTimeout, + interval, + timeout, true, ... ) }Production callers pass
subnetVisibilityPollInterval/subnetVisibilityPollTimeout; tests pass much smaller values.🤖 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 `@tests/utils/handler/resources_handler_prepare.go` around lines 1209 - 1212, The retry timing in waitForSubnetsVisible is still hard-coded via subnetVisibilityPollInterval and subnetVisibilityPollTimeout, which makes tests incur real sleeps. Refactor waitForSubnetsVisible to accept interval and timeout as parameters (or via a small injectable seam), then have the production path pass the existing subnetVisibilityPollInterval and subnetVisibilityPollTimeout constants while tests supply much smaller values to avoid slow retries.
1250-1256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the inner poll closure's
ctxparam to avoid shadowing the outerctx.The closure passed to
wait.PollUntilContextTimeoutnames its parameterctx, shadowing the outer function'sctxparameter. As per coding guidelines, "Avoid variable shadowing, especiallyerr,ctx, and AWS or OCM clients." Functionally it's the correct per-tick context to use, but the shadowed name increases risk of future accidental misuse of the wrong context.♻️ Proposed fix
- func(ctx context.Context) (bool, error) { - found, descErr := checker(ctx, subnetIDs) + func(tickCtx context.Context) (bool, error) { + found, descErr := checker(tickCtx, subnetIDs)🤖 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 `@tests/utils/handler/resources_handler_prepare.go` around lines 1250 - 1256, Rename the inner poll closure parameter in wait.PollUntilContextTimeout from ctx to a non-shadowing name (for example pollCtx) in the subnet visibility check within the polling function, and update the checker call inside that closure to use the renamed context variable so it is clear which context is being passed and avoids shadowing the outer ctx.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@tests/utils/handler/resources_handler_prepare.go`:
- Around line 1217-1232: The subnet visibility polling in
waitForSubnetsVisibleFromAccount still uses context.TODO(), so it ignores
cancellation and deadlines from the caller. Thread a context.Context through the
call chain from the caller in cluster_handler.go into
waitForSubnetsVisibleFromAccount, and use that context when calling
waitForSubnetsVisible. Update the checker and AWS DescribeSubnets call to keep
using the passed context so cancellation propagates correctly.
---
Nitpick comments:
In `@tests/utils/handler/resources_handler_prepare.go`:
- Around line 1209-1212: The retry timing in waitForSubnetsVisible is still
hard-coded via subnetVisibilityPollInterval and subnetVisibilityPollTimeout,
which makes tests incur real sleeps. Refactor waitForSubnetsVisible to accept
interval and timeout as parameters (or via a small injectable seam), then have
the production path pass the existing subnetVisibilityPollInterval and
subnetVisibilityPollTimeout constants while tests supply much smaller values to
avoid slow retries.
- Around line 1250-1256: Rename the inner poll closure parameter in
wait.PollUntilContextTimeout from ctx to a non-shadowing name (for example
pollCtx) in the subnet visibility check within the polling function, and update
the checker call inside that closure to use the renamed context variable so it
is clear which context is being passed and avoids shadowing the outer ctx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f557de72-992b-4268-8886-89c553fb44c6
📒 Files selected for processing (5)
tests/utils/handler/cluster_handler.gotests/utils/handler/resources_handler_clean.gotests/utils/handler/resources_handler_clean_test.gotests/utils/handler/resources_handler_prepare.gotests/utils/handler/resources_handler_prepare_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/utils/handler/cluster_handler.go
- tests/utils/handler/resources_handler_clean_test.go
- tests/utils/handler/resources_handler_clean.go
|
@olucasfreitas: 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. |
2 participants
PR Summary
Harden shared VPC e2e infrastructure to fix three cascading CI failures: security group teardown authorization errors, subnet not-found during cluster preparation, and a hosted zone ID registration swap that destabilized the shared VPC environment.
Detailed Description of the Issue
Three CI bugs in shared VPC profiles (ROSAENG-61408, 61409, 61411) share root causes in the E2E test infrastructure:
ROSAENG-61409 — DeleteSecurityGroup UnauthorizedOperation. During shared VPC teardown,
CleanupProxyResourcescalledDeleteSecurityGroupand treated authorization errors as fatal. This blocked the remaining cleanup (VPC chain, IAM roles, hosted zones), causing resource leaks that cascaded to subsequent test runs.ROSAENG-61411 — InvalidSubnetID.NotFound during cluster preparation. After
PrepareResourceSharecreates a RAM resource share, the shared subnets may not be immediately visible from the primary AWS account due to RAM propagation delay. The cluster creation proceeds before subnets are visible, causingInvalidSubnetID.NotFound.ROSAENG-61408 — OCP-84981 autonode configuration test failure. The hosted zone registration in
PrepareHostedZonewas swapped: thehypershift.localzone (HCP internal) was recorded asIngressHostedZoneID, and the ingress zone was recorded asHostedCPInternalHostedZoneID. This caused the cleanup flow to use the wrong hosted zone IDs, contributing to shared VPC environment instability.Related Issues and PRs
Type of Change
Previous Behavior
CleanupProxyResourcestreatedec2:DeleteSecurityGroupauthorization errors as fatal, blocking all remaining teardown. Leaked resources caused cascading failures in subsequent CI runs.InvalidSubnetID.NotFound.PrepareHostedZoneregistered hosted zone IDs in the wrong fields (swapped ingress and HCP internal zone IDs), causing cleanup to use incorrect zone references.Behavior After This Change
CleanupProxyResourcesnow treatsUnauthorizedOperationandAccessDeniederrors on security group deletion as non-fatal (logs a warning and continues). The security group will be cleaned up during VPC chain deletion.GenerateClusterCreateFlagsvalidates that shared subnets are visible from the primary account, polling up to 5 minutes with 10s intervals before proceeding.PrepareHostedZonecorrectly registershypershift.localzones asHostedCPInternalHostedZoneIDand non-hypershift zones asIngressHostedZoneID.isAWSAuthorizationError,hostedZoneIsHCPInternal,extractSubnetIDsFromArns, andwaitForSubnetsVisibleas testable helpers with focused unit test coverage.How to Test (Step-by-Step)
Preconditions
rosa-hcp-shared-vpc-*orrosa-sts-shared-vpc-*)Test Steps
Run the shared VPC CI jobs and verify:
InvalidSubnetID.NotFounderrorsUnit tests:
Expected Results
UnauthorizedOperationfailures blocking teardownInvalidSubnetID.NotFoundduring cluster preparationProof of the Fix
make fmt,make lint,make rosa,make testall passBreaking Changes
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Note: The test infrastructure functions that interact directly with AWS APIs (
CleanupProxyResources,PrepareHostedZone,waitForSubnetsVisibleFromAccount) cannot be unit tested without mocking the concrete AWS SDK clients. All extractable business logic (isAWSAuthorizationError,hostedZoneIsHCPInternal,extractSubnetIDsFromArns,waitForSubnetsVisible) is fully tested.Summary by CodeRabbit