identifiers: adjust requirements for identifiers #1108
Conversation
|
cc @ijc @xiekeyang |
identifiers/validate.go
Outdated
| @@ -43,6 +49,10 @@ func IsInvalid(err error) bool { | |||
| // In general, identifiers that pass this validation, should be safe for use as | |||
| // a domain identifier or filesystem path component. | |||
| func Validate(s string) error { | |||
| if len(s) > maxLength { | |||
| return errors.Wrapf(errIdentifierInvalid, "identifier %q greater than maximum length") | |||
There was a problem hiding this comment.
nit: to be,
return errors.Wrapf(errIdentifierInvalid, "identifier %q greater than maximum length(%d)",s, maxLength)(at least omit %q, s )
identifiers/validate_test.go
Outdated
| @@ -15,6 +16,9 @@ func TestValidIdentifiers(t *testing.T) { | |||
| "foo.boo", | |||
| "swarmkit.docker.io", | |||
| "zn--e9.org", // or something like it! | |||
| "0912341234", | |||
| "task.0.0123456789", | |||
| strings.Repeat("a", 76), | |||
identifiers/validate_test.go
Outdated
| @@ -34,6 +38,7 @@ func TestInvalidIdentifiers(t *testing.T) { | |||
| "-foo.boo", | |||
| "foo.boo-", | |||
| "foo_foo.boo_underscores", // boo-urns? | |||
| strings.Repeat("a", 77), | |||
|
I tested this with the swarmd containerd executor and it fixes the issues I mentioned in #1083 (comment), thanks! In #1083 (comment) you mentioned hex identifiers, you've gone wider than that here but I just wanted to mention (even though I'm sure you know) that swarmkit identifiers aren't hex, they're base36, plus |
Actually that's not quite accurate, the full swarmkit task names (which I'm using as the containerd id) are |
|
Code looks good to me, apart from the CI fail is: which I don't see how this PR causes... |
|
CI is due to #1109 |
Introduced in containerd/containerd#1083 but might change (see comments on containerd#1083 and https://dockercommunity.slackarchive.io/containerd/page-26/ts-1498485227137668) This can be dropped when containerd/containerd#1108 or something similar is merged. Signed-off-by: Ian Campbell <ian.campbell@docker.com>
There was a problem hiding this comment.
agree with @xiekeyang on the nit to use maxLength const in the test definitions so any changes in the future will automatically follow through to tests.
Beyond that, LGTM
|
LGTM, but I'd also like the maxlength to be shown in the error message (and used in the test) |
Introduced in containerd/containerd#1083 but might change (see comments on containerd#1083 and https://dockercommunity.slackarchive.io/containerd/page-26/ts-1498485227137668) This can be dropped when containerd/containerd#1108 or something similar is merged. Signed-off-by: Ian Campbell <ian.campbell@docker.com>
stevvooe
force-pushed
the
adjust-identifier-requirements
branch
from
0ef5531
to
0c29e75
Compare
Based on feedback, a few adjustments have been made to the identifier requirements. Identifiers that have components that start with a number are now allowed. This violates RFC 1035 but does not do so for dns names practically. A total length, of 76 characters is now also enforced. This decision was completely arbitrary but satifies the requirement for a maximum length. Often, this is used as the maximum length of a line in editors, so it should be a good choice. Signed-off-by: Stephen J Day <stephen.day@docker.com>
stevvooe
force-pushed
the
adjust-identifier-requirements
branch
from
0c29e75
to
0e34531
Compare
Codecov Report
@@ Coverage Diff @@
## master #1108 +/- ##
=======================================
Coverage 54.51% 54.51%
=======================================
Files 16 16
Lines 1229 1229
=======================================
Hits 670 670
Misses 386 386
Partials 173 173Continue to review full report at Codecov.
|
Introduced in containerd/containerd#1083 but might change (see comments on containerd#1083 and https://dockercommunity.slackarchive.io/containerd/page-26/ts-1498485227137668) This can be dropped when containerd/containerd#1108 or something similar is merged. Signed-off-by: Ian Campbell <ian.campbell@docker.com>
Update CNI to v0.7.5.
Based on feedback, a few adjustments have been made to the identifier
requirements. Identifiers that have components that start with a number
are now allowed. This violates RFC 1035 but does not do so for dns names
practically. A total length, of 76 characters is now also enforced. This
decision was completely arbitrary but satifies the requirement for a
maximum length. Often, this is used as the maximum length of a line in
editors, so it should be a good choice.
Signed-off-by: Stephen J Day stephen.day@docker.com