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

Removed awk from kubeadm reset #81494

Merged
merged 1 commit into from Aug 21, 2019

Conversation

@Klaven
Copy link
Contributor

commented Aug 16, 2019

removed awk usage from kubeadm reset in favor of native golang calls
that are not vulnerable to expansion.

What type of PR is this?

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

/kind api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
This PR was created based on results from the security assessment,

Which issue(s) this PR fixes:

Fixes #
xref#/kubernetes/kubeadm/issues/1715

Special notes for your reviewer:

@neolit123 :
Wanted to get feedback. in it's current state it's not very testable. But at the same time I did not want to pollute the core change with changes not related to removing awk.

also, I would not expect a function called absoluteKubeletRunDirectory to unmount directories. but again thought this change would pollute the core change. If it is acceptable I will make a new PR later this week to address this.

Does this PR introduce a user-facing change?:

kubeadm reset: unmount directories under "/var/lib/kubelet" for linux only

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


@Klaven Klaven changed the title removed awk from kubeadm reset [WIP] removed awk from kubeadm reset Aug 16, 2019

@Klaven Klaven changed the title [WIP] removed awk from kubeadm reset Removed awk from kubeadm reset Aug 16, 2019

@Klaven Klaven force-pushed the Klaven:remove_awk branch from a0a005e to d0a5b45 Aug 16, 2019

@Klaven Klaven changed the title Removed awk from kubeadm reset [WIP] Removed awk from kubeadm reset Aug 16, 2019

@Klaven

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

Added WIP due to testing needing to be run on at least one of the other platforms. I should be able to do this over the weekend.

Tested on Ubuntu 18.04 server vm and successfully unmounted correct directories.

@Klaven

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

/test pull-kubernetes-e2e-gce

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Added WIP due to testing needing to be run on at least one of the other platforms. I should be able to do this over the weekend.

i think it's fine to merge without testing !linux as its just a NOOP there.

Does this PR introduce a user-facing change?:
perhaps we should add a release note for this one:

kubeadm: unmount directories under "/var/lib/kubelet" for linux only

thanks.

/approve
/priority backlog
/assign @rosti
PTAL too.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Klaven, neolit123

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

@Klaven Klaven changed the title [WIP] Removed awk from kubeadm reset Removed awk from kubeadm reset Aug 17, 2019

@rosti
Copy link
Member

left a comment

Thanks @Klaven !
Overall, this looks OK, but we may need to think on better platform split for reset (WRT the Windows side of things).

if len(m) < 2 || !strings.HasPrefix(m[1], kubeadmconstants.KubeletRunDirectory) {
continue
}
if err := syscall.Unmount(m[1], syscall.MNT_FORCE); err != nil {

This comment has been minimized.

Copy link
@rosti

rosti Aug 19, 2019

Member

The original call did not imply MNT_FORCE. Also, that flag is somewhat misleading. It is actually used to force unmount a filesystem, which is a remote one and the connection to the server is lost. It does not force unmount a filesystem if it has opened handles.

This comment has been minimized.

Copy link
@Klaven

Klaven Aug 19, 2019

Author Contributor

I can remove this if you like. will have to be tonight as it will mean I need to retest it.

This comment has been minimized.

Copy link
@Klaven

Klaven Aug 20, 2019

Author Contributor

this has been removed and tested. (testing methodology posted.) I agree, it's misleading and really not helpful in this case. Also not used on the original so even if we wanted to add it would probably be better to add in new PR.

@@ -0,0 +1,29 @@
// +build !linux

This comment has been minimized.

Copy link
@rosti

rosti Aug 19, 2019

Member

Let's not do premature platform splitting in reset at this point. It's almost all Linux dependent and we need to have the whole picture in mind to take the best approach.

This comment has been minimized.

Copy link
@neolit123

neolit123 Aug 19, 2019

Member

but this function certainly should not run on _windows.go (NO-OP).

This comment has been minimized.

Copy link
@Klaven

Klaven Aug 19, 2019

Author Contributor

@rosti I guess I don't understand, if I don't have this file the code will not pass checks because it does not have the functions used in the unmount_linux.go file on windows and mac. For both windows and mac this is a no-op. How are you wanting me to handle compiling for windows and linux if not with this file?

This comment has been minimized.

Copy link
@rosti

rosti Aug 20, 2019

Member

Ok, we can leave it like this for now, but we may need to redesign and rename the files.
The question is if we should place absoluteKubeletRunDirectory in the platform specific portions of cleanupnode.go? So the idea is to have cleanupnode_unix.go and cleanupnode_windows.go with absoluteKubeletRunDirectory and a few other things in it.

But, certainly, that must be done in another PR.

@Klaven

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

my test: (with removal of force)

>  sudo cat /proc/mounts | grep /var/lib/kubelet
/dev/loop5 /var/lib/kubelet ext4 rw,relatime,data=ordered 0 0

> sudo kubeadm reset
....

>  sudo cat /proc/mounts | grep /var/lib/kubelet
umountDirsCmd := fmt.Sprintf("awk '$2 ~ path {print $2}' path=%s/ /proc/mounts | xargs -r umount", absoluteKubeletRunDirectory)
klog.V(1).Infof("[reset] Executing command %q", umountDirsCmd)
umountOutputBytes, err := exec.Command("sh", "-c", umountDirsCmd).Output()
err = unmountKubeletDirectory()

This comment has been minimized.

Copy link
@rosti

rosti Aug 20, 2019

Member

A thing, that I've missed on the first run here is, that we need absoluteKubeletRunDirectory to be passed to unmountKubeletDirectory (instead of using a const there).

This comment has been minimized.

Copy link
@neolit123

@Klaven Klaven force-pushed the Klaven:remove_awk branch from 3d4cd2f to 4adf0c4 Aug 20, 2019

@rosti
rosti approved these changes Aug 20, 2019
Copy link
Member

left a comment

Thanks @Klaven !
Let's merge these as they are, but first, please, squash your commits.

removed awk from kubeadm reset
removed awk from kubeadm reset in favor of native go lang calls
that are not vulnerable to expantion.

@Klaven Klaven force-pushed the Klaven:remove_awk branch from 4adf0c4 to 6845c66 Aug 20, 2019

@Klaven

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@rosti
rosti approved these changes Aug 21, 2019
Copy link
Member

left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 21, 2019

@k8s-ci-robot k8s-ci-robot merged commit 17a1859 into kubernetes:master Aug 21, 2019

23 checks passed

cla/linuxfoundation Klaven authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.