awslabs / aws-bootstrap-kit

Apache License 2.0
105 stars 22 forks source link

`OUSpec` not allowing for account-free OUs #117

Closed npvisual closed 2 years ago

npvisual commented 2 years ago

Description

I am trying to build a hierarchy that goes 2 level deep with some OUs being merely containers for other OUs (i.e. don't have any associated accounts).

For example, if I customize the AWS Bootstrap Kit nestedOU as follows :

const nestedOU = [
    {
        name: 'SharedServices',
        accounts: [
            {
                name: 'CICD',
                type: AccountType.CICD
            }
        ]
    },
    {
        name: 'SDLC',
        accounts: [
            {
                name: 'Dev',
                type: AccountType.PLAYGROUND
            }
        ]
    },
    {
        name: 'Tenants',                            <---------- not supported ???
        nestedOU: [
            {
                name: 'tenantA',
                accounts: [
                    {
                        name: 'Staging',
                        type: AccountType.STAGE,
                        stageName: 'staging',
                        stageOrder: 1,
                        hostedServices: ['ALL'],
                        existingAccountId: '1234567890'
                    },
                    {
                        name: 'Prod',
                        type: AccountType.STAGE,
                        stageName: 'prod',
                        stageOrder: 2,
                        hostedServices: ['ALL']
                    }
                ]
            }
        ]
    }
];

Then it seems that the current definition of OUSpec will get in the way and not allow me to specify an "account-free" OU (container for other OUs with accounts).

/**
 * Organizational Unit Input details
 */
export interface OUSpec {
  /**
   * Name of the Organizational Unit
   */
  readonly name: string,

  /**
   * Accounts' specification inside in this Organizational Unit
   */
  readonly accounts: AccountSpec[],                                           <------- make optional ?

  /**
   * Specification of sub Organizational Unit
   */
  readonly nestedOU?: OUSpec[]
}

Is this a current limitation ?

flochaz commented 2 years ago

Hi @npvisual , thanks, good catch , indeed we should make accounts optional to support deepper OU tree. Will fix asap.

npvisual commented 2 years ago

Was trying to fix it locally just to see what would happen and I am getting the following error during token resolution 🤷‍♂️ :

{...}/aws-bootstrap-kit-examples/source/1-SDLC-organization/node_modules/aws-cdk-lib/core/lib/private/uniqueid.ts:42
    throw new Error(`ID components may not include unresolved tokens: ${unresolvedTokens.join(',')}`);
          ^
Error: Resolution error: ID components may not include unresolved tokens: ${Token[TOKEN.1393]}SubZoneDelegationNSRecord.
flochaz commented 2 years ago

thanks for the try. will fix tomorrow

On Mon, Aug 1, 2022, 21:35 Nicolas Philippe @.***> wrote:

Was trying to fix it locally just to see what would happen and I am getting the following error during token resolution 🤷‍♂️ :

{...}/aws-bootstrap-kit-examples/source/1-SDLC-organization/node_modules/aws-cdk-lib/core/lib/private/uniqueid.ts:42

throw new Error(`ID components may not include unresolved tokens: ${unresolvedTokens.join(',')}`);

      ^

Error: Resolution error: ID components may not include unresolved tokens: ${Token[TOKEN.1393]}SubZoneDelegationNSRecord.

— Reply to this email directly, view it on GitHub https://github.com/awslabs/aws-bootstrap-kit/issues/117#issuecomment-1201629148, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJLIUJAJJIWMXHOTM7WLWDVXARJPANCNFSM55BYS23A . You are receiving this because you commented.Message ID: @.***>

npvisual commented 2 years ago

After testing some more, it looks like the unresolved tokens issue above was not related to the nesting but rather to the use of existingAccountId in one of the AccountSpec.

thanks for the try. will fix tomorrow

Running a cdk diff on the SDLC example with a nested OU works fine :

1-SDLC-organization % cdk --profile main-admin diff
Stack AWSBootstrapKit-LandingZone-PipelineStack
IAM Statement Changes
....
Outputs
[+] Output PipelineConsoleUrl PipelineConsoleUrl: {"Value":{"Fn::Join":["",["https://",{"Ref":"AWS::Region"},".console.aws.amazon.com/codesuite/codepipeline/pipelines/AWSBootstrapKit-LandingZone/view?region=",{"Ref":"AWS::Region"}]]}}

Other Changes
[+] Unknown Rules: {"CheckBootstrapVersion":{"Assertions":[{"Assert":{"Fn::Not":[{"Fn::Contains":[["1","2","3","4","5"],{"Ref":"BootstrapVersion"}]}]},"AssertDescription":"CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI."}]}}

Diff of the changes used :

diff --git a/source/aws-bootstrap-kit/lib/aws-organizations-stack.ts b/source/aws-bootstrap-kit/lib/aws-organizations-stack.ts
index b71a505..b572682 100644
--- a/source/aws-bootstrap-kit/lib/aws-organizations-stack.ts
+++ b/source/aws-bootstrap-kit/lib/aws-organizations-stack.ts
@@ -87,7 +87,7 @@ export interface OUSpec {
   /**
    * Accounts' specification inside in this Organizational Unit
    */
-  readonly accounts: AccountSpec[],
+  readonly accounts?: AccountSpec[],

   /**
    * Specification of sub Organizational Unit
@@ -150,7 +150,8 @@ export class AwsOrganizationsStack extends Stack {

     previousSequentialConstruct = organizationalUnit;

-    oUSpec.accounts.forEach(accountSpec => {
+    oUSpec.accounts?.forEach(accountSpec => {
       let accountEmail: string;
       if(accountSpec.email)
       {
npvisual commented 2 years ago

Ran it with tag v0.7.4 this morning (locally built) and the cdk diff is running properly (haven't tried deploy yet).

Btw, it looks like npmjs.org only has 0.7.1 as the most up-to-date version.

flochaz commented 2 years ago

We are having issues with release pipeline . hold on before testing . sorry about that. I'll post back here when fixed

flochaz commented 2 years ago

fixed in 0.7.4