Skip to content

Comments

daemon: separate daemon ID from trust-key, and disable generating#43555

Merged
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:separate_engine_id
May 10, 2022
Merged

daemon: separate daemon ID from trust-key, and disable generating#43555
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:separate_engine_id

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented May 3, 2022 •
edited
Loading

daemon: separate daemon ID from trust-key

This change is in preparation of deprecating support for old manifests.
Currently the daemon's ID is based on the trust-key ID, which will be
removed once we fully deprecate support for old manifests (the trust
key is currently only used in tests).

This patch:

  • looks if a trust-key is present; if so, it migrates the trust-key
    ID to the new "engine-id" file within the daemon's root.
  • if no trust-key is present (so in case it's a "fresh" install), we
    generate a UUID instead and use that as ID.

The migration is to prevent engines from getting a new ID on upgrades;
while we don't provide any guarantees on the engine's ID, users may
expect the ID to be "stable" (not change) between upgrades.

A test has been added, which can be ran with;

make DOCKER_GRAPHDRIVER=vfs TEST_FILTER='TestConfigDaemonID' test-integration

daemon: only create trust-key if DOCKER_ALLOW_SCHEMA1_PUSH_DONOTUSE is set

The libtrust trust-key is only used for pushing legacy image manifests;
pushing these images has been deprecated, and we only need to be able
to push them in our CI.

This patch disables generating the trust-key (and related paths) unless
the DOCKER_ALLOW_SCHEMA1_PUSH_DONOTUSE env-var is set (which we do in
our CI).

- Description for the changelog

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

@thaJeztah thaJeztah added this to the 22.04.0 milestone May 3, 2022
@thaJeztah thaJeztah changed the title daemon: separate daemon ID from trust-key daemon: separate daemon ID from trust-key, and disable generating May 3, 2022
thaJeztah
thaJeztah commented May 3, 2022
View reviewed changes
daemon/id.go
Comment on lines +28 to +32
} else if err != nil {
return "", errors.Wrapf(err, "error loading ID file %s", idPath)
} else {
id = string(idb)
}
Copy link
Member Author

@thaJeztah thaJeztah May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, here I was wondering if we should perhaps still return the ID, even if we were not able to store it in the engine_id file; we don't need the ID for anything ourselves, so failing to persist the UUID to a file is not necessarily a hard failure (it would just be re-generated on restart).

@thaJeztah
Copy link
Member Author

thaJeztah commented May 3, 2022

@tianon @tonistiigi @AkihiroSuda PTAL

@thaJeztah
Copy link
Member Author

thaJeztah commented May 3, 2022

Motivation for this is that I wanted to prevent having these files generated on new installs (creating the key also has been somewhat troublesome in the past w.r.t. permissions of the created directories and files); having the trust-key already separate from the daemon "ID" on fresh installs also prepares us for removing everything.

@thaJeztah
Copy link
Member Author

thaJeztah commented May 3, 2022

Ah, booh. Looks like these tests expect the key to be generated; will have to adjust for that.

=== RUN   TestDockerDaemonSuite/TestDaemonKeyGeneration
    docker_cli_daemon_test.go:567: Error opening key file
    check_test.go:307: [d491341b75f02] daemon is not started
    --- FAIL: TestDockerDaemonSuite/TestDaemonKeyGeneration (0.52s)

=== RUN   TestDockerDaemonSuite/TestDaemonWithWrongkey
docker_cli_daemon_test.go:1221: Error reading key.json file: open /etc/docker/key.json: no such file or directory
check_test.go:307: [d1218537aa136] daemon is not started
--- FAIL: TestDockerDaemonSuite/TestDaemonWithWrongkey (0.53s)

@thaJeztah thaJeztah force-pushed the separate_engine_id branch from 116f61a to 5b82b87 Compare May 3, 2022 22:56
thaJeztah
thaJeztah commented May 3, 2022
View reviewed changes
integration-cli/docker_cli_daemon_test.go
func (s *DockerDaemonSuite) TestDaemonKeyGeneration(c *testing.T) {
// TODO: skip or update for Windows daemon
os.Remove("/etc/docker/key.json")
c.Setenv("DOCKER_ALLOW_SCHEMA1_PUSH_DONOTUSE", "1")
Copy link
Member Author

@thaJeztah thaJeztah May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess this should fix those failures 🤞

tianon
tianon approved these changes May 3, 2022
View reviewed changes
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'd love to bike-shed on /var/lib/docker/engine_id, but I don't think doing so really helps here so I'll try to refrain 🙈❤️)

@thaJeztah
Copy link
Member Author

thaJeztah commented May 3, 2022

(I'd love to bike-shed on /var/lib/docker/engine_id, but I don't think doing so really helps here so I'll try to refrain 🙈❤️)

You mean the name of the file (or the location where it's stored?)

I'm impartial to the name of the file (picked this one from the original in #39384)

As to location; there was some discussion on that; keep it in /var/lib/docker ? (which means that wiping that also resets the ID); some other location? Problem with /etc/docker is that that directory is shared between the CLI and the daemon on Linux (not ideal! permissions on files have been an issue on several occasions)

thaJeztah added 2 commits May 4, 2022 20:17
@thaJeztah
daemon: separate daemon ID from trust-key
bb12086 
This change is in preparation of deprecating support for old manifests.
Currently the daemon's ID is based on the trust-key ID, which will be
removed once we fully deprecate support for old manifests (the trust
key is currently only used in tests).

This patch:

- looks if a trust-key is present; if so, it migrates the trust-key
  ID to the new "engine-id" file within the daemon's root.
- if no trust-key is present (so in case it's a "fresh" install), we
  generate a UUID instead and use that as ID.

The migration is to prevent engines from getting a new ID on upgrades;
while we don't provide any guarantees on the engine's ID, users may
expect the ID to be "stable" (not change) between upgrades.

A test has been added, which can be ran with;

    make DOCKER_GRAPHDRIVER=vfs TEST_FILTER='TestConfigDaemonID' test-integration

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
daemon: only create trust-key if DOCKER_ALLOW_SCHEMA1_PUSH_DONOTUSE i…
070da63 
…s set

The libtrust trust-key is only used for pushing legacy image manifests;
pushing these images has been deprecated, and we only need to be able
to push them in our CI.

This patch disables generating the trust-key (and related paths) unless
the DOCKER_ALLOW_SCHEMA1_PUSH_DONOTUSE env-var is set (which we do in
our CI).

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the separate_engine_id branch from 5b82b87 to 070da63 Compare May 4, 2022 18:19
@thaJeztah
Copy link
Member Author

thaJeztah commented May 4, 2022

Finished tiny bike-shed; changed engine_id to engine-id as underscores aren't always great in terminal fonts, and it's more consistent with other files we use 👍

@thaJeztah
Copy link
Member Author

thaJeztah commented May 4, 2022

Failure is unrelated (TestRunInteractiveWithRestartPolicy is a known flaky test on Windows)

@thaJeztah
Copy link
Member Author

thaJeztah commented May 5, 2022

I'll do a quick test PR to see what happens if we don't set the key at all

@thaJeztah
Copy link
Member Author

thaJeztah commented May 10, 2022

I'll do a quick test PR to see what happens if we don't set the key at all

I had a look at where the trustkey is used, and use of it is all the way to the https://github.com/docker/distribution code, and I don't have a clear view on where it can be removed (and what to replace it with).

I might still have a look in a follow-up, but don't consider it the highest priority, so I'll get this one in

@thaJeztah thaJeztah merged commit 3228dba into moby:master May 10, 2022
@thaJeztah thaJeztah deleted the separate_engine_id branch May 10, 2022 12:27
@agners agners mentioned this pull request Sep 7, 2022
Merged
@thaJeztah thaJeztah mentioned this pull request Apr 5, 2023
Merged
@corhere corhere mentioned this pull request May 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rumpl rumpl rumpl approved these changes

@tianon tianon tianon approved these changes

Assignees

No one assigned

Labels

area/daemon Core Engine impact/changelog status/2-code-review

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah @rumpl @tianon