AzureFile: Volume without secretNamespace fails to mount after translating to CSI #108000

RomanBednar · 2022-02-08T09:04:59Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR fixes a bug which can impact users upon enabling CSI migration. Without the bug fix all pods that use claims which reference an existing InTree PV without namespace defined will cause existing pods to get stuck in ContainerCreating status right after enabling CSI migration.

Which issue(s) this PR fixes:

Volume without pv.spec.azureFile.secretNamespace could not be mounted due to the secret not found causing pod to fail after CSI migration is enabled.

Special notes for your reviewer:

See verification steps below.

Does this PR introduce a user-facing change?

Existing InTree AzureFile PVs which don't have a secret namespace defined will now work properly after enabling CSI migration - the namespace will be obtained from ClaimRef.

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

I verified the patch using the following procedure:

1. created new PV object referencing existing volume (share) while removing secret namespace - after creation null value is assigned

$ oc get pv/pvc-640b7456-1b41-490b-b2e5-933076d916e4-no-namespace -o json | jq '.spec.azureFile.secretNamespace'
null

2. claim the PV created in previous step and verify ClaimRef was added to PVC

$ oc get pvc/claim-2
NAME      STATUS   VOLUME                                                  CAPACITY   ACCESS MODES   STORAGECLASS   AGE
claim-2   Bound    pvc-640b7456-1b41-490b-b2e5-933076d916e4-no-namespace   1Gi        RWO            azurefile      66s

$ oc get pv/pvc-640b7456-1b41-490b-b2e5-933076d916e4-no-namespace -o json | jq '.spec.claimRef.namespace'
"test-namespace"

3. create a pod using the claim and verify it's running

$ oc get pod/example -o json | jq '.spec.volumes[0].persistentVolumeClaim.claimName'
"claim-2"

$ oc get pod/example
NAME      READY   STATUS    RESTARTS   AGE
example   1/1     Running   0          83s

4. enable CSI migration

$ oc patch featuregate cluster -p '{"spec": {"featureSet": "TechPreviewNoUpgrade"}}' --type merge
featuregate.config.openshift.io/cluster patched

5. wait for featuregate to enable and verify pod is still running

$ oc get pod/example
NAME      READY   STATUS    RESTARTS   AGE
example   1/1     Running   0          8m39s

Without the patch the pod gets stuck in "ContainerCreatinng" at this point:

$ oc get pod mydeploy-manual-x8gfe-69f988b468-v9l5w
NAME                                     READY   STATUS              RESTARTS   AGE
mydeploy-manual-x8gfe-69f988b468-v9l5w   0/1     ContainerCreating   0          4h5m

k8s-ci-robot · 2022-02-08T09:05:07Z

Welcome @RomanBednar!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-02-08T09:05:07Z

Hi @RomanBednar. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 kubernetes/test-infra repository.

andyzhangx · 2022-02-08T12:35:44Z

for PV, if secretNamespace is not defined, it should always use default namespace. Here is the original logic:

kubernetes/pkg/volume/azure_file/azure_file.go

Lines 373 to 394 in 5340ae0

    
           func getSecretNameAndNamespace(spec *volume.Spec, defaultNamespace string) (string, string, error) { 
        
           	secretName := "" 
        
           	secretNamespace := "" 
        
           	if spec.Volume != nil && spec.Volume.AzureFile != nil { 
        
           		secretName = spec.Volume.AzureFile.SecretName 
        
           		secretNamespace = defaultNamespace 
        
           	} else if spec.PersistentVolume != nil && 
        
           		spec.PersistentVolume.Spec.AzureFile != nil { 
        
           		secretNamespace = defaultNamespace 
        
           		if spec.PersistentVolume.Spec.AzureFile.SecretNamespace != nil { 
        
           			secretNamespace = *spec.PersistentVolume.Spec.AzureFile.SecretNamespace 
        
           		} 
        
           		secretName = spec.PersistentVolume.Spec.AzureFile.SecretName 
        
           	} else { 
        
           		return "", "", fmt.Errorf("Spec does not reference an AzureFile volume type") 
        
           	} 
        
           	if len(secretNamespace) == 0 { 
        
           		return "", "", fmt.Errorf("invalid Azure volume: nil namespace") 
        
           	} 
        
           	return secretName, secretNamespace, nil

What's your k8s version?

seems I was wrong, defaultNamespace is pod.Namespace here.

andyzhangx

/ok-to-test

andyzhangx · 2022-02-08T13:56:43Z

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

jsafrane · 2022-02-08T15:35:41Z

for PV, if secretNamespace is not defined, it should always use default namespace.

No, it does not use namespace named "default". It uses defaultNamespace parameter, which is namespace of the pod:

kubernetes/pkg/volume/azure_file/azure_file.go

Line 120 in fdb6e2b

    
           secretName, secretNamespace, err := getSecretNameAndNamespace(spec, pod.Namespace)

kubernetes/pkg/volume/azure_file/azure_file.go

Line 178 in fdb6e2b

    
           secretName, secretNamespace, err := getSecretNameAndNamespace(spec, spec.PersistentVolume.Spec.ClaimRef.Namespace)

RomanBednar · 2022-02-08T15:49:57Z

seems I was wrong, defaultNamespace is pod.Namespace here.

exactly, and when the translation happens at a point where pod does not exist yet - it makes sense to try get the namespace from ClaimRef I believe. Maybe I should have also mentioned that the issue occurs only when using a PV without namespace while scheduling a pod in non-default namespace.

andyzhangx

/lgtm

andyzhangx · 2022-02-09T03:21:02Z

/retest

andyzhangx · 2022-02-17T12:11:02Z

/retest

andyzhangx · 2022-02-18T01:53:16Z

/retest

andyzhangx · 2022-02-21T03:40:11Z

retest

andyzhangx · 2022-02-21T03:40:19Z

/retest

andyzhangx · 2022-02-21T07:59:01Z

/retest

andyzhangx · 2022-02-21T08:47:29Z

@jsturtevant @CecileRobertMichon azure tests are still failing now.

andyzhangx · 2022-02-23T12:16:39Z

/retest

msau42 · 2022-02-23T16:49:06Z

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

@@ -129,9 +129,17 @@ func (t *azureFileCSITranslator) TranslateInTreePVToCSI(pv *v1.PersistentVolume)
 			resourceGroup = v
 		}
 	}
+
+	// Secret is required when mounting a volume but pod presence cannot be assumed - we should not try to read pod now.
 	namespace := defaultSecretNamespace


Can you remove this default line? It doesn't look like there is a path for intree PV to use the "default" namespace.

Correct, intree does not use "default". So to keep it consistent we should return an error here as well if the namespace can not be found. See if the latest version of the patch would be ok.

what if pv.Spec.ClaimRef.Namespace is empty? I think it's better not return error.

In CSI driver emtpy equals default as you mentioned previously, and I think Michelle wants to prevent using default as this might point to a secret in different namespace. So I think the only option here is to return error because we can't use any reasonable default.

Also, how possible is it to have pv.Spec.ClaimRef.Namespace empty? It should not be possible to use/mount a PV without PVC correct? So ideally the error should never be returned, but if it ever happens at least we know it's not possible to continue.

And if we don't want to return an error, what should the namespace be set to? We can't leave it empty due to CSI driver interpreting it as default, we can't set it to nil because that might break volumeID now (namespace was added recently #107575)), and we can't just leave it out because pv would never mount (without a secret).

In theory, for all PV operations PV.claimRef must be set (i.e. a PV must be bound).

Attach/detach/mount/unmount work with PVCs, i.e. the PV they get must be bound.
Provisioner creates PVs with claimRef populated.
Deletetion works when a PV has a claimRef set and the referenced PVC does not exist.

PVs without a claimRef must have been created manually and are not bound. Does Kubernetes do anything witch such volumes that needs translation? Did I miss any exotic error case?

andyzhangx · 2022-02-24T00:08:17Z

/retest

When translating InTree pv to CSI pv we use default secret namespace when it's not found in the InTree pv. Using the default is not ideal for several reasons: 1) it can result in failed pod creation after users migrate to cluster with CSI enabled because the existing intree pvs might not have the namespace defined. In that case the "default" is used and mount fails because secret could not be found. 2) falling back to "default" namespace can result in referencing a secret from different namespace which is a security risk However, there is another object we can use to determine correct namespace which presence can be safely assumed - ClaimRef. Mounting a volume is done only through a PVC which is bound. Binding adds ClaimRef to PV and finally the volume gets mounted which is where the translation code is used.

This is the actual fix - attempt to obtain a namespace from ClaimRef. Or fail if namespace could not be found instead of using "default".

andyzhangx · 2022-02-25T04:54:31Z

/retest

andyzhangx · 2022-03-07T11:32:52Z

@msau42 could you approve this PR? Thanks.

msau42 · 2022-03-09T04:57:28Z

/lgtm
/approve

Thanks!

k8s-ci-robot · 2022-03-09T04:58:05Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

…108000-upstream-release-1.22 Automated cherry pick of #108000: azure_file: add namespace tests for InTree to CSI

…108000-upstream-release-1.23 Automated cherry pick of #108000: azure_file: add namespace tests for InTree to CSI

…108000-upstream-release-1.21 Automated cherry pick of #108000: azure_file: add namespace tests for InTree to CSI

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 8, 2022

k8s-ci-robot requested review from andyzhangx and saad-ali February 8, 2022 09:06

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 8, 2022

andyzhangx reviewed Feb 8, 2022

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 8, 2022

andyzhangx approved these changes Feb 9, 2022

View reviewed changes

k8s-ci-robot assigned andyzhangx Feb 9, 2022

RomanBednar force-pushed the af-namespace-fix branch from cf4a37f to eebf761 Compare February 22, 2022 10:05

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2022

msau42 reviewed Feb 23, 2022

View reviewed changes

RomanBednar added 2 commits February 24, 2022 10:59

azure_file: try to get secret namespace from ClaimRef

507c63c

This is the actual fix - attempt to obtain a namespace from ClaimRef. Or fail if namespace could not be found instead of using "default".

RomanBednar force-pushed the af-namespace-fix branch from eebf761 to 507c63c Compare February 24, 2022 10:00

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2022

k8s-ci-robot merged commit bbc2dbb into kubernetes:master Mar 9, 2022

k8s-ci-robot added this to the v1.24 milestone Mar 9, 2022

andyzhangx mentioned this pull request Mar 29, 2022

Mount volume failed due to wrong namespace when looking for secret key Azure/AKS#2871

Closed

k8s-ci-robot added a commit that referenced this pull request Apr 26, 2022

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

12f1e51

…108000-upstream-release-1.22 Automated cherry pick of #108000: azure_file: add namespace tests for InTree to CSI

k8s-ci-robot added a commit that referenced this pull request Apr 26, 2022

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

d962aaf

…108000-upstream-release-1.23 Automated cherry pick of #108000: azure_file: add namespace tests for InTree to CSI

k8s-ci-robot added a commit that referenced this pull request Apr 26, 2022

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

29b6f31

…108000-upstream-release-1.21 Automated cherry pick of #108000: azure_file: add namespace tests for InTree to CSI

RomanBednar mentioned this pull request May 6, 2022

REQUEST: New membership for RomanBednar kubernetes/org#3410

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AzureFile: Volume without secretNamespace fails to mount after translating to CSI #108000

AzureFile: Volume without secretNamespace fails to mount after translating to CSI #108000

RomanBednar commented Feb 8, 2022

k8s-ci-robot commented Feb 8, 2022

k8s-ci-robot commented Feb 8, 2022

andyzhangx commented Feb 8, 2022 •

edited

andyzhangx left a comment

andyzhangx commented Feb 8, 2022

jsafrane commented Feb 8, 2022

RomanBednar commented Feb 8, 2022

andyzhangx left a comment

andyzhangx commented Feb 9, 2022

andyzhangx commented Feb 17, 2022

andyzhangx commented Feb 18, 2022

andyzhangx commented Feb 21, 2022

andyzhangx commented Feb 21, 2022

andyzhangx commented Feb 21, 2022

andyzhangx commented Feb 21, 2022

andyzhangx commented Feb 23, 2022

msau42 Feb 23, 2022

RomanBednar Feb 24, 2022

andyzhangx Feb 24, 2022

RomanBednar Feb 24, 2022

jsafrane Feb 25, 2022

andyzhangx commented Feb 24, 2022

andyzhangx commented Feb 25, 2022

andyzhangx commented Mar 7, 2022

msau42 commented Mar 9, 2022

k8s-ci-robot commented Mar 9, 2022

Navigation Menu

AzureFile: Volume without secretNamespace fails to mount after translating to CSI #108000

AzureFile: Volume without secretNamespace fails to mount after translating to CSI #108000

Conversation

RomanBednar commented Feb 8, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Feb 8, 2022

k8s-ci-robot commented Feb 8, 2022

andyzhangx commented Feb 8, 2022 • edited

andyzhangx left a comment

Choose a reason for hiding this comment

andyzhangx commented Feb 8, 2022

jsafrane commented Feb 8, 2022

RomanBednar commented Feb 8, 2022

andyzhangx left a comment

Choose a reason for hiding this comment

andyzhangx commented Feb 9, 2022

andyzhangx commented Feb 17, 2022

andyzhangx commented Feb 18, 2022

andyzhangx commented Feb 21, 2022

andyzhangx commented Feb 21, 2022

andyzhangx commented Feb 21, 2022

andyzhangx commented Feb 21, 2022

andyzhangx commented Feb 23, 2022

msau42 Feb 23, 2022

Choose a reason for hiding this comment

RomanBednar Feb 24, 2022

Choose a reason for hiding this comment

andyzhangx Feb 24, 2022

Choose a reason for hiding this comment

RomanBednar Feb 24, 2022

Choose a reason for hiding this comment

jsafrane Feb 25, 2022

Choose a reason for hiding this comment

andyzhangx commented Feb 24, 2022

andyzhangx commented Feb 25, 2022

andyzhangx commented Mar 7, 2022

msau42 commented Mar 9, 2022

k8s-ci-robot commented Mar 9, 2022

andyzhangx commented Feb 8, 2022 •

edited