From 484695c9ee0154ee203b04c61f5afa91be69b035 Mon Sep 17 00:00:00 2001 From: Yiqiao Yu Date: Sun, 19 Jul 2026 21:07:45 -0700 Subject: [PATCH] cross account roles cilium --- operator/cmd/provider_aws_flags.go | 3 + operator/option/config.go | 13 ++ pkg/aws/ec2/ec2.go | 76 +++++++-- pkg/aws/ec2/mock/mock.go | 4 + pkg/aws/eni/crossaccount.go | 148 +++++++++++++++++ pkg/aws/eni/crossaccount_test.go | 259 +++++++++++++++++++++++++++++ pkg/aws/eni/instances.go | 1 + pkg/ipam/allocator/aws/aws.go | 27 ++- 8 files changed, 513 insertions(+), 18 deletions(-) create mode 100644 pkg/aws/eni/crossaccount.go create mode 100644 pkg/aws/eni/crossaccount_test.go diff --git a/operator/cmd/provider_aws_flags.go b/operator/cmd/provider_aws_flags.go index 72015f65678bb..a75f411efd0ea 100644 --- a/operator/cmd/provider_aws_flags.go +++ b/operator/cmd/provider_aws_flags.go @@ -50,5 +50,8 @@ func (hook *awsFlagsHooks) RegisterProviderFlag(cmd *cobra.Command, vp *viper.Vi flags.String(operatorOption.EC2APIEndpoint, "", "AWS API endpoint for the EC2 service") option.BindEnv(vp, operatorOption.EC2APIEndpoint) + flags.String(operatorOption.AWSCrossAccountRoleARN, "", "ARN of the IAM role in the dedicated rbx-cilium account for cross-account IPAM") + option.BindEnv(vp, operatorOption.AWSCrossAccountRoleARN) + vp.BindPFlags(flags) } diff --git a/operator/option/config.go b/operator/option/config.go index 41ea837e057a9..42f919653d634 100644 --- a/operator/option/config.go +++ b/operator/option/config.go @@ -131,6 +131,14 @@ const ( // node AWSUsePrimaryAddress = "aws-use-primary-address" + // AWSCrossAccountRoleARN is the ARN of the IAM role in the dedicated rbx-cilium account. + // When not empty, the Cilium Operator assumes this role to execute ENI lifecycle operations + // (create, delete, assign/unassign secondary IPs) in the target account. The local cluster + // instance profile continues to handle local instance-level operations (attach, describe instances). + // Required to enable cross-account IPAM when managing pod subnets shared via AWS RAM + // into the rbx-cilium sandbox account. + AWSCrossAccountRoleARN = "aws-cross-account-role" + // Azure options // AzureSubscriptionID is the subscription ID to use when accessing the Azure API @@ -332,6 +340,10 @@ type OperatorConfig struct { // e.g. "ec2-fips.us-west-1.amazonaws.com" to use a FIPS endpoint in the us-west-1 region. EC2APIEndpoint string + // AWSCrossAccountRoleARN is the ARN of the IAM role in the rbx-cilium account assumed + // for cross-account ENI lifecycle operations. + AWSCrossAccountRoleARN string + // Azure options // AzureSubscriptionID is the subscription ID to use when accessing the Azure API @@ -451,6 +463,7 @@ func (c *OperatorConfig) Populate(logger *slog.Logger, vp *viper.Viper) { c.AWSEnablePrefixDelegation = vp.GetBool(AWSEnablePrefixDelegation) c.AWSUsePrimaryAddress = vp.GetBool(AWSUsePrimaryAddress) c.EC2APIEndpoint = vp.GetString(EC2APIEndpoint) + c.AWSCrossAccountRoleARN = vp.GetString(AWSCrossAccountRoleARN) c.ExcessIPReleaseDelay = vp.GetInt(ExcessIPReleaseDelay) c.ENIGarbageCollectionInterval = vp.GetDuration(ENIGarbageCollectionInterval) diff --git a/pkg/aws/ec2/ec2.go b/pkg/aws/ec2/ec2.go index 99427459c4005..4f1ee8574e53e 100644 --- a/pkg/aws/ec2/ec2.go +++ b/pkg/aws/ec2/ec2.go @@ -16,9 +16,11 @@ import ( "github.com/aws/aws-sdk-go-v2/aws/retry" awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http" awsconfig "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/credentials/stscreds" "github.com/aws/aws-sdk-go-v2/feature/ec2/imds" "github.com/aws/aws-sdk-go-v2/service/ec2" ec2_types "github.com/aws/aws-sdk-go-v2/service/ec2/types" + "github.com/aws/aws-sdk-go-v2/service/sts" "github.com/cilium/cilium/pkg/api/helpers" eniTypes "github.com/cilium/cilium/pkg/aws/eni/types" @@ -42,22 +44,23 @@ const ( // requires looking at the error message to get the actual reason. See SubnetFullErrMsgStr for example. InvalidParameterValueStr = "InvalidParameterValue" - AssignPrivateIpAddresses = "AssignPrivateIpAddresses" - AssociateAddress = "AssociateAddress" - AttachNetworkInterface = "AttachNetworkInterface" - CreateNetworkInterface = "CreateNetworkInterface" - DeleteNetworkInterface = "DeleteNetworkInterface" - DescribeAddresses = "DescribeAddresses" - DescribeInstances = "DescribeInstances" - DescribeInstanceTypes = "DescribeInstanceTypes" - DescribeNetworkInterfaces = "DescribeNetworkInterfaces" - DescribeSecurityGroups = "DescribeSecurityGroups" - DescribeSubnets = "DescribeSubnets" - DescribeVpcs = "DescribeVpcs" - DescribeRouteTables = "DescribeRouteTables" - ModifyNetworkInterface = "ModifyNetworkInterface" - ModifyNetworkInterfaceAttribute = "ModifyNetworkInterfaceAttribute" - UnassignPrivateIpAddresses = "UnassignPrivateIpAddresses" + AssignPrivateIpAddresses = "AssignPrivateIpAddresses" + AssociateAddress = "AssociateAddress" + AttachNetworkInterface = "AttachNetworkInterface" + CreateNetworkInterface = "CreateNetworkInterface" + CreateNetworkInterfacePermission = "CreateNetworkInterfacePermission" + DeleteNetworkInterface = "DeleteNetworkInterface" + DescribeAddresses = "DescribeAddresses" + DescribeInstances = "DescribeInstances" + DescribeInstanceTypes = "DescribeInstanceTypes" + DescribeNetworkInterfaces = "DescribeNetworkInterfaces" + DescribeSecurityGroups = "DescribeSecurityGroups" + DescribeSubnets = "DescribeSubnets" + DescribeVpcs = "DescribeVpcs" + DescribeRouteTables = "DescribeRouteTables" + ModifyNetworkInterface = "ModifyNetworkInterface" + ModifyNetworkInterfaceAttribute = "ModifyNetworkInterfaceAttribute" + UnassignPrivateIpAddresses = "UnassignPrivateIpAddresses" ) var syslogAttr = []any{logfields.LogSubsys, "ec2"} @@ -124,6 +127,29 @@ func NewConfig(ctx context.Context) (aws.Config, error) { return cfg, nil } +// NewCrossAccountConfig returns an aws.Config that assumes the given IAM role ARN. +// The base config (with region and retry settings) is reused; maybe this will need a change? +func NewCrossAccountConfig(ctx context.Context, baseConfig aws.Config, roleARN string) (aws.Config, error) { + stsClient := sts.NewFromConfig(baseConfig) + creds := stscreds.NewAssumeRoleProvider(stsClient, roleARN) + cfg := baseConfig.Copy() + cfg.Credentials = aws.NewCredentialsCache(creds) + // make a call with it confirm the creds work rather than waiting for object's first call + if _, err := cfg.Credentials.Retrieve(ctx); err != nil { + return aws.Config{}, fmt.Errorf("unable to assume cross-account role %s: %w", roleARN, err) + } + return cfg, nil +} + +// GetLocalAccountID returns the AWS account ID of the instance running this process. +func GetLocalAccountID(ctx context.Context, cfg aws.Config) (string, error) { + doc, err := imds.NewFromConfig(cfg).GetInstanceIdentityDocument(ctx, &imds.GetInstanceIdentityDocumentInput{}) + if err != nil { + return "", fmt.Errorf("unable to retrieve instance identity document: %w", err) + } + return doc.AccountID, nil +} + // NewSubnetsFilters transforms a map of tags and values and a slice of subnets // into a slice of ec2.Filter adequate to filter AWS subnets. func NewSubnetsFilters(tags map[string]string, ids []string) []ec2_types.Filter { @@ -733,6 +759,24 @@ func (c *Client) CreateNetworkInterface(ctx context.Context, toAllocate int32, s return eni.ID, eni, nil } +// CreateNetworkInterfacePermission grants INSTANCE-ATTACH permission on the given ENI to a difft AWS account. +// This is required before an instance in accountID can attach an ENI owned by a different account. +func (c *Client) CreateNetworkInterfacePermission(ctx context.Context, eniID string, accountID string) error { + input := &ec2.CreateNetworkInterfacePermissionInput{ + NetworkInterfaceId: aws.String(eniID), + AwsAccountId: aws.String(accountID), + Permission: ec2_types.InterfacePermissionTypeInstanceAttach, + } + + // wrap this in a limiter + c.limiter.Limit(ctx, CreateNetworkInterfacePermission) + // track how long it takes + sinceStart := spanstat.Start() + _, err := c.ec2Client.CreateNetworkInterfacePermission(ctx, input) + c.metricsAPI.ObserveAPICall(CreateNetworkInterfacePermission, deriveStatus(err), sinceStart.Seconds()) + return err +} + // DeleteNetworkInterface deletes an ENI with the specified ID func (c *Client) DeleteNetworkInterface(ctx context.Context, eniID string) error { input := &ec2.DeleteNetworkInterfaceInput{ diff --git a/pkg/aws/ec2/mock/mock.go b/pkg/aws/ec2/mock/mock.go index 32a5c917f781d..9495225d4e529 100644 --- a/pkg/aws/ec2/mock/mock.go +++ b/pkg/aws/ec2/mock/mock.go @@ -798,3 +798,7 @@ func (e *API) GetSecurityGroups(ctx context.Context) (types.SecurityGroupMap, er func (e *API) GetInstanceTypes(ctx context.Context) ([]ec2_types.InstanceTypeInfo, error) { return e.instanceTypes, nil } + +func (e *API) CreateNetworkInterfacePermission(_ context.Context, _ string, _ string) error { + return nil +} diff --git a/pkg/aws/eni/crossaccount.go b/pkg/aws/eni/crossaccount.go new file mode 100644 index 0000000000000..fc33c85b32995 --- /dev/null +++ b/pkg/aws/eni/crossaccount.go @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Authors of Cilium + +package eni + +import ( + "context" + "log/slog" + + ec2_types "github.com/aws/aws-sdk-go-v2/service/ec2/types" + eniTypes "github.com/cilium/cilium/pkg/aws/eni/types" + "github.com/cilium/cilium/pkg/aws/types" + ipamTypes "github.com/cilium/cilium/pkg/ipam/types" + "github.com/cilium/cilium/pkg/logging/logfields" +) + +// CrossAccountEC2Client splits EC2 API calls between two accounts: +// - remote: ownwer of the VPC and pod subnets. This handles all ENI lifecycle operations +// - local: owner of the EC2 instances. This handles all instance-level operations including attachments +// +// After CreateNetworkInterface succeeds on the remote client, a +// CreateNetworkInterfacePermission call grants the local account INSTANCE-ATTACH +// access so that AttachNetworkInterface can be called from the local account. +type CrossAccountEC2Client struct { + logger *slog.Logger + local EC2API + remote EC2API + localAccountID string +} + +// NewCrossAccountEC2Client constructs a CrossAccountEC2Client. +// the localAccountID is needed to setup every CreateNetworkInterfacePermission +func NewCrossAccountEC2Client(logger *slog.Logger, local, remote EC2API, localAccountID string) *CrossAccountEC2Client { + return &CrossAccountEC2Client{ + logger: logger, + local: local, + remote: remote, + localAccountID: localAccountID, + } +} + +// ********************************************************************************* +// --- VPC / Subnet / ENI-owner accouint operations → remote (network account) --- +// ********************************************************************************* + +func (c *CrossAccountEC2Client) GetSubnets(ctx context.Context) (ipamTypes.SubnetMap, error) { + return c.remote.GetSubnets(ctx) +} + +func (c *CrossAccountEC2Client) GetVpcs(ctx context.Context) (ipamTypes.VirtualNetworkMap, error) { + return c.remote.GetVpcs(ctx) +} + +func (c *CrossAccountEC2Client) GetRouteTables(ctx context.Context) (ipamTypes.RouteTableMap, error) { + return c.remote.GetRouteTables(ctx) +} + +// Security groups must come from the remote (VPC-owning) account because +// CreateNetworkInterface executes there and cross-account SG references are rejected by AWS. +func (c *CrossAccountEC2Client) GetSecurityGroups(ctx context.Context) (types.SecurityGroupMap, error) { + return c.remote.GetSecurityGroups(ctx) +} + +func (c *CrossAccountEC2Client) GetDetachedNetworkInterfaces(ctx context.Context, tags ipamTypes.Tags, maxResults int32) ([]string, error) { + return c.remote.GetDetachedNetworkInterfaces(ctx, tags, maxResults) +} + +// CreateNetworkInterface creates the ENI in the remote account's subnet, then +// immediately grants the local account INSTANCE-ATTACH permission so that +// AttachNetworkInterface (local) can succeed. +func (c *CrossAccountEC2Client) CreateNetworkInterface(ctx context.Context, toAllocate int32, subnetID, desc string, groups []string, allocatePrefixes bool) (string, *eniTypes.ENI, error) { + eniID, eni, err := c.remote.CreateNetworkInterface(ctx, toAllocate, subnetID, desc, groups, allocatePrefixes) + if err != nil { + return "", nil, err + } + + if permErr := c.remote.CreateNetworkInterfacePermission(ctx, eniID, c.localAccountID); permErr != nil { + // Permission grant call failed. Delete the orphaned eni and rethrow + c.logger.Warn( + "Failed to grant cross-account ENI attach permission. Deleting orphaned ENI", + logfields.ENI, eniID, + logfields.Error, permErr, + ) + if delErr := c.remote.DeleteNetworkInterface(ctx, eniID); delErr != nil { + //TODO: maybe make a bigger deal of this + c.logger.Warn("Failed to delete orphaned ENI", + logfields.ENI, eniID, + logfields.Error, delErr, + ) + } + return "", nil, permErr + } + + return eniID, eni, nil +} + +func (c *CrossAccountEC2Client) CreateNetworkInterfacePermission(ctx context.Context, eniID string, accountID string) error { + return c.remote.CreateNetworkInterfacePermission(ctx, eniID, accountID) +} + +func (c *CrossAccountEC2Client) DeleteNetworkInterface(ctx context.Context, eniID string) error { + return c.remote.DeleteNetworkInterface(ctx, eniID) +} + +func (c *CrossAccountEC2Client) AssignPrivateIpAddresses(ctx context.Context, eniID string, addresses int32) ([]string, error) { + return c.remote.AssignPrivateIpAddresses(ctx, eniID, addresses) +} + +func (c *CrossAccountEC2Client) UnassignPrivateIpAddresses(ctx context.Context, eniID string, addresses []string) error { + return c.remote.UnassignPrivateIpAddresses(ctx, eniID, addresses) +} + +func (c *CrossAccountEC2Client) AssignENIPrefixes(ctx context.Context, eniID string, prefixes int32) error { + return c.remote.AssignENIPrefixes(ctx, eniID, prefixes) +} + +func (c *CrossAccountEC2Client) UnassignENIPrefixes(ctx context.Context, eniID string, prefixes []string) error { + return c.remote.UnassignENIPrefixes(ctx, eniID, prefixes) +} + +// ********************************************************************************* +// --- Instance-owner operations → local --- +// ********************************************************************************* + +func (c *CrossAccountEC2Client) GetInstance(ctx context.Context, vpcs ipamTypes.VirtualNetworkMap, subnets ipamTypes.SubnetMap, instanceID string) (*ipamTypes.Instance, error) { + return c.local.GetInstance(ctx, vpcs, subnets, instanceID) +} + +func (c *CrossAccountEC2Client) GetInstances(ctx context.Context, vpcs ipamTypes.VirtualNetworkMap, subnets ipamTypes.SubnetMap) (*ipamTypes.InstanceMap, error) { + return c.local.GetInstances(ctx, vpcs, subnets) +} + +// Needed so we can get max limits by type +func (c *CrossAccountEC2Client) GetInstanceTypes(ctx context.Context) ([]ec2_types.InstanceTypeInfo, error) { + return c.local.GetInstanceTypes(ctx) +} + +func (c *CrossAccountEC2Client) AttachNetworkInterface(ctx context.Context, index int32, instanceID, eniID string) (string, error) { + return c.local.AttachNetworkInterface(ctx, index, instanceID, eniID) +} + +func (c *CrossAccountEC2Client) ModifyNetworkInterface(ctx context.Context, eniID, attachmentID string, deleteOnTermination bool) error { + return c.local.ModifyNetworkInterface(ctx, eniID, attachmentID, deleteOnTermination) +} + +func (c *CrossAccountEC2Client) AssociateEIP(ctx context.Context, eniID string, eipTags ipamTypes.Tags) (string, error) { + return c.local.AssociateEIP(ctx, eniID, eipTags) +} diff --git a/pkg/aws/eni/crossaccount_test.go b/pkg/aws/eni/crossaccount_test.go new file mode 100644 index 0000000000000..545c06d1ef6cf --- /dev/null +++ b/pkg/aws/eni/crossaccount_test.go @@ -0,0 +1,259 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Authors of Cilium + +package eni + +import ( + "context" + "errors" + "testing" + + ec2_types "github.com/aws/aws-sdk-go-v2/service/ec2/types" + "github.com/cilium/hive/hivetest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + ec2mock "github.com/cilium/cilium/pkg/aws/ec2/mock" + eniTypes "github.com/cilium/cilium/pkg/aws/eni/types" + ipamTypes "github.com/cilium/cilium/pkg/ipam/types" +) + +// callTrackingAPI wraps an EC2API and records CreateNetworkInterfacePermission +// and DeleteNetworkInterface calls so tests can assert on call counts +type callTrackingAPI struct { + EC2API + + permissionCalls []permissionCall + deleteCalls []string + permissionErr error + deleteErr error +} + +type permissionCall struct { + eniID string + accountID string +} + +func (c *callTrackingAPI) CreateNetworkInterfacePermission(_ context.Context, eniID, accountID string) error { + c.permissionCalls = append(c.permissionCalls, permissionCall{eniID: eniID, accountID: accountID}) + return c.permissionErr +} + +func (c *callTrackingAPI) DeleteNetworkInterface(ctx context.Context, eniID string) error { + c.deleteCalls = append(c.deleteCalls, eniID) + if c.deleteErr != nil { + return c.deleteErr + } + return c.EC2API.DeleteNetworkInterface(ctx, eniID) +} + +func newTestCrossAccountClient(t *testing.T, local, remote EC2API, localAccountID string) *CrossAccountEC2Client { + t.Helper() + return NewCrossAccountEC2Client(hivetest.Logger(t), local, remote, localAccountID) +} + +var ( + remoteSubnets = []*ipamTypes.Subnet{ + {ID: "subnet-remote", AvailableAddresses: 50, VirtualNetworkID: "vpc-remote", AvailabilityZone: "us-east-1"}, + } + localSubnets = []*ipamTypes.Subnet{ + {ID: "subnet-local", AvailableAddresses: 50, VirtualNetworkID: "vpc-local", AvailabilityZone: "us-west-1"}, + } + remoteVpcs = []*ipamTypes.VirtualNetwork{ + {ID: "vpc-remote", PrimaryCIDR: "10.0.0.0/16"}, + } + localVpcs = []*ipamTypes.VirtualNetwork{ + {ID: "vpc-local", PrimaryCIDR: "192.168.0.0/16"}, + } + noRouteTables = []*ipamTypes.RouteTable{} +) + +// MADE BY AI. REVIEWED +// TestCrossAccountInfraOpsRouteToRemote verifies that infrastructure describe +// operations (subnets, vpcs, route tables) are forwarded to the remote +// (network-account) client and not the local client. +func TestCrossAccountInfraOpsRouteToRemote(t *testing.T) { + remote := ec2mock.NewAPI(remoteSubnets, remoteVpcs, nil, noRouteTables) + local := ec2mock.NewAPI(localSubnets, localVpcs, nil, noRouteTables) + client := newTestCrossAccountClient(t, local, remote, "111222333444") + ctx := t.Context() + + t.Run("GetSubnets returns remote subnets", func(t *testing.T) { + got, err := client.GetSubnets(ctx, "") + require.NoError(t, err) + assert.Contains(t, got, "subnet-remote") + assert.NotContains(t, got, "subnet-local") + }) + + t.Run("GetVpcs returns remote vpcs", func(t *testing.T) { + got, err := client.GetVpcs(ctx, "") + require.NoError(t, err) + assert.Contains(t, got, "vpc-remote") + assert.NotContains(t, got, "vpc-local") + }) + + t.Run("GetRouteTables returns remote route tables", func(t *testing.T) { + remoteWithRT := ec2mock.NewAPI(remoteSubnets, remoteVpcs, nil, []*ipamTypes.RouteTable{ + {ID: "rt-remote", VirtualNetworkID: "vpc-remote"}, + }) + localNoRT := ec2mock.NewAPI(localSubnets, localVpcs, nil, noRouteTables) + c := newTestCrossAccountClient(t, localNoRT, remoteWithRT, "111222333444") + + got, err := c.GetRouteTables(ctx, "") + require.NoError(t, err) + assert.Contains(t, got, "rt-remote") + }) +} + +// TestCrossAccountInstanceOpsRouteToLocal verifies that instance-level +// operations (instance types, instance enumeration) use the local client. +func TestCrossAccountInstanceOpsRouteToLocal(t *testing.T) { + remote := ec2mock.NewAPI(remoteSubnets, remoteVpcs, nil, noRouteTables) + local := ec2mock.NewAPI(localSubnets, localVpcs, nil, noRouteTables) + remote.UpdateInstanceTypes([]ec2_types.InstanceTypeInfo{{InstanceType: "m5.large"}}) + local.UpdateInstanceTypes([]ec2_types.InstanceTypeInfo{{InstanceType: "c5.xlarge"}}) + + client := newTestCrossAccountClient(t, local, remote, "111222333444") + ctx := t.Context() + + t.Run("GetInstanceTypes returns local instance types", func(t *testing.T) { + got, err := client.GetInstanceTypes(ctx) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, ec2_types.InstanceType("c5.xlarge"), got[0].InstanceType) + }) + + t.Run("GetInstances returns local instances", func(t *testing.T) { + localWithInstance := ec2mock.NewAPI(localSubnets, localVpcs, nil, noRouteTables) + localWithInstance.UpdateENIs(map[string]ec2mock.ENIMap{ + "i-local": { + "eni-1": &eniTypes.ENI{ + ID: "eni-1", + IP: "10.0.0.1", + Number: 0, + Subnet: eniTypes.AwsSubnet{ID: "subnet-local"}, + VPC: eniTypes.AwsVPC{ID: "vpc-local"}, + }, + }, + }) + remoteNoInstances := ec2mock.NewAPI(remoteSubnets, remoteVpcs, nil, noRouteTables) + c := newTestCrossAccountClient(t, localWithInstance, remoteNoInstances, "111222333444") + + got, err := c.GetInstances(ctx, nil, nil) + require.NoError(t, err) + assert.True(t, got.Exists("i-local"), "expected local instance to be present") + }) +} + +// TestCrossAccountCreateGrantsPermission verifies that after a successful +// CreateNetworkInterface on the remote, CreateNetworkInterfacePermission is +// called with the new ENI's ID and the configured local account ID. +func TestCrossAccountCreateGrantsPermission(t *testing.T) { + const localAccountID = "111122223333" + + remote := ec2mock.NewAPI(remoteSubnets, remoteVpcs, nil, noRouteTables) + tracking := &callTrackingAPI{EC2API: remote} + local := ec2mock.NewAPI(localSubnets, localVpcs, nil, noRouteTables) + client := newTestCrossAccountClient(t, local, tracking, localAccountID) + + eniID, _, err := client.CreateNetworkInterface(t.Context(), 5, "subnet-remote", "test-eni", nil, false) + require.NoError(t, err) + require.NotEmpty(t, eniID) + + require.Len(t, tracking.permissionCalls, 1, "expected exactly one permission grant") + assert.Equal(t, eniID, tracking.permissionCalls[0].eniID) + assert.Equal(t, localAccountID, tracking.permissionCalls[0].accountID) + assert.Empty(t, tracking.deleteCalls, "ENI should not be deleted on success") +} + +// TestCrossAccountCreateCleansUpOnPermissionFailure verifies that when +// CreateNetworkInterfacePermission fails, the freshly created ENI is +// immediately deleted and the error is propagated — preventing an orphaned ENI +// in the network account. +func TestCrossAccountCreateCleansUpOnPermissionFailure(t *testing.T) { + permErr := errors.New("simulated permission grant failure") + + remote := ec2mock.NewAPI(remoteSubnets, remoteVpcs, nil, noRouteTables) + tracking := &callTrackingAPI{EC2API: remote, permissionErr: permErr} + local := ec2mock.NewAPI(localSubnets, localVpcs, nil, noRouteTables) + client := newTestCrossAccountClient(t, local, tracking, "111122223333") + + eniID, eni, err := client.CreateNetworkInterface(t.Context(), 5, "subnet-remote", "test-eni", nil, false) + + assert.ErrorIs(t, err, permErr, "permission error should be propagated to caller") + assert.Empty(t, eniID) + assert.Nil(t, eni) + + require.Len(t, tracking.permissionCalls, 1, "permission grant should have been attempted once") + require.Len(t, tracking.deleteCalls, 1, "the orphaned ENI should have been deleted") + assert.Equal(t, tracking.permissionCalls[0].eniID, tracking.deleteCalls[0], + "the deleted ENI should be the same one whose permission grant failed") +} + +// TestCrossAccountCreateDoesNotGrantPermissionOnCreateFailure verifies that +// when the remote CreateNetworkInterface itself fails, no permission grant is +// attempted — there is no ENI to grant permission on. +func TestCrossAccountCreateDoesNotGrantPermissionOnCreateFailure(t *testing.T) { + remote := ec2mock.NewAPI(remoteSubnets, remoteVpcs, nil, noRouteTables) + remote.SetMockError(ec2mock.CreateNetworkInterface, errors.New("create failed")) + tracking := &callTrackingAPI{EC2API: remote} + local := ec2mock.NewAPI(localSubnets, localVpcs, nil, noRouteTables) + client := newTestCrossAccountClient(t, local, tracking, "111122223333") + + _, _, err := client.CreateNetworkInterface(t.Context(), 5, "subnet-remote", "test-eni", nil, false) + + require.Error(t, err) + assert.Empty(t, tracking.permissionCalls, "no permission grant should be attempted if ENI creation failed") + assert.Empty(t, tracking.deleteCalls) +} + +// TestCrossAccountAttachUsesLocalClient verifies that AttachNetworkInterface +// operates via the local (instance-owning account) client. In production this +// is required because AttachNetworkInterface must be called in the context of +// the account that owns the target instance. +func TestCrossAccountAttachUsesLocalClient(t *testing.T) { + ctx := t.Context() + + localMock := ec2mock.NewAPI(localSubnets, localVpcs, nil, noRouteTables) + remoteMock := ec2mock.NewAPI(remoteSubnets, remoteVpcs, nil, noRouteTables) + + // Create an unattached ENI directly in the local mock so AttachNetworkInterface + // has something to attach. We also prime an instance entry so the mock + // accepts the attach call. + localMock.UpdateENIs(map[string]ec2mock.ENIMap{"i-local": {}}) + eniID, _, err := localMock.CreateNetworkInterface(ctx, 1, "subnet-local", "test", nil, false) + require.NoError(t, err) + require.NotEmpty(t, eniID) + + // Make the same ENI unavailable in remote to confirm routing. + remoteMock.SetMockError(ec2mock.AttachNetworkInterface, errors.New("should not reach remote")) + + client := newTestCrossAccountClient(t, localMock, remoteMock, "111122223333") + + _, err = client.AttachNetworkInterface(ctx, 1, "i-local", eniID) + require.NoError(t, err, "attach should succeed via local client") +} + +// TestCrossAccountCreateOrphanedENIOnDeleteFailure verifies that when both +// CreateNetworkInterfacePermission and the subsequent cleanup DeleteNetworkInterface +// fail, the original permission error is still returned to the caller and the +// delete attempt is recorded — the ENI is acknowledged as orphaned via a log warning. +func TestCrossAccountCreateOrphanedENIOnDeleteFailure(t *testing.T) { + permErr := errors.New("simulated permission grant failure") + delErr := errors.New("simulated delete failure") + + remote := ec2mock.NewAPI(remoteSubnets, remoteVpcs, nil, noRouteTables) + tracking := &callTrackingAPI{EC2API: remote, permissionErr: permErr, deleteErr: delErr} + local := ec2mock.NewAPI(localSubnets, localVpcs, nil, noRouteTables) + client := newTestCrossAccountClient(t, local, tracking, "111122223333") + + eniID, eni, err := client.CreateNetworkInterface(t.Context(), 5, "subnet-remote", "test-eni", nil, false) + + assert.ErrorIs(t, err, permErr, "original permission error should be propagated even when delete also fails") + assert.Empty(t, eniID) + assert.Nil(t, eni) + + require.Len(t, tracking.permissionCalls, 1) + require.Len(t, tracking.deleteCalls, 1, "delete should have been attempted despite the permission error") + assert.Equal(t, tracking.permissionCalls[0].eniID, tracking.deleteCalls[0]) +} diff --git a/pkg/aws/eni/instances.go b/pkg/aws/eni/instances.go index 61be3c11e199c..3646e99338fb8 100644 --- a/pkg/aws/eni/instances.go +++ b/pkg/aws/eni/instances.go @@ -35,6 +35,7 @@ type EC2API interface { GetDetachedNetworkInterfaces(ctx context.Context, tags ipamTypes.Tags, maxResults int32) ([]string, error) CreateNetworkInterface(ctx context.Context, toAllocate int32, subnetID, desc string, groups []string, allocatePrefixes bool) (string, *eniTypes.ENI, error) AttachNetworkInterface(ctx context.Context, index int32, instanceID, eniID string) (string, error) + CreateNetworkInterfacePermission(ctx context.Context, eniID string, accountID string) error DeleteNetworkInterface(ctx context.Context, eniID string) error ModifyNetworkInterface(ctx context.Context, eniID, attachmentID string, deleteOnTermination bool) error AssignPrivateIpAddresses(ctx context.Context, eniID string, addresses int32) ([]string, error) diff --git a/pkg/ipam/allocator/aws/aws.go b/pkg/ipam/allocator/aws/aws.go index 02d0a6cb128d3..d8afe73c36f8e 100644 --- a/pkg/ipam/allocator/aws/aws.go +++ b/pkg/ipam/allocator/aws/aws.go @@ -26,12 +26,13 @@ import ( ) var subsysLogAttr = []any{logfields.LogSubsys, "ipam-allocator-aws"} +var _ eni.EC2API = (*eni.CrossAccountEC2Client)(nil) // AllocatorAWS is an implementation of IPAM allocator interface for AWS ENI type AllocatorAWS struct { rootLogger *slog.Logger logger *slog.Logger - client *ec2shim.Client + client eni.EC2API eniGCTags map[string]string } @@ -109,10 +110,32 @@ func (a *AllocatorAWS) Init(ctx context.Context, logger *slog.Logger) error { } } - a.client = ec2shim.NewClient(a.rootLogger, ec2.NewFromConfig(cfg, optionsFunc), aMetrics, operatorOption.Config.IPAMAPIQPSLimit, + localClient := ec2shim.NewClient(a.rootLogger, ec2.NewFromConfig(cfg, optionsFunc), aMetrics, operatorOption.Config.IPAMAPIQPSLimit, operatorOption.Config.IPAMAPIBurst, subnetsFilters, instancesFilters, eniCreationTags, operatorOption.Config.AWSUsePrimaryAddress) + if operatorOption.Config.AWSCrossAccountRoleARN == "" { + a.client = localClient + return nil + } + + a.logger.Debug("Cross-account ENI mode detected", "roleARN", operatorOption.Config.AWSCrossAccountRoleARN) + + crossAccountCfg, err := ec2shim.NewCrossAccountConfig(ctx, cfg, operatorOption.Config.AWSCrossAccountRoleARN) + if err != nil { + return fmt.Errorf("failed to create cross-account AWS config: %w", err) + } + + localAccountID, err := ec2shim.GetLocalAccountID(ctx, cfg) + if err != nil { + return fmt.Errorf("unable to determine local AWS account ID for cross-account ENI permissions: %w", err) + } + + remoteClient := ec2shim.NewClient(a.rootLogger, ec2.NewFromConfig(crossAccountCfg, optionsFunc), aMetrics, operatorOption.Config.IPAMAPIQPSLimit, + operatorOption.Config.IPAMAPIBurst, subnetsFilters, instancesFilters, eniCreationTags, + operatorOption.Config.AWSUsePrimaryAddress) + + a.client = eni.NewCrossAccountEC2Client(a.rootLogger, localClient, remoteClient, localAccountID) return nil }