Add tests for Unicode names #111
Conversation
Create 2 new tests, one for a volume with a Unicode name, and one for a snapshot with a Unicode name. These tests are intended to catch bugs where plugin authors use the value of the name field in a context where Unicode is not allowed.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bswartz If they are not already assigned, you can assign the PR to them by writing 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 |
k8s-ci-robot
added
size/M
|
/lgtm |
|
@pohly: changing LGTM is restricted to assignees, and only kubernetes-csi/csi-test repo collaborators may be assigned issues. 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. |
|
|
||
| It("should not fail when creating volume with a unicode name", func() { | ||
|
|
||
| name := "sanity-unicode-\xF0\x9F\x92\xA9-name" |
There was a problem hiding this comment.
I do not think this is part of the spec. Although a good test, I do not feel we should be pressing this as it is not a point of the spec.
|
I'm not sure we need this test at all. I am open to discussion, but as long as the spec does not specify this, we should not be forcing this on drivers. |
|
Is a driver allowed to fail when presented with a name that is non-ASCII? I don't think so, because having such restrictions in the driver would be an interoperability issue with spec-compliant COs or worse, break in random cases when users end up choosing such a name. Therefore I think it is okay to have this test. To me, the csi-test suite should not just check for the bare minimum functionality, but also corner cases like this. If a driver developer has a legitimate reason to limit the capabilities of his driver such that it can't pass this test, then we can add a configuration option to disable the test so that the suite remains useful for that driver. But I don't think we should worry too much about that yet, i.e. not add such a config option already now. |
Please do not misunderstand my comment. I mean, if we want this to be a test, it MUST be in the spec. Then csi-sanity will test it. |
|
@bswartz Do you mind creating a PR in the CSI spec updating it to mention the support of Unicode? I would like this test to be added to csi-sanity, but I would like to make sure it is a reflection of the spec. |
|
I did raise an issue on the CSI spec. container-storage-interface/spec#267 The conclusion was that the current wording of the spec allows for any UTF-8 string of 128 bytes or less in name fields. I personally think this should be tightened up, but my main issue is that I don't like allowing control characters ('\x00' - '\x1f') in name fields. I have no problem with allowing Unicode names. Either way, it sounds like this issue has already been litigated and they don't intend to change the wording. |
|
Ben Swartzlander <[email protected]> writes:
I did raise an issue on the CSI spec.
container-storage-interface/spec#267
Can you add that link to a comment for the new test case in the source
code? It'll help users of the suite understand why they have to support
such names.
|
|
I'm closing this issue. Please feel free to open it again if it is part of the CSI spec. |
c6a88c6 Merge pull request #113 from xing-yang/install_snapshot_controller 45ec4c6 Fix the install of snapshot CRDs and controller 5d874cc Merge pull request #112 from xing-yang/cleanup 79bbca7 Cleanup d437673 Merge pull request #111 from xing-yang/update_snapshot_v1_rc 57718f8 Update snapshot CRD version git-subtree-dir: release-tools git-subtree-split: c6a88c6
…v1_rc Update snapshot CRD version
Create 2 new tests, one for a volume with a Unicode name, and one
for a snapshot with a Unicode name. These tests are intended to catch
bugs where plugin authors use the value of the name field in a context
where Unicode is not allowed.
Fixes #108