Serial mkfs #1169

artemvmin · 2023-03-08T00:23:01Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Add concurrency limit to prevent OOMing when issuing multiple concurrent mkfs calls

Which issue(s) this PR fixes:
Fixes #1168

Does this PR introduce a user-facing change?:

Fixes CVE-2022-41723

k8s-ci-robot · 2023-03-08T00:23:10Z

Hi @artemvmin. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

mattcary · 2023-03-08T00:35:50Z

/ok-to-test
/lgtm
/approve

k8s-ci-robot · 2023-03-08T00:36:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: artemvmin, mattcary

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:

~~OWNERS~~ [mattcary]

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

artemvmin · 2023-03-08T22:29:42Z

/retest

artemvmin · 2023-03-09T00:38:38Z

/retest

artemvmin · 2023-03-09T19:32:27Z

/assign @msau42

msau42 · 2023-03-09T20:09:58Z

go.mod

 	gopkg.in/gcfg.v1 v1.2.3
+	k8s.io/api v0.24.1
 	k8s.io/apimachinery v0.24.1
 	k8s.io/client-go v11.0.1-0.20190805182717-6502b5e7b1b5+incompatible


You may be able to get rid of the replace statement if you put 0.24.1 here

I already tried that and wrestled with this for a while:

$> go mod tidy go: finding module for package k8s.io/api/settings/v1alpha1 go: finding module for package k8s.io/api/batch/v2alpha1 go: finding module for package github.com/googleapis/gnostic/OpenAPIv2 go: finding module for package k8s.io/api/auditregistration/v1alpha1 sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/test/k8s-integration imports k8s.io/client-go/kubernetes imports k8s.io/client-go/discovery imports github.com/googleapis/gnostic/OpenAPIv2: module github.com/googleapis/gnostic@latest found (v0.6.9), but does not contain package github.com/googleapis/gnostic/OpenAPIv2 sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/test/k8s-integration imports k8s.io/client-go/kubernetes imports k8s.io/client-go/kubernetes/typed/auditregistration/v1alpha1 imports k8s.io/api/auditregistration/v1alpha1: module k8s.io/api@latest found (v0.26.2), but does not contain package k8s.io/api/auditregistration/v1alpha1 sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/test/k8s-integration imports k8s.io/client-go/kubernetes imports k8s.io/client-go/kubernetes/typed/batch/v2alpha1 imports k8s.io/api/batch/v2alpha1: module k8s.io/api@latest found (v0.26.2), but does not contain package k8s.io/api/batch/v2alpha1 sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/test/k8s-integration imports k8s.io/client-go/kubernetes imports k8s.io/client-go/kubernetes/typed/settings/v1alpha1 imports k8s.io/api/settings/v1alpha1: module k8s.io/api@latest found (v0.26.2), but does not contain package k8s.io/api/settings/v1alpha1

I believe that means that some of our dependencies need us to specify the version via replace.

Looks like v11.0.1-0.20190805182717-6502b5e7b1b5+incompatible is arbitrary and doesn't show up in our go.sum file. Pulling the latest version is just a way for go mod tidy to satisfy the require directive syntax.

msau42 · 2023-03-09T20:20:29Z

We need to be careful with updating compute-gen.go.

We had to manually update it for hyperdisk work https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/commits/master/vendor/google.golang.org/api/compute cc @sunnylovestiramisu

sunnylovestiramisu · 2023-03-09T21:29:44Z

Please do not update the compute-gen.go via go mod tidy/vendor. It will remove the hyperdisk feature support. :(

artemvmin · 2023-03-09T21:30:07Z

pkg/mount-manager/safe-mounter-v1_windows.go

@@ -337,3 +337,17 @@ func (mounter *CSIProxyMounterV1) MountSensitiveWithoutSystemd(source string, ta
 func (mounter *CSIProxyMounterV1) MountSensitiveWithoutSystemdWithMountFlags(source string, target string, fstype string, options []string, sensitiveOptions []string, mountFlags []string) error {
 	return errors.New("MountSensitiveWithoutSystemd is not implemented")
 }
+
+// CanSafelySkipMountPointCheck always returns false on Windows.


@msau42 I had to add this because of two breaking changes that updated the interface. Could you confirm that it's safe to use the upstream implementation?

https://github.com/kubernetes/mount-utils/blob/master/mount_windows.go#L247-L259

Until we switch to Windows host process #1071, we may not be able to do many mount operations directly. @mauriciopoppe can you confirm?

This file only uses the following exported properties from k8s.io/mount-utils:

mount.NormalizeWindowsPath (method)

mount.MountPoint (struct)

None of these paths use these two methods. I believe it's safe to use the upstream implementation, in Windows we're not doing mount directly yet because #1071 isn't merged, if #1071 is merged these two paths won't be used still.

lgtm

sunnylovestiramisu · 2023-03-09T21:34:14Z

vendor/modules.txt

@@ -261,10 +266,9 @@ go.opencensus.io/trace/tracestate
 # go4.org v0.0.0-20201209231011-d4a079459e60
 ## explicit; go 1.13
 go4.org/bytereplacer
-# golang.org/x/net v0.5.0
+# golang.org/x/net v0.7.0


Does this also fix the CVE-2022-41723? It needs golang.org/x/net update.

If yes, please add this CVE fix information to the release note.

Done! Nice catch.

artemvmin · 2023-03-09T21:38:50Z

Please do not update the compute-gen.go via go mod tidy/vendor. It will remove the hyperdisk feature support. :(

@sunnylovestiramisu Could you please elaborate? I don't see how this would break hyperdisk. All e2e tests are passing.

I have also confirmed that the generated compute-gen.go is the 2023 version, which includes Disk.ProvisionedThroughput.

We also shouldn't be manually replacing vendor files. The desired effect in af3994a can be achieved by updating https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/master/go.mod#L6 to a newer version. Was the new version not available when we were doing hyperdisk integration?

mauriciopoppe

lgtm for Windows

mauriciopoppe · 2023-03-10T01:22:52Z

pkg/mount-manager/safe-mounter-v1_windows.go

@@ -337,3 +337,17 @@ func (mounter *CSIProxyMounterV1) MountSensitiveWithoutSystemd(source string, ta
 func (mounter *CSIProxyMounterV1) MountSensitiveWithoutSystemdWithMountFlags(source string, target string, fstype string, options []string, sensitiveOptions []string, mountFlags []string) error {
 	return errors.New("MountSensitiveWithoutSystemd is not implemented")
 }
+
+// CanSafelySkipMountPointCheck always returns false on Windows.


This file only uses the following exported properties from k8s.io/mount-utils:

mount.NormalizeWindowsPath (method)

mount.MountPoint (struct)

None of these paths use these two methods. I believe it's safe to use the upstream implementation, in Windows we're not doing mount directly yet because #1071 isn't merged, if #1071 is merged these two paths won't be used still.

lgtm

mauriciopoppe · 2023-03-10T01:25:48Z

Please do not update the compute-gen.go via go mod tidy/vendor. It will remove the hyperdisk feature support. :(

Maybe we could write the patch to the directory vendor-patches/ to know that this has to be applied to vendor. A check in presubmit would be to have a clean download of vendor/ + applying vendor-patches and this could be diffed with the presubmit vendor/.

sunnylovestiramisu · 2023-03-10T01:57:10Z

Confirmed with the PD team that the current beta model has the hyperdisk fields

/lgtm

This is partial cherry-pick of upstream PR kubernetes-sigs#1169 to update only golang.org/x/net and related packages to version from the PR.

k8s-ci-robot requested review from mattcary and saikat-royc March 8, 2023 00:23

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 8, 2023

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 Mar 8, 2023

k8s-ci-robot assigned mattcary Mar 8, 2023

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

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

artemvmin force-pushed the serial-mkfs branch from 74f05c3 to b162acd Compare March 8, 2023 19:14

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2023

Break dependency on k/k

1b6e7e5

artemvmin force-pushed the serial-mkfs branch from b162acd to 78b4f7d Compare March 8, 2023 22:23

artemvmin force-pushed the serial-mkfs branch from 78b4f7d to 9ce7231 Compare March 8, 2023 22:48

Upgrade mount-utils to v0.27.0

232bd0a

artemvmin force-pushed the serial-mkfs branch from 9ce7231 to 7fa4acd Compare March 8, 2023 23:16

Set mount concurrency to 1

7f1e04e

artemvmin force-pushed the serial-mkfs branch from 7fa4acd to 7f1e04e Compare March 9, 2023 18:52

k8s-ci-robot assigned msau42 Mar 9, 2023

msau42 reviewed Mar 9, 2023

View reviewed changes

artemvmin commented Mar 9, 2023

View reviewed changes

sunnylovestiramisu reviewed Mar 9, 2023

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 9, 2023

mauriciopoppe reviewed Mar 10, 2023

View reviewed changes

k8s-ci-robot assigned sunnylovestiramisu Mar 10, 2023

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

k8s-ci-robot merged commit 9845c11 into kubernetes-sigs:master Mar 10, 2023

artemvmin deleted the serial-mkfs branch March 10, 2023 18:48

amacaskill mentioned this pull request Apr 11, 2023

Automated cherry pick of #1169: Break dependency on k/k #1183

Closed

mattcary mentioned this pull request Jul 18, 2023

Add option to serialize formatAndMount #1294

Merged

jsafrane mentioned this pull request Jul 18, 2023

OCPBUGS-16331: 4.13: UPSTREAM: 1169: Fix CVE-2022-41723 openshift/gcp-pd-csi-driver#38

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serial mkfs #1169

Serial mkfs #1169

artemvmin commented Mar 8, 2023 •

edited

k8s-ci-robot commented Mar 8, 2023

mattcary commented Mar 8, 2023

k8s-ci-robot commented Mar 8, 2023

artemvmin commented Mar 8, 2023

artemvmin commented Mar 9, 2023

artemvmin commented Mar 9, 2023

msau42 Mar 9, 2023

artemvmin Mar 9, 2023

artemvmin Mar 9, 2023

artemvmin Mar 9, 2023

msau42 commented Mar 9, 2023

sunnylovestiramisu commented Mar 9, 2023

artemvmin Mar 9, 2023

msau42 Mar 10, 2023

mauriciopoppe Mar 10, 2023

sunnylovestiramisu Mar 9, 2023

artemvmin Mar 9, 2023

artemvmin commented Mar 9, 2023 •

edited

mauriciopoppe left a comment

mauriciopoppe Mar 10, 2023

mauriciopoppe commented Mar 10, 2023

sunnylovestiramisu commented Mar 10, 2023

Serial mkfs #1169

Serial mkfs #1169

Conversation

artemvmin commented Mar 8, 2023 • edited

k8s-ci-robot commented Mar 8, 2023

mattcary commented Mar 8, 2023

k8s-ci-robot commented Mar 8, 2023

artemvmin commented Mar 8, 2023

artemvmin commented Mar 9, 2023

artemvmin commented Mar 9, 2023

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

msau42 commented Mar 9, 2023

sunnylovestiramisu commented Mar 9, 2023

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

artemvmin commented Mar 9, 2023 • edited

mauriciopoppe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciopoppe commented Mar 10, 2023

sunnylovestiramisu commented Mar 10, 2023

artemvmin commented Mar 8, 2023 •

edited

artemvmin commented Mar 9, 2023 •

edited