fix azure file migration issue #97877

andyzhangx · 2021-01-09T13:40:44Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
fix azure file migration issue

Azure file CSI migration failed due to following error, that's because in Azure file CSI migration scenario, the namespace is set as nil, this PR set the PV namespace as default (by default), I have verified it works well in CSI migration test.

event for azurefile-volume-tester-x2clj: {kubelet k8s-agentpool1-42361127-1} FailedMount: MountVolume.MountDevice failed for volume "pvc-bb75bfa5-72ee-46ec-a770-6bb87580b151" : fetching NodeStageSecretRef default/azure-storage-account-fbf92849c9d7f4cb6ad5691-secret failed: kubernetes.io/csi: failed to find the secret azure-storage-account-fbf92849c9d7f4cb6ad5691-secret in the namespace default with error: secrets "azure-storage-account-fbf92849c9d7f4cb6ad5691-secret" is forbidden: User "system:node:k8s-agentpool1-42361127-1" cannot get resource "secrets" in API group "" in the namespace "default": no relationship found between node 'k8s-agentpool1-42361127-1' and this object

Which issue(s) this PR fixes:

Fixes #96508

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix azure file migration issue

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

fix azure file migration issue

/priority important-soon
/sig cloud-provider
/area provider/azure
/triage accepted

andyzhangx · 2021-01-09T15:13:45Z

/retest

andyzhangx · 2021-01-10T00:35:09Z

/retest

andyzhangx · 2021-01-10T01:28:02Z

/retest

msau42 · 2021-01-11T19:19:42Z

secret creation happens in CSI driver, kubelet does not set relationship between node and secret.

This seems like pretty powerful permissions for a per-node entity to have. Generally, these kinds of operations are delegated to admins or controllers that can be restricted to more secure nodes. Are those options not possible here?

@kubernetes/sig-storage-pr-reviews

Jiawei0227 · 2021-01-11T20:07:16Z

Hey Andy, I want to understand how does Azurefile uses secrets. From the https://github.com/kubernetes-sigs/azurefile-csi-driver/blob/master/deploy/v0.10.0/rbac-csi-azurefile-node.yaml driver deployment, it seems it only have get/list permission. And if I understand correctly, the secret permission is also optional because we can read the cloud-config directly from file. So we do not need secret permission in CSI driver in that case.

Is secret create a necessary only in CSI migration scenario?

andyzhangx · 2021-01-12T01:35:16Z

Hey Andy, I want to understand how does Azurefile uses secrets. From the https://github.com/kubernetes-sigs/azurefile-csi-driver/blob/master/deploy/v0.10.0/rbac-csi-azurefile-node.yaml driver deployment, it seems it only have get/list permission. And if I understand correctly, the secret permission is also optional because we can read the cloud-config directly from file. So we do not need secret permission in CSI driver in that case.

Is secret create a necessary only in CSI migration scenario?

For Azure File CSI driver: only controller has create permission on secrets, and node only has get/list permissions.
https://github.com/kubernetes-sigs/azurefile-csi-driver/blob/master/deploy/rbac-csi-azurefile-controller.yaml#L180-L181

@msau42 let's use this way to fix the secret authorizer issue in Azure File CSI migration, is that ok?

Jiawei0227 · 2021-01-12T01:37:45Z

For Azure File CSI driver: only controller has create permission on secrets, and node only has get/list permissions.
https://github.com/kubernetes-sigs/azurefile-csi-driver/blob/master/deploy/rbac-csi-azurefile-controller.yaml#L180-L181

Thanks for the clarification! I am wondering is the secret get/list permission also optional if CSI migration is not enabled? I thought secret is used only to get cloud-config. But apparently you can also use a json file instead of a secret?

msau42 · 2021-01-12T01:43:16Z

Does that mean the node driver needs permissions to be able to create secrets for migration?

Considering that csi migration is needed for all existing clusters/workloads, I want to make sure we're not requiring existing users to relax their security posture.

andyzhangx · 2021-01-12T01:46:32Z

For Azure File CSI driver: only controller has create permission on secrets, and node only has get/list permissions.
https://github.com/kubernetes-sigs/azurefile-csi-driver/blob/master/deploy/rbac-csi-azurefile-controller.yaml#L180-L181

Thanks for the clarification! I am wondering is the secret get/list permission also optional if CSI migration is not enabled? I thought secret is used only to get cloud-config. But apparently you can also use a json file instead of a secret?

@Jiawei0227 there are two requirements for secret permission:

get cloud config from secret(password needs to stored in secret)
get account key

if we don't store account key in secret, node should have azure identity to access Azure account key through Azure API, there is scenario that agent node does not have this identity(so we have storeAccountKey as optional, true by default), that is what in-tree azure file driver does now.

andyzhangx · 2021-01-12T01:47:49Z

Does that mean the node driver needs permissions to be able to create secrets for migration?

Considering that csi migration is needed for all existing clusters/workloads, I want to make sure we're not requiring existing users to relax their security posture.

@msau42 No, node daemonset only has get/list permissions, only controller has create permission.

msau42 · 2021-01-12T01:54:32Z

Ah ok, I'm misunderstanding this, can you explain it more detail when and where the secrets are created?

in Azure file CSI migration scenario, the secret creation happens in CSI driver

andyzhangx · 2021-01-12T01:57:55Z

Ah ok, I'm misunderstanding this, can you explain it more detail when and where the secrets are created?

in Azure file CSI migration scenario, the secret creation happens in CSI driver

@msau42 In CreateVolume func, CSI driver controller would create a file share first, and then store account key in secret, and later on, CSI driver node would get that secret to get account key and then mount file share. Actually this is CSI driver behavior, not related to CSI migration, this PR did not use NodeStageSecretRef in PV translation to avoid migration failure.

msau42 · 2021-01-12T21:00:50Z

staging/src/k8s.io/csi-translation-lib/plugins/azure_file.go

+						ReadOnly:     azureSource.ReadOnly,
+						VolumeAttributes: map[string]string{
+							azureFileShareName:   azureSource.ShareName,
+							secretNameField:      azureSource.SecretName,


This means that the CSI driver on the node needs permissions to read secrets (and it can read any secrets)?

For better security posture, we have the CSI NodeStageSecretRefs so that kubelet and node authorizer can control access to secrets for node entities. So I think it's still better that we figure out how to get node authorizer to support migration.

cc @kubernetes/sig-auth-pr-reviews

Azure File CSI driver already supports this(has read secrets permission already), so shall we merge this PR first?

This PR just does the in-tree PV translation, I think we could go on this PR first, and then we can use #96508 to track the NodeStageSecretRef permission issue.

I don't think it's a good security practice to give the csi node driver secret read permission, and from a CSI migration perspective is a regression of security requirements on the node (in-tree plugin didn't need broad secret read permissions).

So I think we should work on fixing the CSI driver first and doing the right thing for csi migration.

secret read permission is not required in Azure File CSI driver:
if it cannot read secret, node should have azure identity to access Azure account key through Azure API, and then it would leverage azure identity to read account key directly.

This PR just does the in-tree PV translation, it's up to CSI driver config that whether it has secret read permission or not on node.

I'm actually wondering about Azure API security vs K8s node authorizer. The k8s node authorizer is able to restrict secret access to only pods scheduled to that node. Would Azure API be able to similarly restrict access if the node got compromised?

I suspect the issue with node authorizer is that it's looking for a real PV object that has the secret reference specified in it. Can we set the AzureFilePersistentVolumeSource.Secret when we create a volume for csi migration?

Yes, we could revoke azure identity from VM if that node got compromised.

There is already CSI --> in-tree PV translation in CSI migration, while it does not work:

kubernetes/staging/src/k8s.io/csi-translation-lib/plugins/azure_file.go

Lines 180 to 181 in a098b5b

azureSource.ShareName = fileShareName

azureSource.SecretName = fmt.Sprintf(secretNameTemplate, storageAccount)

I would suggest merge this fix first, and then check whether there is better way to leverage NodeStageSecretRef, that issue was opened more than 2 months ago, need to make it work first, thanks.

the azure volume plugins and CSI translation should use/populate the secret name/namespace fields in the API objects for anything they want the node to be authorized to fetch:

For pods:

kubernetes/pkg/api/v1/pod/util.go

Line 133 in d52fe32

if len(source.AzureFile.SecretName) > 0 && !visitor(source.AzureFile.SecretName) {

kubernetes/pkg/api/v1/pod/util.go

Line 177 in d52fe32

if source.CSI.NodePublishSecretRef != nil && !visitor(source.CSI.NodePublishSecretRef.Name) {

For persistent volumes:

kubernetes/pkg/api/v1/persistentvolume/util.go

Line 41 in d52fe32

if len(source.AzureFile.SecretName) > 0 && !visitor(*source.AzureFile.SecretNamespace, source.AzureFile.SecretName, true /* kubeletVisible */) {

kubernetes/pkg/api/v1/persistentvolume/util.go

Line 45 in d52fe32

if len(source.AzureFile.SecretName) > 0 && !visitor(getClaimRefNamespace(pv), source.AzureFile.SecretName, true /* kubeletVisible */) {

kubernetes/pkg/api/v1/persistentvolume/util.go

Line 129 in d52fe32

if !visitor(source.CSI.NodePublishSecretRef.Namespace, source.CSI.NodePublishSecretRef.Name, true /* kubeletVisible */) {

kubernetes/pkg/api/v1/persistentvolume/util.go

Line 134 in d52fe32

if !visitor(source.CSI.NodeStageSecretRef.Namespace, source.CSI.NodeStageSecretRef.Name, true /* kubeletVisible */) {

Using unstructured volumeattributes to plumb secret name/namespace references is not supported by the node authorizer

hi @liggitt, that's what we do in the beginning(without this PR), secretName and secretNamespace is already set (without this PR), while node authorizer does not work only in CSI migration scenario, detailed issue: #96508, this PR did a workaround, and works in CSI migration.

$ k get pv pvc-01bd1d0b-b965-4d37-8331-817a296c8998 -o yaml apiVersion: v1 kind: PersistentVolume metadata: annotations: kubernetes.io/azure-file-resource-group: andy-1191 pv.kubernetes.io/provisioned-by: file.csi.azure.com spec: accessModes: - ReadWriteMany azureFile: secretName: azure-storage-account-f97cecd00f4d4433ebd6809-secret secretNamespace: null shareName: pvc-01bd1d0b-b965-4d37-8331-817a296c8998 capacity: storage: 100Gi claimRef: apiVersion: v1 kind: PersistentVolumeClaim name: pvc-azurefile namespace: test persistentVolumeReclaimPolicy: Delete storageClassName: azurefile-456-kubernetes.io-azure-file-dynamic-sc-4hv88 volumeMode: Filesystem status: phase: Bound

The node authorizer only permits access to secrets in the pod's namespace based on volume definitions in the pod. That is the case for both azureFile and csi volume definitions in a pod.

It looks like the azurefile volume plugin looks in the default namespace for secrets referenced in a pod's azureFile volume. That means secret access was not permitted by the node authorizer even in non-CSI migration cases.

If non-CSI migration cases were working for pods outside the default namespace, something else must have been granting access to the node. The fix for the CSI-migration case would be to grant access the same way, not to stop using the API fields intended to communicate the name/namespace coordinates of secrets to use.

andyzhangx · 2021-01-21T12:20:38Z

/retest

msau42 · 2021-01-21T18:18:25Z

staging/src/k8s.io/csi-translation-lib/plugins/azure_file_test.go

-							SecretName: "azure-storage-account-st-secret",
-							ShareName:  "pvc-file-dynamic",
-							ReadOnly:   true,
+							SecretName:      "azure-storage-account-st-secret",


Thanks, could you also add unit tests where the share name gets parsed from the VolumeHandle?

msau42 · 2021-01-21T18:21:12Z

staging/src/k8s.io/csi-translation-lib/plugins/azure_file.go

@@ -163,31 +163,48 @@ func (t *azureFileCSITranslator) TranslateCSIPVToInTree(pv *v1.PersistentVolume)
 		ReadOnly: csiSource.ReadOnly,
 	}

+	for k, v := range csiSource.VolumeAttributes {
+		switch strings.ToLower(k) {
+		case shareNameField:


Similar question for shareName. Will it always be identifiable from the volumeHandle?

It would be simpler to eliminate duplicate fields. Then we don't need to think about cases like if the 2 fields have different values.

there is also cases there volumeHandle is arbitrary string, user input shareName by storage class or PV parameters, we also supports that. If no such input, will parse from volumeHandle

Can we consolidate on the volumeHandle format? For example, if the shareName is a storageclass parameter, then the provisioned PV object will have the shareName in the volumeHandle and not the attributes. And for a preprovisioned PV case, the share name has to be provided in the volumeHandle?

kubernetes-sigs/azurefile-csi-driver#536

refactor fix comments fix annoation nil map add ut

andyzhangx · 2021-01-22T10:54:38Z

/retest

andyzhangx · 2021-01-22T15:33:23Z

/retest

msau42 · 2021-01-22T21:37:54Z

/lgtm
/approve

Followup items would be to deprecate usage of duplicate fields in the CSI driver volumeAttributes. But for the purposes of migration, it needs to support both.

k8s-ci-robot · 2021-01-22T21:38:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, msau42

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/csi-translation-lib/OWNERS~~ [msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2021-01-23T00:40:47Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

andyzhangx · 2021-01-23T01:38:31Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

…7877-upstream-release-1.20 Automated cherry pick of #97877: fix azure file migration issue

…7877-upstream-release-1.18 Automated cherry pick of #97877: fix azure file migration issue

…7877-upstream-release-1.19 Automated cherry pick of #97877: fix azure file migration issue

liggitt · 2021-03-18T14:02:45Z

staging/src/k8s.io/csi-translation-lib/plugins/azure_file.go

 		resourceGroup = rg
 	}

+	if azureSource.SecretNamespace == nil {
+		ns := defaultSecretNamespace


this may need to be adjusted as part of undoing the regression in #97417

see #99061 (comment) for details

in csi migration translation, there is no way to get pod namespace, unless we address this, otherwise in CSI migration would break also. We are planning to turn on csi migration in 1.21, it will break in that scenario too.

The main issue is that original inline azure file volume does not have a secretNamespace field, while azure file persistent volume has that field.

the backwards compatibility issue has to be solved before that can be enabled

is it possible to add a new field(secretNamespace) in azure file inline volume, that's the best way I could see to solve this issue. In csi migration translation lib, there is no way to get pod namespace.

is it possible to add a new field(secretNamespace) in azure file inline volume, that's the best way I could see to solve this issue.

No, secret references in pod specs cannot reference cross-namespace secrets.

In csi migration translation lib, there is no way to get pod namespace.

That sounds like an issue that could be solved by plumbing the pod namespace through the go API.

@andyzhangx do you need the secret only for node operations or also attach? If only node, then plumbing in pod namespace should not be difficult:

kubernetes/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go

Line 556 in 54449be

volumeSpec, err = csimigration.TranslateInTreeSpecToCSI(volumeSpec, dswp.intreeToCSITranslator)

Plumbing it through attach will be more effort as we would need to add a field to the VolumeAttachment:

kubernetes/pkg/apis/storage/types.go

Line 164 in 54449be

InlineVolumeSpec *api.PersistentVolumeSpec

cc @Jiawei0227

hi @msau42 we need pod namespace in mount(node operation), expand & delete(controller operations), is that possible? thanks. #97417

Expand and delete operations are not supported for inline volumes.

k8s-ci-robot requested review from msau42 and saad-ali January 9, 2021 13:41

This was referenced Jan 9, 2021

CSI migration from in-tree driver to CSI driver go beta kubernetes-sigs/azurefile-csi-driver#115

Closed

mount failure in Azure File CSI migration #96508

Closed

andyzhangx mentioned this pull request Jan 11, 2021

[WIP]fix: azure file secret auth issue in csi migration #97714

Closed

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jan 11, 2021

msau42 reviewed Jan 12, 2021

View reviewed changes

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Jan 12, 2021

andyzhangx force-pushed the azurefile-migration-fix branch from dd22491 to ced88df Compare January 15, 2021 12:08

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 15, 2021

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 20, 2021

andyzhangx force-pushed the azurefile-migration-fix branch from 31dc489 to f63a875 Compare January 21, 2021 11:22

msau42 reviewed Jan 21, 2021

View reviewed changes

fix azure file migration issue

1df2fcf

refactor fix comments fix annoation nil map add ut

andyzhangx force-pushed the azurefile-migration-fix branch from f63a875 to 1df2fcf Compare January 22, 2021 09:01

k8s-ci-robot assigned msau42 Jan 22, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 22, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 22, 2021

msau42 mentioned this pull request Jan 23, 2021

Deprecate duplicate fields in volumeAttributes kubernetes-sigs/azurefile-csi-driver#536

Closed

k8s-ci-robot merged commit 194e519 into kubernetes:master Jan 23, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 23, 2021

This was referenced Jan 23, 2021

Automated cherry pick of #97877: fix azure file migration issue #98328

Merged

Automated cherry pick of #97877: fix azure file migration issue #98329

Merged

Automated cherry pick of #97877: fix azure file migration issue #98330

Merged

k8s-ci-robot added a commit that referenced this pull request Feb 4, 2021

Merge pull request #98328 from andyzhangx/automated-cherry-pick-of-#9…

47ea493

…7877-upstream-release-1.20 Automated cherry pick of #97877: fix azure file migration issue

k8s-ci-robot added a commit that referenced this pull request Feb 4, 2021

Merge pull request #98330 from andyzhangx/automated-cherry-pick-of-#9…

eb347c7

…7877-upstream-release-1.18 Automated cherry pick of #97877: fix azure file migration issue

k8s-ci-robot added a commit that referenced this pull request Feb 4, 2021

Merge pull request #98329 from andyzhangx/automated-cherry-pick-of-#9…

18f03ef

…7877-upstream-release-1.19 Automated cherry pick of #97877: fix azure file migration issue

andyzhangx mentioned this pull request Feb 4, 2021

azure file migration go beta #96293

Merged

liggitt reviewed Mar 18, 2021

View reviewed changes

andyzhangx mentioned this pull request Mar 29, 2021

After cluster upgrade from 1.18.9 to 1.19.7 azureFile volume secrets are searched in wrong namespace #99061

Closed

This was referenced Apr 13, 2021

test: add inline volume test kubernetes-sigs/azurefile-csi-driver#613

Merged

fix: azure file inline volume namespace issue in csi migration translation #101235

Merged

	azureSource.ShareName = fileShareName
	azureSource.SecretName = fmt.Sprintf(secretNameTemplate, storageAccount)

fix azure file migration issue #97877

fix azure file migration issue #97877

Conversation

andyzhangx commented Jan 9, 2021 • edited

andyzhangx commented Jan 9, 2021

andyzhangx commented Jan 10, 2021

andyzhangx commented Jan 10, 2021

msau42 commented Jan 11, 2021

Jiawei0227 commented Jan 11, 2021

andyzhangx commented Jan 12, 2021

Jiawei0227 commented Jan 12, 2021

msau42 commented Jan 12, 2021

andyzhangx commented Jan 12, 2021

andyzhangx commented Jan 12, 2021

msau42 commented Jan 12, 2021

andyzhangx commented Jan 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Jan 13, 2021 • edited

Choose a reason for hiding this comment

andyzhangx Jan 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx commented Jan 22, 2021

andyzhangx commented Jan 22, 2021

msau42 commented Jan 22, 2021

k8s-ci-robot commented Jan 22, 2021

fejta-bot commented Jan 23, 2021

andyzhangx commented Jan 23, 2021

Choose a reason for hiding this comment

andyzhangx Mar 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx Mar 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx commented Jan 9, 2021 •

edited

andyzhangx commented Jan 12, 2021 •

edited

liggitt Jan 13, 2021 •

edited

andyzhangx Jan 13, 2021 •

edited

andyzhangx Mar 18, 2021 •

edited

andyzhangx Mar 18, 2021 •

edited