From d18f6dbdac1711f8c0fcb94c5215bb73eade836d Mon Sep 17 00:00:00 2001 From: biffgaut Date: Mon, 11 Sep 2023 14:46:51 -0400 Subject: [PATCH] Code Smells --- .../aws-lambda-s3/lib/index.ts | 189 +++++++++--------- .../aws-route53-alb/lib/index.ts | 18 +- .../aws-route53-apigateway/lib/index.ts | 55 ++--- .../core/lib/kendra-helper.ts | 1 - .../core/lib/sqs-helper.ts | 17 +- 5 files changed, 151 insertions(+), 129 deletions(-) diff --git a/source/patterns/@aws-solutions-constructs/aws-lambda-s3/lib/index.ts b/source/patterns/@aws-solutions-constructs/aws-lambda-s3/lib/index.ts index 92c805d0d..aaf234198 100644 --- a/source/patterns/@aws-solutions-constructs/aws-lambda-s3/lib/index.ts +++ b/source/patterns/@aws-solutions-constructs/aws-lambda-s3/lib/index.ts @@ -79,110 +79,117 @@ export interface LambdaToS3Props { * * @default - Default props are used */ - readonly loggingBucketProps?: s3.BucketProps + readonly loggingBucketProps?: s3.BucketProps /** * Whether to turn on Access Logs for the S3 bucket with the associated storage costs. * Enabling Access Logging is a best practice. * * @default - true */ - readonly logS3AccessLogs?: boolean; + readonly logS3AccessLogs?: boolean; } /** * @summary The LambdaToS3 class. */ export class LambdaToS3 extends Construct { - public readonly lambdaFunction: lambda.Function; - public readonly s3Bucket?: s3.Bucket; - public readonly s3LoggingBucket?: s3.Bucket; - public readonly vpc?: ec2.IVpc; - public readonly s3BucketInterface: s3.IBucket; - - /** - * @summary Constructs a new instance of the LambdaToS3 class. - * @param {cdk.App} scope - represents the scope for all the resources. - * @param {string} id - this is a a scope-unique id. - * @param {LambdaToS3Props} props - user provided props for the construct. - * @since 0.8.0 - * @access public - */ - constructor(scope: Construct, id: string, props: LambdaToS3Props) { - super(scope, id); - - // All our tests are based upon this behavior being on, so we're setting - // context here rather than assuming the client will set it - this.node.setContext("@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy", true); - - defaults.CheckProps(props); - - if (props.bucketPermissions) { - defaults.CheckListValues(['Delete', 'Put', 'Read', 'ReadWrite', 'Write'], props.bucketPermissions, 'bucket permission'); - } - - let bucket: s3.IBucket; - - if (props.deployVpc || props.existingVpc) { - this.vpc = defaults.buildVpc(scope, { - defaultVpcProps: defaults.DefaultIsolatedVpcProps(), - existingVpc: props.existingVpc, - userVpcProps: props.vpcProps, - constructVpcProps: { - enableDnsHostnames: true, - enableDnsSupport: true, - }, - }); - - defaults.AddAwsServiceEndpoint(scope, this.vpc, defaults.ServiceEndpointTypes.S3); - } - - // Setup the Lambda function - this.lambdaFunction = defaults.buildLambdaFunction(this, { - existingLambdaObj: props.existingLambdaObj, - lambdaFunctionProps: props.lambdaFunctionProps, - vpc: this.vpc, + public readonly lambdaFunction: lambda.Function; + public readonly s3Bucket?: s3.Bucket; + public readonly s3LoggingBucket?: s3.Bucket; + public readonly vpc?: ec2.IVpc; + public readonly s3BucketInterface: s3.IBucket; + + /** + * @summary Constructs a new instance of the LambdaToS3 class. + * @param {cdk.App} scope - represents the scope for all the resources. + * @param {string} id - this is a a scope-unique id. + * @param {LambdaToS3Props} props - user provided props for the construct. + * @since 0.8.0 + * @access public + */ + constructor(scope: Construct, id: string, props: LambdaToS3Props) { + super(scope, id); + + // All our tests are based upon this behavior being on, so we're setting + // context here rather than assuming the client will set it + this.node.setContext("@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy", true); + + defaults.CheckProps(props); + + if (props.bucketPermissions) { + defaults.CheckListValues(['Delete', 'Put', 'Read', 'ReadWrite', 'Write'], props.bucketPermissions, 'bucket permission'); + } + + let bucket: s3.IBucket; + + if (props.deployVpc || props.existingVpc) { + this.vpc = defaults.buildVpc(scope, { + defaultVpcProps: defaults.DefaultIsolatedVpcProps(), + existingVpc: props.existingVpc, + userVpcProps: props.vpcProps, + constructVpcProps: { + enableDnsHostnames: true, + enableDnsSupport: true, + }, + }); + + defaults.AddAwsServiceEndpoint(scope, this.vpc, defaults.ServiceEndpointTypes.S3); + } + + // Setup the Lambda function + this.lambdaFunction = defaults.buildLambdaFunction(this, { + existingLambdaObj: props.existingLambdaObj, + lambdaFunctionProps: props.lambdaFunctionProps, + vpc: this.vpc, + }); + + // Setup S3 Bucket + if (!props.existingBucketObj) { + const buildS3BucketResponse = defaults.buildS3Bucket(this, { + bucketProps: props.bucketProps, + loggingBucketProps: props.loggingBucketProps, + logS3AccessLogs: props.logS3AccessLogs }); + this.s3Bucket = buildS3BucketResponse.bucket; + this.s3LoggingBucket = buildS3BucketResponse.loggingBucket; - // Setup S3 Bucket - if (!props.existingBucketObj) { - const buildS3BucketResponse = defaults.buildS3Bucket(this, { - bucketProps: props.bucketProps, - loggingBucketProps: props.loggingBucketProps, - logS3AccessLogs: props.logS3AccessLogs - }); - this.s3Bucket = buildS3BucketResponse.bucket; - this.s3LoggingBucket = buildS3BucketResponse.loggingBucket; - - bucket = this.s3Bucket; - } else { - bucket = props.existingBucketObj; - } - - this.s3BucketInterface = bucket; - - // Configure environment variables - const bucketEnvironmentVariableName = props.bucketEnvironmentVariableName || 'S3_BUCKET_NAME'; - this.lambdaFunction.addEnvironment(bucketEnvironmentVariableName, bucket.bucketName); - - // Add the requested or default bucket permissions - if (props.bucketPermissions) { - if (props.bucketPermissions.includes('Delete')) { - bucket.grantDelete(this.lambdaFunction.grantPrincipal); - } - if (props.bucketPermissions.includes('Put')) { - bucket.grantPut(this.lambdaFunction.grantPrincipal); - } - if (props.bucketPermissions.includes('Read')) { - bucket.grantRead(this.lambdaFunction.grantPrincipal); - } - if (props.bucketPermissions.includes('ReadWrite')) { - bucket.grantReadWrite(this.lambdaFunction.grantPrincipal); - } - if (props.bucketPermissions.includes('Write')) { - bucket.grantWrite(this.lambdaFunction.grantPrincipal); - } - } else { - bucket.grantReadWrite(this.lambdaFunction.grantPrincipal); - } + bucket = this.s3Bucket; + } else { + bucket = props.existingBucketObj; + } + + this.s3BucketInterface = bucket; + + // Configure environment variables + const bucketEnvironmentVariableName = props.bucketEnvironmentVariableName || 'S3_BUCKET_NAME'; + this.lambdaFunction.addEnvironment(bucketEnvironmentVariableName, bucket.bucketName); + + // Add the requested or default bucket permissions + if (props.bucketPermissions) { + this.AssignAppropriatePrivileges(props, bucket, this.lambdaFunction); + } else { + bucket.grantReadWrite(this.lambdaFunction.grantPrincipal); + } + } + + private AssignAppropriatePrivileges(props: LambdaToS3Props, bucket: s3.IBucket, clientFunction: lambda.Function) { + if (!props.bucketPermissions) { + throw new Error('bucketPermissions required here'); + } + if (props.bucketPermissions.includes('Delete')) { + bucket.grantDelete(clientFunction.grantPrincipal); + } + if (props.bucketPermissions.includes('Put')) { + bucket.grantPut(clientFunction.grantPrincipal); + } + if (props.bucketPermissions.includes('Read')) { + bucket.grantRead(clientFunction.grantPrincipal); + } + if (props.bucketPermissions.includes('ReadWrite')) { + bucket.grantReadWrite(clientFunction.grantPrincipal); + } + if (props.bucketPermissions.includes('Write')) { + bucket.grantWrite(clientFunction.grantPrincipal); } + } } \ No newline at end of file diff --git a/source/patterns/@aws-solutions-constructs/aws-route53-alb/lib/index.ts b/source/patterns/@aws-solutions-constructs/aws-route53-alb/lib/index.ts index 60a80a31e..6f4bb3489 100644 --- a/source/patterns/@aws-solutions-constructs/aws-route53-alb/lib/index.ts +++ b/source/patterns/@aws-solutions-constructs/aws-route53-alb/lib/index.ts @@ -107,13 +107,7 @@ export class Route53ToAlb extends Construct { // NOTE: We don't call CheckAlbProps() here, because this construct creates an ALB // with no listener or target, so some of those checks don't apply - if (props?.loadBalancerProps?.vpc) { - throw new Error('Specify any existing VPC at the construct level, not within loadBalancerProps.'); - } - - if (props.existingLoadBalancerObj && !props.existingVpc) { - throw new Error('An existing ALB already exists in a VPC, so that VPC must be passed to the construct in props.existingVpc'); - } + this.PropsCustomCheck(props); if (props.existingHostedZoneInterface && !props.publicApi && !props.existingVpc) { throw new Error('An existing Private Hosted Zone already exists in a VPC, so that VPC must be passed to the construct in props.existingVpc'); @@ -167,4 +161,14 @@ export class Route53ToAlb extends Construct { // Before turning off SonarQube for the line, reduce the line to it's minimum new r53.ARecord(this, arecordId, arecordProps); // NOSONAR } + + private PropsCustomCheck(props: Route53ToAlbProps) { + if (props?.loadBalancerProps?.vpc) { + throw new Error('Specify any existing VPC at the construct level, not within loadBalancerProps.'); + } + + if (props.existingLoadBalancerObj && !props.existingVpc) { + throw new Error('An existing ALB already exists in a VPC, so that VPC must be passed to the construct in props.existingVpc'); + } + } } diff --git a/source/patterns/@aws-solutions-constructs/aws-route53-apigateway/lib/index.ts b/source/patterns/@aws-solutions-constructs/aws-route53-apigateway/lib/index.ts index bb1c001e4..ef1a2cf07 100755 --- a/source/patterns/@aws-solutions-constructs/aws-route53-apigateway/lib/index.ts +++ b/source/patterns/@aws-solutions-constructs/aws-route53-apigateway/lib/index.ts @@ -95,32 +95,12 @@ export class Route53ToApiGateway extends Construct { // Existing Public or Private Hosted Zone if (props.existingHostedZoneInterface) { this.hostedZone = props.existingHostedZoneInterface; + this.ExistingHostedZonePropCheck(props); - if (props.existingVpc) { - throw new Error('Cannot provide an existing VPC to an existing Private Hosted Zone.'); - } - if (props.privateHostedZoneProps) { - throw new Error('Must provide either existingHostedZoneInterface or privateHostedZoneProps, but not both.'); - } } else { // Creating a Private Hosted Zone - if (props.publicApi) { - throw new Error('Public APIs require an existingHostedZone be passed in the Props object.'); - } else { - if (!props.privateHostedZoneProps) { - throw new Error('Must provide either existingHostedZoneInterface or privateHostedZoneProps.'); - } - if (props.privateHostedZoneProps.vpc) { - throw new Error('All VPC specs must be provided at the Construct level in Route53ToApiGatewayProps.'); - } - if (!props.privateHostedZoneProps.zoneName) { - throw new Error('Must supply zoneName for Private Hosted Zone Props.'); - } - if ( !this.vpc ) { - throw new Error('Must specify an existingVPC for the Private Hosted Zone in the construct props.'); - } - const manufacturedProps: route53.PrivateHostedZoneProps = defaults.overrideProps(props.privateHostedZoneProps, { vpc: this.vpc }); - this.hostedZone = new route53.PrivateHostedZone(this, `${id}-zone`, manufacturedProps); - } + this.PrivateHostedZonePropsChecks(props); + const manufacturedProps: route53.PrivateHostedZoneProps = defaults.overrideProps(props.privateHostedZoneProps, { vpc: this.vpc }); + this.hostedZone = new route53.PrivateHostedZone(this, `${id}-zone`, manufacturedProps); } // Convert IRestApi to RestApi @@ -140,4 +120,31 @@ export class Route53ToApiGateway extends Construct { // Create A Record in custom domain to route traffic to API Gateway new route53.ARecord(this, 'CustomDomainAliasRecord', arecordProps); // NOSONAR } + + private ExistingHostedZonePropCheck(props: Route53ToApiGatewayProps) { + if (props.existingVpc) { + throw new Error('Cannot provide an existing VPC to an existing Private Hosted Zone.'); + } + if (props.privateHostedZoneProps) { + throw new Error('Must provide either existingHostedZoneInterface or privateHostedZoneProps, but not both.'); + } + } + + private PrivateHostedZonePropsChecks(props: Route53ToApiGatewayProps) { + if (props.publicApi) { + throw new Error('Public APIs require an existingHostedZone be passed in the Props object.'); + } + if (!props.privateHostedZoneProps) { + throw new Error('Must provide either existingHostedZoneInterface or privateHostedZoneProps.'); + } + if (props.privateHostedZoneProps.vpc) { + throw new Error('All VPC specs must be provided at the Construct level in Route53ToApiGatewayProps.'); + } + if (!props.privateHostedZoneProps.zoneName) { + throw new Error('Must supply zoneName for Private Hosted Zone Props.'); + } + if (!this.vpc) { + throw new Error('Must specify an existingVPC for the Private Hosted Zone in the construct props.'); + } + } } \ No newline at end of file diff --git a/source/patterns/@aws-solutions-constructs/core/lib/kendra-helper.ts b/source/patterns/@aws-solutions-constructs/core/lib/kendra-helper.ts index 65d94c116..821552472 100644 --- a/source/patterns/@aws-solutions-constructs/core/lib/kendra-helper.ts +++ b/source/patterns/@aws-solutions-constructs/core/lib/kendra-helper.ts @@ -24,7 +24,6 @@ import { Aws } from 'aws-cdk-lib'; // Note: To ensure CDKv2 compatibility, keep the import statement for Construct separate import { Construct } from 'constructs'; import { DefaultKendraIndexProps } from './kendra-defaults'; -import { } from "./utils"; export interface BuildKendraIndexProps { readonly kendraIndexProps?: kendra.CfnIndexProps | any; diff --git a/source/patterns/@aws-solutions-constructs/core/lib/sqs-helper.ts b/source/patterns/@aws-solutions-constructs/core/lib/sqs-helper.ts index 15fcec8c7..0d93060ee 100644 --- a/source/patterns/@aws-solutions-constructs/core/lib/sqs-helper.ts +++ b/source/patterns/@aws-solutions-constructs/core/lib/sqs-helper.ts @@ -76,12 +76,7 @@ export interface BuildQueueResponse { * @internal This is an internal core function and should not be called directly by Solutions Constructs clients. */ export function buildQueue(scope: Construct, id: string, props: BuildQueueProps): BuildQueueResponse { - - if ((props.queueProps?.encryptionMasterKey || props.encryptionKey || props.encryptionKeyProps) - && props.enableEncryptionWithCustomerManagedKey === true) { - printWarning(`Ignoring enableEncryptionWithCustomerManagedKey because one of - queueProps.encryptionMasterKey, encryptionKey, or encryptionKeyProps was already specified`); - } + CheckEncryptionWarnings(props); // If an existingQueueObj is not specified if (!props.existingQueueObj) { @@ -127,6 +122,16 @@ export function buildQueue(scope: Construct, id: string, props: BuildQueueProps) } } +/** + * @internal This is an internal core function and should not be called directly by Solutions Constructs clients. + */ +function CheckEncryptionWarnings(props: BuildQueueProps) { + if ((props.queueProps?.encryptionMasterKey || props.encryptionKey || props.encryptionKeyProps) + && props.enableEncryptionWithCustomerManagedKey === true) { + printWarning(`Ignoring enableEncryptionWithCustomerManagedKey because one of + queueProps.encryptionMasterKey, encryptionKey, or encryptionKeyProps was already specified`); + } +} export interface BuildDeadLetterQueueProps { /** * Existing instance of SQS queue object, providing both this and queueProps will cause an error.