libnet: Endpoint: remove isAnonymous & myAliases#46853
Merged
thaJeztah merged 10 commits intomoby:masterfrom Dec 20, 2023
Merged
libnet: Endpoint: remove isAnonymous & myAliases#46853thaJeztah merged 10 commits intomoby:masterfrom
thaJeztah merged 10 commits intomoby:masterfrom
Conversation
b2941c6 to
ff4e67e
Compare
corhere
requested changes
Nov 27, 2023
Contributor
corhere
left a comment
There was a problem hiding this comment.
How can the anonymous and myAliases fields be removed from the endpoint KVObjects without migration code? Wouldn't DNS resolution be subtly broken on containers carried across a daemon upgrade?
ff4e67e to
9de20d7
Compare
Member
Author
Yeah, right. I overlooked this part. I added a commit to manage this case (see de2303b). However, I now realize the field Some integration tests are broken as they expect the container short ID to be in |
corhere
reviewed
Nov 29, 2023
Contributor
corhere
left a comment
There was a problem hiding this comment.
Some integration tests are broken as they expect the container short ID to be in
EndpointSettings.Aliases. I think it's not a backward-incompatible change but what's your opinion @thaJeztah @corhere?
It's an API configuration field and integration tests are explicitly testing for that behaviour. Someone's workflow is going to break. I don't think it's safe to change the semantics of EndpointSettings.Aliases on released API versions.
Comment on lines
-67
to
-69
| epMap["anonymous"] = ep.anonymous | ||
| epMap["dnsNames"] = ep.dnsNames | ||
| epMap["disableResolution"] = ep.disableResolution | ||
| epMap["myAliases"] = ep.myAliases |
Contributor
There was a problem hiding this comment.
I wonder if we should keep populating anonymous and myAliases in the marshaled objects so users could downgrade. Though only live-restored containers would be affected given that it's the endpoint object. As we already document that live-restore is only supported across patch upgrades we have no obligation to support major-version downgrades.
Member
Author
There was a problem hiding this comment.
Since we explicitly specify we don't support live-restore for major upgrades, I think we shouldn't care much about live-restore for major downgrades.
Member
|
Interesting; why is this test failing on this PR? (I was just looking at that test failing on my containerd vendor update PR; if it's failing here as well, perhaps it's just an existing issue?) |
9de20d7 to
ab1d641
Compare
bcd3a26 to
6949b0b
Compare
corhere
reviewed
Dec 15, 2023
They just happen to exist on a network that doesn't support DNS-based service discovery (ie. no embedded DNS servers are started for them). Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This new property is meant to replace myAliases and anonymous properties. The end goal is to get rid of both properties by letting the daemon determine what (non fully qualified) DNS names should be associated to them. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
6949b0b to
f93c025
Compare
corhere
approved these changes
Dec 18, 2023
fa06f79 to
89da76b
Compare
Instead of special-casing anonymous endpoints in libnetwork, let the daemon specify what (non fully qualified) DNS names should be associated to container's endpoints. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This new property will be empty if the daemon was upgraded with live-restore enabled. To not break DNS resolutions for restored containers, we need to populate dnsNames based on endpoint's myAliases & anonymous properties. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
89da76b to
ab82c8c
Compare
Instead of special-casing anonymous endpoints, use the list of DNS names associated to the endpoint. `(*Endpoint).isAnonymous()` has no more uses, so let's delete it. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
The `(*Endpoint).rename()` method is changed to only mutate `ep.name` and let a new method `(*Endpoint).UpdateDNSNames()` handle DNS updates. As a consequence, the rollback code that was part of `(*Endpoint).rename()` is now removed, and DNS updates are now rolled back by `ContainerRename`. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
The semantics of an "anonymous" endpoint has always been weird: it was set on endpoints which name shouldn't be taken into account when inserting DNS records into libnetwork's `Controller.svcRecords` (and into the NetworkDB). However, in that case the endpoint's aliases would still be used to create DNS records; thus, making those "anonymous endpoints" not so anonymous. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This property is now unused, let's get rid of it. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
ab82c8c to
1b00b9c
Compare
1b00b9c to
2ba7d74
Compare
Member
Author
|
I investigated the broken docker-py test to discover that the Because of that, the failing integration test |
2ba7d74 to
4828eb5
Compare
No more concept of "anonymous endpoints". The equivalent is now an endpoint with no DNSNames set. Some of the code removed by this commit was mutating user-supplied endpoint's Aliases to add container's short ID to that list. In order to preserve backward compatibility for the ContainerInspect endpoint, this commit also takes care of adding that short ID (and the container hostname) to `EndpointSettings.Aliases` before returning the response. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
4828eb5 to
13915f6
Compare
akerouanton
added a commit
to akerouanton/cli
that referenced
this pull request
Dec 20, 2023
Related to moby/moby#46853 Signed-off-by: Albin Kerouanton <albinker@gmail.com>
akerouanton
added a commit
to akerouanton/cli
that referenced
this pull request
Dec 20, 2023
Related to moby/moby#46853 Signed-off-by: Albin Kerouanton <albinker@gmail.com>
thaJeztah
added a commit
to akerouanton/cli
that referenced
this pull request
Dec 20, 2023
Related to moby/moby#46853 Co-Authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This was referenced Jan 23, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
- What I did
The
Endpointstruct was containing two confusing fields:isAnonymous: was set on endpoints which name shouldn't be taken into account when inserting DNS records into libnetwork'sController.svcRecords(and into the NetworkDB). However, in that case the endpoint's aliases would still be used to create DNS records; thus, making those "anonymous endpoints" not so anonymous.myAliases: there's also analiasesfield in theEndpointstruct.myAliaseswas storing extra DNS names for the endpoint, whilealiasesstores extra DNS names for other endpoints (but those extra DNS names are only accessible for the endpoint storing them -- yeah, it's confusing too).To make it worse, the code that built the list of DNS names for a given endpoint was split in two:
updateNetworkConfig(in thedaemonpkg) was adding the endpoint short ID and hostname to the list of aliases ;This PR replaces both fields with a new
DNSNamescontaining all (non-fully qualified) DNS names for a given endpoint. It's now built by the daemon duringupdateNetworkConfigand reimplements the same logic for aliases as before (a test has been added to make sure of that). An "anonymous" endpoint is now an endpoint with noDNSNames.This new field is also stored in the "operational data" (ie. state) of the
EndpointSettingsstruct exposed by the API viaGET /containers/{name:.*}/json, making it easier for end-users and scripts to know every DNS names a container has on a specific network.- How to verify it
docker run --network testnetwith a variety of:--name;--hostname;--network-alias;- A picture of a cute animal (not mandatory but encouraged)