CNTRLPLANE-3167: support STS/IRSA credentials and standalone Velero#247
Merged
openshift-merge-bot[bot] merged 5 commits intoJun 4, 2026
Merged
Conversation
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds default credential constants and resolver; classifies/parses AWS credentials and adds an STS client; implements Azure Blob account-key SAS and user-delegation SAS with AAD token provider; integrates presigning into restore and etcd backup flows; adds safe resource naming 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 ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
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.
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
Contributor
|
@jparrill I have validated image https://quay.io/repository/jparrill/hypershift-oadp-plugin?tab=tags&tag=[CNTRLPLANE-3167](https://redhat.atlassian.net/browse/CNTRLPLANE-3167)-v3 works in ARO-HCP personal development environments by:
Exact image used (mirrored from quay.io): arohcpsvcdev.azurecr.io/jparrill/hypershift-oadp-plugin@sha256:8c5c1db7cee57d5612db8952bfffd9ddb44408b2ed1ee773b54638e6e90a9d8f |
openshift-ci
Bot
added
the
needs-rebase
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/core/restore_test.go`:
- Around line 345-356: The helper writeTempFile currently ignores the error
returned by f.Close(), which can hide write/flush failures; update writeTempFile
to check Close() and fail the test on error (use t.Fatalf or t.Fatalf-style
error reporting) after WriteString, e.g., capture err = f.Close() and call
t.Fatalf("failed to close temp file: %v", err) if non-nil; ensure this change is
applied inside the writeTempFile function so tests surface flush/close errors.
🪄 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: 244d0273-eb89-47d7-8f35-48652c9faa40
📒 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 due to trivial changes (1)
- pkg/common/credentials.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/s3presign/sts_test.go
- pkg/etcdbackup/orchestrator.go
- pkg/s3presign/sts.go
- pkg/core/restore.go
Comment on lines
+345
to
+356
| func writeTempFile(t *testing.T, content string) string { | ||
| t.Helper() | ||
| f, err := os.CreateTemp(t.TempDir(), "token-*") | ||
| if err != nil { | ||
| t.Fatalf("failed to create temp file: %v", err) | ||
| } | ||
| if _, err := f.WriteString(content); err != nil { | ||
| t.Fatalf("failed to write temp file: %v", err) | ||
| } | ||
| f.Close() | ||
| return f.Name() | ||
| } |
There was a problem hiding this comment.
Handle Close() errors in writeTempFile.
Line 354 ignores the Close() return, which can mask write/flush failures in tests.
✅ Suggested fix
func writeTempFile(t *testing.T, content string) string {
t.Helper()
f, err := os.CreateTemp(t.TempDir(), "token-*")
if err != nil {
t.Fatalf("failed to create temp file: %v", err)
}
+ defer func() {
+ if err := f.Close(); err != nil {
+ t.Fatalf("failed to close temp file: %v", err)
+ }
+ }()
if _, err := f.WriteString(content); err != nil {
t.Fatalf("failed to write temp file: %v", err)
}
- f.Close()
return f.Name()
}As per coding guidelines, "Go security (prodsec-skills): Never ignore error returns".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/core/restore_test.go` around lines 345 - 356, The helper writeTempFile
currently ignores the error returned by f.Close(), which can hide write/flush
failures; update writeTempFile to check Close() and fail the test on error (use
t.Fatalf or t.Fatalf-style error reporting) after WriteString, e.g., capture err
= f.Close() and call t.Fatalf("failed to close temp file: %v", err) if non-nil;
ensure this change is applied inside the writeTempFile function so tests surface
flush/close errors.
…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>
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>
…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>
openshift-ci
Bot
removed
the
needs-rebase
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@jparrill: all tests passed! 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. |
Contributor
Author
|
/verified by @tony-schndr |
openshift-ci-robot
added
the
verified
|
@jparrill: This PR has been marked as verified by 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. |
Contributor
Author
|
/verified by @tony-schndr |
|
@jparrill: This PR has been marked as verified by 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. |
mgencur
reviewed
Jun 4, 2026
mgencur
left a comment
mgencur
left a comment
There was a problem hiding this comment.
Hey @jparrill This looks good! I have a few question here and there. I'm also posting the PR review by /code-review:pr from ai-helpers. There are some minor recommendations but overall it's OK:
Code Review Report: PR #247
PR Info
- Title: CNTRLPLANE-3167: support STS/IRSA credentials and standalone Velero
- Author: jparrill (Juan Manuel Parrilla Madrid)
- Base branch: main
- Link: #247
Files Reviewed
Source files (10):
| File | Status |
|---|---|
pkg/azblobsas/delegation.go |
NEW |
pkg/azblobsas/sas.go |
NEW |
pkg/azblobsas/token.go |
NEW |
pkg/common/credentials.go |
NEW |
pkg/common/types.go |
MODIFIED |
pkg/core/backup.go |
MODIFIED |
pkg/core/restore.go |
MODIFIED |
pkg/etcdbackup/orchestrator.go |
MODIFIED |
pkg/s3presign/presign.go |
MODIFIED |
pkg/s3presign/sts.go |
NEW |
Test files (8):
| File | Status |
|---|---|
pkg/azblobsas/delegation_test.go |
NEW |
pkg/azblobsas/sas_test.go |
NEW |
pkg/azblobsas/token_test.go |
NEW |
pkg/core/restore_test.go |
MODIFIED |
pkg/etcdbackup/orchestrator_test.go |
MODIFIED |
pkg/s3presign/presign_test.go |
MODIFIED |
pkg/s3presign/sts_test.go |
NEW |
Config: go.mod, go.sum (dependency updates + new Azure SDK deps)
Build Verification: PASS
| Check | Result |
|---|---|
go build ./... |
PASS |
go vet ./... |
PASS (clean) |
go test (6 packages) |
PASS (all ok) |
Unit Test Coverage
Overall: Strong. Every new source file has a corresponding _test.go. Tests use table-driven patterns with t.Run(), cover success and error paths, and test edge cases.
Coverage gaps (ordered by severity)
| Severity | Location | Gap |
|---|---|---|
| Medium | pkg/common/credentials.go:11 |
ResolveCredentialRef has no direct unit test. Two branches (explicit ref vs fallback) used in critical backup/restore paths. Tested indirectly but should have a dedicated credentials_test.go. |
| Low | pkg/core/backup.go:266-271 |
Etcd PVC exclusion logic (strings.HasPrefix(name, "data-etcd-")) is not tested in backup_test.go. |
| Low | pkg/azblobsas/delegation.go:112 |
Invalid base64 delegation key error path not tested. |
| Low | pkg/azblobsas/token.go:27 |
NewAADTokenProvider not directly tested (acceptable — requires real Azure infra; mock injection in restore tests is correct). |
Coverage details by file
pkg/azblobsas/delegation.go (NEW) — Tests: delegation_test.go
GetUserDelegationKey— TESTED (success, HTTP 403, HTTP 500, malformed XML). Verifies request method, query params, headers, and response parsing.GenerateUserDelegationSASBlobURL— TESTED (basic URL structure, deterministic output, custom endpoint, http endpoint, missing fields, nil delegation key, different blobs produce different sigs, different keys produce different sigs, default expiry).- Minor gap: no test for invalid endpoint string (
url.Parseerror path atdelegation.go:43). - Minor gap: no test for invalid base64 delegation key value (error path at
delegation.go:112).
pkg/azblobsas/sas.go (NEW) — Tests: sas_test.go
ParseAzBlobURL— TESTED (11 cases: valid public/gov/china/http URLs, missing blob, missing container, wrong scheme, non-Azure URL, empty URL, trailing slash).IsAzBlobURL— TESTED (11 cases: all cloud variants, wrong schemes, empty string, non-URL input).ParseAzureCredentials— TESTED (8 cases: standard key, custom env var, comments/blank lines, whitespace, missing key, empty data, value with equals, multiple entries).ParseAzureAADCredentials— TESTED (6 cases: workload identity, service principal, comments, missing tenant, missing client, empty data).GenerateSASBlobURL— TESTED (10 cases: basic structure, determinism, custom endpoint, http endpoint, default expiry, missing fields, missing key, invalid base64, special chars, sig changes).- No gaps identified.
pkg/azblobsas/token.go (NEW) — Tests: token_test.go
NewAADTokenProvider— NOT DIRECTLY TESTED. Acceptable — creates Azure SDK credential objects requiring real infrastructure. Callers inject a mockTokenProvider.StorageScopeForCloud— TESTED (6 cases covering all cloud variants and empty string).cloudConfigForName— TESTED (6 cases matching cloud names to authority hosts).
pkg/common/credentials.go (NEW) — Tests: NONE
ResolveCredentialRef— NOT TESTED directly. Exercised indirectly through orchestrator and restore tests. A direct unit test is warranted.
pkg/core/restore.go (MODIFIED) — Tests: restore_test.go (MODIFIED)
RestorePlugin.Execute— TESTED viaTestRestoreExecuteEtcdStatefulSet,TestRestoreExecuteSnapshotURL,TestRestoreExecuteHCPSnapshotURL(etcd StatefulSet skip/restore, HC with s3/https/Azure annotations, HCP with s3/https/Azure annotations, missing BSL).presignS3URL— TESTED (9 cases: valid URL, invalid URL, BSL missing, BSL no cred ref fallback, fallback secret missing, wrong key, https URL, IRSA/STS credentials, STS failure, custom endpoint).presignAzBlobUrl— TESTED viaTestPresignAzBlobUrlWithAAD(7 cases: AAD with credentials, missing AAD fields, WI env vars, missing env vars, token acquisition failure, account key SAS path).
pkg/etcdbackup/orchestrator.go (MODIFIED) — Tests: orchestrator_test.go (MODIFIED)
CreateEtcdBackup— TESTED (7 cases: valid AWS BSL, nil HC, BSL not found, unsupported provider, no credential ref fallback, long name truncation, missing fallback secret).copyCredentialSecret— TESTED (5 cases: key remapping, already exists, key=credentials no duplication, missing key, missing secret).safeResourceName— TESTED (5 cases: short/exact/truncation/trailing hyphen).
pkg/s3presign/presign.go (MODIFIED) — Tests: presign_test.go (MODIFIED)
ParseAWSCredentialData(NEW) — TESTED (13 cases: bare ARN, ARN with whitespace, GovCloud/China ARNs, missing env token, INI with role_arn+token, role_arn only env fallback, non-default profile, source_profile error, credential_source error, static creds, static with session token, empty data, whitespace-only, invalid INI).isBareARN(NEW) — TESTED (9 cases).parseSTSProfile(NEW) — TESTED (5 cases).
pkg/s3presign/sts.go (NEW) — Tests: sts_test.go (NEW)
STSClient.AssumeRoleWithWebIdentity— TESTED (success, HTTP error, malformed XML, 4 missing-credential-field combinations, missing token file, empty token file, empty roleARN, empty tokenFile path, default session name).NewSTSClient— TESTED.
Test quality observations
- All test files use table-driven tests with
t.Run()— consistent with project conventions. t.Helper()is used appropriately in helper functions (writeTempFile,writeTokenFile,newHCPUnstructured).t.Parallel()is not used anywhere in the PR. Tests appear independent and could run in parallel, but this is a style choice, not a correctness issue.- Error messages are verified with substring matching, which is robust against message rewording.
- Mock injection (via
newTokenProvider,newSTSClientfunction fields) is a clean pattern for testing external service calls.
Idiomatic Go Code
| Severity | Location | Finding | Recommendation |
|---|---|---|---|
| Medium | restore.go:339,381 |
presignAzBlobUrl / presignAzBlobUrlWithAAD — "URL" should be all-caps per Go naming convention |
Rename to presignAzBlobURL / presignAzBlobURLWithAAD |
| Low | restore.go:411 |
Uses time.Now() directly instead of nowFunc — inconsistent with azblobsas package's testable time pattern |
Use nowFunc or accept time as parameter |
| Low | backup.go:8-9, restore.go:13-15 |
Redundant import aliases that match package names (common "...common", validation "...validation") |
Remove explicit aliases |
| Low | backup.go:139, restore.go:147 |
No-op type conversion context.Context(p.ctx) |
Pre-existing, but should be p.ctx directly |
| Info | presign.go:94,158 |
ParseAWSCredentials vs ParseAWSCredentialData naming overlap could confuse callers |
Consider DetectAWSCredentials for the higher-level function |
| Info | presign.go:167 |
os.Getenv inside a parsing function makes it impure |
Consider accepting the env var value as a parameter |
| Info | orchestrator_test.go:50-52 |
Comment/function name mismatch (TestMapBSLToStorage comment on TestSafeResourceName function) |
Update the comment |
DRY Compliance
| Severity | Location | Finding | Recommendation |
|---|---|---|---|
| Medium | azblobsas/sas.go:240 + s3presign/presign.go:369 |
hmacSHA256 function is character-for-character identical in both packages |
Extract to pkg/common/crypto.go |
| Medium | core/backup.go:82-91 + core/restore.go:90-98 |
DPA CRD detection block is verbatim duplicated (8 identical lines) | Extract to helper like common.DetectDPA(ctx, client, log) bool |
| Medium | s3presign/presign.go:94-149 + presign.go:233-270 |
INI profile iteration logic (~20 lines) duplicated within the same file between ParseAWSCredentials and parseSTSProfile |
Extract parseINIProfile(data, profile) map[string]string |
| Low | azblobsas/sas.go + delegation.go |
SAS URL generation structural overlap (time computation, host/scheme, final URL assembly) | Acceptable — keeping variants self-contained improves security auditability |
| Low | azblobsas/sas.go:18 + s3presign/presign.go:19 |
var nowFunc = time.Now in both packages |
Acceptable — standard Go testing pattern, each package needs its own |
Acceptable non-findings
parseDotenv(azblobsas) vsParseAWSCredentials(s3presign): structurally similar but semantically different (dotenv vs INI-with-sections). Separate implementations justified.uriEncodein both packages: different encoding behavior (PathEscape vs QueryEscape+SigV4 fixups). Intentional per Azure/AWS signing specs.- BSL fetch logic in
restore.govsorchestrator.go: different responsibilities and return types. Too trivial to extract.
SOLID Compliance
| Principle | Verdict | Notes |
|---|---|---|
| SRP | Mostly compliant | azblobsas well-decomposed by file. Minor: s3presign/presign.go mixes credential parsing + URL generation in one file. |
| OCP | Compliant | signSnapshotURL extends behavior (Azure branch) without modifying S3 path. ParseAWSCredentialData extends credential detection without modifying ParseAWSCredentials. |
| LSP | Fully compliant | TokenProvider and STSAssumeRoler implementations (production + mocks) honor contracts. |
| ISP | Fully compliant | Both interfaces are single-method and maximally focused. |
| DIP | Mostly compliant | newTokenProvider and newSTSClient function fields on RestorePlugin are textbook DIP. Minor: direct os.Getenv calls in presign.go:167 and restore.go:390-397 bypass injection. |
SOLID details
SRP findings:
pkg/azblobsas/is well-decomposed:delegation.go(user delegation key),sas.go(Service SAS + credential parsing),token.go(AAD token acquisition).pkg/s3presign/sts.gohas one job: STS AssumeRoleWithWebIdentity. Clean separation from presigning.pkg/common/credentials.godoes exactly one thing: resolve a BSL credential reference with fallback.- Minor:
s3presign/presign.gomixes credential parsing/detection with SigV4 URL generation.
DIP findings:
RestorePlugindepends on abstractions via function fields (newTokenProvider,newSTSClient) — wired with production implementations inNewRestorePluginand overridden in tests.var httpDoindelegation.go:16is a weaker DIP form compared toSTSClient.HTTPClientstruct field insts.go:29.os.Getenvcalls inpresign.go:167andrestore.go:390-397are hard dependencies on the OS environment.
Overall Verdict: PASS WITH RECOMMENDATIONS
This is a well-structured, well-tested PR that adds significant multi-cloud credential support. The architecture decisions (interfaces for testability, dependency injection via function fields, pure-stdlib STS client) are sound. Build, vet, and all tests pass.
Required Actions
None — no blocking issues.
Recommended Improvements
presignAzBlobUrl→presignAzBlobURL(Go acronym convention) —restore.go:339,381- Add
pkg/common/credentials_test.goforResolveCredentialRef— two branches, used in critical paths - Extract duplicated
hmacSHA256topkg/common/crypto.go— identical function in two packages - Extract DPA detection helper — 8 verbatim-identical lines in
backup.goandrestore.go - Extract
parseINIProfileins3presign/presign.go— deduplicate ~20 lines of INI iteration
| "time" | ||
| ) | ||
|
|
||
| var httpDo = http.DefaultClient.Do |
There was a problem hiding this comment.
Nit: It looks like this httpDo is used only once. Probably makes sense to use http.DefaultClient.Do directly and get rid of this variable.
Contributor
Author
There was a problem hiding this comment.
Good catch — it is only called once, but the var is intentional: delegation_test.go replaces httpDo with a fake to avoid real HTTP calls (see TestGetUserDelegationKey). It's the same pattern we use with var nowFunc = time.Now in the SAS package. I can add a short comment above the var if that helps clarify the intent.
| expiryTime := now.Add(expiry) | ||
|
|
||
| signedStart := now.Format("2006-01-02T15:04:05Z") | ||
| signedExpiry := expiryTime.Format("2006-01-02T15:04:05Z") |
There was a problem hiding this comment.
It is strange to see this old date here hardcoded. Does it have any meaning or effect?
Contributor
Author
There was a problem hiding this comment.
Answered above — same Go reference time layout.
|
|
||
| body, err := xml.Marshal(keyInfoRequest{ | ||
| Start: start.UTC().Format("2006-01-02T15:04:05Z"), | ||
| Expiry: expiry.UTC().Format("2006-01-02T15:04:05Z"), |
There was a problem hiding this comment.
It is strange to see this old date here hardcoded. Does it have any meaning or effect? Is it necessary?
Contributor
Author
There was a problem hiding this comment.
That's Go's reference time — it's how time.Format works in Go. The magic date 2006-01-02T15:04:05Z is the layout string (Mon Jan 2 15:04:05 MST 2006), not an actual hardcoded date. The output at runtime is the real current timestamp formatted as ISO 8601.
| p.log.Debugf("Pod found, skipping restore") | ||
| return velero.NewRestoreItemActionExecuteOutput(input.Item).WithoutRestore(), nil | ||
|
|
||
| case kind == "StatefulSet": |
There was a problem hiding this comment.
For me this doesn't belong to this PR. Is this something that was forgotten in previous PRs? Or, it is related to STS/IRSA. Just curious.
Contributor
Author
There was a problem hiding this comment.
This is related to this PR — it was added in commit 0537002. When using the etcdSnapshot restore method, Velero would restore the etcd StatefulSet with the old (expired) snapshot URL baked into its spec. That conflicts with the fresh pre-signed URL we inject into the HostedControlPlane. Skipping the StatefulSet restore lets the HCP controller reconcile it with the correct URL. I can add a comment in the code explaining the why if you think it'd help.
|
/lgtm |
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
4 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
Tests