Usage of --aws-vpc-tags with --aws-vpc-tag-key is unclear

[alfredkrohmer]

This was introduced by @jeswinkoshyninan in #3656

It's not clear to my why --aws-vpc-tags allows to pass a map with an arbitrary number of items if we only ever pass a single key-value pair (the one with the key specified in --aws-vpc-tag-key) as the tag filter for finding a VPC.

aws-load-balancer-controller/pkg/aws/cloud.go

Lines 141 to 143 in 8a01478

	if cfg.VpcTags != nil {
	return inferVPCIDFromTags(ec2Service, cfg.VpcNameTagKey, cfg.VpcTags[cfg.VpcNameTagKey])
	}

aws-load-balancer-controller/pkg/aws/cloud.go

Lines 184 to 192 in 8a01478

	func inferVPCIDFromTags(ec2Service services.EC2, VpcNameTagKey string, VpcNameTagValue string) (string, error) {
	vpcs, err := ec2Service.DescribeVPCsAsList(context.Background(), &ec2.DescribeVpcsInput{
	Filters: []*ec2.Filter{
	{
	Name: aws.String("tag:" + VpcNameTagKey),
	Values: []*string{aws.String(VpcNameTagValue)},
	},
	},
	})

Shouldn't it rather just convert the map from --aws-vpc-tags into a list of tag filters, one for each item? This would remove the need for --aws-vpc-tag-key altogether.

Ref: #3656 (comment)

[pinkavaj]

@jeswinkoshyninan Mey-be you can help to shed more light here?

[jeswinkoshyninan]

@pinkavaj okay sure

@alfredkrohmer sorry for the late reply, I was away for my vacation. make sense, I will create a PR to update this behaviour based on your suggestion. But just to give the context here, this feature doesn't make any change in behaviour of fetching VPC ID from metadata-service as it was before. But added as an optional feature to fetch VPC-ID explicitly from a given AWS tag with value as name of the VPC and by default the tag-key will be 'Name' itself and which is defined here.

So in this case you don't want to pass values for --aws-vpc-tags and --aws-vpc-tag-key altogether. But the users should have a way for override the default tag to a custom and in that case they might can use --aws-vpc-tag-key to override. But in most case this is not required. Hope this helps ?

[alfredkrohmer]

But just to give the context here, this feature doesn't make any change in behaviour of fetching VPC ID from metadata-service as it was before.

That's clear.

But the users should have a way for override the default tag to a custom and in that case they might can use --aws-vpc-tag-key to override.

This still explain why you need two parameters for this. It looks like --aws-vpc-tag-key is just entirely redundant. You could just pass --aws-vpc-tags=<tag key>=<tag value> (even with multiple values) and just pass forward this map as a filter to the DescribeVpcs API call.

[jeswinkoshyninan]

@alfredkrohmer Agreed already, it makes sense. I will create a PR to remove this.