Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CNI to ecs-agent/netlib/model and update dependencies #3897

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

Realmonia
Copy link
Contributor

@Realmonia Realmonia commented Sep 11, 2023

Summary

This PR introduces CNI models from Fargate agent implementation to ecs-agent module. Later ecs agent will be integrate with those models.

Implementation details

Besides mentioned change, I also updated some dependency version to match Fargate's dependency versions. Notably:

  • github.com/Microsoft/hcsshim from 0.8.25 to 0.9.9
  • github.com/stretchr/testify from 1.7.0 to 1.8.1
  • github.com/vishvananda/netlink from 1.1.1-0.20201029203352-d40f9887b852 to 1.2.1-beta.2
  • golang.org/x/sys 0.6.0 to 0.7.0
  • golang.org/x/tools 0.6.0 to 0.7.0

github.com/vishvananda/netlink upgrade to is a beta version but it has been out for a year and fargate is using it. And in our codebase, we are simply using LinkByName and LinkList methods that are not modified.

One dependency update we want to do but contains a breaking change is github.com/containernetworking/plugins. In the upgrade from 0.9.1 to 1.3.0, it now depends on containerd/cgroup 1.1.0, which contains a breaking change. It is later fixed in v3.0.2 (thanks @mythri-garaga and @singholt), but to integrate with that version we need to change non trivial code in agent. This will be done in a follow up PR.

Related links: #3623 containerd/cgroups#296

SetV2NDstPortAndDeviceName's function signature is changed. For Fargate to consume this, it needs to do data client operations before entering this function.

Also removed vpctunnel_linux_integ_test due to the different plugin path/setup in agent. For amazon-ecs-agent, currently integration test does not build CNI plugins, while Fargate does. These types of integration tests should be added when the code is actually integrated with agent.

Testing

New tests cover the changes: Yes. Added unit tests.

Note that - since the added changes are imported from Fargate agent, we do not build CNI plugins in unit/integ test; we can change this behavior (make the integ test builds cni plugins too), or move it to functional tests when we integrate with netlib but will call it out of scope for this PR. A list of missing/removed tests due to this limitation follows:

  • func (c *cniClient) Version(plugin string) (string, error) test in client.go. Agent currently checks the VERSION value in git submodule instead of calling --version command using built plugin.
  • func SetV2NDstPortAndDeviceName(taskENI *networkinterface.NetworkInterface, dstPort uint16) test in vpctunnel_config.go as mentioned in earlier section of this PR description.

Passed Github workflow.

Description for the changelog

Add CNI to ecs-agent/netlib/model and update dependencies

Licensing

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

@Realmonia Realmonia changed the title add netlibcni Add CNI to ecs-agent/netlib/model Sep 11, 2023
@Realmonia Realmonia force-pushed the netlibcni branch 4 times, most recently from fd75e83 to 464497c Compare September 11, 2023 22:32
@Realmonia Realmonia changed the title Add CNI to ecs-agent/netlib/model Add CNI to ecs-agent/netlib/model and update dependencies Sep 11, 2023
@Realmonia Realmonia force-pushed the netlibcni branch 2 times, most recently from e51e394 to a3cbe0f Compare September 11, 2023 22:50
@Realmonia Realmonia marked this pull request as ready for review September 11, 2023 23:07
@Realmonia Realmonia requested a review from a team as a code owner September 11, 2023 23:07
@Realmonia Realmonia force-pushed the netlibcni branch 13 times, most recently from e643672 to 393ed6f Compare September 13, 2023 09:28
@Realmonia Realmonia force-pushed the netlibcni branch 12 times, most recently from 6c7b434 to e8986fe Compare September 14, 2023 04:40
samjkon
samjkon previously approved these changes Sep 14, 2023
mye956
mye956 previously approved these changes Sep 14, 2023
@Realmonia Realmonia merged commit 6d77a26 into aws:dev Sep 21, 2023
7 checks passed
@Realmonia Realmonia deleted the netlibcni branch September 22, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants