feat: add cognito idp example with facebook integration #298

ericzbeard · 2020-06-04T20:08:49Z

This example uses the new UserPoolIdentityProvider to create a REST API that controls access via Cognito and the Facebook IDP. It also has L3 constructs for the creation of a static site and for configuring Lambda API handlers.

See aws/aws-cdk#8134

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

nija-at

Some initial comments about the setup before I go deeper into the implementation.

nija-at · 2020-06-08T11:37:09Z

scripts/build-npm.sh

+        # cdk synth breaks in examples that rely on environment variables
+        if [[ -f DO_NOT_AUTOSYNTH ]]; then exit 0; fi
+


Can we avoid? Synthesizing during the build is the validation that none of the code here has broken due to changes in the main CDK library.

This is especially true/useful for ones that use experimental APIs (viz Cognito).

What is stopping us from making this a synthesizeable stack?

I use a number of environment variables, and it's important for the stack to fail if any of them are not set, to avoid later errors that are harder to troubleshoot. It might be possible to replace some of them with parameter store, but this complicates setup, and I'm not sure if it's practical to get rid of all of them. I will look into that today.

typescript/cognito-idp/README.md

nija-at · 2020-06-08T11:48:48Z

typescript/cognito-idp/bin/cognito-idp.ts

+const region = util.getEnv('AWS_REGION');
+const account = util.getEnv('AWS_ACCOUNT');
+
+const app = new cdk.App();
+const stack = new CognitoIdpStack(app, 'CognitoIdpStack', {
+    env: {
+        account,
+        region
+    }


Can we leave out region and account as optional and let the defaults do their magic?

I thought this was best practice? I seem to remember an error or warning at some point for not setting these.

An error will occur if CDK can’t produce an environment agnostic template. Personally I leave these out as a way of letting me know when I’ve crossed that threshold.

nija-at · 2020-06-08T11:51:33Z

typescript/cognito-idp/README.md

+Put the following into a .env file in the root folder for this example, replacing each placeholder value with your specific environment values.
+
+```
+AWS_REGION=us-east-1
+AWS_ACCOUNT=123456789123
+WEB_DOMAIN=myapp.example.com
+API_DOMAIN=myapi.example.com
+WEB_CERTIFICATE_ARN=
+API_CERTIFICATE_ARN=
+FACEBOOK_APP_ID=
+FACBOOK_VERSION=v7.0
+FACEBOOK_SECRET_ARN=
+COGNITO_REDIRECT_URI=
+COGNITO_DOMAIN_PREFIX=
+COGNITO_REGION=
+```


Since you've modeled this whole set up as a Stack, can we make these options part of the stack's Props argument instead? This will make this in line with the intended use of CDK constructs and stacks.

Yes I can do that, but maybe I should wait until we decide the best way to handle environment variables.

Would these be best located in the CDK context variable file? Instead of external environment variables?

maybe I should wait until we decide the best way to handle environment variables.

What is the pending decision you're referring to?

I suppose you're referring to the comment here - #298 (comment)

How would that work on a team, though? Each developer would need their own values for each of those. And then dev, beta, gamma, prod environments would all need their own values.

Our stance in general is that “degrees of freedom” should generally be expressed through code APIs and fully resolved during synthesis so that synthesis is “pure” and remains deterministic. The invariant is that given a commit in the repo, the resulting cloud assembly will always be exactly the same for any call of “synth”. Since CDK apps are executed during build, this is a similar invariant that compilers have and we have assumptions in our framework that rely on this, and generally a best practice from an operational standpoint. That’s also the reason we discourage querying external APIs during synthesis.

We should also consider the fact that this is an “official” example. As such it should not encourage anti patterns.

Having said all that, the development use case is slightly different, because as @ericzbeard noted, each developer would have their own environment and those values will be different for each developer, in which case I do think that environment variables are a viable approach, but there are numerous other solutions that can be used and perhaps will be easier to work with.

For example, the code can read those values from a simple json file. By default it can read the “production” file from the project directory and an environment variable can be used to point it to a different file, for each developer.

Here is what I propose:

Define a MyStackProps interface with all these attributes and read its contents from a json file. Commit a file to source control with dummy “production” values (normally these would be the actual production values but since this is a sample, we will just put some dummy values).

Allow the developer to override the file’s location through an environment variable.

Sketch:

const configFile = process.env.CONFIG_FILE ?? “./config.json”; new MyStack(app, “stack”, require(configFile));

What do you guys think? We know we need to publish more guidance on this topic and we even have an rfc tracking item for that, but never got around to it. @ericzbeard help!

This change won't be too hard to implement and I suppose it's more idiomatic for CDK, but I'm not sure if it's what developers will expect. A .env file is an extremely common and well-understood pattern.

I'm also not sure about checking in a "production" config file. I have always seen checking in config values for any environment as an anti-pattern. Especially if eventually the checked in config file has actual production values and not placeholders - that could potentially cause outages if developers have privileges to the production system.

I often create a sample config file for reference and check that in, or simply document it in a README. I suppose if we want the project to build and synth with no additional action from a user (and for the purposes of an automated CI server) we don't have a choice, but I worry about a user cloning the project, building, and attempting to deploy it without creating their own config file. That will definitely fail and might be confusing. As it is, synth fails with a clear message that you are missing a necessary environment variable. How do we solve that problem?

Once we figure this out, it should definitely be built into the CLI somehow, probably using context variables. We already have cdk.json but that gets checked in. Maybe we should have a cdk.local.json that gets picked up automatically if present and is added to the .gitignore file when creating a new app?

For the time being, I committed a change to move all environment variable loading to the file in bin/ where we create the app. The stack no longer has any direct references to .env, and uses props that are passed in. This still leaves several open questions from above.

nija-at · 2020-06-08T11:55:07Z

typescript/cognito-idp/yolo.sh

@@ -0,0 +1,2 @@
+#!/usr/bin/env bash


What is the purpose of this file?

Just a quick way for me to build and deploy during development.

nija-at · 2020-06-08T11:56:17Z

typescript/cognito-idp/package.json

+    "cognito-test": "jest --group=cognito",
+    "api-test": "jest --group=api",
+    "handler-test": "jest --group=handler",
+    "deploy": "cdk deploy --require-approval never",


Would be better to drop this script and ask the user to run cdk deploy instead.

I find that to be super annoying during development. Can't tell you how many times I have run cdk deploy and then took a break, and then I come back 15 minutes later and see the confirmation prompt waiting for me.

nija-at · 2020-06-08T11:56:25Z

typescript/cognito-idp/package.json

+    "api-test": "jest --group=api",
+    "handler-test": "jest --group=handler",
+    "deploy": "cdk deploy --require-approval never",
+    "link-cdk": "../../../aws-cdk-fork/link-all.sh"


Did you mean drop all 4 lines? Or just one of them? I removed link-cdk.

typescript/cognito-idp/clean.js

Co-authored-by: Niranjan Jayakar <nija@amazon.com>

Pull request has been modified.

eladb

Initial comments. Haven’t reviewed deeply yet

eladb · 2020-06-09T06:25:32Z

typescript/cognito-idp/functions/create-config.ts

+            let contents = '';
+
+            if (requestType === 'Create' || requestType === 'Update') {
+                const configFileName = 'config.js';


Indentation should be 2 spaces.

eladb · 2020-06-09T06:25:57Z

typescript/cognito-idp/functions/database.ts

+    /**
+     * Convert a DynamoDB user to a User object.
+     */
+    userConvert(item: any): User {


Run tslint/eslint

eladb · 2020-06-10T06:14:04Z

typescript/cognito-idp/copy-dist.ts

+fs.ensureDirSync('functions/node_modules');
+
+fs.emptyDirSync('dist');
+


I would recommend moving all build/dev related scripts/tools to a “tools” or “scripts” directory so they won’t mix up with the actual app.

eladb · 2020-06-10T06:15:32Z

typescript/cognito-idp/functions/user-delete.ts

+import { APIGatewayEvent } from 'aws-lambda';
+import { Handler } from './handler';
+import * as AWS from 'aws-sdk';
+import { APIEventResponse } from './handler';


Rename the functions directory to lambda (that has been our convention so far, see cdkworkshop.com as an example)

Addressed these comments in my latest commit.

Pull request has been modified.

ericzbeard · 2020-06-22T22:51:21Z

Now that 1.46 is released this PR is building, but I'm still not happy with how I'm handling environment variables. I was not able to use a placeholder "prod" stack since context lookups need a real account with credentials. I ended up removing the context file, since it had configuration values from my local development environment, which I didn't want to commit. And I'm basically omitting synth on the build server, which is probably not a good practice. @nija-at @eladb any suggestions?

How do we handle a scenario where a developer who does not have access to prod, etc, wants to add resources that require a context lookup? And how do we handle an example like this where it's not possible to build/synth in a meaningful way without local environment variables?

Still learning!

nija-at · 2020-06-23T13:43:19Z

How do we handle a scenario where a developer who does not have access to prod, etc, wants to add resources that require a context lookup?

Can you point me to where you're doing this in your code so I can better understand what you're looking to do?

how do we handle an example like this where it's not possible to build/synth in a meaningful way without local environment variables?

I suppose the following lines are what you're referring to here.

I suppose if we want the project to build and synth with no additional action from a user (and for the purposes of an automated CI server) we don't have a choice, but I worry about a user cloning the project, building, and attempting to deploy it without creating their own config file. That will definitely fail and might be confusing. As it is, synth fails with a clear message that you are missing a necessary environment variable. How do we solve that problem?

This isn't a problem we should be solving here. The examples repo is simply providing a set of "examples" for users to get inspiration from. Any code that is copy-pasted/cloned should be done with understanding that this is not production code and will need modification and line-by-line review.
The example repo is not vending a set of CDK templates.

ericzbeard · 2020-06-23T14:32:44Z

@nija-at See bin/congito-idp.ts

ericzbeard · 2020-06-23T23:48:30Z

In my latest commit I think I have something that's satisfactory. I changed how I was referencing the hosted zone so I don't actually need a context lookup, so the prod stack with fake properties synthesizes Ok. I have a better understanding of context lookups now and why we need them, but it seems best to avoid them if possible. @eladb @nija-at

eladb · 2020-06-24T06:20:48Z

In my latest commit I think I have something that's satisfactory. I changed how I was referencing the hosted zone so I don't actually need a context lookup, so the prod stack with fake properties synthesizes Ok. I have a better understanding of context lookups now and why we need them, but it seems best to avoid them if possible. @eladb @nija-at

I agree that if those can be avoided, the better.

nija-at · 2020-06-25T13:57:51Z

@ericzbeard - I see you still have some code for skipping synth. Re-request a review when you've cleaned this up and is ready for another round.

ericzbeard · 2020-06-25T15:42:00Z

@nija-at Done.

nija-at

As I go through this PR, I feel more strongly now that this should not be part of the aws-cdk-examples repo. We should move this into a separate repository under aws-samples. We can of course add links to the repo from other locations.

Apps under the CDK examples repos showcase non-trivial use cases of the CDK or integrations across AWS services. It has insofar not carried a full blown app.
This code here not just defines infra via CDK but also application logic, opinionated ways to set up the application, projects and organize lambda functions.

This will be probably the only CDK application is this repository of its kind. I don't expect more of these to land here. This variation is likely going to be harder to maintain (if it breaks or needs to fixed up) and keep consistent with the rest of the examples as we evolve this repository.

Finally, this is a massive PR! 70 files, with each file being ~100 lines, that's a ballpark of 7000 LOC which becomes impossible to effectively do any kind of review (I've spent an hour total on it so far and don't feel like I'm able to effectively continue). Separating this into its own repo significantly lowers the cost of reviewing such a repo and trust that it works.

@eladb - what do you think of this? How hard is it to create a new repo in this GH org?

eladb · 2020-06-28T07:37:30Z

I tend to agree that this feels like it deserves it's own repo. It will be much easier to review/maintain this way.

what do you think of this? How hard is it to create a new repo in this GH org?

Very easy. We have self-service resources for that internally.

richardhboyd · 2020-06-28T12:30:04Z

might also be a good time to document this decision in something like this #177

ericzbeard · 2020-08-04T21:02:47Z

Moved to https://github.com/aws-samples/cdk-cognito-idp

ericzbeard added 16 commits May 24, 2020 10:10

Initial commit of Cognito IDP Example

68f0e3a

Comment out Facebook IDP for now

614f431

Builds and deploys

4915313

Fix .env in Readme

16bb1cd

Replace FB direct integration with Cognito

d3b797f

L2 IDP works

b28b0c3

Refactor into constructs, custom resource for config.js

fa79c89

Test REST API after login

8a6c792

Upgrade CDK version, build against master

c8a9908

Merge remote-tracking branch 'upstream/master'

4fa1153

Fix cdk package versions

ec4bb4b

Add lambda dependencies to top level package for GitHub docker build

c89a6c7

Add dev dependency for jsonwebtoken

9bf093f

Ensure lambda node_modules dir for automated build

f903c1b

npm test unit tests only

40b0c4f

Do not autosynth during build test if there are environment variables

42ab108

SomayaB assigned nija-at Jun 5, 2020

nija-at assigned ericzbeard and unassigned ericzbeard and nija-at Jun 8, 2020

nija-at self-requested a review June 8, 2020 11:30

nija-at added the contribution/core This is a PR that came from AWS. label Jun 8, 2020

nija-at previously requested changes Jun 8, 2020

View reviewed changes

Update typescript/cognito-idp/README.md

4082447

Co-authored-by: Niranjan Jayakar <nija@amazon.com>

ericzbeard added 2 commits June 8, 2020 11:59

Remove link script

690e304

Move env var loading and incorporate latest CDK release

b0313a5

eladb previously requested changes Jun 10, 2020

View reviewed changes

Changed how we load env vars and modified tslint

1cc9375

ericzbeard added 6 commits June 22, 2020 07:50

Merge branch 'master' into master

f5e0fa6

Fix build path

f4d8a83

Merge branch 'master' of github.com:ericzbeard/aws-cdk-examples

1ca4590

Remove context file

6504558

Ignore context file

865c3af

Placeholder

7309eaa

Readme env and testing

4d3424d

Modify hosted zone so that we don't need a context lookup

21d18ab

Remove no autosynth from build

1d275ce

nija-at reviewed Jun 26, 2020

View reviewed changes

ericzbeard closed this Aug 4, 2020

		# cdk synth breaks in examples that rely on environment variables
		if [[ -f DO_NOT_AUTOSYNTH ]]; then exit 0; fi

		fs.ensureDirSync('functions/node_modules');

		fs.emptyDirSync('dist');

feat: add cognito idp example with facebook integration #298

feat: add cognito idp example with facebook integration #298

Conversation

ericzbeard commented Jun 4, 2020

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericzbeard commented Jun 22, 2020

nija-at commented Jun 23, 2020 • edited Loading

ericzbeard commented Jun 23, 2020

ericzbeard commented Jun 23, 2020

eladb commented Jun 24, 2020

nija-at commented Jun 25, 2020

ericzbeard commented Jun 25, 2020

nija-at left a comment

Choose a reason for hiding this comment

eladb commented Jun 28, 2020

richardhboyd commented Jun 28, 2020

ericzbeard commented Aug 4, 2020

nija-at commented Jun 23, 2020 •

edited

Loading