schubergphilis / awsapilib

A python library exposing services that are not covered by the official boto3 library but are driven by undocumented APIs.
MIT License
60 stars 8 forks source link

Invalid regions us-gov-west-1, us-gov-east-1 fail Control Tower deployment #58

Open iainelder opened 9 months ago

iainelder commented 9 months ago

See superwerker issue 417 for the original report and analysis.

I cross post it here because I think the solution depends on a change to awsapilib.

If you prefer to discuss it here, I'll copy the relevant details.

costastf commented 9 months ago

Hi @iainelder , thanks for reporting, i noticed the initial report. Not sure what you have in mind as a solution though. The deploy method already accepts an argument of regions that respects. So if gov regions should not be used one should construct the argument before calling the deploy method. What else did you have in mind?

iainelder commented 9 months ago

I think this code needs to change. If I've misunderstood anything please let me know.

https://github.com/schubergphilis/awsapilib/blob/0c8432a38d3618b8ea0583e29a7732acedf3aa33/awsapilib/controltower/controltower.py#L1236-L1238

The regions argument doesn't control which regions awsapilib writes in the payload.

awsapilib takes the regions from the "AWS Services by Region" API and writes all of them into the payload.

The regions argument controls whether awsapilib writes "ENABLED" or "DISABLED" for each region.

The call in Superwerker issue 417 happened in us-west-2. It didn't pass a region list to the deploy function.

So regions would have become ["us-west-2"].

So regions_list would have become

[
    {"Region": "us-west-2", "RegionConfigurationStatus": "ENABLED"},
    {"Region": "us-east-1", "RegionConfigurationStatus": "DISABLED"},
    ...
    {"Region": "us-gov-west-1", "RegionConfigurationStatus": "DISABLED"},
    {"Region": "us-gov-east-1", "RegionConfigurationStatus": "DISABLED"},
]

Control Tower rejects those GovCloud regions us-gov-west-1 and us-gov-east-1.

I think there's nothing the client can do to stop awsapilib writing them into the payload.

I think you can fix it by leaving the GovCloud regions out of the payload.

For now that would mean awsapilib doesn't support GovCloud, but that changes nothing since it didn't support it before either.

If you want to support GovCloud in the future, you can maybe change the region-handling code to select regions based on on the home partition.

costastf commented 9 months ago

That makes perfect sense. So what you are saying is that deploying control tower fails even if it has the gov regions set as disabled in the call? That is the part that I missed then, i thought that since they are now supported it should work fine. I think that providing the capability to filter out the gov ones would be the way to go for now, what do you think?

costastf commented 9 months ago

How does https://github.com/schubergphilis/awsapilib/pull/59/files look like to you @iainelder ?

pragprogrammer commented 9 months ago

For improved readability @costastf I would change the name of the new argument from enable_gov_regions=False -> disable_gov_regions=True. Overall I think everything looks great, this change could just help understand that by default the new argument is disabling the gov regions without having to go look for what value was set for it.

costastf commented 9 months ago

🤣 The negation broke my brain to be honest, i am just doing 120 things at the same time. Thanks of course it makes perfect sense! 🙇

iainelder commented 9 months ago

@costastf merged https://github.com/schubergphilis/awsapilib/pull/59 and released version 3.1.4.

I say we keep this issue open until we confirm that it solves the problem downstream. I think it will be easier to test this as part of superwerker.

costastf commented 9 months ago

Sure, makes perfect sense.