Skip to content

Conversation

@ravinadhruve10
Copy link
Contributor

@ravinadhruve10 ravinadhruve10 commented Nov 14, 2024

Change summary:

  • Added support to install govcloud single account and org in foundational template.
  • Added same support in log_ingestion event-bridge template.
  • For log_ingestion s3, no changes required to the template. Added minor nits.

Testing done:

With the UI PR changes was able to test below,

Single AWS Govcloud account,

  • Foundational onboarding - ✅
  • CDR install with EB - Unsupported (CFT stack creation & BE block this as expected) - ✅
  • CDR install with S3 - ✅

AWS Govcloud Org,

  • Foundational onboarding - ✅
  • CDR install with EB - Unsupported (CFT stack creation & BE block this as expected) - ✅
  • CDR install with S3 - NA

Change summary:
----------------
- Added support to install govcloud single account and org in foundational template.
- Added same support in log_ingestion event-bridge template.
- For log_ingestion s3, no changes required to the template. Added minor nits.
@ravinadhruve10 ravinadhruve10 marked this pull request as ready for review November 15, 2024 06:24
@ravinadhruve10 ravinadhruve10 requested review from a team as code owners November 15, 2024 06:24
@nkraemer-sysdig
Copy link
Collaborator

Please note the new tagging process needed to release CFTs

Copy link
Contributor

@cgeers cgeers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the use of a prefix here. ARNs are not arbitrary strings who's contents we control entirely. AWS dictates the valid format of the ARN, but we're exposing broadly scoped manipulation of the whole ARN string. The basic problem with this can be summarized as "how much of the ARN is being prefixed?" It's not described, users don't know, but then again, nor should they.

Instead this variable should be simply be "partition" with a default value representing commercial.

@ravinadhruve10
Copy link
Contributor Author

ravinadhruve10 commented Nov 19, 2024

I don't like the use of a prefix here. ARNs are not arbitrary strings who's contents we control entirely. AWS dictates the valid format of the ARN, but we're exposing broadly scoped manipulation of the whole ARN string. The basic problem with this can be summarized as "how much of the ARN is being prefixed?" It's not described, users don't know, but then again, nor should they.

So this input does not come from the user actually (nor should it like you mentioned), but is rather passed from the UI in quickcreate URL (similar to how we have for externalID etc). Hence, we control what prefix string we exactly send and we also do have a default. Do you think it will help if I update the description to mention an example of an ARN and how much of it is the prefix.

Instead this variable should be simply be "partition" with a default value representing commercial.

So we instead pass partition field with default for commercial = "aws" and for gov it will be = "aws-us-gov". correct?
If so, yes that can be done and would be better I agree. (this will be in line with aws docs link)

@ravinadhruve10
Copy link
Contributor Author

Did another round of testing with latest changes, lgtm!

@ravinadhruve10 ravinadhruve10 merged commit 57c2dcc into main Nov 19, 2024
3 checks passed
@ravinadhruve10 ravinadhruve10 deleted the fedramp-support branch November 19, 2024 23:52
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

Successfully merging this pull request may close these issues.

6 participants