Add network builder and platform APIs #3939

samjkon · 2023-09-29T02:01:03Z

Summary

Added the network builder and platform APIs into the shared library package. Network builder is intended to act as the API to be consumed by the agent to setup networking resources on the host. Network builder invokes the platform APIs to execute platform specific operations like creation of network namespaces etc.

In this PR, we mostly have the skeleton of the APIs + implementation of the methods to translate the task payload into the task network configuration. Remaining workflows in the builder and platform layers will follow in subsequent PRs.

Implementation details

The most significant additions are the ecs-agent/netlib/network_builder.go and ecs-agent/netlib/platform/* files. The NetworkBuilder interface exposes higher level functions for network setup and platform.API exposes platform specific methods to be consumed by NetworkBuilder methods.

As of now, the only method from NetworkBuilder we have implemented is BuildTaskNetworkConfiguration. The remaining will be implemented in the subsequent PRs.

Testing

go test -tags unit ./ecs-agent/...
Existing and new unit tests pass.

New tests cover the changes: Yes

Description for the changelog

Add network builder and platform APIs

Does this PR include breaking model changes? If so, Have you added transformation functions?
No

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Added the network builder and platform APIs into the shared library package.

Realmonia

Overall LGTM, some nit and clarification questions

ecs-agent/netlib/model/networkinterface/networkinterface.go

agent/acs/session/payload_responder.go

ecs-agent/netlib/platform/common_linux.go

fierlion · 2023-10-17T17:06:30Z

ecs-agent/netlib/model/networkinterface/networkinterface.go

@@ -84,6 +84,10 @@ type NetworkInterface struct {
 	ipv4SubnetCIDRBlock    string
 	ipv6SubnetCIDRBlock    string

+	// Primary denotes whether the interface is responsible
+	// for handling default route within the netns it resides in.
+	Primary bool


nit: naming -- if this designates the Default route, should this be Default?

That makes sense. Will modify accordingly.

fierlion · 2023-10-17T17:23:29Z

ecs-agent/netlib/network_builder_linux_test.go

+)
+
+func TestNewNetworkBuilder(t *testing.T) {
+


should this have a test body?

Will add the body. Thanks for catching.

ecs-agent/netlib/model/networkinterface/networkinterface.go

fierlion · 2023-10-17T17:37:54Z

ecs-agent/netlib/platform/common_linux.go

+	case WarmpoolPlatform:
+		return &containerd{
+			common: commonPlatform,
+		}, nil
+	}


Can we add a TODO for the missing platforms (so we can see our way toward EC2 integration)?

fierlion · 2023-10-17T17:45:29Z

ecs-agent/netlib/platform/common_linux.go

+	}
+
+	// The number of network namespaces required to create depends on the
+	// number of unique interface names list across all container definitions


I believe device names are unique instance-wide at a single given time, but are not guaranteed to be unique over time. if we use the device name as our identifier we'll run the risk of collision.
So for handling a single task payload, device name is fine for differentiation, but we'll want to reference the NetworkInterfaceId where possible to guarantee uniqueness over time.

I will reword this comment here. When I mentioned interface names, I wasn't referring to the device name on the host. I was referring to ElasticNetworkInterface.Name in the task payload. This name field is configurable by the customer in the task definition. If the customer choses to not configure it, we default it to a string derived from the ENI's mac address.

chienhanlin · 2023-10-19T07:12:22Z

ecs-agent/netlib/network_builder.go

@@ -0,0 +1,46 @@
+package netlib


nit & non-blocking comment

Doc comments are missing in new files

// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"). You may // not use this file except in compliance with the License. A copy of the // License is located at // // http://aws.amazon.com/apache2.0/ // // or in the "license" file accompanying this file. This file is distributed // on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either // express or implied. See the License for the specific language governing // permissions and limitations under the License.

We can add log messages to help debug. Find examples as follows.

Info level log: here

Error level log: here

Warn level log: here

Debug level log: here

Thank @chienhanlin for calling that out. This PR is one of the first in a list changes for UNL. I have already put in place the copyright comments for the missing pages in the next commit and I will make sure the required logging is also put in place.

Added the network builder and platform APIs into the shared library package. Network builder is intended to act as the API to be consumed by the agent to setup networking resources on the host. Network builder invokes the platform APIs to execute platform specific operations like creation of network namespaces etc. --------- Co-authored-by: Samuel Konat <[email protected]>

This reverts commit 75db5cb. 2. Revert "update daemon log path host root in container (aws#3982)" This reverts commit 6166b6e. 3. Revert "Fix loading CSI driver container from state if it exists" This reverts commit 1391502. 4. Revert "Latest FE model files (aws#3974)" This reverts commit b589a08. 5. Revert "Move utils function Uint16SliceToStringSlice to shared lib" This reverts commit 882bcb2. 6. Revert "Move remaining credentials module to shared library" This reverts commit f58f228. 7. Revert "fix merge import failure (aws#3976)" This reverts commit 46e21e6. 8. Revert "Fixing task volumes from payload for EBS-backed tasks" This reverts commit fc97633. 9. Revert "Create Generic Attachment to decouple from ENI and save attachment state. (aws#3969)" This reverts commit 12e23ed. 10. Revert "Add support for finding EBS devices on Xen instances (aws#3971)" This reverts commit 3500ac2. 11. Revert "Add network builder and platform APIs (aws#3939)" This reverts commit b8d2a7f. 12. Revert "Minor refactor of TTL cache (allow nil TTL + get TTL)" This reverts commit 4b33664. 13. Revert "Move httpclient to ecs-agent/ and minor refactor" This reverts commit 041ca3f. 14. Revert "Add missing EBSTaskAttach changes from feature branch" This reverts commit 7a0d116.

1. Revert "Load Managed Daemon images in background (aws#3984)" This reverts commit 75db5cb. 2. Revert "update daemon log path host root in container (aws#3982)" This reverts commit 6166b6e. 3. Revert "Fix loading CSI driver container from state if it exists" This reverts commit 1391502. 4. Revert "Latest FE model files (aws#3974)" This reverts commit b589a08. 5. Revert "Move utils function Uint16SliceToStringSlice to shared lib" This reverts commit 882bcb2. 6. Revert "Move remaining credentials module to shared library" This reverts commit f58f228. 7. Revert "fix merge import failure (aws#3976)" This reverts commit 46e21e6. 8. Revert "Fixing task volumes from payload for EBS-backed tasks" This reverts commit fc97633. 9. Revert "Create Generic Attachment to decouple from ENI and save attachment state. (aws#3969)" This reverts commit 12e23ed. 10. Revert "Add support for finding EBS devices on Xen instances (aws#3971)" This reverts commit 3500ac2. 11. Revert "Add network builder and platform APIs (aws#3939)" This reverts commit b8d2a7f. 12. Revert "Minor refactor of TTL cache (allow nil TTL + get TTL)" This reverts commit 4b33664. 13. Revert "Move httpclient to ecs-agent/ and minor refactor" This reverts commit 041ca3f. 14. Revert "Add missing EBSTaskAttach changes from feature branch" This reverts commit 7a0d116.

1. Revert "Load Managed Daemon images in background (#3984)" This reverts commit 75db5cb. 2. Revert "update daemon log path host root in container (#3982)" This reverts commit 6166b6e. 3. Revert "Fix loading CSI driver container from state if it exists" This reverts commit 1391502. 4. Revert "Latest FE model files (#3974)" This reverts commit b589a08. 5. Revert "Move utils function Uint16SliceToStringSlice to shared lib" This reverts commit 882bcb2. 6. Revert "Move remaining credentials module to shared library" This reverts commit f58f228. 7. Revert "fix merge import failure (#3976)" This reverts commit 46e21e6. 8. Revert "Fixing task volumes from payload for EBS-backed tasks" This reverts commit fc97633. 9. Revert "Create Generic Attachment to decouple from ENI and save attachment state. (#3969)" This reverts commit 12e23ed. 10. Revert "Add support for finding EBS devices on Xen instances (#3971)" This reverts commit 3500ac2. 11. Revert "Add network builder and platform APIs (#3939)" This reverts commit b8d2a7f. 12. Revert "Minor refactor of TTL cache (allow nil TTL + get TTL)" This reverts commit 4b33664. 13. Revert "Move httpclient to ecs-agent/ and minor refactor" This reverts commit 041ca3f. 14. Revert "Add missing EBSTaskAttach changes from feature branch" This reverts commit 7a0d116.

Add network builder and platform APIs

f790a3c

Added the network builder and platform APIs into the shared library package.

samjkon requested a review from a team as a code owner September 29, 2023 02:01

samjkon added the bot/test label Sep 29, 2023

amazon-ecs-bot removed the bot/test label Sep 29, 2023

Samuel Konat and others added 6 commits September 29, 2023 15:26

Fix failing checks

f369d80

Merge branch 'aws:dev' into dev

05951d2

Fix import issues

693fbe5

Fix vendor files

99ac0bf

Fix windows code

bb1f92c

Fix vendor update issue

197e290

samjkon added the bot/test label Sep 29, 2023

amazon-ecs-bot removed the bot/test label Sep 29, 2023

Add primary attribute to network interface

0bbd262

samjkon added the bot/test label Oct 2, 2023

amazon-ecs-bot removed the bot/test label Oct 2, 2023

Merge branch 'aws:dev' into dev

2cf471a

samjkon added the bot/test label Oct 9, 2023

amazon-ecs-bot removed the bot/test label Oct 9, 2023

Realmonia reviewed Oct 17, 2023

View reviewed changes

fierlion reviewed Oct 17, 2023

View reviewed changes

samjkon and others added 2 commits October 17, 2023 12:46

Merge branch 'aws:dev' into dev

d40005d

Address review comments

4da2af6

samjkon added the bot/test label Oct 18, 2023

amazon-ecs-bot removed the bot/test label Oct 18, 2023

Realmonia approved these changes Oct 18, 2023

View reviewed changes

chienhanlin reviewed Oct 19, 2023

View reviewed changes

chienhanlin approved these changes Oct 19, 2023

View reviewed changes

samjkon merged commit b8d2a7f into aws:dev Oct 19, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add network builder and platform APIs #3939

Add network builder and platform APIs #3939

samjkon commented Sep 29, 2023

Realmonia left a comment

fierlion Oct 17, 2023

samjkon Oct 17, 2023

fierlion Oct 17, 2023

samjkon Oct 17, 2023

fierlion Oct 17, 2023

samjkon Oct 17, 2023

fierlion Oct 17, 2023

samjkon Oct 17, 2023

chienhanlin Oct 19, 2023

samjkon Oct 19, 2023

Add network builder and platform APIs #3939

Add network builder and platform APIs #3939

Conversation

samjkon commented Sep 29, 2023

Summary

Implementation details

Testing

Description for the changelog

Licensing

Realmonia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment