Warn when reserved-namespace engine labels are configured #36921
Conversation
daemon/info.go
Outdated
| @@ -119,7 +134,7 @@ func (daemon *Daemon) SystemInfo() (*types.Info, error) { | |||
| MemTotal: meminfo.MemTotal, | |||
| GenericResources: daemon.genericResources, | |||
| DockerRootDir: daemon.configStore.Root, | |||
| Labels: daemon.configStore.Labels, | |||
| Labels: daemon.getFilteredLabels(), | |||
There was a problem hiding this comment.
Wondering if this filter should be applied when reading the configuration, or when changing/updating the configuration (i.e., filter out user-provided labels with the prefix)
There was a problem hiding this comment.
Possibly - I was wondering the same. I left it unchanged on the config.Config object, since that seems to be the configuration as specified by the user, but I wasn't sure. I'd be happy to make the change if you feel that'd be a better place to do it, though.
There was a problem hiding this comment.
Yes; so train of thought is that there's possibly other locations where we use this information, and those may not take the filtering into account.
Perhaps we should validate when loading, and print a warning that the label will be ignored; in future we could turn that warning into an error
Codecov Report
@@ Coverage Diff @@
## master #36921 +/- ##
=========================================
Coverage ? 35.09%
=========================================
Files ? 614
Lines ? 45707
Branches ? 0
=========================================
Hits ? 16041
Misses ? 27561
Partials ? 2105 |
|
Ugh; this hit the jackpot on flaky tests Experimental (https://jenkins.dockerproject.org/job/Docker-PRs-experimental/40409/console) failing on Janky (https://jenkins.dockerproject.org/job/Docker-PRs/49151/console) failing on PowerPC (https://jenkins.dockerproject.org/job/Docker-PRs-powerpc/9592/console) failure is being tracked through #32673 And s390x (https://jenkins.dockerproject.org/job/Docker-PRs-s390x/9517/console) failure is being tracked through #36877 |
cmd/dockerd/daemon.go
Outdated
| filtered = append(filtered, label) | ||
| } | ||
| if len(filtered) < len(labels) { | ||
| logrus.Warn("The com.docker.*, io.docker.*, and org.dockerprojects.* namespaces are reserved. Labels using these namespaces will be ignored.") |
There was a problem hiding this comment.
nit:
- there's two spaces before
Labels - perhaps
s/will be ignored/are ignored/
Wondering if during the deprecation period we should still apply the labels, and only print the warning. Although "unlikely", there may be someone somewhere actually using labels with this prefix 😞
There was a problem hiding this comment.
Perhaps it's okay, given that this was already documented; would like some others to kick in on this though 😅
There was a problem hiding this comment.
I do agree with @thaJeztah and I think there is someone using labels with those prefixes.
There was a problem hiding this comment.
@cyli why all of those three ? Wouldn't just com.docker.* be enough ? (or maybe io.docker.* with it, but I don't see why we would reserve org.dockerprojects.* too)
|
These three were the ones originally decided on when implementing labels |
|
@vdemeester Not sure - the documentation specifies all 3, so I was just following the stated warning. I'll update to just log the warning then. |
…ckerproject.* labels as per https://docs.docker.com/config/labels-custom-metadata/. Signed-off-by: Ying Li <[email protected]>
|
@cyli fair enough :) |
I'm ok with adding that as well, but let's do that in a follow-up (we may want to look at which moby prefixes to reserve, not entirely sure which domains are registered (I think there was a |
|
@cyli can you do a follow-up to add this to the deprecation list? https://github.com/docker/cli/blob/master/docs/deprecated.md |
|
@thaJeztah Done: docker/cli#1040 |
- What I did
Filter out engine labels that start withcom.docker.*,io.docker.*, andorg.dockerproject.*as per https://docs.docker.com/config/labels-custom-metadata/.Warn when engine labels are configured that start with
com.docker.*,io.docker.*, andorg.dockerproject.*as per https://docs.docker.com/config/labels-custom-metadata/.- How I did it
I filter out the labels every timeSystemInfo()is called. The labels are left as defined by thedaemon.jsonor daemon CLI args in theconfigStore.SystemInfo()labels are what is used by swarm.Filtered out the labels when the configuration is loaded and issue a warning that the labels are being ignored. In the future an error can be returned, as suggested by @thaJeztah and as per this example: e4c9079#diff-a348195a1b645d5b477946b1efae728bR274Added a label validation function that, when it errors, the daemon warns the user that label with a reserved namespace has been configured. In the future loading the configuration can just fail, as per this example: e4c9079#diff-a348195a1b645d5b477946b1efae728bR274
- How to verify it
daemon/info_test.gocmd/dockerd/daemon_test.godaemon/config_test.goand configuring a daemon with acom.docker.*label, and checking the daemon logs- Description for the changelog
Enforce reserved namespaces in engine labels by filtering out any configured labels that start withcom.docker.*,io.docker.*, ororg.dockerproject.*as per https://docs.docker.com/config/labels-custom-metadata/.Warn when an engine label using a reserved namespace (
com.docker.*,io.docker.*, ororg.dockerproject.*) is configured, as per https://docs.docker.com/config/labels-custom-metadata/.- A picture of a cute animal (not mandatory but encouraged)
