Update containernetworking/cni dependency to v1.1.2 and the vpc-cni plugin version #3702

ohsoo · 2023-05-17T23:29:59Z

Summary

Update the CNI dependency to v1.1.2 and the amazon-vpc-cni-plugin submodule

Implementation details

CNI Spec Version used will now be 1.0.0, version convert logic simplified, and unnecessary constant currentCNISpec removed.
go.mod updated to use the newer version
Remove references to github.com/containernetworking/cni/pkg/types/current and instead refer to github.com/containernetworking/cni/pkg/types/100
Regenerate relevant mock files
Pull in the newest commits of the vpc cni plugin that include an upgrade to its CNI dependency to v1.1.2 and new support for CNI spec version 1.0.0.

Testing

Ran existing tests.

Manual tests were also run to verify that there are no errors with the DEL command (nor any other failures) with any of the different network modes.

New tests cover the changes: no

Description for the changelog

Update the containernetworking/cni dependency to v1.1.2 and the vpc-cni plugin version

Licensing

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

agent/ecscni/plugin_linux.go

danehlim

Perhaps out of scope of this PR, but would we want to do a similar upgrade with our github.com/containernetworking/plugins dependency?

fierlion · 2023-05-24T18:14:05Z

agent/ecscni/namespace_helper.go

@@ -16,9 +16,10 @@ package ecscni
 import (
 	"context"

+	current "github.com/containernetworking/cni/pkg/types/100"


aws/amazon-ecs-cni-plugins@f216cc7

We'll need to validate that the updated version doesn't break any of our calls. The above change in the cni-plugins library was reverted because the api contract changed between versions, which meant that the delete function for IPAM cleanup was not being executed.

+1. It would be great if you add the Testing Done section to the PR. The DEL command of the CNI plugin can fail silently. It would be nice to make sure there are no failures for an awsvpc task with/without trunking.

Testing has been completed with the latest changes to the PR (both commits), and we have verified that there were no errors with the DEL command (nor any other failures.) ✅

agent/engine/docker_task_engine_linux_test.go

ofiliz · 2023-06-09T22:56:59Z

agent/ecscni/plugin_windows_test.go

 	"github.com/golang/mock/gomock"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/containernetworking/cni/libcni"
+	current "github.com/containernetworking/cni/pkg/types/100"


"Current" is too generic for a package name. Can we please call this "cniTypesCurrent" to remain consistent with how we name it in CNI repos? Multiple instances in this PR.

Thanks for pointing this out, I'll update the naming.

ofiliz · 2023-06-09T22:57:43Z

agent/ecscni/plugin_windows.go

 	"github.com/cihub/seelog"
 	"github.com/containernetworking/cni/libcni"
-	"github.com/containernetworking/cni/pkg/types/current"
+	cnitypes "github.com/containernetworking/cni/pkg/types"
+	current "github.com/containernetworking/cni/pkg/types/100"


See my other comment. "cniTypes" and "cniTypesCurrent" might be better names.

ofiliz · 2023-06-15T19:26:41Z

agent/config/config_unix_test.go

 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/aws/amazon-ecs-agent/agent/dockerclient"
+	"github.com/aws/amazon-ecs-agent/agent/ec2"


Nit: The convention is to list the packages imported from the same repo (github.com/aws/amazon-ecs-agent) first, and then 3rd party repos. Hence the original position seem correct. Were these changes made by your editor, or by you?

This was done by my editor, I have some settings enabled to organize specific groupings but it won't re-order the groups. A lot of the import groupings/ordering throughout the repo aren't conventional/organized so if I touch a file, the imports are almost always automatically modified. A full re-ordering causes a larger diff which isn't important for this PR but I think this is a good step towards correcting these files for now.

Forgot to mention: the original placement was deemed incorrect because they were grouped with 3rd party imports.

Agreed that we don't need a full re-ordering. But if you do touch them (as in here), please make sure you follow the convention of
Group1: Standard go packages (fmt, net, etc.)
Group2: Packages from same repo (anything that starts with amazon-ecs-agent)
Gorup3: Everything else.

ofiliz · 2023-06-15T19:30:49Z

agent/config/config_unix_test.go

-	"github.com/aws/amazon-ecs-agent/agent/dockerclient"
-	"github.com/aws/amazon-ecs-agent/agent/ec2"
-	cnitypes "github.com/containernetworking/cni/pkg/types"
+	cniTypes "github.com/containernetworking/cni/pkg/types"


Nit: Thanks for making these changes. I didn't know that some parts of the agent code already used the alias "cnitypes". My original comment was only about renaming "types" and "current" to "cniTypes" and "cniTypesCurrent". We didn't need to touch code that used "cnitypes", we probably don't care about lowercase vs. uppercase "t".

No worries, we can just leave these as-is since they're here now.

ofiliz · 2023-06-15T19:41:27Z

agent/ecscni/plugin.go


-const (
-	currentCNISpec = "0.3.1"
+	"github.com/aws/amazon-ecs-agent/ecs-agent/logger"


Same as my comment before. Not sure why we moved the location of the logger package import.

ohsoo added the bot/test label May 17, 2023

ohsoo changed the base branch from master to dev May 17, 2023 23:40

ohsoo force-pushed the update-cni-current-dep branch 4 times, most recently from 065763e to 9e8cc30 Compare May 18, 2023 14:50

ohsoo added bot/test and removed bot/test labels May 18, 2023

ohsoo marked this pull request as ready for review May 18, 2023 15:00

ohsoo requested a review from a team as a code owner May 18, 2023 15:00

fierlion added bot/test and removed bot/test labels May 18, 2023

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

ohsoo force-pushed the update-cni-current-dep branch from 9e8cc30 to 1eb0a87 Compare May 19, 2023 23:30

danehlim added the bot/test label May 22, 2023

amazon-ecs-bot removed the bot/test label May 22, 2023

ohsoo force-pushed the update-cni-current-dep branch from 1eb0a87 to 0b2a4b7 Compare May 22, 2023 18:45

danehlim added the bot/test label May 22, 2023

amazon-ecs-bot removed the bot/test label May 22, 2023

fierlion reviewed May 24, 2023

View reviewed changes

agent/ecscni/plugin_linux.go Outdated Show resolved Hide resolved

danehlim previously approved these changes May 24, 2023

View reviewed changes

fierlion reviewed May 24, 2023

View reviewed changes

ohsoo requested a review from ofiliz May 25, 2023 22:28

ofiliz reviewed Jun 9, 2023

View reviewed changes

ohsoo dismissed danehlim’s stale review via dd1e8b0 June 13, 2023 20:05

ohsoo force-pushed the update-cni-current-dep branch from 0b2a4b7 to dd1e8b0 Compare June 13, 2023 20:05

Update containernetworking/cni dependency to v1.1.2

6733da5

ohsoo force-pushed the update-cni-current-dep branch from dd1e8b0 to 6733da5 Compare June 13, 2023 20:18

fierlion added the bot/test label Jun 13, 2023

amazon-ecs-bot removed the bot/test label Jun 13, 2023

Update VPC CNI plugin submodule

f946a86

ohsoo changed the title ~~Update containernetworking/cni dependency to v1.1.2~~ Update containernetworking/cni dependency to v1.1.2 and the vpc-cni plugin Jun 13, 2023

ohsoo changed the title ~~Update containernetworking/cni dependency to v1.1.2 and the vpc-cni plugin~~ Update containernetworking/cni dependency to v1.1.2 and the vpc-cni plugin version Jun 13, 2023

fierlion added the bot/test label Jun 13, 2023

amazon-ecs-bot removed the bot/test label Jun 13, 2023

ubhattacharjya approved these changes Jun 15, 2023

View reviewed changes

ofiliz approved these changes Jun 15, 2023

View reviewed changes

fierlion approved these changes Jun 15, 2023

View reviewed changes

fierlion merged commit 7565a9a into aws:dev Jun 15, 2023

prateekchaudhry mentioned this pull request Jun 22, 2023

Release 1.73.0 #3759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update containernetworking/cni dependency to v1.1.2 and the vpc-cni plugin version #3702

Update containernetworking/cni dependency to v1.1.2 and the vpc-cni plugin version #3702

ohsoo commented May 17, 2023 •

edited

Loading

danehlim left a comment

fierlion May 24, 2023

ubhattacharjya May 24, 2023

ohsoo Jun 15, 2023

ofiliz Jun 9, 2023

ohsoo Jun 13, 2023

ofiliz Jun 9, 2023

ofiliz Jun 15, 2023

ohsoo Jun 15, 2023

ohsoo Jun 15, 2023

ofiliz Jun 15, 2023

ofiliz Jun 15, 2023

ohsoo Jun 15, 2023

ofiliz Jun 15, 2023

Update containernetworking/cni dependency to v1.1.2 and the vpc-cni plugin version #3702

Update containernetworking/cni dependency to v1.1.2 and the vpc-cni plugin version #3702

Conversation

ohsoo commented May 17, 2023 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

danehlim 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohsoo commented May 17, 2023 •

edited

Loading