CNTRLPLANE-3167: support STS/IRSA credentials and standalone Velero#247
Draft
jparrill wants to merge 5 commits into
Draft
CNTRLPLANE-3167: support STS/IRSA credentials and standalone Velero#247jparrill wants to merge 5 commits into
jparrill wants to merge 5 commits into
Conversation
…or etcd backup/restore Add support for AWS STS/IRSA credential detection and pre-signing, and handle standalone Velero deployments (no DPA) where the BSL has no explicit credential reference. - Add ParseAWSCredentialData to auto-detect credential type (static, bare ARN, INI with role_arn) and STSClient for AssumeRoleWithWebIdentity without AWS SDK dependency - Detect OADP+DPA vs standalone Velero at plugin init via DPA CRD check - Fall back to well-known cloud-credentials secret when BSL has no .spec.credential (ARO with WI, future ROSA without DPA) - Preserve original secret key alongside "credentials" in copied secret so the HCPEtcdBackup controller can auto-detect credential type - Refactor copyCredentialSecret to accept resolved SecretKeySelector Ref: openshift/hypershift#8368 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
@jparrill: This pull request references CNTRLPLANE-3167 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 story to target the "5.0.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. |
WalkthroughAdds default credential constants and a credential resolver; detects DPA CRD presence in plugins; implements AWS credential-type parsing and STS web-identity client; adds Azure Blob SAS and User Delegation SAS generation with AAD token provider; integrates presigning into restore/etcd backup flows and expands tests and go.mod deps. ChangesCommon credential defaults & helper
Plugin wiring & restore entrypoints
S3 presign: credential parsing + STS
etcd backup orchestrator & credential copying
Azure Blob SAS, user delegation, and AAD token provider
Restore & integration tests
Module dependency updates
Sequence Diagram(s)sequenceDiagram
participant RestorePlugin
participant BSL as BackupStorageLocation
participant Secret as Credential Secret
participant Presigner
participant STSClient
participant STSEndpoint
participant AzBlob as Azure Blob Service
RestorePlugin->>Presigner: presign(snapshotURL, resourceName)
Presigner->>BSL: check spec.credential
alt explicit credential ref
Presigner->>Secret: get(secretName, key)
Secret-->>Presigner: credential bytes
else fallback to default secret
Presigner->>Secret: get(DefaultCredentialSecretName, DefaultCredentialSecretKey)
Secret-->>Presigner: credential bytes
end
alt S3 + STS detected
Presigner->>STSClient: AssumeRoleWithWebIdentity(roleARN, tokenFile, sessionName)
STSClient->>STSEndpoint: POST web-identity request
STSEndpoint-->>STSClient: 200 + STS XML
STSClient-->>Presigner: temp AWS creds
Presigner->>Presigner: generate presigned S3 URL with temp creds
Presigner-->>RestorePlugin: presigned S3 URL
else S3 + static creds
Presigner->>Presigner: generate presigned S3 URL with static creds
Presigner-->>RestorePlugin: presigned S3 URL
else Azure Blob + AAD (user delegation)
Presigner->>AzBlob: GetUserDelegationKey(account, bearerToken)
AzBlob-->>Presigner: UserDelegationKey XML
Presigner->>Presigner: GenerateUserDelegationSASBlobURL(opts)
Presigner-->>RestorePlugin: SAS URL
else Azure Blob + account-key
Presigner->>Presigner: GenerateSASBlobURL(accountKey)
Presigner-->>RestorePlugin: SAS URL
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill 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
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/core/restore.go`:
- Around line 325-330: The STS session name built as sessionName :=
fmt.Sprintf("oadp-restore-%s", hcName) can exceed AWS's 64-char RoleSessionName
limit; update the code that sets sessionName (used in the call to
s3presign.NewSTSClient().AssumeRoleWithWebIdentity) to produce a string capped
at 64 characters while preserving the "oadp-restore-" prefix (e.g., build a
helper like buildSTSSessionName(prefix, name) that concatenates prefix+name and
truncates to 64 bytes/characters before passing it into
AssumeRoleWithWebIdentity). Ensure the truncated sessionName is used in the
existing AssumeRoleWithWebIdentity call so AWS validation errors are avoided.
In `@pkg/s3presign/presign_test.go`:
- Around line 549-553: Replace the os.Unsetenv call with t.Setenv to avoid
unchecked errors: in the test block that currently checks tt.envTokenFile and
sets AWS_WEB_IDENTITY_TOKEN_FILE, always call
t.Setenv("AWS_WEB_IDENTITY_TOKEN_FILE", tt.envTokenFile) (use empty string when
tt.envTokenFile == "") instead of calling os.Unsetenv; update the code around
the tt.envTokenFile branch in presign_test.go (the block referencing
tt.envTokenFile and AWS_WEB_IDENTITY_TOKEN_FILE) so the test harness t.Setenv is
used for both branches.
In `@pkg/s3presign/sts_test.go`:
- Around line 37-45: Tests in pkg/s3presign/sts_test.go are ignoring errors
returned by w.Write in several HTTP test handlers (e.g., the handler that writes
the AssumeRoleWithWebIdentityResponse and similar handlers around the other
occurrences), which trips errcheck; modify each handler to capture the error
returned by w.Write and handle it (fail the test) instead of discarding it —
e.g., assign the result of w.Write(...) to a variable and call t.Fatalf/t.Errorf
or use require.NoError(t, err) when err != nil so the test stops on write/parse
failures; apply this change to every handler that currently calls w.Write
without checking the returned error.
In `@pkg/s3presign/sts.go`:
- Around line 111-119: The STS response handling in the function that builds
AWSCredentials (using resp.Result.Credentials from AssumeRoleWithWebIdentity)
currently only checks AccessKeyID and SecretAccessKey; update that check to also
require a non-empty SessionToken and return an error if it's missing so we never
return AWSCredentials without SessionToken (which would cause presigned S3 URLs
to omit X-Amz-Security-Token and fail). Locate the creds :=
resp.Result.Credentials block and change the conditional to validate
creds.SessionToken as well, returning a descriptive fmt.Errorf when the token is
empty before constructing the AWSCredentials.
- Around line 37-75: Modify AssumeRoleWithWebIdentity to accept a
context.Context as its first parameter and use it for the HTTP call: change the
signature of AssumeRoleWithWebIdentity(roleARN, tokenFile, sessionName string)
to AssumeRoleWithWebIdentity(ctx context.Context, roleARN, tokenFile,
sessionName string), build the url.Values as before but replace
c.HTTPClient.PostForm with creating an http.NewRequestWithContext(ctx, "POST",
endpoint, strings.NewReader(params.Encode())), set the Content-Type header to
"application/x-www-form-urlencoded" and call c.HTTPClient.Do(req); keep the
existing error wrapping and resp.Body.Close() handling so
cancellations/deadlines propagate correctly.
- Around line 26-31: NewSTSClient currently hardcodes Endpoint to
defaultSTSEndpoint which breaks non-commercial ARNs (arn:aws-us-gov,
arn:aws-cn); change NewSTSClient/STSClient so the STS endpoint is derived from
the ARN partition rather than always using defaultSTSEndpoint: detect partition
(aws, aws-cn, aws-us-gov) from the parsed ARN and map to the appropriate STS
endpoint (e.g., aws -> https://sts.amazonaws.com or regional variant, aws-cn ->
https://sts.cn-north-1.amazonaws.com.cn, aws-us-gov ->
https://sts.us-gov-west-1.amazonaws.com), and set STSClient.Endpoint accordingly
(or accept the partition/region as an argument) instead of using
defaultSTSEndpoint.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0f862226-a971-46ed-88fc-b01b1fd13e40
📒 Files selected for processing (10)
pkg/common/types.gopkg/core/backup.gopkg/core/restore.gopkg/core/restore_test.gopkg/etcdbackup/orchestrator.gopkg/etcdbackup/orchestrator_test.gopkg/s3presign/presign.gopkg/s3presign/presign_test.gopkg/s3presign/sts.gopkg/s3presign/sts_test.go
Comment on lines
+325
to
+330
| stsClient := s3presign.NewSTSClient() | ||
| sessionName := fmt.Sprintf("oadp-restore-%s", hcName) | ||
| creds, err = stsClient.AssumeRoleWithWebIdentity( | ||
| parsed.STSRole.RoleARN, | ||
| parsed.STSRole.WebIdentityTokenFile, | ||
| sessionName, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pkg/core/restore.go | sed -n '320,335p'Repository: openshift/hypershift-oadp-plugin
Length of output: 693
🌐 Web query:
AWS STS RoleSessionName maximum length constraint
💡 Result:
The maximum length of the RoleSessionName parameter in AWS STS AssumeRole (and related operations like AssumeRoleWithSAML, AssumeRoleWithWebIdentity) is 64 characters. It has a minimum length of 2 characters and must match the pattern [\w+=,.@-]* (upper/lowercase alphanumeric, plus _, =, +, ,, ., @, -; no spaces).
Citations:
- 1: https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRole.html
- 2: https://docs.aws.amazon.com/cli/latest/reference/sts/assume-role.html
- 3: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_iam-quotas.html
- 4: https://docs.aws.amazon.com/STS/latest/APIReference/API%5FAssumeRoleWithWebIdentity.html
🏁 Script executed:
# Search for hcName definition and how it's derived
rg "hcName\s*:=" pkg/core/restore.go -B 5 -A 2Repository: openshift/hypershift-oadp-plugin
Length of output: 409
🏁 Script executed:
# Check if there are any existing length validations in the STS or session code
rg "sessionName|RoleSessionName" --type go -A 3 -B 1Repository: openshift/hypershift-oadp-plugin
Length of output: 2218
🏁 Script executed:
# Look for test files that might show typical hcName values
fd "test\.go|_test\.go" pkg/core/ -x cat {} \; | head -100Repository: openshift/hypershift-oadp-plugin
Length of output: 3055
Bound the generated STS session name to 64 characters.
Line 326 builds oadp-restore-<hcName> without truncation. AWS STS RoleSessionName has a 64-character maximum; the prefix consumes 13 characters, leaving only 51 characters for hcName. Since HostedCluster names are Kubernetes DNS subdomains (up to 253 characters), typical cluster names easily exceed this budget, causing AssumeRoleWithWebIdentity to fail with a validation error from AWS.
Suggested direction
- sessionName := fmt.Sprintf("oadp-restore-%s", hcName)
+ sessionName := buildSTSSessionName("oadp-restore-", hcName)func buildSTSSessionName(prefix, name string) string {
session := prefix + name
if len(session) > 64 {
session = session[:64]
}
return session
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/core/restore.go` around lines 325 - 330, The STS session name built as
sessionName := fmt.Sprintf("oadp-restore-%s", hcName) can exceed AWS's 64-char
RoleSessionName limit; update the code that sets sessionName (used in the call
to s3presign.NewSTSClient().AssumeRoleWithWebIdentity) to produce a string
capped at 64 characters while preserving the "oadp-restore-" prefix (e.g., build
a helper like buildSTSSessionName(prefix, name) that concatenates prefix+name
and truncates to 64 bytes/characters before passing it into
AssumeRoleWithWebIdentity). Ensure the truncated sessionName is used in the
existing AssumeRoleWithWebIdentity call so AWS validation errors are avoided.
Comment on lines
+549
to
+553
| if tt.envTokenFile != "" { | ||
| t.Setenv("AWS_WEB_IDENTITY_TOKEN_FILE", tt.envTokenFile) | ||
| } else { | ||
| os.Unsetenv("AWS_WEB_IDENTITY_TOKEN_FILE") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the file at the specified lines
cat -n pkg/s3presign/presign_test.go | sed -n '545,560p'Repository: openshift/hypershift-oadp-plugin
Length of output: 558
🏁 Script executed:
# Check if there's a parser that handles the AWS_WEB_IDENTITY_TOKEN_FILE env var
rg -A 10 -B 5 "AWS_WEB_IDENTITY_TOKEN_FILE" pkg/s3presign/ --type goRepository: openshift/hypershift-oadp-plugin
Length of output: 5223
🏁 Script executed:
# Look for the test function containing these lines to understand context
rg -B 30 "os.Unsetenv.*AWS_WEB_IDENTITY_TOKEN_FILE" pkg/s3presign/presign_test.go --type goRepository: openshift/hypershift-oadp-plugin
Length of output: 1007
Use t.Setenv for both branches to avoid the errcheck error.
Line 552's os.Unsetenv error is not being handled, so errcheck will flag this. Since the parser treats an empty AWS_WEB_IDENTITY_TOKEN_FILE value the same as an unset variable (both return empty string from os.Getenv), you can simplify this to use t.Setenv in both branches.
Suggested change
- if tt.envTokenFile != "" {
- t.Setenv("AWS_WEB_IDENTITY_TOKEN_FILE", tt.envTokenFile)
- } else {
- os.Unsetenv("AWS_WEB_IDENTITY_TOKEN_FILE")
- }
+ t.Setenv("AWS_WEB_IDENTITY_TOKEN_FILE", tt.envTokenFile)📝 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.
Suggested change
| if tt.envTokenFile != "" { | |
| t.Setenv("AWS_WEB_IDENTITY_TOKEN_FILE", tt.envTokenFile) | |
| } else { | |
| os.Unsetenv("AWS_WEB_IDENTITY_TOKEN_FILE") | |
| } | |
| t.Setenv("AWS_WEB_IDENTITY_TOKEN_FILE", tt.envTokenFile) |
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 552-552: Error return value of os.Unsetenv is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/s3presign/presign_test.go` around lines 549 - 553, Replace the
os.Unsetenv call with t.Setenv to avoid unchecked errors: in the test block that
currently checks tt.envTokenFile and sets AWS_WEB_IDENTITY_TOKEN_FILE, always
call t.Setenv("AWS_WEB_IDENTITY_TOKEN_FILE", tt.envTokenFile) (use empty string
when tt.envTokenFile == "") instead of calling os.Unsetenv; update the code
around the tt.envTokenFile branch in presign_test.go (the block referencing
tt.envTokenFile and AWS_WEB_IDENTITY_TOKEN_FILE) so the test harness t.Setenv is
used for both branches.
Comment on lines
+37
to
+45
| w.Write([]byte(`<AssumeRoleWithWebIdentityResponse> | ||
| <AssumeRoleWithWebIdentityResult> | ||
| <Credentials> | ||
| <AccessKeyId>ASIATESTACCESSKEY</AccessKeyId> | ||
| <SecretAccessKey>testSecretKey123</SecretAccessKey> | ||
| <SessionToken>testSessionToken456</SessionToken> | ||
| </Credentials> | ||
| </AssumeRoleWithWebIdentityResult> | ||
| </AssumeRoleWithWebIdentityResponse>`)) |
There was a problem hiding this comment.
Handle write/parse errors in test handlers to avoid errcheck failures.
Line 37, Line 79, Line 106, and Line 204 currently ignore returned errors (same pattern also appears at Line 130 and Line 207). With errcheck enabled, this can break CI.
✅ Suggested patch
@@
- w.Write([]byte(`<AssumeRoleWithWebIdentityResponse>
+ if _, err := w.Write([]byte(`<AssumeRoleWithWebIdentityResponse>
<AssumeRoleWithWebIdentityResult>
<Credentials>
<AccessKeyId>ASIATESTACCESSKEY</AccessKeyId>
<SecretAccessKey>testSecretKey123</SecretAccessKey>
<SessionToken>testSessionToken456</SessionToken>
</Credentials>
</AssumeRoleWithWebIdentityResult>
-</AssumeRoleWithWebIdentityResponse>`))
+</AssumeRoleWithWebIdentityResponse>`)); err != nil {
+ t.Fatalf("failed to write response: %v", err)
+ }
@@
- w.Write([]byte(`<ErrorResponse><Error><Code>AccessDenied</Code><Message>Not authorized</Message></Error></ErrorResponse>`))
+ if _, err := w.Write([]byte(`<ErrorResponse><Error><Code>AccessDenied</Code><Message>Not authorized</Message></Error></ErrorResponse>`)); err != nil {
+ t.Fatalf("failed to write response: %v", err)
+ }
@@
- w.Write([]byte(`not xml at all`))
+ if _, err := w.Write([]byte(`not xml at all`)); err != nil {
+ t.Fatalf("failed to write response: %v", err)
+ }
@@
- w.Write([]byte(`<AssumeRoleWithWebIdentityResponse>
+ if _, err := w.Write([]byte(`<AssumeRoleWithWebIdentityResponse>
<AssumeRoleWithWebIdentityResult>
<Credentials>
<AccessKeyId></AccessKeyId>
<SecretAccessKey></SecretAccessKey>
<SessionToken></SessionToken>
</Credentials>
</AssumeRoleWithWebIdentityResult>
-</AssumeRoleWithWebIdentityResponse>`))
+</AssumeRoleWithWebIdentityResponse>`)); err != nil {
+ t.Fatalf("failed to write response: %v", err)
+ }
@@
- r.ParseForm()
+ if err := r.ParseForm(); err != nil {
+ t.Fatalf("failed to parse form: %v", err)
+ }
@@
- w.Write([]byte(`<AssumeRoleWithWebIdentityResponse>
+ if _, err := w.Write([]byte(`<AssumeRoleWithWebIdentityResponse>
<AssumeRoleWithWebIdentityResult>
<Credentials>
<AccessKeyId>ASIA123</AccessKeyId>
<SecretAccessKey>secret</SecretAccessKey>
<SessionToken>token</SessionToken>
</Credentials>
</AssumeRoleWithWebIdentityResult>
-</AssumeRoleWithWebIdentityResponse>`))
+</AssumeRoleWithWebIdentityResponse>`)); err != nil {
+ t.Fatalf("failed to write response: %v", err)
+ }Also applies to: 79-79, 106-106, 130-138, 204-215
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 37-37: Error return value of w.Write is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/s3presign/sts_test.go` around lines 37 - 45, Tests in
pkg/s3presign/sts_test.go are ignoring errors returned by w.Write in several
HTTP test handlers (e.g., the handler that writes the
AssumeRoleWithWebIdentityResponse and similar handlers around the other
occurrences), which trips errcheck; modify each handler to capture the error
returned by w.Write and handle it (fail the test) instead of discarding it —
e.g., assign the result of w.Write(...) to a variable and call t.Fatalf/t.Errorf
or use require.NoError(t, err) when err != nil so the test stops on write/parse
failures; apply this change to every handler that currently calls w.Write
without checking the returned error.
Comment on lines
+26
to
+31
| // NewSTSClient creates a default STSClient pointing at the global STS endpoint. | ||
| func NewSTSClient() *STSClient { | ||
| return &STSClient{ | ||
| HTTPClient: &http.Client{Timeout: 30 * time.Second}, | ||
| Endpoint: defaultSTSEndpoint, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -name "sts.go" -o -name "*presign*test*" | head -20Repository: openshift/hypershift-oadp-plugin
Length of output: 132
🏁 Script executed:
cd /repo && git ls-files | grep -E "(sts\.go|presign.*test)" | head -20Repository: openshift/hypershift-oadp-plugin
Length of output: 132
🏁 Script executed:
cd /repo && rg "arn:aws-us-gov|arn:aws-cn" --max-count 20Repository: openshift/hypershift-oadp-plugin
Length of output: 132
🏁 Script executed:
git ls-files | grep -E "(sts\.go|presign)" | head -30Repository: openshift/hypershift-oadp-plugin
Length of output: 234
🏁 Script executed:
find . -type f -name "*.go" | xargs grep -l "sts\|STS" | head -20Repository: openshift/hypershift-oadp-plugin
Length of output: 791
🏁 Script executed:
rg "arn:aws-us-gov|arn:aws-cn" -A 2 -B 2Repository: openshift/hypershift-oadp-plugin
Length of output: 1579
🏁 Script executed:
cat -n pkg/s3presign/sts.goRepository: openshift/hypershift-oadp-plugin
Length of output: 4152
🏁 Script executed:
cat -n pkg/s3presign/presign.go | head -100Repository: openshift/hypershift-oadp-plugin
Length of output: 3406
🏁 Script executed:
cat -n pkg/s3presign/presign.go | grep -A 50 "ParseAWSCredentials\|STSRole"Repository: openshift/hypershift-oadp-plugin
Length of output: 6349
🏁 Script executed:
rg "NewSTSClient\|STSClient\{" -A 3 -B 1Repository: openshift/hypershift-oadp-plugin
Length of output: 58
🏁 Script executed:
cat -n pkg/s3presign/presign_test.go | head -150Repository: openshift/hypershift-oadp-plugin
Length of output: 4471
🏁 Script executed:
cat -n pkg/s3presign/presign_test.go | grep -A 40 "GovCloud ARN\|China region ARN"Repository: openshift/hypershift-oadp-plugin
Length of output: 4522
🏁 Script executed:
rg "NewSTSClient\|NewSTSClientWithEndpoint\|\.Endpoint\s*=" pkg/s3presign/Repository: openshift/hypershift-oadp-plugin
Length of output: 58
🏁 Script executed:
cat -n pkg/s3presign/sts_test.goRepository: openshift/hypershift-oadp-plugin
Length of output: 9610
🏁 Script executed:
rg "NewSTSClient\(\)" -A 5 -B 2Repository: openshift/hypershift-oadp-plugin
Length of output: 3159
🏁 Script executed:
rg "AssumeRoleWithWebIdentity" -A 3 -B 2 pkg/Repository: openshift/hypershift-oadp-plugin
Length of output: 7914
🏁 Script executed:
cat -n pkg/s3presign/presign.go | grep -A 30 "func GetPresignedURL\|STSClient"Repository: openshift/hypershift-oadp-plugin
Length of output: 58
🏁 Script executed:
rg "arn:aws" pkg/s3presign/presign.go -B 3 -A 3 | head -50Repository: openshift/hypershift-oadp-plugin
Length of output: 730
🏁 Script executed:
rg "partition\|Partition\|aws-us-gov\|aws-cn" pkg/s3presign/ -iRepository: openshift/hypershift-oadp-plugin
Length of output: 58
The STS endpoint must be derived from the ARN partition, not hardcoded.
NewSTSClient() always targets https://sts.amazonaws.com, but the parser also accepts arn:aws-us-gov and arn:aws-cn ARNs (verified in presign_test.go). When non-commercial ARNs are provided, they parse successfully but the STS call will fail because the endpoint is wrong. For example, GovCloud requires https://sts.us-gov-west-1.amazonaws.com and China requires https://sts.cn-north-1.amazonaws.com.cn.
Extract the partition from the ARN and route to the correct regional STS endpoint accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/s3presign/sts.go` around lines 26 - 31, NewSTSClient currently hardcodes
Endpoint to defaultSTSEndpoint which breaks non-commercial ARNs (arn:aws-us-gov,
arn:aws-cn); change NewSTSClient/STSClient so the STS endpoint is derived from
the ARN partition rather than always using defaultSTSEndpoint: detect partition
(aws, aws-cn, aws-us-gov) from the parsed ARN and map to the appropriate STS
endpoint (e.g., aws -> https://sts.amazonaws.com or regional variant, aws-cn ->
https://sts.cn-north-1.amazonaws.com.cn, aws-us-gov ->
https://sts.us-gov-west-1.amazonaws.com), and set STSClient.Endpoint accordingly
(or accept the partition/region as an argument) instead of using
defaultSTSEndpoint.
Comment on lines
+111
to
+119
| creds := resp.Result.Credentials | ||
| if creds.AccessKeyID == "" || creds.SecretAccessKey == "" { | ||
| return nil, fmt.Errorf("STS response missing credentials") | ||
| } | ||
|
|
||
| return &AWSCredentials{ | ||
| AccessKeyID: creds.AccessKeyID, | ||
| SecretAccessKey: creds.SecretAccessKey, | ||
| SessionToken: creds.SessionToken, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pkg/s3presign/sts.go | sed -n '100,125p'Repository: openshift/hypershift-oadp-plugin
Length of output: 892
🏁 Script executed:
# Get broader context around the function
cat -n pkg/s3presign/sts.go | head -150Repository: openshift/hypershift-oadp-plugin
Length of output: 4152
🏁 Script executed:
# Search for where SessionToken is used in the codebase
rg "SessionToken" --type goRepository: openshift/hypershift-oadp-plugin
Length of output: 1998
🏁 Script executed:
# Find presigned URL generation code
rg "presign|Presign" --type go -A 3 -B 3Repository: openshift/hypershift-oadp-plugin
Length of output: 33840
Reject STS responses that omit the session token.
The credentials returned by AssumeRoleWithWebIdentity are temporary. If SessionToken is empty here, later presigned URLs will be generated without X-Amz-Security-Token and fail against S3 even though this function reported success.
Suggested change
- if creds.AccessKeyID == "" || creds.SecretAccessKey == "" {
+ if creds.AccessKeyID == "" || creds.SecretAccessKey == "" || creds.SessionToken == "" {
return nil, fmt.Errorf("STS response missing credentials")
}📝 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.
Suggested change
| creds := resp.Result.Credentials | |
| if creds.AccessKeyID == "" || creds.SecretAccessKey == "" { | |
| return nil, fmt.Errorf("STS response missing credentials") | |
| } | |
| return &AWSCredentials{ | |
| AccessKeyID: creds.AccessKeyID, | |
| SecretAccessKey: creds.SecretAccessKey, | |
| SessionToken: creds.SessionToken, | |
| creds := resp.Result.Credentials | |
| if creds.AccessKeyID == "" || creds.SecretAccessKey == "" || creds.SessionToken == "" { | |
| return nil, fmt.Errorf("STS response missing credentials") | |
| } | |
| return &AWSCredentials{ | |
| AccessKeyID: creds.AccessKeyID, | |
| SecretAccessKey: creds.SecretAccessKey, | |
| SessionToken: creds.SessionToken, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/s3presign/sts.go` around lines 111 - 119, The STS response handling in
the function that builds AWSCredentials (using resp.Result.Credentials from
AssumeRoleWithWebIdentity) currently only checks AccessKeyID and
SecretAccessKey; update that check to also require a non-empty SessionToken and
return an error if it's missing so we never return AWSCredentials without
SessionToken (which would cause presigned S3 URLs to omit X-Amz-Security-Token
and fail). Locate the creds := resp.Result.Credentials block and change the
conditional to validate creds.SessionToken as well, returning a descriptive
fmt.Errorf when the token is empty before constructing the AWSCredentials.
The HyperShift hcpetcdbackup controller sets the CR name as a label value on the Job and Pod template. Kubernetes label values are capped at 63 bytes (RFC 1123). Schedule-triggered backups produce names that exceed this limit, causing Job creation to fail. Truncate the backup name portion of the CR name so the total never exceeds 63 bytes, and trim any trailing hyphens left by truncation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tore Adds Service SAS (account key) and User Delegation SAS (AAD) signing for Azure Blob Storage snapshot URLs during OADP restore, matching the existing S3 pre-signing capability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When using the etcdSnapshot backup method, the etcd StatefulSet was being restored by Velero with a stale RESTORE_URL_ETCD environment variable baked into its pod template from a previous backup/restore cycle. The StatefulSet controller would create etcd pods with the wrong snapshot URL before the HyperShift operator could reconcile the correct URL from the HCP spec, causing etcd to restore data from the wrong snapshot. Skip restoring the etcd StatefulSet when etcdBackupMethod is etcdSnapshot, matching the existing pattern for etcd Pods and PVCs. The control plane operator creates the StatefulSet fresh with the correct RESTORE_URL_ETCD derived from the RestoreSnapshotURL already injected into the HCP spec by this plugin. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/azblobsas/delegation.go (1)
80-80: ⚡ Quick winUse lowercase for error messages.
Error strings should not be capitalized per Go conventions (ST1005).
♻️ Proposed fix
- return nil, fmt.Errorf("Get User Delegation Key returned %d: %s", resp.StatusCode, string(respBody)) + return nil, fmt.Errorf("get user delegation key returned %d: %s", resp.StatusCode, string(respBody))🤖 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 `@pkg/azblobsas/delegation.go` at line 80, The error string is capitalized; change it to follow Go convention of lowercase error messages by updating the fmt.Errorf call in the GetUserDelegationKey code path to start with a lowercase letter (e.g., "get user delegation key returned %d: %s") while keeping the same formatting placeholders and returning the error as before.go.mod (1)
8-9: ⚡ Quick winConsider updating azcore to v1.21.1.
Both versions exist and are valid releases. azidentity v1.13.1 is the current latest version, but azcore v1.20.0 has a newer version available (v1.21.1 as of May 2026). If not pinned for stability reasons, consider updating azcore to the latest patch version for potential bug fixes and improvements.
🤖 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 `@go.mod` around lines 8 - 9, Update the azcore dependency to the latest patch release by changing the version for github.com/Azure/azure-sdk-for-go/sdk/azcore from v1.20.0 to v1.21.1 in go.mod so it aligns with the newer patch while keeping github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.1 unchanged; after updating, run `go mod tidy` to verify and refresh module checksums.
🤖 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 `@pkg/azblobsas/delegation.go`:
- Line 72: The deferred resp.Body.Close() call in delegation.go currently
ignores its error; wrap the defer in an anonymous function that calls
resp.Body.Close(), captures its returned error, and handle it (return it up the
stack or log it via the existing logger) so cleanup failures aren’t
swallowed—look for the resp variable and the line with defer resp.Body.Close()
and replace it with a deferred closure that checks and handles the Close()
error.
---
Nitpick comments:
In `@go.mod`:
- Around line 8-9: Update the azcore dependency to the latest patch release by
changing the version for github.com/Azure/azure-sdk-for-go/sdk/azcore from
v1.20.0 to v1.21.1 in go.mod so it aligns with the newer patch while keeping
github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.13.1 unchanged; after
updating, run `go mod tidy` to verify and refresh module checksums.
In `@pkg/azblobsas/delegation.go`:
- Line 80: The error string is capitalized; change it to follow Go convention of
lowercase error messages by updating the fmt.Errorf call in the
GetUserDelegationKey code path to start with a lowercase letter (e.g., "get user
delegation key returned %d: %s") while keeping the same formatting placeholders
and returning the error as before.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7a52f984-546c-42e3-acc3-a4e5e7ca7e57
⛔ Files ignored due to path filters (216)
go.sumis excluded by!**/*.sumvendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/LICENSE.txtis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_identifier.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/internal/resource/resource_type.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/policy/policy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/pipeline.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_bearer_token.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_register_rp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/policy_trace_namespace.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/runtime/runtime.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/ci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/cloud.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/core.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/etag.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/exported.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/pipeline.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/request.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/exported/response_error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/log/log.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/async/async.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/body/body.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/fake/fake.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/loc/loc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/op/op.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/poller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/pollers/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/constants.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/internal/shared/shared.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/log/log.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/policy/policy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/pipeline.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_api_version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_bearer_token.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_body_download.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_header.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_http_trace.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_include_response.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_key_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_logging.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_request_id.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_retry.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_sas_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/policy_telemetry.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/poller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/request.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/response.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_other.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_dialer_wasm.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime/transport_default_http_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming/progress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/constants.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore/tracing/tracing.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/BREAKING_CHANGES.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/LICENSE.txtis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/MIGRATION.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/TOKEN_CACHING.MDis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/TROUBLESHOOTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/assets.jsonis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/authentication_record.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/azidentity.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/azure_cli_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/azure_developer_cli_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/azure_pipelines_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/azure_powershell_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/chained_token_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/ci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/client_assertion_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/client_certificate_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/client_secret_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/confidential_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/default_azure_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/developer_credential_util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/developer_credential_util_nonwindows.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/developer_credential_util_windows.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/device_code_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/environment_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/interactive_browser_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/internal/cache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/logging.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/managed-identity-matrix.jsonis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/managed_identity_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/managed_identity_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/on_behalf_of_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/public_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/test-resources-post.ps1is excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/test-resources-pre.ps1is excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/test-resources.bicepis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/username_password_credential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity/workload_identity.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/LICENSE.txtis excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/diag.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/diag/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo/errorinfo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/exported/exported.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/log/log.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/poller/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/temporal/resource.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/Azure/azure-sdk-for-go/sdk/internal/uuid/uuid.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/cache/cache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/confidential/confidential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/errors/error_design.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/errors/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/base/base.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/base/storage/items.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/base/storage/partitioned_storage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/base/storage/storage.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/exported/exported.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/json/design.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/json/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/json/mapslice.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/json/marshal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/json/struct.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/json/types/time/time.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/local/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/oauth.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/accesstokens/accesstokens.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/accesstokens/apptype_string.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/accesstokens/tokens.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/authority/authority.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/authority/authorizetype_string.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/internal/comm/comm.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/internal/comm/compress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/internal/grant/grant.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/ops.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/wstrust/defs/endpointtype_string.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/wstrust/defs/mex_document_definitions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/wstrust/defs/saml_assertion_definitions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/wstrust/defs/version_string.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/wstrust/defs/wstrust_endpoint.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/wstrust/defs/wstrust_mex_document.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/wstrust/wstrust.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/resolvers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/options/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/shared/shared.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/version/version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/managedidentity/azure_ml.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/managedidentity/cloud_shell.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/managedidentity/managedidentity.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/managedidentity/servicefabric.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/AzureAD/microsoft-authentication-library-for-go/apps/public/public.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/MIGRATION_GUIDE.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/SECURITY.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/VERSION_HISTORY.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/claims.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/ecdsa.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/ecdsa_utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/ed25519.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/ed25519_utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/hmac.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/map_claims.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/none.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/parser_option.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/registered_claims.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/rsa.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/rsa_pss.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/rsa_utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/signing_method.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/staticcheck.confis excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/token.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/token_option.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/golang-jwt/jwt/v5/validator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/kylelemons/godebug/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/kylelemons/godebug/diff/diff.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/kylelemons/godebug/pretty/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/kylelemons/godebug/pretty/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/kylelemons/godebug/pretty/public.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/kylelemons/godebug/pretty/reflect.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/kylelemons/godebug/pretty/structure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pkg/browser/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/pkg/browser/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/pkg/browser/browser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pkg/browser/browser_darwin.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pkg/browser/browser_freebsd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pkg/browser/browser_linux.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pkg/browser/browser_netbsd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pkg/browser/browser_openbsd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pkg/browser/browser_unsupported.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/pkg/browser/browser_windows.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/PATENTSis excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/pkcs12/bmp-string.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/pkcs12/crypto.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/pkcs12/errors.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/pkcs12/internal/rc2/rc2.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/pkcs12/mac.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/pkcs12/pbkdf.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/pkcs12/pkcs12.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/pkcs12/safebags.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (11)
go.modpkg/azblobsas/delegation.gopkg/azblobsas/delegation_test.gopkg/azblobsas/sas.gopkg/azblobsas/sas_test.gopkg/azblobsas/token.gopkg/azblobsas/token_test.gopkg/core/restore.gopkg/core/restore_test.gopkg/etcdbackup/orchestrator.gopkg/etcdbackup/orchestrator_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/etcdbackup/orchestrator.go
- pkg/etcdbackup/orchestrator_test.go
- pkg/core/restore.go
| if err != nil { | ||
| return nil, fmt.Errorf("calling Get User Delegation Key: %w", err) | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
Check the error from resp.Body.Close().
The deferred resp.Body.Close() call's error return value is not checked, which could silently hide cleanup failures.
🛡️ Proposed fix
- defer resp.Body.Close()
+ defer func() {
+ if cerr := resp.Body.Close(); cerr != nil && err == nil {
+ err = fmt.Errorf("closing response body: %w", cerr)
+ }
+ }()Alternatively, if you prefer to just log the error:
- defer resp.Body.Close()
+ defer func() {
+ if cerr := resp.Body.Close(); cerr != nil {
+ // Log the error if needed
+ }
+ }()📝 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.
Suggested change
| defer resp.Body.Close() | |
| defer func() { | |
| if cerr := resp.Body.Close(); cerr != nil && err == nil { | |
| err = fmt.Errorf("closing response body: %w", cerr) | |
| } | |
| }() |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 72-72: Error return value of resp.Body.Close is not checked
(errcheck)
🤖 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 `@pkg/azblobsas/delegation.go` at line 72, The deferred resp.Body.Close() call
in delegation.go currently ignores its error; wrap the defer in an anonymous
function that calls resp.Body.Close(), captures its returned error, and handle
it (return it up the stack or log it via the existing logger) so cleanup
failures aren’t swallowed—look for the resp variable and the line with defer
resp.Body.Close() and replace it with a deferred closure that checks and handles
the Close() error.
…rt to STS - Add context.Context to STSClient.AssumeRoleWithWebIdentity for cancellation support - Define STSAssumeRoler interface and inject into RestorePlugin for testability - Centralize credential fallback logic into common.ResolveCredentialRef() - Extract signSnapshotURL() dispatcher to eliminate duplicate s3/azblob dispatch - Extract fetchBSLCredentials() to consolidate BSL+secret fetch pattern - Add missing test for AAD token acquisition failure path - Replace custom contains/searchSubstring with strings.Contains in sts_test.go Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/common/credentials.go`:
- Around line 11-13: ResolveCredentialRef currently dereferences the
BackupStorageLocation pointer bsl without checking for nil; update the function
ResolveCredentialRef to first guard for a nil bsl and return nil immediately if
bsl is nil, then proceed to check bsl.Spec.Credential as before so callers won't
panic when passing a nil BackupStorageLocation.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a35af491-e8e2-4ff1-ab15-2a97ebbffd5f
📒 Files selected for processing (6)
pkg/common/credentials.gopkg/core/restore.gopkg/core/restore_test.gopkg/etcdbackup/orchestrator.gopkg/s3presign/sts.gopkg/s3presign/sts_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/etcdbackup/orchestrator.go
- pkg/core/restore.go
- pkg/core/restore_test.go
Comment on lines
+11
to
+13
| func ResolveCredentialRef(bsl *velerov1api.BackupStorageLocation) *corev1.SecretKeySelector { | ||
| if bsl.Spec.Credential != nil { | ||
| return bsl.Spec.Credential |
There was a problem hiding this comment.
Guard against nil bsl to avoid panic on credential resolution.
Line 12 dereferences bsl unconditionally. If a caller passes nil, this will panic and can fail the reconcile flow.
Proposed fix
func ResolveCredentialRef(bsl *velerov1api.BackupStorageLocation) *corev1.SecretKeySelector {
+ if bsl == nil || bsl.Spec.Credential == nil {
+ return &corev1.SecretKeySelector{
+ LocalObjectReference: corev1.LocalObjectReference{Name: DefaultCredentialSecretName},
+ Key: DefaultCredentialSecretKey,
+ }
+ }
- if bsl.Spec.Credential != nil {
- return bsl.Spec.Credential
- }
- return &corev1.SecretKeySelector{
- LocalObjectReference: corev1.LocalObjectReference{Name: DefaultCredentialSecretName},
- Key: DefaultCredentialSecretKey,
- }
+ return bsl.Spec.Credential
}📝 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.
Suggested change
| func ResolveCredentialRef(bsl *velerov1api.BackupStorageLocation) *corev1.SecretKeySelector { | |
| if bsl.Spec.Credential != nil { | |
| return bsl.Spec.Credential | |
| func ResolveCredentialRef(bsl *velerov1api.BackupStorageLocation) *corev1.SecretKeySelector { | |
| if bsl == nil || bsl.Spec.Credential == nil { | |
| return &corev1.SecretKeySelector{ | |
| LocalObjectReference: corev1.LocalObjectReference{Name: DefaultCredentialSecretName}, | |
| Key: DefaultCredentialSecretKey, | |
| } | |
| } | |
| return bsl.Spec.Credential | |
| } |
🤖 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 `@pkg/common/credentials.go` around lines 11 - 13, ResolveCredentialRef
currently dereferences the BackupStorageLocation pointer bsl without checking
for nil; update the function ResolveCredentialRef to first guard for a nil bsl
and return nil immediately if bsl is nil, then proceed to check
bsl.Spec.Credential as before so callers won't panic when passing a nil
BackupStorageLocation.
3 tasks
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
3 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.
Summary
cloud-credentialssecret (key:cloud) when BSL has no.spec.credentialreference (ARO with Workload Identity, future ROSA without DPA)credentialsin copied secret so the HCPEtcdBackup controller can auto-detect credential typeDescription
ROSA HCP and ARO HCP use short-lived federated credentials instead of long-lived static keys. This PR enables the OADP plugin to handle these environments:
ParseAWSCredentialDataauto-detects 3 credential formats (bare ARN, INI withrole_arn, static keys). When IRSA credentials are detected during restore, the plugin assumes the role viaAssumeRoleWithWebIdentitybefore pre-signing the S3 URLdataprotectionapplications.oadp.openshift.io). When absent (ARO, future ROSA without DPA), falls back to the well-knowncloud-credentialssecret in the Velero namespacecopyCredentialSecretnow copies the credential data with bothcredentials(for S3 controller) and the original key (e.g.,cloudfor Azure WI detection)Scenarios covered
.spec.credential(existing flow)cloud-credentialsin velero NScloud-credentialsin velero NSRelated PRs
Jira
Test plan
go build ./...passespkg/s3presign,pkg/core,pkg/etcdbackup).spec.credentialpresent pass (no regression)ParseAWSCredentialData— bare ARN, INI STS, static keys, invalid inputSTSClient.AssumeRoleWithWebIdentity— success, error, XML parsing🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements