aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.6k stars 3.9k forks source link

aws-cdk: Incorrect/Insufficient Aspect Documentation #25760

Open lsmarsden opened 1 year ago

lsmarsden commented 1 year ago

Describe the issue

The CDK documentation for Aspects doesn't seem to correctly demonstrate what is seen when running the code against a CDK app/stack. The documentation also doesn't clarify much of the purpose behind the use of code, and why it works (or how it can be generally applied to aspects). This issue might also extend to the Token documentation, but I don't know enough about the CDK to judge that.

I'm working with the AWS CDK in Java, so will focus on the Java Aspect examples in the given documentation. The documentation demonstrates Aspects by creating an Aspect that can validate S3 buckets have versioning enabled.

Sample reproduction steps following the linked documentation:

  1. Create an example BucketVersioningChecker aspect:

    public class BucketVersioningChecker implements IAspect {
    
      @Override
      public void visit( @NotNull IConstruct node ) {
    
          if ( node instanceof CfnBucket bucket ) {
              Object versioningConfiguration = bucket.getVersioningConfiguration();
    
              if ( versioningConfiguration == null
                  || !Tokenization.isResolvable( versioningConfiguration.toString() )
                      && !versioningConfiguration.toString().contains( "Enabled" ) ) {
    
                      Annotations.of( bucket ).addError( "S3 bucket does not have versioning enabled" );
              }
          }
      }
    }
  2. Create a CDK stack that has an S3 bucket, and apply the aspect to the stack:

    public class TestApp {
    
    public static void main( final String[] args ) {
        App app = new App()
        Stack stack = new Stack(app, "stack");
        Bucket bucket = Bucket.Builder.create(stack, "versionedBucket")
          .versioned(true)
          .build();
    
        Aspects.of(stack).add(new BucketVersioningChecker());
        app.synth()
    }
    }
  3. Run a cdk synth

This returns the following error: [Error at /stack/versionedBucket/Resource] S3 bucket does not have versioning enabled

After doing a lot of debugging (and trial and error), I've come across the following information:

I have found through trial and error that resolving the JsiiObject in the following way works, but I don't know exactly why or if there are easier/better ways of doing so:

// returns a LinkedHashMap {status=Enabled}
Object resolved = Tokenization.resolve( versioningConfiguration , ResolveOptions.builder()
                    .scope( bucket )
                    .resolver( new DefaultTokenResolver( new StringConcat() ) )
                    .build() );

I found this was also required in order to create a similar Aspect that validates S3 buckets have some lifecycle policy applied (similar setup, except using bucket.getLifecycleConfiguration()). Of course, this is a chunky way of having to resolve an object to a Java type (and I don't know enough about Jsii to know if this is a safe thing to do). Having to use this in any Aspects I create would be a slight pain, but could be centralised somewhere.

I can't for the life of me figure out whether I'm supposed to be checking things are resolvable constantly in Aspects (I'm very new to the CDK), but with this method it seems like any Aspect that inspects properties would need to do some form of JsiiObject conversion before being able to check anything.

Missing documentation/information that would be helpful for aspect creation:

Links

https://docs.aws.amazon.com/cdk/v2/guide/aspects.html

pahud commented 1 year ago

Agree. We should improve the doc on Aspects with more details.

lsmarsden commented 1 year ago

Thanks @pahud. What's the usual process for documentation issues like this one - do you need anything more from me?

Also, what's the usual lead time on documentation updates? We're using the workaround mentioned above at the moment, but we're eager to take on board guidance from documentation updates sooner rather than later, before our CDK project starts to mature.