awsapilib icon indicating copy to clipboard operation
awsapilib copied to clipboard

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

Open iainelder opened this issue 2 years ago • 8 comments

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.

iainelder avatar Nov 27 '23 10:11 iainelder

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?

costastf avatar Nov 27 '23 10:11 costastf

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.

iainelder avatar Nov 27 '23 11:11 iainelder

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 avatar Nov 27 '23 12:11 costastf

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

costastf avatar Nov 27 '23 12:11 costastf

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.

pragprogrammer avatar Nov 27 '23 15:11 pragprogrammer

🤣 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! 🙇

costastf avatar Nov 27 '23 15:11 costastf

@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.

iainelder avatar Nov 29 '23 06:11 iainelder

Sure, makes perfect sense.

costastf avatar Nov 29 '23 07:11 costastf