no-jira: azure: Disable shared access key by default#10574
Conversation
|
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:
Note
|
| Layer / File(s) | Summary |
|---|---|
Type definition and field documentation pkg/types/azure/platform.go |
The AllowSharedKeyAccess field documentation is updated to specify default false, mark the field as deprecated, and clarify the "Storage Blob Data Contributor" permission requirement. |
Implementation default initializations pkg/infrastructure/azure/azure.go |
The sharedKey boolean defaults to false in both InfraReady and Ignition functions, changing downstream storage account and blob creation behavior when AllowSharedKeyAccess is unset. The Ignition path also uses the function-scoped ctx for GetUserDelegationCredential. |
Schema and test documentation updates data/data/install.openshift.io_installconfigs.yaml, pkg/explain/printer_test.go |
CRD schema and test expectations are updated to document the new false default, add deprecation notice, and reflect updated permission wording. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 11 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (11 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Stable And Deterministic Test Names | ✅ Passed | PR contains no Ginkgo tests. The only test file modified (pkg/explain/printer_test.go) uses standard Go testing framework without Ginkgo, so the check is not applicable. |
| Test Structure And Quality | ✅ Passed | The PR introduces standard Go tests, not Ginkgo tests. The custom check is specifically for Ginkgo test code quality, which is not applicable to this PR. |
| Microshift Test Compatibility | ✅ Passed | PR does not add new Ginkgo e2e tests. Only changes are to CRD schema, infrastructure code, type docs, and unit test expectations. The check applies only to new Ginkgo e2e tests. |
| Single Node Openshift (Sno) Test Compatibility | ✅ Passed | No new Ginkgo e2e tests were added in this PR. Changes are limited to Azure configuration defaults, type definitions, and unit test expectations. SNO compatibility check is not applicable. |
| Topology-Aware Scheduling Compatibility | ✅ Passed | PR introduces only Azure platform configuration changes (shared key access default) with no deployment manifests, controller code, or scheduling constraints affecting OpenShift topology compatibility. |
| Ote Binary Stdout Contract | ✅ Passed | PR modifies Azure configuration defaults and documentation only. No stdout writes in main(), init(), suite-level, or other process-level code. All fmt usage is for errors/strings, not stdout. |
| Ipv6 And Disconnected Network Test Compatibility | ✅ Passed | PR does not add new Ginkgo e2e tests; it only modifies Azure infrastructure code, type definitions, schema documentation, and standard Go test expectations (not Ginkgo tests). |
| Title check | ✅ Passed | The title clearly and specifically describes the main change: disabling shared access key by default for Azure, which is the primary objective across all modified files. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
|
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/infrastructure/azure/storage.go`:
- Around line 281-291: The CreatePageBlob flow currently builds pageBlobClient
using in.TokenCredential, but later the StackCloud branch calls
pageBlobClient.GetSASURL which requires a SharedKey-authenticated client; update
CreatePageBlob to instantiate a second pageblob.Client (or conditionally
construct it) using the SharedKeyCredential (e.g., in.SharedKeyCredential) when
running the StackCloud path so GetSASURL is called against a
SharedKey-authenticated client (leave token-based client for other operations),
ensuring you still set the same ClientOptions (e.g., Cloud from in.ClientOpts)
and return or use the SharedKey-backed pageBlobClient for the GetSASURL call.
🪄 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: 81354f54-4ca1-4ecf-8d04-73db22138f4a
⛔ Files ignored due to path filters (1)
pkg/types/azure/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (5)
data/data/install.openshift.io_installconfigs.yamlpkg/infrastructure/azure/azure.gopkg/infrastructure/azure/storage.gopkg/types/azure/platform.gopkg/types/azure/validation/platform.go
💤 Files with no reviewable changes (1)
- pkg/types/azure/validation/platform.go
rna-afk
force-pushed
the
azure_set_sak_default
branch
from
7f10e44 to
79082ea
Compare
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/infrastructure/azure/storage.go`:
- Around line 100-103: CreateStorageAccount currently disables shared-key auth
for non-Stack accounts via AllowSharedKeyAccess but still calls ListKeys and
returns keys in CreateStorageAccountOutput; restrict key retrieval so non-Stack
flows never request or return secrets. Update CreateStorageAccount to only call
the ListKeys path (or populate CreateStorageAccountOutput.Keys) when
in.CloudName == aztypes.StackCloud (i.e., when AllowSharedKeyAccess is true),
and for other clouds skip ListKeys and ensure CreateStorageAccountOutput.Keys is
nil/empty (or remove it) so no listKeys permission or secrets are exercised in
the non-Stack happy path.
🪄 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: 41c44f64-c9a1-4a7b-b5be-d0b091b38b06
⛔ Files ignored due to path filters (1)
pkg/types/azure/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (6)
data/data/install.openshift.io_installconfigs.yamlpkg/explain/printer_test.gopkg/infrastructure/azure/azure.gopkg/infrastructure/azure/storage.gopkg/types/azure/platform.gopkg/types/azure/validation/platform.go
💤 Files with no reviewable changes (1)
- pkg/types/azure/validation/platform.go
✅ Files skipped from review due to trivial changes (1)
- data/data/install.openshift.io_installconfigs.yaml
rna-afk
force-pushed
the
azure_set_sak_default
branch
from
79082ea to
fb3ed63
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/types/azure/platform.go (1)
54-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the exported field name stable when deprecating.
Renaming
AllowSharedKeyAccesstoDeprecatedAllowSharedKeyAccessis a compile-time breaking change for Go consumers ofazure.Platform, even though the JSON name stays compatible. Please keep the field identifier asAllowSharedKeyAccessand rely on theDeprecated:comment for the deprecation signal instead.Possible fix
// AllowSharedKeyAccess specifies if shared access key should be enabled for the storage account. // Default value is false. // This will require a new permission "Storage Blob Data Contributor" in azure. // // Deprecated: Avoid using shared key access in the installer. // +optional - DeprecatedAllowSharedKeyAccess *bool `json:"allowSharedKeyAccess,omitempty"` + AllowSharedKeyAccess *bool `json:"allowSharedKeyAccess,omitempty"`🤖 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/types/azure/platform.go` around lines 54 - 60, The exported field name was renamed to DeprecatedAllowSharedKeyAccess which is a breaking change; revert the struct field identifier back to AllowSharedKeyAccess while keeping the deprecation marker in the comment (use the existing "Deprecated:" comment above the field), preserve the json tag `json:"allowSharedKeyAccess,omitempty"` and `+optional` tag, and remove the new DeprecatedAllowSharedKeyAccess identifier so consumers keep the original Go field name on azure.Platform.
🤖 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/infrastructure/azure/azure.go`:
- Around line 884-885: The fallback branch that calls
GetUserDelegationCredential currently discards the request context by using
context.Background(); instead pass the existing ctx so cancellation, timeouts
and retries propagate. Update the call site that checks if sasURL == "" to call
serviceClient.GetUserDelegationCredential(ctx, info, nil) (and any other similar
calls in this function) so the original context is preserved for the SAS
fallback path.
---
Outside diff comments:
In `@pkg/types/azure/platform.go`:
- Around line 54-60: The exported field name was renamed to
DeprecatedAllowSharedKeyAccess which is a breaking change; revert the struct
field identifier back to AllowSharedKeyAccess while keeping the deprecation
marker in the comment (use the existing "Deprecated:" comment above the field),
preserve the json tag `json:"allowSharedKeyAccess,omitempty"` and `+optional`
tag, and remove the new DeprecatedAllowSharedKeyAccess identifier so consumers
keep the original Go field name on azure.Platform.
🪄 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: ff659667-1be9-4ffa-8f20-93110efb0f68
⛔ Files ignored due to path filters (1)
pkg/types/azure/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (5)
data/data/install.openshift.io_installconfigs.yamlpkg/infrastructure/azure/azure.gopkg/infrastructure/azure/storage.gopkg/types/azure/platform.gopkg/types/azure/validation/platform.go
💤 Files with no reviewable changes (1)
- pkg/types/azure/validation/platform.go
✅ Files skipped from review due to trivial changes (1)
- data/data/install.openshift.io_installconfigs.yaml
rna-afk
force-pushed
the
azure_set_sak_default
branch
2 times, most recently
from
6cc1747 to
62da49e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/infrastructure/azure/azure.go (1)
853-856:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
ctxin the default SAS fallback.Because this change makes
sharedKey == falsethe default, the user-delegation path below now runs on the common install flow. CallingGetUserDelegationCredentialwithcontext.Background()drops cancellation and timeouts for that Azure request.Possible fix
- udc, err := serviceClient.GetUserDelegationCredential(context.Background(), info, nil) + udc, err := serviceClient.GetUserDelegationCredential(ctx, info, nil)#!/bin/bash # Verify there are no remaining user-delegation calls that discard request context. rg -nC2 'GetUserDelegationCredential\(context\.Background\(\)' --type=goAlso applies to: 907-909
🤖 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/infrastructure/azure/azure.go` around lines 853 - 856, The code sets sharedKey to false by default and then calls GetUserDelegationCredential with context.Background(), which discards the request context; update the GetUserDelegationCredential calls to use the existing request context (ctx) instead of context.Background() so timeouts/cancellations propagate; look for usages around the sharedKey handling and the subsequent user-delegation credential code (references: sharedKey variable and GetUserDelegationCredential function calls) and replace the background context with the passed-in ctx in both places mentioned.
🤖 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/explain/printer_test.go`:
- Around line 318-323: The help text for allowSharedKeyAccess is ambiguous about
when the "Storage Blob Data Contributor" permission is required; update the CRD
description and any help text to say explicitly that the installer requires that
permission when shared key access is disabled (the default), e.g. "When shared
key access is disabled (the default), the installer requires the 'Storage Blob
Data Contributor' permission." Then update the test expectation in
printer_test.go that asserts the printed help output so it matches the new CRD
description text; search for allowSharedKeyAccess and the CRD description string
to locate all places to change.
---
Duplicate comments:
In `@pkg/infrastructure/azure/azure.go`:
- Around line 853-856: The code sets sharedKey to false by default and then
calls GetUserDelegationCredential with context.Background(), which discards the
request context; update the GetUserDelegationCredential calls to use the
existing request context (ctx) instead of context.Background() so
timeouts/cancellations propagate; look for usages around the sharedKey handling
and the subsequent user-delegation credential code (references: sharedKey
variable and GetUserDelegationCredential function calls) and replace the
background context with the passed-in ctx in both places mentioned.
🪄 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: 08d5b02d-3b1a-47fa-8b2c-ac909e15bb54
📒 Files selected for processing (4)
data/data/install.openshift.io_installconfigs.yamlpkg/explain/printer_test.gopkg/infrastructure/azure/azure.gopkg/types/azure/platform.go
✅ Files skipped from review due to trivial changes (1)
- pkg/types/azure/platform.go
rna-afk
force-pushed
the
azure_set_sak_default
branch
3 times, most recently
from
c743cf8 to
bbbdca5
Compare
Setting the allow shared access key to false by default for better security practices.
rna-afk
force-pushed
the
azure_set_sak_default
branch
from
bbbdca5 to
4652e26
Compare
|
/retest-required |
|
@rna-afk: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest-required |
rna-afk
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@rna-afk: This pull request explicitly references no jira issue. 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. |
2 participants
Setting the allow shared access key to false by default for
better security practices.
Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation
Tests