Include init NetworkSettings within inspect response #3318
Conversation
|
I have added the requested test cases as well as caught an update to the pre-existing test, which I was responsible for. PTAL at your convenience. |
|
PTAL at your convenience. |
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testcase { | ||
| fmt.Fprintln(os.Stderr, tc.name) |
There was a problem hiding this comment.
remove the printline.
There was a problem hiding this comment.
This print statement was actually an attempt to clarify the sub-test cases, similar to the precedence of other unit tests. (See reference example below)
I will remove the print statements.
If I can easily iterate on the test to uniquely report the sub-case PASS, then I will do that too.
--- PASS: TestBuildKitFile (0.00s)
--- PASS: TestBuildKitFile/only_Dockerfile_is_present (0.00s)
--- PASS: TestBuildKitFile/only_Containerfile_is_present (0.00s)
--- PASS: TestBuildKitFile/both_Dockerfile_and_Containerfile_are_present (0.00s)
--- PASS: TestBuildKitFile/Dockerfile_and_Containerfile_have_different_contents (0.00s)
--- PASS: TestBuildKitFile/Custom_file_is_specfied (0.00s)
--- PASS: TestBuildKitFile/Absolute_path_is_specified_along_with_custom_file (0.00s)
--- PASS: TestBuildKitFile/Absolute_path_is_specified_along_with_Docker_file (0.00s)
--- PASS: TestBuildKitFile/Absolute_path_is_specified_with_Container_file_in_the_path (0.00s)
There was a problem hiding this comment.
ah ok. its fine then
There was a problem hiding this comment.
@Shubhranshu153 within commit adf4035, successfully corrected the syntax to execute the sub-case test names.
| // Given native.NetNS with single Interface without Port Annotations, Return populated NetworkSettings | ||
| // UseCase: Inspect a Running Container without published ports | ||
| { | ||
| name: "Given NetNS with single Interface without Port Annotations, Return populated NetworkSettings", |
There was a problem hiding this comment.
nit: initialized network setting.
There was a problem hiding this comment.
This was a difficult comment and test case name to write.
Technically, a "Running Container without published ports" will still have a populated NetworkSettings entity that contains an "initialized Ports" child entity.
I will try to update the comment to reflect this statement, if this clarification makes sense to you.
There was a problem hiding this comment.
@Shubhranshu153 within commit adf4035, I also updated the test description/name to reflect this conversation.
Related to [containerd#3310](containerd#3310) New behavior will always initialize a NetworkSettings entity for the inspect response, including Ports child member. If inspecting a running container with published ports, then all information will be correctly returned. If inspecting a running container without published ports, then NetworkSettings will include an initialized Ports member. If inspecting a stopped/exited container, then an entirely initialized, "empty-value" NetworkSettings is returned. Signed-off-by: Sam Chew <[email protected]>
| @@ -186,7 +186,7 @@ type ContainerState struct { | |||
| } | |||
|
|
|||
| type NetworkSettings struct { | |||
| Ports *nat.PortMap `json:",omitempty"` | |||
| Ports *nat.PortMap | |||
There was a problem hiding this comment.
Docker seems to use Ports nat.PortMap
https://github.com/moby/moby/blob/v27.1.2/api/types/types.go#L302
Related to containerd#3310
Replacing Previous Pull Request 3311
New behavior will always initialize a NetworkSettings entity for the inspect response, including Ports child member.
If inspecting a running container with published ports, then all information will be correctly returned.
If inspecting a running container without published ports, then NetworkSettings will include an initialized Ports member. If inspecting a stopped/exited container,
then an entirely initialized, "empty-value" NetworkSettings is returned.
Background
Research related to Scenario 1:
According to dockercompat line189 and the logical evaluation of Port Annotations, the
Portsare not assigned within the response when values are present within the Annotations.Therefore, containers without published ports yield an
NetworkSettingsstructure without aPortskey-value pair.Research related to Scenario 2:
According to dockercompat line401, the
native.NetNSof a stopped/exited container is a null pointer, which would returnnil.Solution
To resolve this issue, the
NetworkSettingsandPortMapare initialized at the beginning of the method to ensure that at least "Empty-value" structs are always included within theinspectresponse.When "Port Annotations" are available, the new logic will add the key-value pairs into the initialized
PortMapof the response.Since
Portsshould not be omitted, this change also removes thejson:",omitempty"decorator for theNetworkSettingsstruct.Additional Tasks
This bug is also present within
v1.7.6and the change can be easily reconciled with the released version.Release/1.7branch.