Skip to content

[WIP] feat: default to kubelet identity when AzureStorageIdentityClientID is empty for MSI auth #2430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
andyzhangx wants to merge 4 commits into
base: master
Choose a base branch
from
+131 −9
Open

[WIP] feat: default to kubelet identity when AzureStorageIdentityClientID is empty for MSI auth #2430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa042bf
feat: default to kubelet identity when AzureStorageIdentityClientID i…
andyzhangx Apr 27, 2026
cc21f55
fix: add nolint:dogsled for test with expected error return
andyzhangx Apr 27, 2026
ab09ab0
fix: use case-insensitive MSI check, handle empty clientID, warn inst…
andyzhangx Apr 27, 2026
85b77c4
fix: gofmt alignment
andyzhangx Apr 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions pkg/blob/blob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,20 +754,28 @@ func (d *Driver) GetAuthEnv(ctx context.Context, volumeID, protocol string, attr
authEnv = append(authEnv, "AZURE_STORAGE_SPN_TENANT_ID="+storageSPNTenantID)
}

if azureStorageAuthType == storageAuthTypeMSI {
// check whether authEnv contains AZURE_STORAGE_IDENTITY_ prefix
if strings.EqualFold(azureStorageAuthType, storageAuthTypeMSI) {
// check whether authEnv contains a non-empty AZURE_STORAGE_IDENTITY_ value
containsIdentityEnv := false
Comment on lines +757 to 759
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

azureStorageAuthType is compared using == storageAuthTypeMSI, but docs/examples commonly set AzureStorageAuthType: MSI (uppercase). This means the MSI-specific identity defaulting/error path won’t run for those values. Use strings.EqualFold(azureStorageAuthType, storageAuthTypeMSI) here (consistent with the earlier secret-handling check) and add/adjust a unit test to cover uppercase MSI.

Copilot uses AI. Check for mistakes.
for _, env := range authEnv {
if strings.HasPrefix(env, "AZURE_STORAGE_IDENTITY_") {
klog.V(2).Infof("AZURE_STORAGE_IDENTITY_ is already set in authEnv, skip setting it again")
containsIdentityEnv = true
break
// skip empty values like "AZURE_STORAGE_IDENTITY_CLIENT_ID="
parts := strings.SplitN(env, "=", 2)
if len(parts) == 2 && parts[1] != "" {
klog.V(2).Infof("AZURE_STORAGE_IDENTITY_ is already set in authEnv, skip setting it again")
containsIdentityEnv = true
break
}
}
}
if !containsIdentityEnv && d.cloud != nil && d.cloud.Config.AzureAuthConfig.UserAssignedIdentityID != "" {
klog.V(2).Infof("azureStorageAuthType is set to %s, add AZURE_STORAGE_IDENTITY_CLIENT_ID(%s) into authEnv",
azureStorageAuthType, d.cloud.Config.AzureAuthConfig.UserAssignedIdentityID)
authEnv = append(authEnv, "AZURE_STORAGE_IDENTITY_CLIENT_ID="+d.cloud.Config.AzureAuthConfig.UserAssignedIdentityID)
if !containsIdentityEnv {
if d.cloud != nil && d.cloud.Config.AzureAuthConfig.UserAssignedIdentityID != "" {
klog.V(2).Infof("MSI auth: AzureStorageIdentityClientID not specified, default to kubelet identity (%s)",
d.cloud.Config.AzureAuthConfig.UserAssignedIdentityID)
Comment on lines +771 to +774
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes MSI auth fail fast when neither AzureStorageIdentityClientID nor cloud.Config.AzureAuthConfig.UserAssignedIdentityID is set. That can be a breaking behavior change for clusters that rely on system-assigned MSI/IMDS (where omitting the client ID can still be valid). If the intention is to preserve IMDS fallback, consider logging a warning and continuing instead of returning an error; otherwise, please update the public docs/parameters to reflect that an identity client ID (explicit or kubelet) is now required for AzureStorageAuthType=MSI.

Copilot uses AI. Check for mistakes.
authEnv = append(authEnv, "AZURE_STORAGE_IDENTITY_CLIENT_ID="+d.cloud.Config.AzureAuthConfig.UserAssignedIdentityID)
} else {
klog.Warningf("MSI auth: no identity client ID provided and kubelet identity not available, MSI/IMDS will be used as fallback")
}
}
}

Expand Down
114 changes: 114 additions & 0 deletions pkg/blob/blob_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2663,6 +2663,120 @@ func TestGetAuthEnvMSIAuthTypeSkipsIdentityEnvIfAlreadySet(t *testing.T) {
assert.True(t, found, "Explicit AZURE_STORAGE_IDENTITY_CLIENT_ID should be preserved")
}

func TestGetAuthEnvMSIDefaultsToKubeletIdentity(t *testing.T) {
d := NewFakeDriver()
d.cloud = &storage.AccountRepo{}
d.cloud.Config.AzureAuthConfig = azclient.AzureAuthConfig{
UserAssignedIdentityID: "kubelet-identity-client-id",
}

attrib := map[string]string{
containerNameField: "containername",
storageAccountField: "accountname",
storageAuthTypeField: storageAuthTypeMSI,
}
Comment thread
andyzhangx marked this conversation as resolved.
secret := map[string]string{
accountNameField: "accountname",
accountKeyField: "testkey",
}
volumeID := "rg#accountname#containername"

_, _, _, _, authEnv, err := d.GetAuthEnv(context.TODO(), volumeID, "", attrib, secret) //nolint:dogsled
assert.NoError(t, err)

found := false
for _, env := range authEnv {
if env == "AZURE_STORAGE_IDENTITY_CLIENT_ID=kubelet-identity-client-id" {
found = true
break
}
}
assert.True(t, found, "Should default to kubelet identity when AzureStorageIdentityClientID is not specified")
}

func TestGetAuthEnvMSIWarningWhenNoIdentityAvailable(t *testing.T) {
d := NewFakeDriver()
d.cloud = &storage.AccountRepo{}
// UserAssignedIdentityID is empty — no kubelet identity available, falls back to IMDS

attrib := map[string]string{
containerNameField: "containername",
storageAccountField: "accountname",
storageAuthTypeField: storageAuthTypeMSI,
}
secret := map[string]string{
accountNameField: "accountname",
accountKeyField: "testkey",
}
volumeID := "rg#accountname#containername"

_, _, _, _, _, err := d.GetAuthEnv(context.TODO(), volumeID, "", attrib, secret) //nolint:dogsled
assert.NoError(t, err) // should not error, falls back to IMDS
}

Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new MSI defaulting behavior isn’t covered for two important inputs that occur in docs/config: (1) AzureStorageAuthType: MSI (uppercase), and (2) AzureStorageIdentityClientID present but empty/whitespace. Adding targeted unit tests for these cases will prevent regressions and will catch the current case-sensitivity/empty-value pitfalls.

Suggested change
func TestGetAuthEnvMSIDefaultsToKubeletIdentityWhenClientIDWhitespace(t *testing.T) {
d := NewFakeDriver()
d.cloud = &storage.AccountRepo{}
d.cloud.Config.AzureAuthConfig = azclient.AzureAuthConfig{
UserAssignedIdentityID: "kubelet-identity-client-id",
}
attrib := map[string]string{
containerNameField: "containername",
storageAccountField: "accountname",
storageAuthTypeField: storageAuthTypeMSI,
storageIdentityClientIDField: " ",
}
secret := map[string]string{
accountNameField: "accountname",
accountKeyField: "testkey",
}
volumeID := "rg#accountname#containername"
_, _, _, _, authEnv, err := d.GetAuthEnv(context.TODO(), volumeID, "", attrib, secret) //nolint:dogsled
assert.NoError(t, err)
found := false
for _, env := range authEnv {
if env == "AZURE_STORAGE_IDENTITY_CLIENT_ID=kubelet-identity-client-id" {
found = true
break
}
}
assert.True(t, found, "Should default to kubelet identity when AzureStorageIdentityClientID is empty or whitespace")
}

Copilot uses AI. Check for mistakes.
func TestGetAuthEnvMSIUppercase(t *testing.T) {
d := NewFakeDriver()
d.cloud = &storage.AccountRepo{}
d.cloud.Config.AzureAuthConfig = azclient.AzureAuthConfig{
UserAssignedIdentityID: "kubelet-identity-client-id",
}

attrib := map[string]string{
containerNameField: "containername",
storageAccountField: "accountname",
storageAuthTypeField: "MSI", // uppercase as commonly used in docs/examples
}
secret := map[string]string{
accountNameField: "accountname",
accountKeyField: "testkey",
}
volumeID := "rg#accountname#containername"

_, _, _, _, authEnv, err := d.GetAuthEnv(context.TODO(), volumeID, "", attrib, secret) //nolint:dogsled
assert.NoError(t, err)

found := false
for _, env := range authEnv {
if env == "AZURE_STORAGE_IDENTITY_CLIENT_ID=kubelet-identity-client-id" {
found = true
break
}
}
assert.True(t, found, "Should default to kubelet identity even with uppercase MSI")
}

func TestGetAuthEnvMSIEmptyClientIDFallsBackToKubelet(t *testing.T) {
d := NewFakeDriver()
d.cloud = &storage.AccountRepo{}
d.cloud.Config.AzureAuthConfig = azclient.AzureAuthConfig{
UserAssignedIdentityID: "kubelet-identity-client-id",
}

attrib := map[string]string{
containerNameField: "containername",
storageAccountField: "accountname",
storageAuthTypeField: storageAuthTypeMSI,
storageIdentityClientIDField: "", // empty value
}
secret := map[string]string{
accountNameField: "accountname",
accountKeyField: "testkey",
}
volumeID := "rg#accountname#containername"

_, _, _, _, authEnv, err := d.GetAuthEnv(context.TODO(), volumeID, "", attrib, secret) //nolint:dogsled
assert.NoError(t, err)

found := false
for _, env := range authEnv {
if env == "AZURE_STORAGE_IDENTITY_CLIENT_ID=kubelet-identity-client-id" {
found = true
break
}
}
assert.True(t, found, "Empty AzureStorageIdentityClientID should fall back to kubelet identity")
}

func TestIsValidSubscriptionID(t *testing.T) {
tests := []struct {
desc string
Expand Down
Loading