Skip to content
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.

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

Update AzureFile and CephFS to use MountSensitive #88684

Merged
merged 4 commits into from Mar 4, 2020

Conversation

saad-ali
Copy link
Member

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

  • Update the k8s.io/utils dependency to f9c14454073b to pick up Introduce parameter for sensitive mount options. utils#138
  • Modify the AzureFile volume plugin to use the new MountSensitive method to prevent logging of sensitive mount options.
  • Modify the CephFS volume plugin to use the new MountSensitive method to prevent logging of sensitive mount options.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

AzureFile and CephFS use new Mount library that prevents logging of sensitive mount options.

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


/assign @liggitt

/assign @andyzhangx
@andyzhangx can you please help me test this patch with AzureFile to ensure it doesn't break anything?

/assign @jsafrane
@jsafrane can you please help me test this patch with CephFS to ensure it doesn't break anything?

/sig storage
/priority important-soon

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 29, 2020
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 29, 2020
@saad-ali saad-ali changed the title Update mount lib Update AzureFile and CephFS to use MountSensitive Feb 29, 2020
@k8s-ci-robot k8s-ci-robot added area/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes area/kubectl sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. labels Feb 29, 2020
@liggitt
Copy link
Member

liggitt commented Feb 29, 2020

Looks like a compile error in mount_windows :-/

Looks like we need cross-platform typecheck CI in the utils repo

@liggitt
Copy link
Member

liggitt commented Feb 29, 2020

mechanics of the bump lgtm, once the error is fixed and the functionality is verified by Jan and Andy

@andyzhangx
Copy link
Member

thanks @saad-ali
there is an e2e test for azure file: pull-kubernetes-e2e-azure-file, let's see whether it could pass.

@saad-ali
Copy link
Member Author

@liggitt kubernetes/utils#143 to fix the build issue.

@andyzhangx Thanks.

@jsafrane
Copy link
Member

jsafrane commented Mar 2, 2020

ceph changes lgtm

@saad-ali
Copy link
Member Author

saad-ali commented Mar 3, 2020

Thanks @andyzhangx and @jsafrane.

Do either of you want to test the patch (against real AzureFile and Ceph) before merge?

@liggitt PTAL. Build issues are fixed. Note that I had to fix a unit test due to changes made in kubernetes/utils#134

@andyzhangx
Copy link
Member

Thanks @andyzhangx and @jsafrane.

Do either of you want to test the patch (against real AzureFile and Ceph) before merge?

@liggitt PTAL. Build issues are fixed. Note that I had to fix a unit test due to changes made in kubernetes/utils#134

despite e2e test of pull-kubernetes-e2e-azure-file pass and I also verified it works manually,
@saad-ali

Below are the kubelet logs showing that secret is masked, thanks for the effort!

mount_linux.go:146] Mounting cmd (systemd-run) with arguments (--description=Kubernetes transient mount for /var/lib/kubelet/pods/43920b5c-2950-47c9-82fb-04b2282e6d13/volumes/kubernetes.io~azure-file/pvc-c103bb36-1a0c-45f9-ae65-761732114eb0 --scope -- mount -t cifs -o cache=strict,dir_mode=0777,file_mode=0777,gid=0,mfsymlinks,uid=0,vers=3.0,<masked> //xxx.file.core.windows.net/andy-1180beta1x-dynami-pvc-c103bb36-1a0c-45f9-ae65-761732114eb0 /var/lib/kubelet/pods/43920b5c-2950-47c9-82fb-04b2282e6d13/volumes/kubernetes.io~azure-file/pvc-c103bb36-1a0c-45f9-ae65-761732114eb0)

@liggitt
Copy link
Member

liggitt commented Mar 3, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, saad-ali

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2020
@jsafrane
Copy link
Member

jsafrane commented Mar 3, 2020

lgtm-ish, please rebase

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2020
@saad-ali
Copy link
Member Author

saad-ali commented Mar 3, 2020

Thank you for testing it @andyzhangx

Rebased. PTAL

@saad-ali
Copy link
Member Author

saad-ali commented Mar 3, 2020

/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-gce-device-plugin-gpu
/test pull-kubernetes-bazel-build
/test pull-kubernetes-e2e-azure-disk
/test pull-kubernetes-e2e-gce-csi-serial

@saad-ali
Copy link
Member Author

saad-ali commented Mar 4, 2020

/test pull-kubernetes-e2e-gce-csi-serial

Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit cd23e78 into kubernetes:master Mar 4, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Mar 4, 2020
@liggitt
Copy link
Member

liggitt commented Mar 5, 2020

Thanks. Is this pickable to 1.15/1.16/1.17?

@liggitt
Copy link
Member

liggitt commented Mar 17, 2020

Is this pickable to 1.15/1.16/1.17?

cc @enj

@jmcmeek
Copy link
Contributor

jmcmeek commented Aug 13, 2020

Is this going to be cherry picked to 1.16? I see this PR referenced wrt CVE-2019-11252

@liggitt
Copy link
Member

liggitt commented Aug 13, 2020

no, see discussion in #89494 (comment) (which was against 1.17... the changes to backport to 1.16 were even larger)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants