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

Best practices for building company default constructs #3235

Closed
lapair opened this issue Jul 8, 2019 · 12 comments
Closed

Best practices for building company default constructs #3235

lapair opened this issue Jul 8, 2019 · 12 comments
Assignees
Labels
@aws-cdk/core Related to core CDK functionality effort/medium Medium work item – several days of effort feature-request A feature should be added or improved.

Comments

@lapair
Copy link

lapair commented Jul 8, 2019

Having built a few production stacks we are seeing patterns in the component parts we use, and want to enforce internal best practices: timeouts / env values / roles etc as well as keep actual stack logic as small/focused as possible. I was discussing this with @eladb on gitter...

I've been playing with something like: lib/our-company/lambda.ts (with a lib/our-company/index.ts so we can import all constructs with a simple single import):

interface OurCompanyILamdaProps {
    assetPath: string, // what are we deploying
    environment?: any,
    handler?: string,
    description?: string,
    timeout?: number
}

export class Lambda extends cdk.Construct {

    public readonly function: lambda.Function;

    constructor(scope: cdk.Construct, id: string, props: OurCompanyILamdaProps) {
        super(scope, id);

        let env: any = props.environment ? props.environment : {};

        // make sure we always have the ENV available to the lambda function.
        env.DEPLOY_ENV = process.env.DEPLOY_ENV; 

        // construct ID needs to be unique
        let constructId = id + env.DEPLOY_ENV;

        let lam = new lambda.Function(scope, constructId, {
            code: lambda.Code.asset(props.assetPath),
            handler: props.handler ? props.handler : "index.handler",
            runtime: lambda.Runtime.NODEJS_10_X,
            environment: props.environment,
            description: props.description,
            timeout: cdk.Duration.seconds(props.timeout ? props.timeout : 60)
        })
 
       // Do stuff.. 
       // if prop.allowAccessSecrets ... create role and assign to lambda
       // if prop.addMonitoring ...
       // if prop.someThingElse ...

        this.function = lam;
    }
}

Then assuming stacks/NAME/index.ts:

        import * as OurCompany from '../constructs/';

        let lambFunction = new OurCompany.Lambda(this, 'aThing', {
            assetPath: __dirname + '/lambda/function_a/',
            environment: environment
        }).function; // note .function here to get the underlying construct if needed
        aDynamoDBTable.grantReadWriteData(lambFunction);

Other sorts of defaults we are thinking of:

  • All queues get a DLQ
  • Monitoring / alerts
  • Permissions to secrets (by creating specific role), but just pass in the secret string
  • Backup settings on DynamoDB table
  • possibly tagging - though I know you should be able to do that at stack level, last time I tried it hasn't worked.

Additionally we could build feature factory constructs... 'swagger file + lambda functions' = apigateway (I've basically used https://gist.github.com/abbottdev/17379763ebc14a5ecbf2a111ffbcdd86 - from #1461 - and mundged it to hide the implementation of parsing the swagger file).

Some of this will include differences we want between production and development environments - but the goal being standards and best practices (for our company) over all.

Feedback / thoughts would be most appreciated.

@lapair lapair added the needs-triage This issue or PR still needs to be triaged. label Jul 8, 2019
@eladb
Copy link
Contributor

eladb commented Jul 8, 2019

A few thoughts/patterns/ideas (definitely not to be taken as guidance):

Inheritance

You could technically inherit from lambda.Function. I am not sure this is something we recommend or not, but perhaps for this use case it makes sense. I can see some issues with this if you want to create constructs before the super call (you won't be able to access this, which means you'll need to scope it out yourself or create another scope:

export class MyCompanyFunction extends lambda.Function {
  constructor(scope: Construct, id: string, props: MyCompanyFunctionProps) {
    // you might need a scope to create constructs before the "super" call so 
    // you can do something like this, which will create an "inner" scope for you:
    const inner = new Construct(scope, id + '+inner');
    const specialRole = new iam.Role(inner, 'Role', ...);
    super(scope, id, { role, ...props });

    // now, "this" is a lambda.Function
    this.node.applyAspect(new Tag('foo', 'bar'));
  }
}

Base Stack

We are starting to see value in vending a common base stack construct within the team/company to offer access to shared resources such as a shared VPC, a domain name, etc. It is a also a good place to override the standard behavior of how various stack-level attributes are implemented. For example, you could override the stack.availabilityZones property and return something that suites your needs or modify how environments work. AWS Constructs will always roll up to the stack to query this information so it's a good hook.

@eladb
Copy link
Contributor

eladb commented Jul 8, 2019

Copy @NetaNir

@lapair
Copy link
Author

lapair commented Jul 8, 2019

I can see the inheritance case.. some constructs might be too complex for that (multiple resources needing to be accessed in the stack) but for something like lambda that has potential...

I'll have a play with the stack base for sure (we already overwrite stackName/env with region and account for example, so this makes sense).

@lapair
Copy link
Author

lapair commented Jul 8, 2019

I've stuck with a construct for now... my lambda is currently looking like:

import * as cdk from "@aws-cdk/core";
// For... lambda!
import * as lambda from '@aws-cdk/aws-lambda';
// For cron
import events = require('@aws-cdk/aws-events');
import targets = require('@aws-cdk/aws-events-targets');
// For Secrets permissions
import { Role, PolicyStatement } from '@aws-cdk/aws-iam';

interface XXILamdaProps {
    assetPath: string, // what are we deploying
    environment?: object, // { TABLE: ..., FOO: 'bar'}
    handler?: string,
    description?: string,
    timeout?: number, // seconds
    secretAccess?: string,
    role?: Role, // if we need to assume a specific role
    cron?: events.CronOptions
}

export class Lambda extends cdk.Construct {

    public readonly function: lambda.Function;

    constructor(scope: cdk.Construct, id: string, props: XXILamdaProps) {
        super(scope, id);

        let env: any = props.environment ? props.environment : {};
        env.DEPLOY_ENV = process.env.DEPLOY_ENV; // required for our stacks

        let constructId = id + '+' + env.DEPLOY_ENV;

        let role: any = props.role ? props.role : undefined;

        let lam = new lambda.Function(scope, constructId, {
            code: lambda.Code.asset(props.assetPath),
            handler: props.handler ? props.handler : "index.handler",
            runtime: lambda.Runtime.NODEJS_10_X,
            environment: props.environment,
            description: props.description,
            role: role,
            timeout: cdk.Duration.seconds(props.timeout ? props.timeout : 60)
        });

        if (props.secretAccess) {
            // Access secrets
            lam.addToRolePolicy(new PolicyStatement({
                resources: ['*'], // TODO: lock down the supplied string
                actions: ['secretsmanager:getSecretValue']
            }));
        }

        if (props.cron && process.env.DEPLOY_ENV === 'production') {
            // Add cloud watch cron job, but NOT on dev!
            const rule = new events.Rule(this, constructId + '+Rule', {
                schedule: events.Schedule.cron(props.cron)
            });
            rule.addTarget(
                new targets.LambdaFunction(lam)
            );
        }

        this.function = lam;
    }

}

Usage:

        const someLam = new XXX.Lambda(this, 'ANAME', {
            assetPath: __dirname + '/lambda/foo',
            environment: {
                SOME: 'stuff'
            },
            secretAccess: 'path/to/secret',
            cron: {
                hour: '5',
                minute: '0'
            },
        }).function;

Not bad for:

  • deploy lambda
  • setup environment
  • enable secret access
  • trigger on a cron event - but only in production

@ghost
Copy link

ghost commented Jul 21, 2019

I don't know if this is the appropriate place to put these, but I'm looking at this issue too. I have the following basic cases for far:

  • Cloudwatch logs: enforcing expiry times
  • IAM Roles: adding boundary policy by default
  • S3/EC2: Forcing encryption
  • Certs: Enforce email validation and validation domain

My issue is our developers are writing code in .net, javascript and python so we really need to expose these constructors/classes to all languages that CDK allows us to use. So before I go and learn java/type script, I'd like to know if that is the case with the above code.

Question is constructors or inheritance?

@jderose9
Copy link

I've frequently seen the props param extended in examples the way that the initial poster did, including in the CDK documentation. But, I'm wondering if this is a preferred method compared to making an additional constructor parameter for the additional properties? I'm concerned about conflict with future props additions or that some special handling will happen on the props fields for some constructs?

@kevinslin
Copy link

something i've been doing is creating factory functions to initialize constructs with common defaults. makes it easy to override properties as needed.

  • eg: create a bucket that is locked down, with optional accss logs
import _ from 'lodash'
export type scopePlus = cdk.App | cdk.Construct

export function createBucket({scope, id, bucketProps = {}, accessLogBucket}: {
  scope: scopePlus,
  id: string,
  bucketProps?: BucketProps
  accessLogBucket?: IBucket
}) {
  bucketProps = _.defaults({}, bucketProps, {
    encryption: BucketEncryption.KMS_MANAGED,
    blockPublicAccess: BlockPublicAccess.BLOCK_ALL
  })
  const bucket = new Bucket(scope, id, bucketProps)

  if (!_.isUndefined(accessLogBucket)) {
    let logFilePrefix = `${id}/`
    addS3AccessLogs({srcBucket: bucket, destBucket: accessLogBucket, logFilePrefix})
  }
  return bucket
}

@RomainMuller RomainMuller assigned rix0rrr and eladb and unassigned rix0rrr Aug 26, 2019
@abhinavrungta
Copy link

In my current organization, we use Service Catalog to enforce company defaults and requirements around various aws services. We restrict end users IAM permissions, to be able to primarily operate only via Service Catalog.
While we can customize the CDK to enforce those defaults (based on the best practices), is there guidance on how to enforce usage of the customized CDK libs to build and deploy out apps into AWS cloud ?

@SomayaB SomayaB added @aws-cdk/core Related to core CDK functionality feature-request A feature should be added or improved. and removed needs-triage This issue or PR still needs to be triaged. labels Oct 1, 2019
@eladb eladb added the effort/medium Medium work item – several days of effort label Jan 23, 2020
@ywauto83
Copy link

Another pattern that an Enterprise needs is an easy way to scale out the same resource. For example, provision multiple s3 buckets, provision n x ec2 instances. There should be a construct to be able to scale out/in resources.

@eladb
Copy link
Contributor

eladb commented Apr 6, 2020

Duplicate aws/aws-cdk-rfcs#25

@eladb eladb closed this as completed Apr 6, 2020
@JohGiz
Copy link

JohGiz commented Mar 15, 2021

It has been a while now since your posted this @lapair
I am in a position trying to solve the exact same thing and found this page where you have the exact description of my problem I'm looking to best solve.

How have things been working out for the last two years since you posted this @lapair ? Still using the way you described above? What is your learnings?

@deuscapturus
Copy link

IMHO I prefer the inheritance approach. It requires less maintenance and allows the most freedom. We also have a standard Aspect application that enforces all organization standards so users aren't constrained to just constructs that we have written.

import { Construct } from 'constructs';
import { ApplicationLoadBalancedFargateService, ApplicationLoadBalancedFargateServiceProps } from 'aws-cdk-lib/aws-ecs-patterns';
import { HostedZone } from 'aws-cdk-lib/aws-route53';
import { Certificate } from 'aws-cdk-lib/aws-certificatemanager';
import { StringParameter } from 'aws-cdk-lib/aws-ssm';

export interface MyOrgInternalApplicationLoadBalancedFargateServiceProps extends Omit<
ApplicationLoadBalancedFargateServiceProps,
"assignPublicIp" | "domainZone" | "domainName" | "certificate" | "publicLoadBalancer" | "redirectHTTP" | "protocol" >{

    /**
     * @param companyDomain
     */
    readonly companyDomain: string;

    /**
     * @param name Name of this application.
     */
    readonly name: string;
}

/**
 * A extension of aws-ecs-patterns.ApplicationLoadBalancedFargateService
 *
 * Will create a load balanced ecs service on the company internal domain with tls certificate and internal dns record.
 */
export class MyOrgInternalApplicationLoadBalancedFargateService extends ApplicationLoadBalancedFargateService {

    constructor(
        scope: Construct,
        id: string,
        props: MyOrgInternalApplicationLoadBalancedFargateServiceProps
    ) {
        
        const domainName = StringParameter.valueFromLookup(scope, "/standard/hosted_zone_internal_subdomain_name")

        super(
            scope,
            id,
            {
                ...props,
                domainZone: HostedZone.fromLookup(scope, "HostedZone", {
                    domainName,
                    privateZone: true
                }),  
                domainName: `${props.name}.${props.companyDomain}.${domainName}`,
                certificate: Certificate.fromCertificateArn(scope, "InternalCert", StringParameter.valueFromLookup(scope, "/standard/certificate_internal_subdomain_wildcard_arn")),
                publicLoadBalancer: false,
                redirectHTTP: true,
            }
        );

    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality effort/medium Medium work item – several days of effort feature-request A feature should be added or improved.
Projects
None yet
Development

No branches or pull requests

10 participants