api: Deprecate outdated networking fields & structs#45945
api: Deprecate outdated networking fields & structs#45945akerouanton wants to merge 5 commits intomoby:masterfrom
Conversation
akerouanton
added
area/api
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
How do you propose to remove types and struct fields without breaking the build of API clients? Pinning the version is not an acceptable answer because transitive dependencies may force an upgrade of the moby/moby dependency whether the consumer wants to or not.
akerouanton
force-pushed
the
deprecate-outdated-api-fields
branch
3 times, most recently
from
ffa1036 to
efcbbdd
Compare
| // NetworkSettingsBase holds basic information about networks | ||
| // NetworkSettingsBase holds the networking state of a specific container when inspecting it. | ||
| // | ||
| // Deprecated: this struct mostly contains deprecated fields and will be removed in v26.0. |
There was a problem hiding this comment.
Don't think we can remove the fields if we still support the older API versions.
There was a problem hiding this comment.
OH, I guess that's the approach with the duplicated fields 🤔
There was a problem hiding this comment.
How to completely get rid of these fields is still an unanswered question for me right now. One option could be through some clever use of struct tags and a custom JSON encoder, maybe.
But at least, this change clearly separates what's deprecated and what is not, making maintainers' life easier I believe.
api/types/types.go
Outdated
| SandboxID string // SandboxID uniquely represents a container's network stack | ||
| SandboxKey string // SandboxKey is the full path of the netns handle | ||
| Ports nat.PortMap // Ports is a collection of PortBinding indexed by Port |
There was a problem hiding this comment.
Duplicating these fields is a breaking change; the insidious kind of breaking change that doesn't break compilation.
myNetworkSettings.SandboxID != myNetworkSettings.NetworkSettingsBase.SandboxIDI'm afraid that NetworkSettingsBase can't be deprecated or removed until we migrate to Go modules and release a new major version (read: with a /vN module path). We can still deprecate the fields inside NetworkSettingsBase at least, and drop the NetworkSettingsBase type without any ceremony in a new module version.
There was a problem hiding this comment.
For now, we'll continue to write both fields so I don't think that's gonna be a problem. In a release or two, we should think about removing the whole NetworkSettingsBase struct, or at least the fields that were copied to NetworkSettings (ie. Bridge, SandboxID, SandboxKey). This should give enough time for our Go users to migrate to the new fields.
There was a problem hiding this comment.
There was a problem hiding this comment.
Mh right. So, is it okay if I just remove the fields I added to NetworkSettings and keep the deprecation warning on NetworkSettingsBase while mentioning that non-deprecated fields will be moved to NetworkSettings in v26?
There was a problem hiding this comment.
That'd work, so long as linters don't complain about references to fields like myNetworkSettings.SandboxID which would be well-formed in both v25 and v26. The deprecation warning on the struct itself might have to go.
akerouanton
force-pushed
the
deprecate-outdated-api-fields
branch
2 times, most recently
from
c437ed1 to
9adb056
Compare
|
We discussed this PR few weeks ago during a maintainers' call and we agreed that every major release is a major release of the Go API, even if we don't have a proper Go module, and thus we shouldn't feel obliged to not break our Go API. As explained in the commit messages, these fields have been deprecated a long time ago and are of no practical use as they're always empty. As such, this PR marks those fields / structs as deprecated. When they will be removed, and how to handle older API versions that still them, is an unanswered question but that could be sorted out at a later time. For now the main focus is to make it easier for maintainers to understand those fields aren't used. In the future, removing these fields from API reponses will make |
akerouanton
force-pushed
the
deprecate-outdated-api-fields
branch
from
9adb056 to
39d0acd
Compare
|
As there's still concerns about some parts of this PR (the Go changes which wouldn't fail but could silently cause issues), I'll move some of the more trivial / non-concentioous parts to separate PRs. First two; |
|
@akerouanton can you rebase this one, so that the things already merged are removed from this PR? |
This struct is only used to report the networking state of the default bridge when inspecting a container. It was deprecated in v1.09 (API v1.21) but has never been removed since then. Unfortunately, the deprecation warning was wrongly formatted, thus automation tools and IDEs are not flagging it as deprecated. This commit fixes the deprecation comment. Signed-off-by: Albin Kerouanton <[email protected]>
This struct holds the overall networking state of a container, and still have a bunch of relic of the past (ie. < Docker v1.09). All the following fields are never written and are only used to create the equivalent fields in the api `NetworkSettingsBase` (which was properly deprecated in ce57494): - `HairpinMode` - `LinkLocalIPv6Address` - `LinkLocalIPv6PrefixLen` - `SecondaryIPAddresses` - `SecondaryIPv6Addresses` As such, all those fields are now marked as deprecated and their values aren't copied into `NetworkSettingsBase` anymore. Signed-off-by: Albin Kerouanton <[email protected]>
Signed-off-by: Albin Kerouanton <[email protected]>
Signed-off-by: Albin Kerouanton <[email protected]>
Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
force-pushed
the
deprecate-outdated-api-fields
branch
from
39d0acd to
21f18bd
Compare
5 participants
- What I did
DefaultNetworkSettings;NetworkSettingsBaseas deprecated ;- How to verify it
CI is green
- A picture of a cute animal (not mandatory but encouraged)