enforce reserve internal labels. #40394

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 zhangjintao9020@gmail.com

- 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

opts/opts.go

 // Labels are in the form on key=value.
 func ValidateLabel(val string) (string, error) {
 	if strings.Count(val, "=") < 1 {
 		return "", fmt.Errorf("bad attribute format: %s", val)
 	}
+
+	lowered := strings.ToLower(val)
+	if strings.HasPrefix(lowered, "com.docker.") || strings.HasPrefix(lowered, "io.docker.") ||


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?

opts/opts.go

opts/opts_test.go

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

opts/opts.go

 // Labels are in the form on key=value.
 func ValidateLabel(val string) (string, error) {
 	if strings.Count(val, "=") < 1 {
 		return "", fmt.Errorf("bad attribute format: %s", val)
 	}
+
+	lowered := strings.ToLower(val)
+	if strings.HasPrefix(lowered, "com.docker.") || strings.HasPrefix(lowered, "io.docker.") ||


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:

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 <thaJeztah@users.noreply.github.com> Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enforce reserve internal labels. #40394

enforce reserve internal labels. #40394

tao12345666333 commented Jan 19, 2020

tao12345666333 commented Jan 20, 2020

thaJeztah Jan 20, 2020

tao12345666333 Jan 20, 2020

thaJeztah Feb 11, 2020

tao12345666333 Feb 11, 2020

thaJeztah left a comment

thaJeztah Feb 11, 2020

thaJeztah commented Feb 11, 2020

thaJeztah commented Feb 11, 2020

thaJeztah commented Feb 11, 2020

tao12345666333 commented Feb 11, 2020

thaJeztah commented Feb 11, 2020

tao12345666333 commented Feb 11, 2020 •

edited

Loading

thaJeztah left a comment

cpuguy83 left a comment

tao12345666333 commented Feb 12, 2020

thaJeztah commented Feb 12, 2020

tao12345666333 commented Feb 12, 2020

thaJeztah commented Feb 12, 2020

thaJeztah commented Feb 13, 2020

tao12345666333 commented Feb 13, 2020

enforce reserve internal labels. #40394

enforce reserve internal labels. #40394

Conversation

tao12345666333 commented Jan 19, 2020

tao12345666333 commented Jan 20, 2020

thaJeztah Jan 20, 2020

Choose a reason for hiding this comment

tao12345666333 Jan 20, 2020

Choose a reason for hiding this comment

thaJeztah Feb 11, 2020

Choose a reason for hiding this comment

tao12345666333 Feb 11, 2020

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Feb 11, 2020

Choose a reason for hiding this comment

thaJeztah commented Feb 11, 2020

thaJeztah commented Feb 11, 2020

thaJeztah commented Feb 11, 2020

tao12345666333 commented Feb 11, 2020

thaJeztah commented Feb 11, 2020

tao12345666333 commented Feb 11, 2020 • edited Loading

thaJeztah left a comment

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

tao12345666333 commented Feb 12, 2020

thaJeztah commented Feb 12, 2020

tao12345666333 commented Feb 12, 2020

thaJeztah commented Feb 12, 2020

thaJeztah commented Feb 13, 2020

tao12345666333 commented Feb 13, 2020

tao12345666333 commented Feb 11, 2020 •

edited

Loading