DPTP-4731: fix cluster profile secret race in aggregated runs#5083
DPTP-4731: fix cluster profile secret race in aggregated runs#5083deepsm007 wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@deepsm007: This pull request references DPTP-4731 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
WalkthroughCluster-profile secret handling changed to import secrets using the source secret name unchanged and to propagate that resolved name into wrapped steps via a new SetProfileSecretName(name string) API when supported. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Command failed Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/steps/lease.go`:
- Around line 270-272: The duck-typed SetProfileSecretName call on
multiStageTestStep.s.wrapped doesn't reach the inner LeaseStep when wrapper
types like ipPoolStep or clusterClaimStep sit between them; add a
SetProfileSecretName(string) method to ipPoolStep and clusterClaimStep that
simply forwards the call to their wrapped step (e.g., if setter, ok :=
s.wrapped.(interface{ SetProfileSecretName(string) }); ok {
setter.SetProfileSecretName(name) }) so the innermost step that actually
implements SetProfileSecretName receives the value and
multiStageTestStep.resolvedProfileSecretName is set (alternatively implement a
single helper to recursively unwrap and call the method, but prefer adding
forwarding methods on ipPoolStep and clusterClaimStep).
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1789de8-c232-4720-ab68-84729131a6f5
📒 Files selected for processing (4)
pkg/steps/lease.gopkg/steps/lease_test.gopkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.go
deepsm007
force-pushed
the
fix-cluster-profile-secret-race
branch
from
42dbc5f to
ef0c5e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/steps/multi_stage/multi_stage.go`:
- Around line 216-219: The fallback unconditionally returning s.name +
"-cluster-profile" can change the secret name used when SetProfileSecretName
isn't called; update the logic in the accessor (the code that currently checks
s.resolvedProfileSecretName) to preserve prior behavior by returning
s.profileSecretName if that field is non-empty, otherwise fall back to s.name +
"-cluster-profile"; keep the resolvedProfileSecretName check first, then
profileSecretName, then the name-based default so getProfileData() and
SetProfileSecretName remain compatible.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4885c122-d01f-4465-bd24-9c7b78362dc1
📒 Files selected for processing (5)
pkg/steps/ip_pool.gopkg/steps/lease.gopkg/steps/lease_test.gopkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/steps/lease_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/steps/multi_stage/multi_stage_test.go
| if s.resolvedProfileSecretName != "" { | ||
| return s.resolvedProfileSecretName | ||
| } | ||
| return name + "-cluster-profile" | ||
| return s.name + "-cluster-profile" |
There was a problem hiding this comment.
Fallback secret-name derivation may regress non-forwarded paths.
Line 219 now always uses s.name + "-cluster-profile". If SetProfileSecretName is not reached for any execution path, getProfileData() will query a different secret name than before and can fail with secret-not-found.
Suggested compatibility-safe fallback
func (s *multiStageTestStep) profileSecretName() string {
if s.resolvedProfileSecretName != "" {
return s.resolvedProfileSecretName
}
- return s.name + "-cluster-profile"
+ baseName := s.name
+ if s.additionalSuffix != "" {
+ baseName = strings.TrimSuffix(baseName, "-"+s.additionalSuffix)
+ }
+ return baseName + "-cluster-profile"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if s.resolvedProfileSecretName != "" { | |
| return s.resolvedProfileSecretName | |
| } | |
| return name + "-cluster-profile" | |
| return s.name + "-cluster-profile" | |
| if s.resolvedProfileSecretName != "" { | |
| return s.resolvedProfileSecretName | |
| } | |
| baseName := s.name | |
| if s.additionalSuffix != "" { | |
| baseName = strings.TrimSuffix(baseName, "-"+s.additionalSuffix) | |
| } | |
| return baseName + "-cluster-profile" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/steps/multi_stage/multi_stage.go` around lines 216 - 219, The fallback
unconditionally returning s.name + "-cluster-profile" can change the secret name
used when SetProfileSecretName isn't called; update the logic in the accessor
(the code that currently checks s.resolvedProfileSecretName) to preserve prior
behavior by returning s.profileSecretName if that field is non-empty, otherwise
fall back to s.name + "-cluster-profile"; keep the resolvedProfileSecretName
check first, then profileSecretName, then the name-based default so
getProfileData() and SetProfileSecretName remain compatible.
|
@deepsm007: 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. |
|
/test images e2e |
2 participants
Aggregated jobs run multiple ci-operator instances in the same namespace. Each tries to create a secret named -cluster-profile to hold cloud credentials. When runs get different cloud accounts, they race on that shared name causing failures like
secrets "e2e-azure-ovn-cluster-profile" not found.The fix uses the source secret name (e.g. cluster-secrets-azure4) as the target in the test namespace instead. Runs resolving to the same account reuse the same secret safely via UpsertImmutableSecret's DeepEqual check. Runs with different accounts get different names and never conflict. leaseStep notifies multiStageTestStep of the resolved name so pods mount the right secret.
https://prow.ci.openshift.org/view/gs/test-platform-results/logs/aggregator-periodic-ci-openshift-release-main-ci-4.22-e2e-azure-ovn/2039355967551836160
/cc @droslean @openshift/test-platform