sanity: Add more input validation tests to node service #266
Conversation
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: NicolasT 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 |
|
Hi @NicolasT. 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. |
k8s-ci-robot
added
needs-ok-to-test
|
/assign @lpabon |
|
Same applies to |
According to the CSI spec, a plugin must return a `NOT_FOUND` error when `NodeStageVolume` or `NodeUnstageVolume` is called with a `volume_id` of a volume that does not (or no longer?) exist. The `sanity` package has tests asserting these `NOT_FOUND` reports for various RPC calls, including `NodeUnpublishVolume`, but not for `NodeStageVolume` and `NodeUnstageVolume`. This patch adds them for completeness. See: https://github.com/container-storage-interface/spec/blob/87a27a2496565857ed564284eaa762587feb9453/spec.md#nodestagevolume-errors See: https://github.com/container-storage-interface/spec/blob/87a27a2496565857ed564284eaa762587feb9453/spec.md#nodeunstagevolume-errors
NicolasT
force-pushed
the
test-node-stage-notfound
branch
from
a7e8665 to
ae0d21a
Compare
k8s-ci-robot
added
size/L
NicolasT
changed the title
NOT_FOUND on Node(Un)StageVolume
NicolasT
force-pushed
the
test-node-stage-notfound
branch
from
ae0d21a to
fac58c5
Compare
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
According to the CSI spec, a plugin must return a `NOT_FOUND` error when `NodePublishVolume` is called with a `volume_id` of a volume that does not (or no longer?) exist. The `sanity` package has a test asserting this `NOT_FOUND` is returned on `NodeUnpublishVolume`, but not for `NodePublishVolume`. This patch adds this for completeness. See: https://github.com/container-storage-interface/spec/blob/87a27a2496565857ed564284eaa762587feb9453/spec.md#nodepublishvolume-errors
The input validation tests for the `NodePublishVolume` RPC (and others) depends on the ordering by which a CSI node plugin performs the validation checks. This was hidden because all checks tested for return the same error. However, when adding a test which expects a different error code, this becomes an issue. Instead of constructing 'mostly-invalid' requests in many of the tests, this patch changes the strategy to construct a *valid* request by default, then in every test only switch the field under test to some invalid value, then asserting the expected error code is returned.
According to the CSI spec, a `NodePublishVolume` RPC must return a `FAILED_PRECONDITION` error when invoked with no `staging_target_path` set, if `STAGE_UNSTAGE_VOLUME` is supported by the driver. This patch adds a test to assert this. See: https://github.com/container-storage-interface/spec/blob/87a27a2496565857ed564284eaa762587feb9453/spec.md#nodepublishvolume-errors
NicolasT
force-pushed
the
test-node-stage-notfound
branch
from
fac58c5 to
4ee7e7c
Compare
|
@NicolasT: The following test 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. |
|
Prow fails because the E2E tests run against the May look into this. However, as discussed on Slack, I'm starting to believe the requirements in the spec for a node plugin to validate a given |
|
Nicolas Trangez <[email protected]> writes:
Prow fails because the E2E tests run against the `csi-driver-hostpath`
driver, which seems to be incompatible with the changes introduced in
this PR (may one then conclude `csi-driver-hostpath` is not fully
spec-compliant itself?)
It wouldn't be the first time that adding more tests reveals
deficiencies in the example driver.
|
That happens 😃 Is there any 'standard' process to get things in sync? It appears the tests in the hostpath driver repository rely on a released version of |
A developer will have to test locally that the updated driver passes testing with the updated csi-test, then we gradually have to roll out the changes: first merge the fixed into the driver, do a new release with it, then update csi-release-tools to test against that new release. At that time, the new test can be added to csi-test without test failures. It's a lengthy process, but there's no way around that when different repos are involved. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
@NicolasT: 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. |
k8s-ci-robot
added
the
needs-rebase
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
@fejta-bot: Closed this PR. In response to this:
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. |
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 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 43bde06 Bump csi-sanity to 5.3.1 18b6ac6 chore: update CSI driver version to 1.15 git-subtree-dir: release-tools git-subtree-split: 90efb2ca59900f19eba05e65da28beda79c5bb28
What type of PR is this?
/kind feature
What this PR does / why we need it:
NOT_FOUNDonNodeStageVolumeandNodeUnstageVolumefor unknown volumesNOT_FOUNDonNodePublishVolumefor unknown volumesNodePublishVolumetests to not depend on input validation ordering inside the driverFAILED_PRECONDITIONonNodePublishVolumeifSTAGE_UNSTAGE_VOLUMEis supportedDoes this PR introduce a user-facing change?: