-
Notifications
You must be signed in to change notification settings - Fork 35
Service endpoint alias fix #57
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kudos for tackling this @lakkeger ! 🎉 (Also discovered this recently, and had to put in a local quick fix to prepare for a demo.. ;) )
Added a few suggestions and clarification questions.. Let me know if anything is unclear, happy to discuss more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of hardcoding the service names here, could we pass in a list of expected services as a parameter of the function?
Also, I would have expected this to be something like this - wouldn't we expect only one of the two be contained in the endpoints list (and not both)..? 🤔
if "config" not in endpoints and "configservice" in endpoints:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, not a problem to use them es parameters.
However I'd argue the logic change here:
If the list order changes in tflocal that breaks the test for no reason, and not necessarily wrong to use configservice instead of config or vice versa as it's a valid option, we just want to use exclusively one in any given point to hide the warnings, so we made the decision to do it with the first item from the aliases in tflocal.
Or do you think we should focus more here to test the actual values we should see in the generated file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I think the logic makes sense, thanks for clarifying. Maybe we could still make the logic parametrizable, rather than harcoding the service names.
Btw, if I understand correctly, we want to ensure that no duplicates are being generated for the service endpoints - could we simply iterate over all alias pairs within SERVICE_ALIASES, and ensure that only one of the aliases is defined in the config..? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's already happenning here :)
Line 150 in 6d9f75b
Basically we iterate through all aliases, if the alias is among the service endpoints we add the service endpoint by the first alias name so in case we find it multiple times we simply just updating the service endpoint dict at the respective key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that makes sense, great to see that we have this logic in place for create_provider_config_file. 👍 I was more referring to the test assertion here, if we are deliberately checking for config/configservice only. But if we're happy with the assertion here, I'm fine with merging as-is (we can evolve this as we go.. ➿ ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see these enhancements 🚀 - thanks for addressing all the comments.
Found a bug with the new v5.55.0 aws provider, hence the timeout in the pipelines.
It seems by the migration of the S3 service into the new aws-sdk-go-v2 in the provider it overrides the us-east-1 region with a pseudo one, called aws-global. This results in an exception in LocalStack <v3.5 and our Terraform runs just hanging waiting for the resource. According to the provider's roadmap, this behaviour will be deprecated by >v6.
See related issue here: hashicorp/terraform-provider-aws#33028
To apply a temporary fix I changed . -> - in the test runs bucket names and left a note in the code for future reference. Additionally this is the recommended work around for anyone who runs into this error.
Related exception in LocalStack:
2024年06月24日 13:59:18 2024年06月24日T11:59:18.335 INFO --- [et.reactor-1] localstack.request.aws : AWS s3.HeadBucket => 500 (InternalError); 000000000000/aws-global; HeadBucketRequest({'Bucket': 'bucket.36311f6c', 'ExpectedBucketOwner': None}, headers={'Host': 'bucket.36311f6c.s3.localhost.localstack.cloud:4566', 'User-Agent': 'APN/1.0 HashiCorp/1.0 Terraform/1.5.7 (+https://www.terraform.io/) terraform-provider-aws/5.55.0 (+https://registry.terraform.io/providers/hashicorp/aws) aws-sdk-go-v2/1.30.0 os/macos lang/go#1.22.4 md/GOOS#darwin md/GOARCH#arm64 api/s3#1.56.1', 'Accept-Encoding': 'identity', 'Amz-Sdk-Invocation-Id': 'f1c22843-5bc1-4ee7-8869-372fbf1b4fa6', 'Amz-Sdk-Request': 'attempt=10; max=25', 'Authorization': 'AWS4-HMAC-SHA256 Credential=test/20240624/aws-global/s3/aws4_request, SignedHeaders=accept-encoding;amz-sdk-invocation-id;amz-sdk-request;host;x-amz-content-sha256;x-amz-date, Signature=8e052d59f42a0125ab95cc319e3326efd573fc9e7ff9d69f187266601563e4ef', 'X-Amz-Content-Sha256': 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855', 'X-Amz-Date': '20240624T115917Z', 'x-moto-account-id': '000000000000'}); InternalError(exception while calling s3.HeadBucket: Traceback (most recent call last):
2024年06月24日 13:59:18 File "/opt/code/localstack/.venv/lib/python3.11/site-packages/rolo/gateway/chain.py", line 166, in handle
2024年06月24日 13:59:18 handler(self, self.context, response)
2024年06月24日 13:59:18 File "/opt/code/localstack/.venv/lib/python3.11/site-packages/localstack/aws/handlers/service.py", line 113, in __call__
2024年06月24日 13:59:18 handler(chain, context, response)
2024年06月24日 13:59:18 File "/opt/code/localstack/.venv/lib/python3.11/site-packages/localstack/aws/handlers/service.py", line 83, in __call__
2024年06月24日 13:59:18 skeleton_response = self.skeleton.invoke(context)
2024年06月24日 13:59:18 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2024年06月24日 13:59:18 File "/opt/code/localstack/.venv/lib/python3.11/site-packages/localstack/aws/skeleton.py", line 154, in invoke
2024年06月24日 13:59:18 return self.dispatch_request(serializer, context, instance)
2024年06月24日 13:59:18 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2024年06月24日 13:59:18 File "/opt/code/localstack/.venv/lib/python3.11/site-packages/localstack/aws/skeleton.py", line 168, in dispatch_request
2024年06月24日 13:59:18 result = handler(context, instance) or {}
2024年06月24日 13:59:18 ^^^^^^^^^^^^^^^^^^^^^^^^^^
2024年06月24日 13:59:18 File "/opt/code/localstack/.venv/lib/python3.11/site-packages/localstack/aws/skeleton.py", line 118, in __call__
2024年06月24日 13:59:18 return self.fn(*args, **kwargs)
2024年06月24日 13:59:18 ^^^^^^^^^^^^^^^^^^^^^^^^
2024年06月24日 13:59:18 File "/opt/code/localstack/.venv/lib/python3.11/site-packages/localstack/services/s3/provider.py", line 550, in head_bucket
2024年06月24日 13:59:18 store = self.get_store(context.account_id, context.region)
2024年06月24日 13:59:18 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2024年06月24日 13:59:18 File "/opt/code/localstack/.venv/lib/python3.11/site-packages/localstack/services/s3/provider.py", line 421, in get_store
2024年06月24日 13:59:18 return s3_stores[account_id][region_name]
2024年06月24日 13:59:18 ~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
2024年06月24日 13:59:18 File "/opt/code/localstack/.venv/lib/python3.11/site-packages/localstack/services/stores.py", line 231, in __getitem__
2024年06月24日 13:59:18 raise ValueError(
2024年06月24日 13:59:18 ValueError: 'aws-global' is not a valid AWS region name for s3
2024年06月24日 13:59:18 , headers={'Content-Type': 'application/xml', 'Content-Length': '2084', 'x-amz-request-id': '3322584d-ee0c-47ba-9a04-ee4a55e961a0', 'x-amz-id-2': 's9lzHYrFp76ZVxRcpX9+5cjAnEH2ROuNkd2BHfIa6UkFVdtjf5mKR3/eTPFvsiP/XV/VLi31234='})
And the debug log line in Terraform where it happens:
2024年06月24日T15:33:58.159+0200 [INFO] provider.terraform-provider-aws_v5.55.0_x5: overriding region: tf_req_id=de47ff28-7c23-41e4-98f0-30f04cc080a9 tf_rpc=ApplyResourceChange @module=aws original_region=us-east-1 override_region=aws-global tf_aws.service_package=s3 tf_provider_addr=registry.terraform.io/hashicorp/aws @caller=github.com/hashicorp/terraform-provider-aws/internal/service/s3/service_package.go:29 tf_mux_provider=*schema.GRPCProviderServer tf_resource_type=aws_s3_bucket timestamp=2024年06月24日T15:33:58.159+0200
Uh oh!
There was an error while loading. Please reload this page.
Motivation
Issue #55 pointed out the warning below:
The source of it is the generated provider file contains endpoints which are each other's aliases.
Changes