enforce reserve internal labels. by tao12345666333 · Pull Request #40394 · moby/moby

tao12345666333 · 2020-01-19T10:26:50Z

The namespaces com.docker., io.docker., org.dockerproject.*
have been documented to be reserved for Docker's internal use.

Signed-off-by: Jintao Zhang [email protected]

- What I did

- How I did it

- How to verify it

- Description for the changelog

Enforce reserve internal labels.

- A picture of a cute animal (not mandatory but encouraged)

tao12345666333 · 2020-01-20T16:51:56Z

@thaJeztah PTAL, thanks!

thaJeztah · 2020-01-20T17:11:47Z

Is this ValidateLabel only used by the daemon config? Double checking because if this one is also used in the CLI, we can't reject the label here (because the docker cli will set labels with this prefix (e.g. when doing docker stack deploy, the docker cli will set com.docker.xx labels based on the stack that services, networks and so on are part of 🤔 )

Thanks. I have checked. CLI has its own ValidateLabel function. https://github.com/docker/cli/blob/d443b7409192f29bf82258396f0170dff6cba707/opts/opts.go#L280

Thinking if we should rename it to ValidateEngineLabel to prevent possible confusion 🤔 (not important, we could do that as a follow-up if we want to)

yes, I agree.

thaJeztah

Thanks! I left some suggestions (I quickly made those changes locally; I'll post the patch so that you can apply it); could you also squash both commits?

thaJeztah · 2020-02-11T15:57:52Z

Thinking if we should rename it to ValidateEngineLabel to prevent possible confusion 🤔 (not important, we could do that as a follow-up if we want to)

thaJeztah · 2020-02-11T16:02:57Z

Patch with the changes I suggested:

Details

diff --git a/opts/opts.go b/opts/opts.go
index 72ed4b314d..60a093f28c 100644
--- a/opts/opts.go
+++ b/opts/opts.go
@@ -267,7 +267,7 @@ func ValidateLabel(val string) (string, error) {
 	if strings.HasPrefix(lowered, "com.docker.") || strings.HasPrefix(lowered, "io.docker.") ||
 		strings.HasPrefix(lowered, "org.dockerproject.") {
 		return "", fmt.Errorf(
-			"label %s not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for Docker's internal use",
+			"label %s is not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for internal use",
 			val)
 	}
 
diff --git a/opts/opts_test.go b/opts/opts_test.go
index 826292c427..3c90a24068 100644
--- a/opts/opts_test.go
+++ b/opts/opts_test.go
@@ -4,6 +4,9 @@ import (
 	"fmt"
 	"strings"
 	"testing"
+
+	"gotest.tools/assert"
+	is "gotest.tools/assert/cmp"
 )
 
 func TestValidateIPAddress(t *testing.T) {
@@ -181,10 +184,9 @@ func TestValidateLabel(t *testing.T) {
 		expectedErr    string
 	}{
 		{
-			name:           "lable with bad attribute format",
-			label:          "label",
-			expectedResult: "",
-			expectedErr:    "bad attribute format: label",
+			name:        "lable with bad attribute format",
+			label:       "label",
+			expectedErr: "bad attribute format: label",
 		},
 		{
 			name:           "label with general format",
@@ -217,64 +219,49 @@ func TestValidateLabel(t *testing.T) {
 			expectedResult: "org.docker.not=reserved",
 		},
 		{
-			name:           "label with reserved com.docker.*",
-			label:          "com.docker.feature=enabled",
-			expectedResult: "",
-			expectedErr:    "label com.docker.feature=enabled not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for Docker's internal use",
+			name:        "label with reserved com.docker.*",
+			label:       "com.docker.feature=enabled",
+			expectedErr: "label com.docker.feature=enabled is not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for internal use",
 		},
 		{
-			name:           "label with reserved upcase com.docker.* ",
-			label:          "COM.docker.feature=enabled",
-			expectedResult: "",
-			expectedErr:    "label COM.docker.feature=enabled not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for Docker's internal use",
+			name:        "label with reserved upcase com.docker.* ",
+			label:       "COM.docker.feature=enabled",
+			expectedErr: "label COM.docker.feature=enabled is not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for internal use",
 		},
 		{
-			name:           "label with reserved io.docker.*",
-			label:          "io.docker.configuration=0",
-			expectedResult: "",
-			expectedErr:    "label io.docker.configuration=0 not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for Docker's internal use",
+			name:        "label with reserved io.docker.*",
+			label:       "io.docker.configuration=0",
+			expectedErr: "label io.docker.configuration=0 is not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for internal use",
 		},
 		{
-			name:           "label with reserved upcase io.docker.*",
-			label:          "io.DOCKER.CONFIGURATion=0",
-			expectedResult: "",
-			expectedErr:    "label io.DOCKER.CONFIGURATion=0 not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for Docker's internal use",
+			name:        "label with reserved upcase io.docker.*",
+			label:       "io.DOCKER.CONFIGURATion=0",
+			expectedErr: "label io.DOCKER.CONFIGURATion=0 is not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for internal use",
 		},
 		{
-			name:           "label with reserved org.dockerproject.*",
-			label:          "org.dockerproject.setting=on",
-			expectedResult: "",
-			expectedErr:    "label org.dockerproject.setting=on not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for Docker's internal use",
+			name:        "label with reserved org.dockerproject.*",
+			label:       "org.dockerproject.setting=on",
+			expectedErr: "label org.dockerproject.setting=on is not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for internal use",
 		},
 		{
-			name:           "label with reserved upcase org.dockerproject.*",
-			label:          "Org.Dockerproject.Setting=on",
-			expectedResult: "",
-			expectedErr:    "label Org.Dockerproject.Setting=on not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for Docker's internal use",
+			name:        "label with reserved upcase org.dockerproject.*",
+			label:       "Org.Dockerproject.Setting=on",
+			expectedErr: "label Org.Dockerproject.Setting=on is not allowed: the namespaces com.docker.*, io.docker.*, and org.dockerproject.* are reserved for internal use",
 		},
 	}
 
 	for _, testCase := range testCases {
-		testCase = testCase
+		testCase := testCase
 		t.Run(testCase.name, func(t *testing.T) {
 			result, err := ValidateLabel(testCase.label)
 
-			if testCase.expectedResult != "" {
-				if err != nil {
-					t.Fatalf("expected no error, got error [%s]", err)
-				}
-
-				if result != testCase.expectedResult {
-					t.Fatalf("expected [%s], got [%s]", testCase.expectedResult, result)
-				}
+			if testCase.expectedErr != "" {
+				assert.Error(t, err, testCase.expectedErr)
 			} else {
-				if err != nil {
-					if err.Error() != testCase.expectedErr {
-						t.Fatalf("expected error [%s], got error [%s]", testCase.expectedErr, err.Error())
-					}
-				} else {
-					t.Fatalf("expected error '%s', got no error", testCase.expectedErr)
-				}
+				assert.NilError(t, err)
+			}
+			if testCase.expectedResult != "" {
+				assert.Check(t, is.Equal(result, testCase.expectedResult))
 			}
 		})

Or as a file if that's easier:

suggestions.patch.txt

thaJeztah · 2020-02-11T16:04:11Z

After this is merged, we'll have to update the deprecated.md in the CLI repository; I'll prepare a PR for that.

thaJeztah · 2020-02-11T16:11:17Z

oh! you probably used the "suggested changes" from GitHub; unfortunately those don't work well with our workflow, as they are added as separate commits, and don't have a DCO sign-off 😥

You should be able to apply the patch I provided above instead though from your cli (let me know if you need help on that)

Meanwhile, I prepared the draft PR for the docs changes in docker/cli: docker/cli#2326

tao12345666333 · 2020-02-11T16:13:48Z

Sorry, I didn't know much about this feature of GitHub. I will fix it!

thaJeztah · 2020-02-11T16:33:15Z

No worries! It's a bit unfortunate that the GitHub feature doesn't work well for us (as it's a nice way to show what changes could be made)

Let me know if you need a hand, otherwise I can push to your branch.

tao12345666333 · 2020-02-11T16:37:10Z

Yes, I didn't find how to get "suggested changes" commits in the GitHub documentation.

thaJeztah

LGTM, thanks!

cpuguy83

LGTM

tao12345666333 · 2020-02-12T03:57:39Z

[2020-02-11T17:38:55.832Z] === Errors

[2020-02-11T17:38:55.832Z] package github.com/docker/docker/opts (test)

[2020-02-11T17:38:55.832Z] 	imports gotest.tools/assert: cannot find package "gotest.tools/assert" in any of:

[2020-02-11T17:38:55.832Z] 	/go/src/github.com/docker/docker/vendor/gotest.tools/assert (vendor tree)

[2020-02-11T17:38:55.832Z] 	/usr/local/go/src/gotest.tools/assert (from $GOROOT)

[2020-02-11T17:38:55.832Z] 	/go/src/gotest.tools/assert (from $GOPATH)

[2020-02-11T17:38:55.832Z]

Sorry. I need rebase and fix import path to gotest.tools/v3/assert.

The namespaces com.docker.*, io.docker.*, org.dockerproject.* have been documented to be reserved for Docker's internal use. Co-Authored-By: Sebastiaan van Stijn <[email protected]> Signed-off-by: Jintao Zhang <[email protected]>

thaJeztah · 2020-02-12T07:50:15Z

CI Failures look unrelated

=== RUN   TestDiff
--- FAIL: TestDiff (0.66s)
    diff_test.go:42: assertion failed: 
        --- expected
        +++ items
        {[]container.ContainerChangeResponseItem}:
        	-: []container.ContainerChangeResponseItem{{Kind: 0x01, Path: "/foo"}, {Kind: 0x01, Path: "/foo/bar"}}
        	+: []container.ContainerChangeResponseItem(nil)

tao12345666333 · 2020-02-12T11:00:59Z

CI failed （unrelated

[2020-02-12T08:22:21.410Z] === Failed
[2020-02-12T08:22:21.410Z] === FAIL: amd64.integration.service TestInspect (12.05s)
[2020-02-12T08:22:21.410Z]     inspect_test.go:39: timeout hit after 10s: waiting for tasks to enter run state. task failed with error: task: non-zero exit (1)
[2020-02-12T08:22:21.410Z] 
[2020-02-12T08:22:21.410Z] 
[2020-02-12T08:22:21.410Z] DONE 23 tests, 1 skipped, 1 failure in 188.519s

thaJeztah · 2020-02-12T11:18:43Z

Yes, that test is known to be flaky; let's see if it goes green this time 🤞. There's also some flakiness in the docker-py tests, which should be resolved after #40500 and #40499 are merged

thaJeztah · 2020-02-13T00:47:00Z

Windows failure is unrelated (see #40506); I'll bring this one in

tao12345666333 · 2020-02-13T00:48:11Z

Thanks!

tao12345666333 requested a review from thaJeztah January 19, 2020 10:27

tao12345666333 force-pushed the reserved-namespace-labels branch from 6b7d27c to 56a93ae Compare January 19, 2020 16:38

tao12345666333 requested review from AkihiroSuda and cpuguy83 January 19, 2020 16:48

thaJeztah reviewed Jan 20, 2020

View reviewed changes

thaJeztah added impact/changelog impact/deprecation status/2-code-review labels Feb 11, 2020

thaJeztah added this to the 20.03.0 milestone Feb 11, 2020

thaJeztah requested changes Feb 11, 2020

View reviewed changes

thaJeztah mentioned this pull request Feb 11, 2020

deprecated: use of reserved namespaces for engine labels docker/cli#2326

Merged

tao12345666333 force-pushed the reserved-namespace-labels branch from f2b1a2c to 4bf4fb1 Compare February 11, 2020 17:22

tao12345666333 requested a review from thaJeztah February 11, 2020 17:27

tao12345666333 force-pushed the reserved-namespace-labels branch from 4bf4fb1 to f239aaf Compare February 11, 2020 17:34

thaJeztah approved these changes Feb 11, 2020

View reviewed changes

cpuguy83 approved these changes Feb 11, 2020

View reviewed changes

cpuguy83 added status/4-merge and removed status/2-code-review labels Feb 11, 2020

enforce reserve internal labels.

35d6c18

The namespaces com.docker.*, io.docker.*, org.dockerproject.* have been documented to be reserved for Docker's internal use. Co-Authored-By: Sebastiaan van Stijn <[email protected]> Signed-off-by: Jintao Zhang <[email protected]>

tao12345666333 force-pushed the reserved-namespace-labels branch from f239aaf to 35d6c18 Compare February 12, 2020 04:04

thaJeztah merged commit 3af8d48 into moby:master Feb 13, 2020

tao12345666333 deleted the reserved-namespace-labels branch February 13, 2020 00:48

coderabbitai Bot mentioned this pull request Jun 29, 2026

deploy: container deployment support sun-praise/static-html#64

Merged

Uh oh!

Conversation

tao12345666333 commented Jan 19, 2020

Uh oh!

tao12345666333 commented Jan 20, 2020

Uh oh!

thaJeztah Jan 20, 2020

Choose a reason for hiding this comment

Uh oh!

tao12345666333 Jan 20, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

tao12345666333 Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 11, 2020

Uh oh!

thaJeztah commented Feb 11, 2020

Uh oh!

thaJeztah commented Feb 11, 2020

Uh oh!

tao12345666333 commented Feb 11, 2020

Uh oh!

thaJeztah commented Feb 11, 2020

Uh oh!

tao12345666333 commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

tao12345666333 commented Feb 12, 2020

Uh oh!

thaJeztah commented Feb 12, 2020

Uh oh!

tao12345666333 commented Feb 12, 2020

Uh oh!

thaJeztah commented Feb 12, 2020

Uh oh!

thaJeztah commented Feb 13, 2020

Uh oh!

tao12345666333 commented Feb 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tao12345666333 commented Feb 11, 2020 •

edited

Loading