Addition of implicit partition support

[andrew-mcgrath]

Issue #, if available: #792

Description of changes:

This PR contains the code changes necessary for implicitly supporting partitions as requested by issue #792 and the details of its comments.

• Utilities
  • URL suffix lookup from the botocore EndpointResolver based on service and region (also from an existing ARN)
• Deployment/Planning
  • Modifications to the intrinsic function parse_arn to return the dns_suffix for the ARN.
  • A new intrinsic function interrogate_profile has been added where parse_arn is unable to be utilized in the deployment plan.
• Packaging
  • Cloud Formation
    • AWS::Partition and AWS::URLSuffix have been used to fill in all the necessary characteristics for the resources that would need to be modified.
  • Terraform
    • The partition and dns_suffix attributes from data "aws_partition" "chalice" {} have been used in the templates to populate the necessary policies and resource definitions.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[codecov-io]

Codecov Report

Merging #1417 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1417   +/-   ##
=======================================
  Coverage   95.39%   95.39%           
=======================================
  Files          29       29           
  Lines        5565     5565           
  Branches      707      707           
=======================================
  Hits         5309     5309           
  Misses        179      179           
  Partials       77       77           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba20f6f...ba20f6f. Read the comment docs.

[jamesls]

Thanks for the pull request! This is a fairly large change so it's taking some time to go through it but just wanted to give an update that I am looking at it.

[andrew-mcgrath]

Thanks for taking the time to look!

[jamesls]

Hi, thanks for the pull request, I think this is really well done and it looks good to me.

I like that we're using the endpoints.json file from botocore so we'll automatically support any partitions in the endpoints.json file.

The only thing I'm a little hesitant on is using the EndpointResolver directly. While using the botocore loaders should be fine (it's a publicly documented API in botocore), the EndpointResolver isn't a public API (on a botocore session it's registered as an "internal component"). While it's not as ideal in terms of code duplication, I think it might be worth using the loaders directly to load up the endpoints.json file and have a separate implementation just for chalice to use.

I can put together a patch that shows what I had in mind, but overall I think it would be a fairly small change. Let me know what you think.

Thanks again for the pull request.

[andrew-mcgrath]

The only thing I'm a little hesitant on is using the EndpointResolver directly. While using the botocore loaders should be fine (it's a publicly documented API in botocore), the EndpointResolver isn't a public API (on a botocore session it's registered as an "internal component"). While it's not as ideal in terms of code duplication, I think it might be worth using the loaders directly to load up the endpoints.json file and have a separate implementation just for chalice to use.

I can put together a patch that shows what I had in mind, but overall I think it would be a fairly small change. Let me know what you think.

I had this exact debate internally on whether or not to duplicate the code from botocore. If it isn't a problem to have the duplication, then it makes complete sense to have the resolving code be encapsulated within chalice itself.

Did you envision pulling the entire botocore/regions.py since it only contains BaseEndpointResolver and EndpointResolver?

[jamesls]

Did you envision pulling the entire botocore/regions.py since it only contains BaseEndpointResolver and EndpointResolver?

Yeah, fortunately that module is pretty self-contained so there shouldn't be any issues vendoring that file. There's also some additional logic that builds on top of the endpoint resolver that botocore uses (ClientEndpointBridge) to handle some additional cases, for example if you don't explicitly configure a region. I don't think we'd need to pull any of that in because you have to have a region configured with chalice, we don't have any default regions.

If you want to take a crack at this, feel free.

[andrew-mcgrath]

Sounds good, I can make the updates and push the changes to the PR. Do you have a preference on where the vendored regions.py ends up or would you like the classes found therein to be simply located in the chalice/awsclient.py file?

[jamesls]

If we're going to be pulling in the file in its entirety it might make sense to start a chalice/vendored/botocore/regions.py. That way it's easy to pull in any new updates to the file.

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (master@9c258f7). Click here to learn what that means.
The diff coverage is n/a.

[andrew-mcgrath]

If we're going to be pulling in the file in its entirety it might make sense to start a chalice/vendored/botocore/regions.py. That way it's easy to pull in any new updates to the file.

Okay, I took a crack at it earlier this morning and had the file collocated with the other chalice code. I'll move the contents to the requested location but issues will occur with the code being unaltered due to the prcheck requirements.

botocore does not currently contain the same restrictions on code formatting and documentation that exist within the chalice framework so there will either need to be exceptions in the CI tooling or modifications to the original file.

Since it's a donated package would it be amenable to add an exemption for this file?

[andrew-mcgrath]

@jamesls I moved around the vendored package to the location that you specified and added some exceptions to the CI tooling so that the vendored module would no longer trip up code validation.

[jamesls]

Ok sounds good. Thanks for updating. Would you mind rebasing off of the latest master? I also don't mind if you want to consolidate your commits as well.

[andrew-mcgrath]

Ok sounds good. Thanks for updating. Would you mind rebasing off of the latest master? I also don't mind if you want to consolidate your commits as well.

Happy to rebase off master again, just saw the conflicts this morning and I'll consolidate the commits as well.

[andrew-mcgrath]

@jamesls the commit has been rebased and collapsed down to a single action. Thanks again for all your help and interaction.

[jamesls]

Thanks for all your work on this, I think this is looking pretty good.

When I try this out in the cn-north-1 region, I'm getting this error when it tries to call CreateRole for the app created from chalice new-project:

2020-06-12 12:09:56,994 botocore.endpoint [DEBUG] Sending http request: <AWSPreparedRequest stream_output=False, method=POST, url=https://iam.cn-north-1.amazonaws.com.cn/, headers={'Content-Type': b'application/x-www-form-urlencoded; charset=utf-8', 'User-Agent': b'aws-chalice/1.14.1 Python/3.7.3 Darwin/18.7.0 exec-env/AWS_Lambda aws-chalice/1.14.1 Botocore/1.12.253', 'X-Amz-Date': b'20200612T160956Z', 'Authorization': b'', 'Content-Length': '333'}>
2020-06-12 12:09:56,995 urllib3.util.retry [DEBUG] Converted retries value: False -> Retry(total=False, connect=None, read=None, redirect=0, status=None)
2020-06-12 12:09:57,291 urllib3.connectionpool [DEBUG] https://iam.cn-north-1.amazonaws.com.cn:443 "POST / HTTP/1.1" 400 339
2020-06-12 12:09:57,292 botocore.parsers [DEBUG] Response headers: {'x-amzn-RequestId': '', 'Content-Type': 'text/xml', 'Content-Length': '339', 'Date': 'Fri, 12 Jun 2020 16:09:56 GMT', 'Connection': 'close'}
2020-06-12 12:09:57,292 botocore.parsers [DEBUG] Response body:
b'<ErrorResponse xmlns="https://iam.amazonaws.com/doc/2010-05-08/">\n  <Error>\n    <Type>Sender</Type>\n    <Code>MalformedPolicyDocument</Code>\n    <Message>Invalid principal in policy: &quot;SERVICE&quot;:&quot;lambda.amazonaws.com.cn&quot;</Message>\n  </Error>\n  <RequestId>7d25ff0d-d960-48b2-b2eb-07cebadc39c6</RequestId>\n</ErrorResponse>\n'
2020-06-12 12:09:57,293 botocore.hooks [DEBUG] Event needs-retry.iam.CreateRole: calling handler <botocore.retryhandler.RetryHandler object at 0x10543c2e8>
2020-06-12 12:09:57,293 botocore.retryhandler [DEBUG] No retry needed.
2020-06-12 12:09:57,293 botocore.hooks [DEBUG] Event after-call.iam.CreateRole: calling handler <function json_decode_policies at 0x104356598>
botocore.errorfactory.MalformedPolicyDocumentException: An error occurred (MalformedPolicyDocument) when calling the CreateRole operation: Invalid principal in policy: "SERVICE":"lambda.amazonaws.com.cn"

Are you seeing this as well?

[jamesls]

Seems like the CDK ran into similar issues (aws/aws-cdk#3491)

[andrew-mcgrath]

So for the case of IAM policy we'd need to implement a similar implementation to aws-cdk/packages/@aws-cdk/region-info/lib/default.ts

It's unfortunate that the assumption about dnsuffix being a 1:1 mapping was invalid.

[andrew-mcgrath]

Digging deeper into other changes done to that aws-cdk feature we'd want to mirror the master branch implementation to get all of the partitions for parity. master/packages/aws-cdk/region-info/lib/default.ts

[jamesls]

Yeah that's unfortunate that they don't consistently use DNS suffix. Sounds like mirroring what CDK's region-info package does is our best option.

[andrew-mcgrath]

The deployer and planner changes are relatively simple since they'll have access to the session information via the intrinsic functions that have already been established in this PR. What do we do about the CFN and Terraform deployments though?

When we were utilizing AWS::URLSuffix in Cloud Formation and data.aws_partition.chalice.dns_suffix in Terraform, this was relatively simple. As we can no longer use those features, we no longer have a simple mechanism in which to find service principals.

[jamesls]

What if we pass in some sort of Options object where we can add misc data to it? I imagine this won't be the last time we'll need additional data in the packagers.

[andrew-mcgrath]

Options leaves us with pardon the pun, a number of options. The question becomes though was the initial intent of the chalice package to create something that was agnostic to the deployment environment (aka region) or was it supposed to be a targeted solution?

Would it make sense to pass the TypedAWSClient down in this Options object so that we could leverage whatever was available in our current resolver pattern?

[jamesls]

The question becomes though was the initial intent of the chalice package to create something that was agnostic to the deployment environment (aka region) or was it supposed to be a targeted solution?

Preferably agnostic to the deployment environment because it's more flexible for users, and there's less possibility for things to go wrong (e.g. different regions between package vs. deploying your cfn stack).

I think as a min bar we should preserve the existing behavior of being agnostic to region when we're in the aws partition (people could be relying on that). While possible, I don't think it's likely someone would package in aws-cn but then try to deploy in an aws partition. I would be ok with a tradeoff that if you want to package for a non-aws partition, we'll use your currently configured region to look at that info.

In terms of implementation, could we look up the configured region before calling the packagers and, if not in the aws partition, fill in the Options object with the relevant info?

[andrew-mcgrath]

Sounds good, I believe this would require the --profile option be added to the package subcommand as it exists in deploy including some of the interactions with the CLIFactory. Since we'll need the session information in order to determine if we're outside of the standard aws partition.

[andrew-mcgrath]

@jamesls I've done the work to port the region-info lookups from the CDK into chalice and added a new executor intrinsic function that can be utilized with this lookup code. The deployer part seems to be functional, (aka all tests pass).

For chalice.pipeline I don't believe any changes need to made because there are no exceptions to how the principal behaves in the non-aws partitions.

The packager on the other hand definitely needs access to the profile information. This already exists in the Config object and can be simply added to the package command with a --profile in the same way that deploy utilizes it by setting it on the CLI factory. After this point, I think we need to diverge from the deploy implementation when it comes to creating and utilizing the session downstream.

Since the Config already exists within the TemplateGenerator we can access the profile and create a session for use by the extending classes. Thus we'll be able to provide the service principal without creating a long chain of passed objects from the CLI factory down.

class TemplateGenerator(object):
    def __init__(self, config):
        # type: (Config) -> None
        self._config = config
        self._session = Session(profile=self._config.profile)

    def _service_principal(self, service_name):
        client = self._session.create_client(service_name)
        region = client.meta.region_name
        dns_suffix = regioninfo.endpoint_dns_suffix(service_name, region)
        return regioninfo.service_principal(service_name, region, dns_suffix)

This would require the migration of the region utility methods back out of the AWSTypeClient which we had stashed within the context of an instance of the client.

Thoughts?

[jamesls]

Is this proposal in lieu of or in addition to the Options idea proposed previously? I was thinking that we'd roughly go:

# chalice/cli/__init__.py

options = factory.create_options_thing()
...
packager.package_app(config, dirname, stage, options)


# chalice/package.py::AppPackager
self._templater.generate(resources, options)


# chalice/package.py::TemplateGenerator
def generate(self, resources, options):
   # Then either:
   options.service_principals[service_name]
   # or a function call:
   options.service_principal(service_name)

Or add it to the __init__ of the template generator and pass it down through the CLI factory as you mentioned.

[andrew-mcgrath]

@jamesls thanks for the pseudo code, it gave me the kick in the right direction. I was concerned with the modification of the method signatures and repeated object passing to the _methods internal to the different generators. I should have the necessary changes ready shortly so that we can put this issue to 🛌 .

[andrew-mcgrath]

@jamesls I've implemented the changes for the service_principle lookup within the context of a PackageOptions class. Please have a gander and see how we could make it better.

[jamesls]

Sorry for the delay, the changes look good to me. I rebased and had to make a few changes to work with the latest master branch. Thanks for all your work on this!

[andrew-mcgrath]

Sorry for the delay, the changes look good to me. I rebased and had to make a few changes to work with the latest master branch. Thanks for all your work on this!

@jamesls My pleasure, I'm sure/know that there are some functional pieces still missing but I believe it's a good start. The next big challenge is supporting role/policy generation for the non-standard partitions. Thank you for allowing me to contribute.