-
Notifications
You must be signed in to change notification settings - Fork 62
Migrate to v2.0 to become a library #221
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
Migrate to v2.0 to become a library #221
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alexander-ding The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @alexander-ding! |
Hi @alexander-ding. Thanks for your PR. I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass review:
- Add a more descriptive comment to the PR explaining what were the steps that you took in each commit
- The PR is huge! I'd split it in different PRs, this is so that it's easier to review / traceback changes.
@@ -0,0 +1,238 @@ | |||
package disk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this file copied or moved? I think we should move files first tracking them with git and then make changes
SERVICE_STATUS_CONTINUE_PENDING = 5 | ||
SERVICE_STATUS_PAUSE_PENDING = 6 | ||
SERVICE_STATUS_PAUSED = 7 | ||
SERVICE_STATUS_UNKNOWN ServiceStatus = iota |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, thanks for this change
@@ -67,36 +64,34 @@ func TestServiceCommands(t *testing.T) { | |||
require.NoError(t, err, "failed unmarshalling json out=%v", out) | |||
|
|||
assert.Equal(t, serviceInfo.Status, uint32(response.Status)) | |||
assert.Equal(t, v1alpha1.ServiceStatus_STOPPED, response.Status) | |||
assert.Equal(t, system.SERVICE_STATUS_STOPPED, response.Status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I see that this was generated from protoc but replaced with a const that was already defined in pkg/system/types.go
integrationtests/volume_test.go
Outdated
|
||
// Empty volume test | ||
runNegativeIsVolumeFormattedRequest(t, client, "") | ||
// runNegativeIsVolumeFormattedRequest(t, client, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happened with these tests?
@alexander-ding: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
Looks like this not only enables using the CSI proxy as a library but removes the ability to run it as a gRPC server application? Please let me know if that's not true. Running the CSI Proxy separately from the CSI driver that uses it provides really valuable security isolation. Our current implementation for Windows SMB support (at NetApp) lets us have our Windows pods be unprivileged with no host access, leaving those privileges to the CSI Proxy. This greatly reduces the attack surface. |
You're right. Note that the existing client-server model will still be available on the I'll defer to @mauriciopoppe to chime in on the security considerations. |
@ameade Thanks for the feedback, CSI Proxy does provide format/mount isolation to some directories in the filesystem https://github.com/kubernetes-csi/csi-proxy/blob/v1.x/cmd/csi-proxy/main.go#L49 but with a few drawbacks.
In addition the current client/server model has some drawbacks, this is one of them:
By becoming similar to the Linux node component which runs as a privileged container I think we'll have a similar deployment for both OSs. At the same time, enabling HostProcess Pods is also a security concern as it provides no filesystem isolation, this is a point I'll cover in the KEP and strategies on how to use it in a cluster. Let's continue the discussion in the KEP. |
@ameade KEP is up in kubernetes/enhancements#3641, if you have further questions about the motivation, security or any other question please add it to the PR. |
@alexander-ding: PR needs rebase. 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. |
4133d1d Merge pull request kubernetes-csi#226 from msau42/cloudbuild 8d519d2 Pin buildkit to v0.10.6 to workaround v0.11 bug with docker manifest 6e04a03 Merge pull request kubernetes-csi#224 from msau42/cloudbuild 26fdfff Update cloudbuild image 6613c39 Merge pull request kubernetes-csi#223 from sunnylovestiramisu/update 0e7ae99 Update k8s image repo url 77e47cc Merge pull request kubernetes-csi#222 from xinydev/fix-dep-version 155854b Fix dep version mismatch 8f83905 Merge pull request kubernetes-csi#221 from sunnylovestiramisu/go-update 1d3f94d Update go version to 1.20 to match k/k v1.27 e322ce5 Merge pull request kubernetes-csi#220 from andyzhangx/fix-golint-error b74a512 test: fix golint error aa61bfd Merge pull request kubernetes-csi#218 from xing-yang/update_csi_driver 7563d19 Update CSI_PROW_DRIVER_VERSION to v1.11.0 a2171be Merge pull request kubernetes-csi#216 from msau42/process cb98782 Merge pull request kubernetes-csi#217 from msau42/owners a11216e add new reviewers and remove inactive reviewers dd98675 Add step for checking builds b66c082 Merge pull request kubernetes-csi#214 from pohly/junit-fixes b9b6763 filter-junit.go: fix loss of testcases when parsing Ginkgo v2 JUnit d427783 filter-junit.go: preserve system error log 38e1146 prow.sh: publish individual JUnit files as separate artifacts git-subtree-dir: release-tools git-subtree-split: 4133d1df083eaa65bdeddd0530d54278529c7a60
c10b678 Merge pull request kubernetes-csi#227 from coulof/check-sidecar-supported-versions b055535 Header bd0a10b typo c39d73c Add comments f6491af Script to verify EOL sidecar version 4133d1d Merge pull request kubernetes-csi#226 from msau42/cloudbuild 8d519d2 Pin buildkit to v0.10.6 to workaround v0.11 bug with docker manifest 6e04a03 Merge pull request kubernetes-csi#224 from msau42/cloudbuild 26fdfff Update cloudbuild image 6613c39 Merge pull request kubernetes-csi#223 from sunnylovestiramisu/update 0e7ae99 Update k8s image repo url 77e47cc Merge pull request kubernetes-csi#222 from xinydev/fix-dep-version 155854b Fix dep version mismatch 8f83905 Merge pull request kubernetes-csi#221 from sunnylovestiramisu/go-update 1d3f94d Update go version to 1.20 to match k/k v1.27 e322ce5 Merge pull request kubernetes-csi#220 from andyzhangx/fix-golint-error b74a512 test: fix golint error aa61bfd Merge pull request kubernetes-csi#218 from xing-yang/update_csi_driver 7563d19 Update CSI_PROW_DRIVER_VERSION to v1.11.0 a2171be Merge pull request kubernetes-csi#216 from msau42/process cb98782 Merge pull request kubernetes-csi#217 from msau42/owners a11216e add new reviewers and remove inactive reviewers dd98675 Add step for checking builds b66c082 Merge pull request kubernetes-csi#214 from pohly/junit-fixes b9b6763 filter-junit.go: fix loss of testcases when parsing Ginkgo v2 JUnit d427783 filter-junit.go: preserve system error log 38e1146 prow.sh: publish individual JUnit files as separate artifacts git-subtree-dir: release-tools git-subtree-split: c10b67804e07a324fe33595040afd13f020ee000
74e066a Merge pull request kubernetes-csi#279 from Aishwarya-Hebbar/update-csi-prow-version 6f236be Update CSI prow driver version to v1.17.0 0ee5589 Merge pull request kubernetes-csi#280 from xing-yang/update_go_1.24.4 9af1015 update to go 1.24.4 f5fec3e Merge pull request kubernetes-csi#275 from chrishenzie/emeritus c5d285d Remove chrishenzie from kubernetes-csi-reviewers 0a435bf Merge pull request kubernetes-csi#274 from andyzhangx/patch-5 cd7b4bb Bump golang to 1.24.2 to fix CVE-2025-22871 701dc34 Merge pull request kubernetes-csi#273 from andyzhangx/patch-4 aeebd30 Bump golang to 1.24.0 f277d56 Merge pull request kubernetes-csi#270 from carlory/update-kind-version 90efb2c Merge pull request kubernetes-csi#272 from andyzhangx/patch-3 9b616fe Bump golang to 1.23.6 to fix CVE-2024-45336, CVE-2025-22866 6dcb96a update default kind version to v0.25.0 0496593 Merge pull request kubernetes-csi#268 from huww98/cloudbuild 119aee1 Merge pull request kubernetes-csi#266 from jsafrane/bump-sanity-5.3.1 0ae5e52 Update cloudbuild image with go 1.21+ 406a79a Merge pull request kubernetes-csi#267 from huww98/gomodcache 9cec273 Set GOMODCACHE to avoid re-download toolchain 98f2307 Merge pull request kubernetes-csi#260 from TerryHowe/update-csi-driver-version e9d8712 Merge pull request kubernetes-csi#259 from stmcginnis/deprecated-kind-kube-root faf79ff Remove --kube-root deprecated kind argument 734c2b9 Merge pull request kubernetes-csi#265 from Rakshith-R/consider-main-branch 43bde06 Bump csi-sanity to 5.3.1 f95c855 Merge pull request kubernetes-csi#262 from huww98/golang-toolchain 3c8d966 Treat main branch as equivalent to master branch e31de52 Merge pull request kubernetes-csi#261 from huww98/golang fd153a9 Bump golang to 1.23.1 a8b3d05 pull-test.sh: fix "git subtree pull" errors 6b05f0f use new GOTOOLCHAIN env to manage go version 18b6ac6 chore: update CSI driver version to 1.15 227577e Merge pull request kubernetes-csi#258 from gnufied/enable-race-detection e1ceee2 Always enable race detection while running tests 988496a Merge pull request kubernetes-csi#257 from jakobmoellerdev/csi-prow-sidecar-e2e-path 028f8c6 chore: bump to Go 1.22.5 69bd71e chore: add CSI_PROW_SIDECAR_E2E_PATH f40f0cc Merge pull request kubernetes-csi#256 from solumath/master cfa9210 Instruction update 379a1bb Merge pull request kubernetes-csi#255 from humblec/sidecar-md a5667bb fix typo in sidecar release process 4967685 Merge pull request kubernetes-csi#254 from bells17/add-github-actions d9bd160 Update skip list in codespell GitHub Action adb3af9 Merge pull request kubernetes-csi#252 from bells17/update-go-version f5aebfc Add GitHub Actions workflows b82ee38 Merge pull request kubernetes-csi#253 from bells17/fix-typo c317456 Fix typo 0a78505 Bump to Go 1.22.3 edd89ad Merge pull request kubernetes-csi#251 from jsafrane/add-logcheck 043fd09 Add test-logcheck target d7535ae Merge pull request kubernetes-csi#250 from jsafrane/go-1.22 b52e7ad Update go to 1.22.2 14fdb6f Merge pull request kubernetes-csi#247 from msau42/prow dc4d0ae Merge pull request kubernetes-csi#249 from jsafrane/use-go-version e681b17 Use .go-version to get Kubernetes go version 9b4352e Update release playbook c7bb972 Fix release notes script to use fixed tags 463a0e9 Add script to update specific go modules b54c1ba Merge pull request kubernetes-csi#246 from xing-yang/go_1.21 5436c81 Change go version to 1.21.5 267b40e Merge pull request kubernetes-csi#244 from carlory/sig-storage b42e5a2 nominate self (carlory) as kubernetes-csi reviewer a17f536 Merge pull request kubernetes-csi#210 from sunnylovestiramisu/sidecar 011033d Use set -x instead of die 5deaf66 Add wrapper script for sidecar release f8c8cc4 Merge pull request kubernetes-csi#237 from msau42/prow b36b5bf Merge pull request kubernetes-csi#240 from dannawang0221/upgrade-go-version adfddcc Merge pull request kubernetes-csi#243 from pohly/git-subtree-pull-fix c465088 pull-test.sh: avoid "git subtree pull" error 7b175a1 Update csi-test version to v5.2.0 987c90c Update go version to 1.21 to match k/k 2c625d4 Add script to generate patch release notes f9d5b9c Merge pull request kubernetes-csi#236 from mowangdk/feature/bump_csi-driver-host-path_version b01fd53 Bump csi-driver-host-path version up to v1.12.0 984feec Merge pull request kubernetes-csi#234 from siddhikhapare/csi-tools 1f7e605 fixed broken links of testgrid dashboard de2fba8 Merge pull request kubernetes-csi#233 from andyzhangx/andyzhangx-patch-1 cee895e remove windows 20H2 build since it's EOL long time ago 670bb0e Merge pull request kubernetes-csi#229 from marosset/fix-codespell-errors 35d5e78 Merge pull request kubernetes-csi#219 from yashsingh74/update-registry 63473cc Merge pull request kubernetes-csi#231 from coulof/bump-go-version-1.20.5 29a5c76 Merge pull request kubernetes-csi#228 from mowangdk/chore/adopt_kubernetes_recommand_labels 8dd2821 Update cloudbuild image with go 1.20.5 1df23db Merge pull request kubernetes-csi#230 from msau42/prow 1f92b7e Add ginkgo timeout to e2e tests to help catch any stuck tests 2b8b80e fixing some codespell errors c10b678 Merge pull request kubernetes-csi#227 from coulof/check-sidecar-supported-versions 72984ec chore: adopt kubernetes recommand label b055535 Header bd0a10b typo c39d73c Add comments f6491af Script to verify EOL sidecar version 4133d1d Merge pull request kubernetes-csi#226 from msau42/cloudbuild 8d519d2 Pin buildkit to v0.10.6 to workaround v0.11 bug with docker manifest 6e04a03 Merge pull request kubernetes-csi#224 from msau42/cloudbuild 26fdfff Update cloudbuild image 6613c39 Merge pull request kubernetes-csi#223 from sunnylovestiramisu/update 0e7ae99 Update k8s image repo url 77e47cc Merge pull request kubernetes-csi#222 from xinydev/fix-dep-version 155854b Fix dep version mismatch 8f83905 Merge pull request kubernetes-csi#221 from sunnylovestiramisu/go-update 1d3f94d Update go version to 1.20 to match k/k v1.27 e322ce5 Merge pull request kubernetes-csi#220 from andyzhangx/fix-golint-error b74a512 test: fix golint error 901bcb5 Update registry k8s.gcr.io -> registry.k8s.io aa61bfd Merge pull request kubernetes-csi#218 from xing-yang/update_csi_driver 7563d19 Update CSI_PROW_DRIVER_VERSION to v1.11.0 a2171be Merge pull request kubernetes-csi#216 from msau42/process cb98782 Merge pull request kubernetes-csi#217 from msau42/owners a11216e add new reviewers and remove inactive reviewers dd98675 Add step for checking builds b66c082 Merge pull request kubernetes-csi#214 from pohly/junit-fixes b9b6763 filter-junit.go: fix loss of testcases when parsing Ginkgo v2 JUnit d427783 filter-junit.go: preserve system error log 38e1146 prow.sh: publish individual JUnit files as separate artifacts git-subtree-dir: release-tools git-subtree-split: 74e066a82d2ea14a68ff37f0e02ddc34619bbccd
What type of PR is this?
/kind api-change
What this PR does / why we need it:
In light of HostProcess containers on K8s Windows, we no longer need the client/server model, with a separate binary running on each Windows node. Instead, we can provide CSI Proxy's functionalities as a Go library to be imported by CSI driver implementations running in HostProcess containers. The change involves a complete restructure of the package structure and bumping the Go version to v2. Each API group is now provided under /pkg/, with the OS package written in /pkg//api.
Which issue(s) this PR fixes:
Part of #217
Special notes for your reviewer:
Does this PR introduce a user-facing change?: