api/types/container: remove support for config mac address#51152
api/types/container: remove support for config mac address#51152austinvazquez wants to merge 1 commit intomoby:masterfrom
Conversation
3e2a370 to
857f9b9
Compare
857f9b9 to
010dc7c
Compare
|
Signed-off-by: Austin Vazquez <austin.vazquez@docker.com>
010dc7c to
c020b45
Compare
Root caused this test failure to be from embedding the |
| // Regression test for https://github.com/moby/moby/issues/47441 | ||
| // Migration of a container-wide MAC address to the new per-endpoint setting, | ||
| // where NetworkMode uses network id, and the key in endpoint settings is the | ||
| // network name. | ||
| func TestWatchtowerCreate(t *testing.T) { |
There was a problem hiding this comment.
Client would no longer support sending MacAddress field even when pinned to an older API version. cc: @robmry WDYT, should this test be made to use the API endpoint instead?
There was a problem hiding this comment.
As it's still in the API, it should be tested - but it looks like I added unit test TestHandleMACAddressBC as part of the fix that added this integration test. It tests handleMACAddressBC, and the fix was all in there. So that's probably good enough.
But the test above (TestInspectCfgdMAC) is no longer testing what it says, and that one's not really unit testable...
// Regression test for https://github.com/moby/moby/issues/47228 - check that a
// generated MAC address is not included in the Config section of 'inspect'
// output, but a configured address is.
TestMacAddressIsAppliedToMainNetworkWithShortID is also there to regression test an issue related to the container-wide MAC address not being mapped to the right network. Changing its API request to use the per-network MAC address means it's not doing anything very useful.
So, yes - if there is a way to test against the API, that'd be good ... are you thinking of just sending a blob of JSON from the test?
(This may be heresy - but alternatively we could just stick with the deprecated field in the API? We can still return an error if it's used in API 1.52 - and if we drop support for old versions of the API at some point, we can delete these tests and all the messy compatibility code at the same time. Until then, if we have to maintain this stuff, not having the field in the API makes things a bit more tricky?)
| } else if ep.MacAddress != deprecatedMacAddress { | ||
| return "", errdefs.InvalidParameter(errors.New("the container-wide MAC address must match the endpoint-specific MAC address for the main network, or be left empty")) | ||
|
|
||
| // API v1.44 to v1.51: the container-wide MacAddress field is deprecated but still supported for backward-compatibility. |
There was a problem hiding this comment.
Did we actually support though? Because at least the client removed the field on API v1.44 and up, and I think the daemon also ignored it, so other than the swagger mentioning the field (which it shouldn't have) the field effectively was already removed.
There was a problem hiding this comment.
Chatted some offline. Between 1.44 and 1.52 the daemon would migrate the mac address to the endpoint for requests coming in with the field set and warn users to use the endpoint settings instead.
Reference:
|
Superseded by #51194 |
- What I did
Dropped support for the container wide MacAddress in container configuration.
Deprecated in API v1.44; the MacAddress field is no longer supported starting with API v1.52. Newer clients will no longer be able to send the field even when specifying the lower API versions. The daemon will continue to migrate the field when using API v1.44 - v1.51. Specifying the field will now result in an error for v1.52 and onwards.
- How I did it
🧹 ... 🗑️
- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)