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.

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

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

Open
alfredkrohmer opened this issue Oct 14, 2024 · 7 comments · May be fixed by #3954
Open

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

alfredkrohmer opened this issue Oct 14, 2024 · 7 comments · May be fixed by #3954

Comments

@alfredkrohmer
Copy link
Contributor

alfredkrohmer commented Oct 14, 2024

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.

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

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

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

@jeswinkoshyninan
Copy link
Contributor

@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
Copy link
Contributor Author

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

@alfredkrohmer I have went into it again and wanted to confirm few more things, If list --aws-vpc-tags has a single value given, it is pretty straight to use the key as the tag key to identify the VPC. But in the case of having multiple tag values, we need an identifier to identify the name tag-key of the VPC Name defined. Assuming "Name" as tag always is not something which we can expect. thoughts ?

for eg: --aws-vpc-tags=VPCName=myvpc,EnvName=myenv, In this case which one can be used as an identifier ? --aws-vpc-tag-key is basically to overcome this problem and allowing user to define.

@alfredkrohmer
Copy link
Contributor Author

alfredkrohmer commented Nov 26, 2024

What do you need an name/identifier for? I thought the feature is used to find the VPC ID, which you need for API calls.

@jeswinkoshyninan
Copy link
Contributor

jeswinkoshyninan commented Nov 26, 2024

please refer, So here we are trying to fetch VPC-ID from tags given. More details about the problem statement is given in the PR desc.
This is useful for cases where access to AWS metadata is blocked and the VPC ID is not known at deploy time, options to infer the VPC ID were limited before.

@jeswinkoshyninan
Copy link
Contributor

I have refactored the feature to not use the flag --aws-vpc-tag-key, please review and merge the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants