fix: remove extra quotes from profile, region, external_id, aws_cli_query params

[GarrettBlinkhorn]

Hey guys, great module and solves some specific pain points we're having around rendering certain data sets from AWS. I believe I've discovered a small bug while trying to use the latest version of the module however (please correct me if I'm wrong).

Context:

Terraform v1.6.5
on darwin_arm64
+ provider registry.terraform.io/hashicorp/aws v5.30.0

I instantiate the module using code like:

module "my_module" {
  source  = "digitickets/cli/aws"
  version = "5.2.0"
  region = "MY_REGION"
  assume_role_arn = "MY_ROLE_ARN"

  aws_cli_commands = [
    "aws", 
    "commands", 
    "go", 
    "here"
  ]
}

but applying the module throws the following error:

The data source received an unexpected error while attempting to execute the program.
│ 
│ Program: .terraform/modules/a_module.my_module/scripts/aws_cli_runner.sh
│ Error Message: 
│ Provided region_name ''MY_REGION'' doesn't match a supported format.

You'll note that there are two sets of single-quotes on either side of MY_REGION - I believe the way that your script handles this parameter (and likely a few others that are handled similarly) is the root cause. After running a terraform init to pull the module, if I manually make the changes to the script that this PR is incorporating, then your module runs without an issue and returns the desired result. In other words, the region parameter is correctly interpreted by the script after this change.

Please review the issue and let me know if there is any additional information that you need. If not, I would appreciate you merging this PR so that we can put the module to use using the region parameter as intended. Thanks :)

[rquadling]

Interesting ... the tests are passing using this module. What region are you using? The error could be that "MY-REGION" is being rejected as regions are (for example) "eu-west-1". Can you give a concrete example?

[rquadling]

I've added a couple of tests that replicate the specific error message you have generated. The issue is that region names are only the short form (us-east-2), not the long form (US East (Ohio) us-east-2).

[GarrettBlinkhorn]

Thanks @rquadling for taking a look at this. In my case, I am using eu-central-1 as my region. Let me try to give a more specific example.

This is what I am trying to execute:

module "regional_public_addresses" {
  source  = "digitickets/cli/aws"
  version = "5.2.0"
  region = "eu-central-1"
  assume_role_arn = "arn:aws:iam::ACCOUNT_ID:role/MyReaderRole"

  aws_cli_commands = [
    "ec2", 
    "get-ipam-discovered-public-addresses", 
    "--ipam-resource-discovery-id=RESOURCE_DISCOVERY_ID", 
    "--address-region=eu-central-1"
  ]
}

Elsewhere, I instantiate the module with a simple module call:

module "regional_addresses" {
  source = "[email protected]:ORG/REPO//terraform/modules/regional_public_addresses?ref=main"
}

This still throws the original error as described:

The data source received an unexpected error while attempting to
│ execute the program.
│ 
│ Program:
│ .terraform/modules/regional_addresses.regional_public_addresses/scripts/aws_cli_runner.sh
│ Error Message: 
│ Provided region_name ''eu-central-1'' doesn't match a supported
│ format.

Let me know if I can provide any more information or if you would like me to test out any changes. You'll note that ''eu-central-1'' is surrounded by two sets of single quotes, which is why I drafted this PR removing a set of quotes from the parameter string generation.

[rquadling]

We run the code on MacOS (host) and in a Linux container (pipeline) and so far no issues with this. We use sh rather than bash to run the actual AWS CLI. Do you have any specific setup to alter this?

Adding the single quotes is a way to ensure the right thing is passed as a whole unit, and not using space to inject multiple "things". It is expected.

$ aws s3api list-objects --bucket=ryft-public-sample-data --no-sign-request --region 'eu-west-1' --query 'max_by(Contents, &Size)'
{
    "Key": "wikipedia-20150518.bin",
    "LastModified": "2017-06-22T14:54:07+00:00",
    "ETag": "\"a609253bd231c6c7b3153f5b2bc4342a-2694\"",
    "Size": 22593382637,
    "StorageClass": "STANDARD",
    "Owner": {
        "DisplayName": "CMS-BlackLynx-AWSProducts",
        "ID": "0ae8f910a30bc83fd81c4e3c1a6bbd9bab0afe4e0762b56a2807d22fcd77d517"
    }
}

I'd like to be able to generate this error to know what the appropriate fix is. Enclosing a string in quotes removes the potential of having/needing a space in something that gets called and the command line is broken.

Can you add the debug file and load that up?

The top 2 lines of my 'bad_region_with_debug' test shows:

2023-12-11 16:34:24,351 - MainThread - awscli.clidriver - DEBUG - CLI version: aws-cli/2.13.13 Python/3.11.5 Darwin/21.6.0 source/x86_64
2023-12-11 16:34:24,351 - MainThread - awscli.clidriver - DEBUG - Arguments entered to CLI: ['s3api', 'list-objects', '--bucket=ryft-public-sample-data', '--no-sign-request', '--region', 'US East (Ohio) us-east-2', '--query', 'max_by(Contents, &Size)', '--output', 'json', '--debug']

This is the debug from AWS.

I could go from --region 'us-east-2 to --region=us-east-2. But the = is not a requirement. For a test, I did try changing my aws_cli_commands to use that pattern:

aws_cli_commands   = ["s3api", "list-objects", "--bucket=ryft-public-sample-data", "--no-sign-request"]

Worked fine.

I've tried with and without =. The --query absolutely requires ' as it can contain spaces.

So, the debug log file needs is sort of what I'd need to be able to replicate the error.

OOI, what version of awscli you running? I've been running the tests locally on 2.13.13 and now upgrading to 2.15.0. Our pipelines are running 2.14.5.

Further oddness:

$ aws s3api list-objects --bucket=ryft-public-sample-data --no-sign-request --region ''eu-central-1'' --query 'max_by(Contents, &Size)'
{
    "Key": "wikipedia-20150518.bin",
    "LastModified": "2017-06-22T14:54:07+00:00",
    "ETag": "\"a609253bd231c6c7b3153f5b2bc4342a-2694\"",
    "Size": 22593382637,
    "StorageClass": "STANDARD",
    "Owner": {
        "DisplayName": "CMS-BlackLynx-AWSProducts",
        "ID": "0ae8f910a30bc83fd81c4e3c1a6bbd9bab0afe4e0762b56a2807d22fcd77d517"
    }
}

But using double quotes around a single quoted region (something not part of our code):

$ aws s3api list-objects --bucket=ryft-public-sample-data --no-sign-request --region "'eu-central-1'" --query 'max_by(Contents, &Size)'

Provided region_name ''eu-central-1'' doesn't match a supported format.

So, something odd is happening with the quotes here as the CLI happily handles ''eu-central-1''! Using 2 single quotes is the same as not supplying any. A double quote being added somehow ... is just odd.

I wonder if there is another quote being passed from somewhere else. Are you hard-coding the region for the module? Or is it coming from somewhere else? Maybe as a JSON value which will be double quoted and then wrapped in single quotes?

But that produces a different error:

$ aws s3api list-objects --bucket=ryft-public-sample-data --no-sign-request --region '"eu-central-1"' --query 'max_by(Contents, &Size)'

Provided region_name '"eu-central-1"' doesn't match a supported format.

Similar, but not exact.

I've not managed to replicate the error message you're getting. And dropping the quotes is not something I would do until I can replicate the issue.

As a test, locally, I've created a test using SSO credentials and the following tfvars:

aws_cli_commands = [
  "ec2",
  "get-ipam-discovered-public-addresses",
  "--address-region=eu-west-1",
  "--ipam-resource-discovery-id=ipam-res-disco-0df3fdf756d735416",
]
aws_cli_query="IpamDiscoveredPublicAddresses[*].Address"
region = "eu-central-1"
debug_log_filename = "test-reports/with_credentials/debug.log"

and I get output:

{[
    "79.125.52.176",
    "52.211.123.164",
    "52.49.58.248",
    "34.254.101.80",
    "52.17.72.163",
    "3.249.72.2",
    "34.241.167.73",
    "52.51.171.93",
    "54.246.139.79",
    "3.252.229.107",
    "34.253.83.113",
    "54.217.167.211",
    "54.195.122.194",
    "54.170.55.108",
    "34.240.57.130",
    "34.244.98.82",
    "3.254.59.23",
    "3.249.101.26"
]

The debug log top 2 lines are:

2023-12-11 17:24:34,892 - MainThread - awscli.clidriver - DEBUG - CLI version: aws-cli/2.15.0 Python/3.11.6 Darwin/21.6.0 source/x86_64
2023-12-11 17:24:34,892 - MainThread - awscli.clidriver - DEBUG - Arguments entered to CLI: ['ec2', 'get-ipam-discovered-public-addresses', '--address-region=eu-west-1', '--ipam-resource-discovery-id=ipam-res-disco-0df3fdf756d735416', '--region', 'eu-central-1', '--output', 'json', '--debug']

I can't think of anything else that would be breaking this.

[GarrettBlinkhorn]

Thanks for the detailed response, and for including aws_cli_query="IpamDiscoveredPublicAddresses[*].Address" which is a much better alternative to the nested for loop I had thrown together for this list.

I've looked through the log file. The first two lines indicate that the region was correctly parsed, and then execution goes on to fail :

2023-12-11 13:42:29,594 - MainThread - awscli.clidriver - DEBUG - CLI version: aws-cli/2.14.6 Python/3.11.6 Darwin/22.6.0 exe/x86_64
2023-12-11 13:42:29,594 - MainThread - awscli.clidriver - DEBUG - Arguments entered to CLI: ['ec2', 'get-ipam-discovered-public-addresses', '--ipam-resource-discovery-id=ipam-res-disco-087f91ae34a145376', '--address-region=eu-central-1', '--region', 'eu-central-1', '--query', 'IpamDiscoveredPublicAddresses[*].Address', '--output', 'json', '--debug']

However, the script fails for a different reason than the ERROR: Provided region_name ''eu-central-1'' doesn't match a supported stated above:

botocore.exceptions.ClientError: An error occurred (InvalidIpamResourceDiscoveryId.NotFound) when calling the GetIpamDiscoveredPublicAddresses operation: The ipam-resource-discovery ID 'ipam-res-disco-NUMBERS' does not exist

An error occurred (InvalidIpamResourceDiscoveryId.NotFound) when calling the GetIpamDiscoveredPublicAddresses operation: The ipam-resource-discovery ID 'ipam-res-disco-NUMBERS' does not exist

I can verify that 'ipam-res-disco-NUMBERS' does actually exist in eu-central-1 by making a local change to the script like this:
AWS_CLI_REGION_PARAM="--region=${REGION_NAME}" and then I get the same result as you:

output = [
      + "ALL",
      + "MY",
      + "PUBLIC_IPS",
]

and when I check those debug logs:

2023-12-11 13:59:33,991 - MainThread - awscli.clidriver - DEBUG - CLI version: aws-cli/2.14.6 Python/3.11.6 Darwin/22.6.0 exe/x86_64
2023-12-11 13:59:33,991 - MainThread - awscli.clidriver - DEBUG - Arguments entered to CLI: ['ec2', 'get-ipam-discovered-public-addresses', '--ipam-resource-discovery-id=ipam-res-disco-087f91ae34a145376', '--address-region=eu-central-1', '--region=eu-central-1', '--query', 'IpamDiscoveredPublicAddresses[*].Address', '--output', 'json', '--debug']

then the only apparent difference to me is how the argument list is handling the region string. In the original case, it gets passed to the cli as [...,'--region', 'eu-central-1',...] whereas during the successful case, it gets passed as [...,'--region=eu-central-1',...].

I think its also possible that the earlier aws command: TEMP_ROLE=$(aws sts assume-role ${AWS_CLI_PROFILE_PARAM:-} ${AWS_CLI_REGION_PARAM:-} --output json --role-arn "${ASSUME_ROLE_ARN}" ${AWS_CLI_EXTERNAL_ID_PARAM:-} --role-session-name "${ROLE_SESSION_NAME:-AssumingRole}") is also failing because of a bad parse which is resulting in the ''eu-central-1'' bad input.

Both of these issues were resolved by switching to using the = like --region=${REGION_NAME}, so I've fixed the query quotes but left the others. Let me know your thoughts

[rquadling]

I think if I add space detection/rejection to the variables that have been quoted, then that's comes under a "good enough" solution I think. It won't alter anyone's current usage.

Let me have a small play with that and get the testing in.

[rquadling]

A small ish upgrade. All Terraform variables are now tested to reject inappropriate format/structure. Tests are passing. Using it in our projects.

V6.0.0 is for you to try out.

[rquadling]

And purely as a "thing" ... you don't need the query module property. You can just add --query='...' to the aws_cli_commands! And I've no idea why I bothered with that!

[GarrettBlinkhorn]

Hey @rquadling thanks for your work on this. I'm happy to report that the issue was resolved when I upgraded the module to 6.0.0 so I can carry on with my work as intended. Closing this PR since it is no longer relevant. Thanks!

[rquadling]

You're welcome.

…

On Tue, 19 Dec 2023, 20:25 Garrett Blinkhorn, ***@***.***> wrote: Hey @rquadling <https://github.com/rquadling> thanks for your work on this. I'm happy to report that the issue was resolved when I upgraded the module to 6.0.0 so I can carry on with my work as intended. Closing this PR since it is no longer relevant. Thanks! — Reply to this email directly, view it on GitHub <#10 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADEAMLOXMCA5Q3Q3DNKATYKHZ2DAVCNFSM6AAAAABANIGFBKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRTGQZDKMRTGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>