Skip to content
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

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

Open
iainelder opened this issue Nov 27, 2023 · 8 comments

Comments

@iainelder
Copy link

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
Copy link
Collaborator

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
Copy link
Author

iainelder commented Nov 27, 2023

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

regions = self._validate_regions(regions or [self.region])
region_list = [{"Region": region, "RegionConfigurationStatus": "ENABLED" if region in regions else "DISABLED"}
for region in self.get_available_regions()]

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@pragprogrammer
Copy link

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
Copy link
Collaborator

costastf commented Nov 27, 2023

🤣 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
Copy link
Author

@costastf merged #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
Copy link
Collaborator

Sure, makes perfect sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants