resolve pr comments for multi account tgb

Author: zac-nixon

apis/elbv2/v1alpha1/targetgroupbinding_types.go

@@ -128,6 +128,14 @@ type TargetGroupBindingSpec struct {
 	// networking provides the networking setup for ELBV2 LoadBalancer to access targets in TargetGroup.
 	// +optional
 	Networking *TargetGroupBindingNetworking `json:"networking,omitempty"`
+
+	// IAM Role ARN to assume when calling AWS APIs. Useful if the target group is in a different AWS account
+	// +optional
+	IamRoleArnToAssume string `json:"iamRoleArnToAssume,omitempty"`
+
+	// IAM Role ARN to assume when calling AWS APIs. Needed to assume a role in another account and prevent the confused deputy problem. https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html
+	// +optional
+	AssumeRoleExternalId string `json:"assumeRoleExternalId,omitempty"`
 }
 
 // TargetGroupBindingStatus defines the observed state of TargetGroupBinding

apis/elbv2/v1beta1/targetgroupbinding_types.go

@@ -160,11 +160,11 @@ type TargetGroupBindingSpec struct {
 
 	// IAM Role ARN to assume when calling AWS APIs. Useful if the target group is in a different AWS account
 	// +optional
-	IamRoleArnToAssume string `json:"-"` // `json:"iamRoleArnToAssume,omitempty"`
+	IamRoleArnToAssume string `json:"iamRoleArnToAssume,omitempty"`
 
 	// IAM Role ARN to assume when calling AWS APIs. Needed to assume a role in another account and prevent the confused deputy problem. https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html
 	// +optional
-	AssumeRoleExternalId string `json:"-"` // `json:"assumeRoleExternalId,omitempty"`
+	AssumeRoleExternalId string `json:"assumeRoleExternalId,omitempty"`
 }
 
 // TargetGroupBindingStatus defines the observed state of TargetGroupBinding

config/crd/bases/elbv2.k8s.aws_targetgroupbindings.yaml

@@ -65,6 +65,15 @@ spec:
           spec:
             description: TargetGroupBindingSpec defines the desired state of TargetGroupBinding
             properties:
+              assumeRoleExternalId:
+                description: IAM Role ARN to assume when calling AWS APIs. Needed
+                  to assume a role in another account and prevent the confused deputy
+                  problem. https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html
+                type: string
+              iamRoleArnToAssume:
+                description: IAM Role ARN to assume when calling AWS APIs. Useful
+                  if the target group is in a different AWS account
+                type: string
               multiClusterTargetGroup:
                 description: MultiClusterTargetGroup Denotes if the TargetGroup is
                   shared among multiple clusters
@@ -242,6 +251,15 @@ spec:
           spec:
             description: TargetGroupBindingSpec defines the desired state of TargetGroupBinding
             properties:
+              assumeRoleExternalId:
+                description: IAM Role ARN to assume when calling AWS APIs. Needed
+                  to assume a role in another account and prevent the confused deputy
+                  problem. https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html
+                type: string
+              iamRoleArnToAssume:
+                description: IAM Role ARN to assume when calling AWS APIs. Useful
+                  if the target group is in a different AWS account
+                type: string
               ipAddressType:
                 description: ipAddressType specifies whether the target group is of
                   type IPv4 or IPv6. If unspecified, it will be automatically inferred.

docs/guide/targetgroupbinding/spec.md

@@ -55,15 +55,6 @@ Kubernetes meta/v1.ObjectMeta
 <table>
 <tr><td><code>annotations</code></td><td>
 
-<table>
-<tr><td><code>alb.ingress.kubernetes.io/IamRoleArnToAssume</code><br><i>string</i></td>
-<td><i>(Optional)</i> In case the target group is in a differet AWS account, you put here the role that needs to be assumed in order to manipulate the target group.
-</td></tr>
-<tr><td><code>alb.ingress.kubernetes.io/AssumeRoleExternalId</code><br><i>string</i></td>
-<td><i>(Optional)</i> The external ID for the assume role operation. Optional, but recommended. It helps you to prevent the <a href="https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html" target="_blank">confused deputy problem</a>.
-</td></tr>
-</table>
-
 <tr><td colspan=2>
 Refer to the Kubernetes API documentation for the other fields of the
 <code>metadata</code> field.

docs/guide/targetgroupbinding/targetgroupbinding.md

@@ -112,10 +112,10 @@ spec:
 ### AssumeRole
 
 Sometimes the AWS LoadBalancer controller needs to manipulate target groups from different AWS accounts.
-The way to do that is assuming a role from such account. There are annotations that can help you with that:
+The way to do that is assuming a role from such account. The following spec fields help you with that.
 
-* `alb.ingress.kubernetes.io/IamRoleArnToAssume`: the ARN that you need to assume
-* `alb.ingress.kubernetes.io/AssumeRoleExternalId`: the external ID for the assume role operation. Optional, but recommended. It helps you to prevent the confused deputy problem ( https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html )
+* `iamRoleArnToAssume`: the ARN that you need to assume
+* `assumeRoleExternalId`: the external ID for the assume role operation. Optional, but recommended. It helps you to prevent the confused deputy problem ( https://docs.aws.amazon.com/IAM/latest/UserGuide/confused-deputy.html )
 
 
 ## Sample YAML
@@ -125,10 +125,9 @@ apiVersion: elbv2.k8s.aws/v1beta1
 kind: TargetGroupBinding
 metadata:
   name: my-tgb
-  annotations:
-    alb.ingress.kubernetes.io/IamRoleArnToAssume: "arn:aws:iam::999999999999:role/alb-controller-policy-to-assume"
-    alb.ingress.kubernetes.io/AssumeRoleExternalId: "some-magic-string"
 spec:
+  iamRoleArnToAssume: "arn:aws:iam::999999999999:role/alb-controller-policy-to-assume"
+  assumeRoleExternalId: "some-magic-string"
   ...
 ```
 

go.sum

@@ -60,6 +60,7 @@ github.com/aws/aws-sdk-go-v2/service/ec2 v1.173.0 h1:ta62lid9JkIpKZtZZXSj6rP2AqY
 github.com/aws/aws-sdk-go-v2/service/ec2 v1.173.0/go.mod h1:o6QDjdVKpP5EF0dp/VlvqckzuSDATr1rLdHt3A5m0YY=
 github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2 v1.43.1 h1:L9Wt9zgtoYKIlaeFTy+EztGjL4oaXBBGtVXA+jaeYko=
 github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2 v1.43.1/go.mod h1:yxzLdxt7bVGvIOPYIKFtiaJCJnx2ChlIIvlhW4QgI6M=
+github.com/aws/aws-sdk-go-v2/service/iam v1.36.3/go.mod h1:HSvujsK8xeEHMIB18oMXjSfqaN9cVqpo/MtHJIksQRk=
 github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3 h1:dT3MqvGhSoaIhRseqw2I0yH81l7wiR2vjs57O51EAm8=
 github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3/go.mod h1:GlAeCkHwugxdHaueRr4nhPuY+WW+gR8UjlcqzPr1SPI=
 github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.17 h1:HGErhhrxZlQ044RiM+WdoZxp0p+EGM62y3L6pwA4olE=

main.go

@@ -90,7 +90,7 @@ func main() {
 		awsMetricsCollector = awsmetrics.NewCollector(metrics.Registry)
 	}
 
-	cloud, err := aws.NewCloud(controllerCFG.AWSConfig, awsMetricsCollector, ctrl.Log, nil)
+	cloud, err := aws.NewCloud(controllerCFG.AWSConfig, controllerCFG.ClusterName, awsMetricsCollector, ctrl.Log, nil)
 	if err != nil {
 		setupLog.Error(err, "unable to initialize AWS cloud")
 		os.Exit(1)

pkg/aws/cloud.go

@@ -3,10 +3,13 @@ package aws
 import (
 	"context"
 	"fmt"
-	"log"
+	"k8s.io/apimachinery/pkg/util/cache"
 	"net"
 	"os"
+	"regexp"
 	"strings"
+	"sync"
+	"time"
 
 	awsmiddleware "github.com/aws/aws-sdk-go-v2/aws/middleware"
 	"github.com/aws/aws-sdk-go-v2/aws/ratelimit"
@@ -32,10 +35,15 @@ import (
 	aws_metrics "sigs.k8s.io/aws-load-balancer-controller/pkg/metrics/aws"
 )
 
-const userAgent = "elbv2.k8s.aws"
+const (
+	userAgent          = "elbv2.k8s.aws"
+	cacheTTLBufferTime = 30 * time.Second
+)
+
+var illegalValuesInSessionName = regexp.MustCompile(`[^a-zA-Z0-9=,.@-]+`)
 
 // NewCloud constructs new Cloud implementation.
-func NewCloud(cfg CloudConfig, metricsCollector *aws_metrics.Collector, logger logr.Logger, awsClientsProvider provider.AWSClientsProvider) (services.Cloud, error) {
+func NewCloud(cfg CloudConfig, clusterName string, metricsCollector *aws_metrics.Collector, logger logr.Logger, awsClientsProvider provider.AWSClientsProvider) (services.Cloud, error) {
 	hasIPv4 := true
 	addrs, err := net.InterfaceAddrs()
 	if err == nil {
@@ -119,14 +127,16 @@ func NewCloud(cfg CloudConfig, metricsCollector *aws_metrics.Collector, logger l
 
 	thisObj := &defaultCloud{
 		cfg:         cfg,
+		clusterName: clusterName,
 		ec2:         ec2Service,
 		acm:         services.NewACM(awsClientsProvider),
 		wafv2:       services.NewWAFv2(awsClientsProvider),
 		wafRegional: services.NewWAFRegional(awsClientsProvider, cfg.Region),
 		shield:      services.NewShield(awsClientsProvider),
 		rgt:         services.NewRGT(awsClientsProvider),
 
-		assumeRoleElbV2:    make(map[string]services.ELBV2),
+		assumeRoleElbV2Cache: cache.NewExpiring(),
+
 		awsClientsProvider: awsClientsProvider,
 		logger:             logger,
 	}
@@ -220,77 +230,65 @@ type defaultCloud struct {
 	shield      services.Shield
 	rgt         services.RGT
 
-	assumeRoleElbV2    map[string]services.ELBV2
+	clusterName string
+
+	// A cache holding elbv2 clients that are assuming a role.
+	assumeRoleElbV2Cache *cache.Expiring
+	// assumeRoleElbV2CacheMutex protects assumeRoleElbV2Cache
+	assumeRoleElbV2CacheMutex sync.RWMutex
+
 	awsClientsProvider provider.AWSClientsProvider
 	logger             logr.Logger
 }
 
-// returns ELBV2 client for the given assumeRoleArn, or the default ELBV2 client if assumeRoleArn is empty
-func (c *defaultCloud) GetAssumedRoleELBV2(ctx context.Context, assumeRoleArn string, externalId string) services.ELBV2 {
-
+// GetAssumedRoleELBV2 returns ELBV2 client for the given assumeRoleArn, or the default ELBV2 client if assumeRoleArn is empty
+func (c *defaultCloud) GetAssumedRoleELBV2(ctx context.Context, assumeRoleArn string, externalId string) (services.ELBV2, error) {
 	if assumeRoleArn == "" {
-		return c.elbv2
+		return c.elbv2, nil
 	}
 
-	assumedRoleELBV2, exists := c.assumeRoleElbV2[assumeRoleArn]
+	c.assumeRoleElbV2CacheMutex.RLock()
+	assumedRoleELBV2, exists := c.assumeRoleElbV2Cache.Get(assumeRoleArn)
+	c.assumeRoleElbV2CacheMutex.RUnlock()
+
 	if exists {
-		return assumedRoleELBV2
+		return assumedRoleELBV2.(services.ELBV2), nil
 	}
 	c.logger.Info("awsCloud", "method", "GetAssumedRoleELBV2", "AssumeRoleArn", assumeRoleArn, "externalId", externalId)
 
-	////////////////
 	existingAwsConfig, _ := c.awsClientsProvider.GetAWSConfig(ctx, "GetAWSConfigForIAMRoleImpersonation")
 
 	sourceAccount := sts.NewFromConfig(*existingAwsConfig)
 	response, err := sourceAccount.AssumeRole(ctx, &sts.AssumeRoleInput{
 		RoleArn:         aws.String(assumeRoleArn),
-		RoleSessionName: aws.String("aws-load-balancer-controller"),
+		RoleSessionName: aws.String(c.makeClusterNameSessionNameSafe()),
 		ExternalId:      aws.String(externalId),
 	})
 	if err != nil {
-		log.Fatalf("Unable to assume target role, %v. Attempting to use default client", err)
-		return c.elbv2
+		c.logger.Error(err, "Unable to assume target role, %v")
+		return nil, err
 	}
 	assumedRoleCreds := response.Credentials
 	newCreds := credentials.NewStaticCredentialsProvider(*assumedRoleCreds.AccessKeyId, *assumedRoleCreds.SecretAccessKey, *assumedRoleCreds.SessionToken)
 	newAwsConfig, err := config.LoadDefaultConfig(ctx, config.WithRegion(c.cfg.Region), config.WithCredentialsProvider(newCreds))
 	if err != nil {
-		log.Fatalf("Unable to load static credentials for service client config, %v. Attempting to use default client", err)
-		return c.elbv2
+		c.logger.Error(err, "Unable to load static credentials for service client config, %v. Attempting to use default client")
+		return nil, err
 	}
 
-	existingAwsConfig.Credentials = newAwsConfig.Credentials //  response.Credentials
+	cacheTTL := assumedRoleCreds.Expiration.Sub(time.Now())
+	existingAwsConfig.Credentials = newAwsConfig.Credentials
+	elbv2WithAssumedRole := services.NewELBV2(c.awsClientsProvider, c)
 
-	// // var assumedRoleCreds *stsTypes.Credentials = response.Credentials
-
-	// // Create config with target service client, using assumed role
-	// cfg, err = config.LoadDefaultConfig(ctx, config.WithRegion(region), config.WithCredentialsProvider(credentials.NewStaticCredentialsProvider(*assumedRoleCreds.AccessKeyId, *assumedRoleCreds.SecretAccessKey, *assumedRoleCreds.SessionToken)))
-	// if err != nil {
-	// 	log.Fatalf("unable to load static credentials for service client config, %v", err)
-	// }
-
-	// ////////////////
-	// appCreds := stscreds.NewAssumeRoleProvider(client, assumeRoleArn)
-	// value, err := appCreds.Retrieve(context.TODO())
-	// if err != nil {
-	// 	// handle error
-	// }
-	// /////////
-
-	// ///////////// OLD
-	// creds := stscreds.NewCredentials(c.session, assumeRoleArn, func(p *stscreds.AssumeRoleProvider) {
-	// 	p.ExternalID = &externalId
-	// })
-	// //////////////
-
-	// c.awsConfig.Credentials = creds
-	// // newObj := services.NewELBV2(c.session, c, c.awsCFG)
-	// newObj := services.NewELBV2(*c.awsConfig, c.endpointsResolver, c)
-
-	newObj := services.NewELBV2(c.awsClientsProvider, c)
-	c.assumeRoleElbV2[assumeRoleArn] = newObj
+	c.assumeRoleElbV2CacheMutex.Lock()
+	defer c.assumeRoleElbV2CacheMutex.Unlock()
+	c.assumeRoleElbV2Cache.Set(assumeRoleArn, elbv2WithAssumedRole, cacheTTL-cacheTTLBufferTime)
+	return elbv2WithAssumedRole, nil
+}
 
-	return newObj
+func (c *defaultCloud) makeClusterNameSessionNameSafe() string {
+	safeClusterName := illegalValuesInSessionName.ReplaceAllString(c.clusterName, "")
+	return fmt.Sprintf("AWS-LBC-%s", safeClusterName)
 }
 
 func (c *defaultCloud) EC2() services.EC2 {

pkg/aws/services/cloudInterface.go

@@ -30,5 +30,5 @@ type Cloud interface {
 	// VpcID for the LoadBalancer resources.
 	VpcID() string
 
-	GetAssumedRoleELBV2(ctx context.Context, assumeRoleArn string, externalId string) ELBV2
+	GetAssumedRoleELBV2(ctx context.Context, assumeRoleArn string, externalId string) (ELBV2, error)
 }

pkg/aws/services/elbv2.go

@@ -60,7 +60,7 @@ type ELBV2 interface {
 	ModifyListenerAttributesWithContext(ctx context.Context, input *elasticloadbalancingv2.ModifyListenerAttributesInput) (*elasticloadbalancingv2.ModifyListenerAttributesOutput, error)
 	ModifyCapacityReservationWithContext(ctx context.Context, input *elasticloadbalancingv2.ModifyCapacityReservationInput) (*elasticloadbalancingv2.ModifyCapacityReservationOutput, error)
 	DescribeCapacityReservationWithContext(ctx context.Context, input *elasticloadbalancingv2.DescribeCapacityReservationInput) (*elasticloadbalancingv2.DescribeCapacityReservationOutput, error)
-	AssumeRole(ctx context.Context, assumeRoleArn string, externalId string) ELBV2
+	AssumeRole(ctx context.Context, assumeRoleArn string, externalId string) (ELBV2, error)
 }
 
 func NewELBV2(awsClientsProvider provider.AWSClientsProvider, cloud Cloud) ELBV2 {
@@ -76,9 +76,9 @@ type elbv2Client struct {
 	cloud              Cloud
 }
 
-func (c *elbv2Client) AssumeRole(ctx context.Context, assumeRoleArn string, externalId string) ELBV2 {
+func (c *elbv2Client) AssumeRole(ctx context.Context, assumeRoleArn string, externalId string) (ELBV2, error) {
 	if assumeRoleArn == "" {
-		return c
+		return c, nil
 	}
 	return c.cloud.GetAssumedRoleELBV2(ctx, assumeRoleArn, externalId)
 }