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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Jun 15, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6733da5
Update containernetworking/cni dependency to v1.1.2
ohsoo May 17, 2023
f946a86
Update VPC CNI plugin submodule
ohsoo Jun 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions agent/config/config_unix_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@ import (
"testing"
"time"

"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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

func TestConfigDefault(t *testing.T) {
Expand Down Expand Up @@ -124,7 +125,7 @@ func TestConfigFromFile(t *testing.T) {
assert.Equal(t, testPauseImageName, cfg.PauseContainerImageName, "should read PauseContainerImageName")
assert.Equal(t, testPauseTag, cfg.PauseContainerTag, "should read PauseContainerTag")
assert.Equal(t, 1, len(cfg.AWSVPCAdditionalLocalRoutes), "should have one additional local route")
expectedLocalRoute, err := cnitypes.ParseCIDR("169.254.172.1/32")
expectedLocalRoute, err := cniTypes.ParseCIDR("169.254.172.1/32")
assert.NoError(t, err)
assert.Equal(t, expectedLocalRoute.IP, cfg.AWSVPCAdditionalLocalRoutes[0].IP, "should match expected route IP")
assert.Equal(t, expectedLocalRoute.Mask, cfg.AWSVPCAdditionalLocalRoutes[0].Mask, "should match expected route Mask")
Expand Down
6 changes: 3 additions & 3 deletions agent/config/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/utils"

"github.com/cihub/seelog"
cnitypes "github.com/containernetworking/cni/pkg/types"
cniTypes "github.com/containernetworking/cni/pkg/types"
"github.com/docker/go-connections/nat"
)

Expand Down Expand Up @@ -216,8 +216,8 @@ func parseInstanceAttributes(errs []error) (map[string]string, []error) {
return instanceAttributes, errs
}

func parseAdditionalLocalRoutes(errs []error) ([]cnitypes.IPNet, []error) {
var additionalLocalRoutes []cnitypes.IPNet
func parseAdditionalLocalRoutes(errs []error) ([]cniTypes.IPNet, []error) {
var additionalLocalRoutes []cniTypes.IPNet
additionalLocalRoutesEnv := os.Getenv("ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES")
if additionalLocalRoutesEnv != "" {
err := json.Unmarshal([]byte(additionalLocalRoutesEnv), &additionalLocalRoutes)
Expand Down
7 changes: 4 additions & 3 deletions agent/config/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ package config
import (
"time"

cniTypes "github.com/containernetworking/cni/pkg/types"

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

// ImagePullBehaviorType is an enum variable type corresponding to different agent pull
Expand Down Expand Up @@ -247,12 +248,12 @@ type Config struct {
// will limit you to running one `awsvpc` task at a time. IPv4 addresses
// must be specified in decimal-octet form and also specify the subnet
// size (e.g., "169.254.172.42/22").
OverrideAWSVPCLocalIPv4Address *cnitypes.IPNet
OverrideAWSVPCLocalIPv4Address *cniTypes.IPNet

// AWSVPCAdditionalLocalRoutes allows the specification of routing table
// entries that will be added in the task's network namespace via the
// instance bridge interface rather than via the ENI.
AWSVPCAdditionalLocalRoutes []cnitypes.IPNet
AWSVPCAdditionalLocalRoutes []cniTypes.IPNet

// ContainerMetadataEnabled specifies if the agent should provide a metadata
// file for containers.
Expand Down
6 changes: 3 additions & 3 deletions agent/ecscni/mocks/ecscni_mocks.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions agent/ecscni/mocks/namespace_helper_mocks.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions agent/ecscni/namespace_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,17 @@ package ecscni
import (
"context"

cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100"

"github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi"
apieni "github.com/aws/amazon-ecs-agent/ecs-agent/api/eni"
"github.com/containernetworking/cni/pkg/types/current"
)

// NamespaceHelper defines the methods for performing additional actions to setup/clean the task namespace.
// Task namespace in awsvpc network mode is configured using pause container which is the first container
// launched for the task. These commands are executed inside that container.
type NamespaceHelper interface {
ConfigureTaskNamespaceRouting(ctx context.Context, taskENI *apieni.ENI, config *Config, result *current.Result) error
ConfigureTaskNamespaceRouting(ctx context.Context, taskENI *apieni.ENI, config *Config, result *cniTypesCurrent.Result) error
}

// helper is the client for executing methods of NamespaceHelper interface.
Expand Down
5 changes: 3 additions & 2 deletions agent/ecscni/namespace_helper_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,13 @@ package ecscni
import (
"context"

cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100"

apieni "github.com/aws/amazon-ecs-agent/ecs-agent/api/eni"
"github.com/containernetworking/cni/pkg/types/current"
)

// ConfigureTaskNamespaceRouting executes the commands required for setting up appropriate routing inside task namespace.
// This is applicable only for Windows.
func (nsHelper *helper) ConfigureTaskNamespaceRouting(ctx context.Context, taskENI *apieni.ENI, config *Config, result *current.Result) error {
func (nsHelper *helper) ConfigureTaskNamespaceRouting(ctx context.Context, taskENI *apieni.ENI, config *Config, result *cniTypesCurrent.Result) error {
return nil
}
5 changes: 3 additions & 2 deletions agent/ecscni/namespace_helper_unsupported.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,13 @@ package ecscni
import (
"context"

cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100"

apieni "github.com/aws/amazon-ecs-agent/ecs-agent/api/eni"
"github.com/containernetworking/cni/pkg/types/current"
)

// ConfigureTaskNamespaceRouting executes the commands required for setting up appropriate routing inside task namespace.
// This is applicable only for Windows.
func (nsHelper *helper) ConfigureTaskNamespaceRouting(ctx context.Context, taskENI *apieni.ENI, config *Config, result *current.Result) error {
func (nsHelper *helper) ConfigureTaskNamespaceRouting(ctx context.Context, taskENI *apieni.ENI, config *Config, result *cniTypesCurrent.Result) error {
return nil
}
9 changes: 5 additions & 4 deletions agent/ecscni/namespace_helper_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,13 @@ import (
"net"
"strings"

"github.com/aws/amazon-ecs-agent/agent/dockerclient"
apieni "github.com/aws/amazon-ecs-agent/ecs-agent/api/eni"
"github.com/cihub/seelog"
"github.com/containernetworking/cni/pkg/types/current"
cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100"
"github.com/docker/docker/api/types"
"github.com/pkg/errors"

"github.com/aws/amazon-ecs-agent/agent/dockerclient"
apieni "github.com/aws/amazon-ecs-agent/ecs-agent/api/eni"
)

const (
Expand Down Expand Up @@ -57,7 +58,7 @@ const (
// netsh interface ipv4 add route prefix=169.254.169.254/32 interface="vEthernet (task-br-<mac>-ep-<container_id>)
// netsh interface ipv4 add route prefix=169.254.169.254/32 interface="Loopback"
// netsh interface ipv4 add route prefix=<local-route> interface="vEthernet (nat-ep-<container_id>)
func (nsHelper *helper) ConfigureTaskNamespaceRouting(ctx context.Context, taskENI *apieni.ENI, config *Config, result *current.Result) error {
func (nsHelper *helper) ConfigureTaskNamespaceRouting(ctx context.Context, taskENI *apieni.ENI, config *Config, result *cniTypesCurrent.Result) error {
// Obtain the ecs-bridge endpoint's subnet IP address from the CNI plugin execution result.
ecsBridgeSubnetIPAddress := &net.IPNet{
IP: result.IPs[0].Address.IP.Mask(result.IPs[0].Address.Mask),
Expand Down
18 changes: 9 additions & 9 deletions agent/ecscni/namespace_helper_windows_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,25 +24,25 @@ import (
"testing"
"time"

cnitypes "github.com/containernetworking/cni/pkg/types"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"

cniTypes "github.com/containernetworking/cni/pkg/types"
cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100"
"github.com/docker/docker/api/types"
"github.com/stretchr/testify/assert"

mock_dockerapi "github.com/aws/amazon-ecs-agent/agent/dockerclient/dockerapi/mocks"
"github.com/containernetworking/cni/pkg/types/current"
"github.com/golang/mock/gomock"
)

const (
containerID = "abc12345"
containerExecID = "container1234"
)

// getECSBridgeResult returns an instance of current.Result.
func getECSBridgeResult() *current.Result {
return &current.Result{
IPs: []*current.IPConfig{{
// getECSBridgeResult returns an instance of cniTypesCurrent.Result.
func getECSBridgeResult() *cniTypesCurrent.Result {
return &cniTypesCurrent.Result{
IPs: []*cniTypesCurrent.IPConfig{{
Address: net.IPNet{
IP: net.ParseIP(ipv4),
Mask: net.CIDRMask(24, 32),
Expand Down Expand Up @@ -77,7 +77,7 @@ func TestConfigureTaskNamespaceRouting(t *testing.T) {
cniConfig := getCNIConfig()
cniConfig.BlockInstanceMetadata = tt.blockIMDS

cniConfig.AdditionalLocalRoutes = append(cniConfig.AdditionalLocalRoutes, cnitypes.IPNet{
cniConfig.AdditionalLocalRoutes = append(cniConfig.AdditionalLocalRoutes, cniTypes.IPNet{
IP: net.ParseIP("10.0.0.0"),
Mask: net.CIDRMask(24, 32),
})
Expand Down
4 changes: 2 additions & 2 deletions agent/ecscni/netconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
"github.com/aws/amazon-ecs-agent/ecs-agent/logger"

"github.com/containernetworking/cni/libcni"
cnitypes "github.com/containernetworking/cni/pkg/types"
cniTypes "github.com/containernetworking/cni/pkg/types"
)

// newNetworkConfig converts a network config to libcni's NetworkConfig.
Expand All @@ -35,7 +35,7 @@ func newNetworkConfig(netcfg interface{}, plugin string, cniVersion string) (*li
}

netConfig := &libcni.NetworkConfig{
Network: &cnitypes.NetConf{
Network: &cniTypes.NetConf{
Type: plugin,
CNIVersion: cniVersion,
Name: defaultNetworkName,
Expand Down
6 changes: 3 additions & 3 deletions agent/ecscni/netconfig_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (

"github.com/cihub/seelog"
"github.com/containernetworking/cni/libcni"
cnitypes "github.com/containernetworking/cni/pkg/types"
cniTypes "github.com/containernetworking/cni/pkg/types"
)

// NewBridgeNetworkConfig creates the config of bridge for ADD command, where
Expand Down Expand Up @@ -89,7 +89,7 @@ func newIPAMConfig(cfg *Config) (IPAMConfig, error) {
return IPAMConfig{}, err
}

routes := []*cnitypes.Route{
routes := []*cniTypes.Route{
{
Dst: *dst,
},
Expand All @@ -98,7 +98,7 @@ func newIPAMConfig(cfg *Config) (IPAMConfig, error) {
for _, route := range cfg.AdditionalLocalRoutes {
seelog.Debugf("[ECSCNI] Adding an additional route for %s", route)
ipNetRoute := (net.IPNet)(route)
routes = append(routes, &cnitypes.Route{Dst: ipNetRoute})
routes = append(routes, &cniTypes.Route{Dst: ipNetRoute})
}

ipamConfig := IPAMConfig{
Expand Down
11 changes: 4 additions & 7 deletions agent/ecscni/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,12 @@ import (
"sync"
"time"

"github.com/aws/amazon-ecs-agent/ecs-agent/logger"
"github.com/cihub/seelog"
"github.com/containernetworking/cni/libcni"
"github.com/containernetworking/cni/pkg/types/current"
cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100"
"github.com/pkg/errors"
)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

// CNIClient defines the method of setting/cleaning up container namespace
Expand All @@ -40,7 +37,7 @@ type CNIClient interface {
// Capabilities returns the capabilities supported by a plugin
Capabilities(string) ([]string, error)
// SetupNS sets up the namespace of container
SetupNS(context.Context, *Config, time.Duration) (*current.Result, error)
SetupNS(context.Context, *Config, time.Duration) (*cniTypesCurrent.Result, error)
// CleanupNS cleans up the container namespace
CleanupNS(context.Context, *Config, time.Duration) error
// ReleaseIPResource marks the ip available in the ipam db
Expand Down Expand Up @@ -96,7 +93,7 @@ func (client *cniClient) init() {
func (client *cniClient) SetupNS(
ctx context.Context,
cfg *Config,
timeout time.Duration) (*current.Result, error) {
timeout time.Duration) (*cniTypesCurrent.Result, error) {
ctx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()
return client.setupNS(ctx, cfg)
Expand Down
24 changes: 7 additions & 17 deletions agent/ecscni/plugin_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,13 @@ import (
"os"
"time"

"github.com/aws/amazon-ecs-agent/ecs-agent/logger"
"github.com/cihub/seelog"
"github.com/containernetworking/cni/libcni"
cnitypes "github.com/containernetworking/cni/pkg/types"
"github.com/containernetworking/cni/pkg/types/current"
cniTypes "github.com/containernetworking/cni/pkg/types"
cniTypesCurrent "github.com/containernetworking/cni/pkg/types/100"
"github.com/pkg/errors"

"github.com/aws/amazon-ecs-agent/ecs-agent/logger"
)

const (
Expand All @@ -45,10 +46,10 @@ func newCNIGuard() cniGuard {
}

// setupNS is the called by SetupNS to setup the task namespace by invoking ADD for given CNI configurations
func (client *cniClient) setupNS(ctx context.Context, cfg *Config) (*current.Result, error) {
func (client *cniClient) setupNS(ctx context.Context, cfg *Config) (*cniTypesCurrent.Result, error) {
seelog.Debugf("[ECSCNI] Setting up the container namespace %s", cfg.ContainerID)

var bridgeResult cnitypes.Result
var bridgeResult cniTypes.Result
runtimeConfig := libcni.RuntimeConf{
ContainerID: cfg.ContainerID,
NetNS: fmt.Sprintf(NetnsFormat, cfg.ContainerPID),
Expand Down Expand Up @@ -83,19 +84,8 @@ func (client *cniClient) setupNS(ctx context.Context, cfg *Config) (*current.Res
// Not every netns setup involves ECS Bridge Plugin
return nil, nil
}
if _, err := bridgeResult.GetAsVersion(currentCNISpec); err != nil {
seelog.Warnf("[ECSCNI] Unable to convert result to spec version %s; error: %v; result is of version: %s",
currentCNISpec, err, bridgeResult.Version())
return nil, err
}
var curResult *current.Result
curResult, ok := bridgeResult.(*current.Result)
if !ok {
return nil, errors.Errorf(
"cni setup: unable to convert result to expected version '%v'", bridgeResult)
}

return curResult, nil
return cniTypesCurrent.GetResult(bridgeResult)
}

// ReleaseIPResource marks the ip available in the ipam db
Expand Down
Loading