feat(elbv2): full Action support by rix0rrr · Pull Request #7741 · aws/aws-cdk

rix0rrr · 2020-05-01T15:17:32Z

Commit Message

feat(elbv2): full Action support

Add support for more complex Action setups. Adds authentication
using OIDC or Cognito, and proper support for fixed responses,
redirects, and weighted TargetGroup forwarding and stickiness.

Fixes #2563, fixes #6310, fixes #6308.

End Commit Message

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

Add support for more complex Action setups. Adds authentication using OIDC or Cognito, and proper support for fixed responses, redirects, and weighted TargetGroup forwarding and stickiness. Fixes #2563, fixes #6310, fixes #6308.

aws-cdk-automation · 2020-05-01T15:34:13Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 5eb4f09
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-04T13:35:23Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 57a7531
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

eladb · 2020-05-04T10:38:49Z

@@ -0,0 +1,27 @@
+-----BEGIN RSA PRIVATE KEY-----


Put these files under a fixtures directory perhaps?

Uh whoops. This shouldn't have been committed.

eladb · 2020-05-04T10:49:46Z

+  /**
+   * Default action to take for requests to this listener
+   *
+   * This allows full control of the default Action of the load balancer,


Suggested change

* This allows full control of the default Action of the load balancer,

* This allows full control of the default action of the load balancer,

Why is this called "default action"? Is there a non-default one? Why not just action

The default action is the one that doesn't have any conditions.

eladb · 2020-05-04T13:39:53Z

+  /**
+   * Forward to one or more Target Groups which are weighted differently
+   */
+  public static weightedForward(options: WeightedForwardOptions): ApplicationListenerAction {


Why isn't this just an option to forward?

eladb · 2020-05-04T13:40:47Z

+   *
+   * Must be a 2xx, 4xx or 5xx response code.
+   */
+  readonly statusCode: number;


I think this should be a positional argument (e.g. fixedResponse(200) and the rest as options)

eladb · 2020-05-04T13:41:19Z

+  /**
+   * The list of target groups to forward to
+   */
+  readonly targetGroups: IApplicationTargetGroup[];


I think this should be a positional argument

Yep, that actually makes sense.

eladb · 2020-05-04T13:41:37Z

+ *
+ * @experimental
+ */
+export interface WeightedForwardOptions {


Why is this a different type from ForwardOptions?

One just takes a list of target groups (common case), the other one takes a list of structures of targetgroups and weights (more complex, only when you need it).

aws-cdk-automation · 2020-05-05T10:08:59Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b5c7da1
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-05T11:51:18Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 0479616
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

nija-at

Comments so far. To be continued...

nija-at · 2020-05-12T11:01:44Z

@@ -0,0 +1,2 @@
+const baseConfig = require('../../../tools/cdk-build-tools/config/eslintrc');


Heads up on incoming PRs - #7880 and #6869

nija-at · 2020-05-12T11:03:09Z

+
+Here's an example:
+
+[Example of using AuthenticateCognitoAction](test/integ.cognito.lit.ts)


Does this work on our documentation page?
Would be nice to put a small code snippet inline, along with the link to the full example.

I guess. That's a feature request to jsii at the moment.

But we can still add an inline example manually.

I've tried this example and it seems not to work for me while trying to use this with ApplicationLoadBalancedFargateService

nija-at · 2020-05-12T11:04:50Z

+  /**
+   * The Amazon Cognito user pool.
+   */
+  readonly userPool: cognito.IUserPool;


Both the userpool domain and userpool clients can get you the underlying user pool. Might need to add a getter in the cognito module.

It's probably not correct to pass domain and clients of a different user pool from this, so omitting this might reduce human error.

I am wondering whether a better solution is to make the client optional and automatically generate it if not supplied, as its configuration is quite finicky.

But for now, I just want to get this out there and start collecting feedback and bug reports.

Also, IUserPool definitely does not have a method to get all clients (could it even?), nor does an IUserPoolClient have a way to get the UserPool associated with it.

does an IUserPoolClient have a way to get the UserPool associated with it.

This is the first use case, so we'll need to add it.

I am wondering whether a better solution is to make the client optional and automatically generate it if not supplied, as its configuration is quite finicky.

I like it. Let's do this.

nija-at · 2020-05-12T11:19:57Z

+  to the Target Groups yourself (or access one of the other ELB routing features).
+
+Using `addAction()` gives you access to some of the features of an Elastic Load
+Balancer that the convenience methods don't:


Suggested change

Balancer that the convenience methods don't:

Balancer that the other two convenience methods don't:

nija-at · 2020-05-12T11:21:58Z

+ */
+export class ListenerAction implements IListenerAction {
+  /**
+   * Authenticate using an identity provide (IdP) that is compliant with OpenID Connect (OIDC)


Suggested change

* Authenticate using an identity provide (IdP) that is compliant with OpenID Connect (OIDC)

* Authenticate using an identity provider (IdP) that is compliant with OpenID Connect (OIDC)

…to-action.ts Co-authored-by: Niranjan Jayakar <nija@amazon.com>

aws-cdk-automation · 2020-05-14T09:42:34Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 1bbf524
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-14T09:47:19Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: fc1f76b
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

nija-at

I strongly suggest breaking similar PRs in the future into smaller chunks that can be reviewed.
It did feel like this could be broken up or at least an initial slimmed down PR that sets up the right patterns, followed on by ones that can build this up.

nija-at · 2020-05-14T10:08:09Z

+ * You can reuse URI components using the following reserved keywords:
+ *
+ * - `#{protocol}`
+ * - `#{host}`
+ * - `#{port}`
+ * - `#{path}` (the leading "/" is removed)
+ * - `#{query}`
+ *
+ * For example, you can change the path to "/new/#{path}", the hostname to
+ * "example.#{host}", or the query to "#{query}&value=xyz".
+ *
+ * @experimental
+ */
+export interface RedirectOptions {


Would be useful to provide a property called url and parses out these, besides having to define each of these explicitly.

nija-at · 2020-05-14T10:12:25Z

+    // I'd like to throw here but there might be existing programs that happen
+    // to work even though they followed an illegal call pattern. Just add a warning.
+    if (this.action) {
+      this.node.addWarning('An Action already existed on this ListenerRule and was replaced. Configure exactly one default Action.');
+    }


Not sure I follow why you're not throwing here.

Added more explanation.

nija-at · 2020-05-14T10:22:09Z

+    if (props.defaultAction && props.defaultTargetGroups) {
+      throw new Error('Specify at most one of \'defaultAction\' and \'defaultTargetGroups\'');
+    }


can't find tests. sorry, if they're somewhere and I just can't spot them. I blame the PR size 😉

nija-at · 2020-05-14T10:29:06Z

+  public bindToListener(scope: Construct, listener: IApplicationListener, associatingConstruct?: IConstruct) {
+    // Empty on purpose
+    Array.isArray(scope);
+    Array.isArray(listener);
+    Array.isArray(associatingConstruct);
+  }


Why not just call this bind() like we usually do in other places?

Also why not abstract?

https://github.com/aws/aws-cdk/search?q=%22bind%22&unscoped_q=%22bind%22

Because it is empty on purpose, for most of the actions.

aws-cdk-automation · 2020-05-14T13:40:59Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 585b5c8
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr · 2020-05-15T10:01:46Z

I know the build is broken, will fix it with the next round of change requests (or on approval)

aws-cdk-automation · 2020-05-18T13:44:59Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 3fec181
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-18T15:00:45Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 468b2e9
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-18T15:55:24Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: aa21a3a
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-05-18T17:01:15Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2020-05-18T17:53:01Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-05-18T17:55:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 35da020
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-18T18:42:15Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 34e397a
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-05-18T18:44:15Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2020-05-18T19:34:29Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-05-18T19:36:39Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 0d03153
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-05-18T20:22:42Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-05-18T20:24:10Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: c635d49
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-05-18T21:13:04Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-05-18T21:14:19Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 0e408f0
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-18T21:22:52Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: c7665dd
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-05-18T22:01:31Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

feat(elbv2): full Action support

5eb4f09

Add support for more complex Action setups. Adds authentication using OIDC or Cognito, and proper support for fixed responses, redirects, and weighted TargetGroup forwarding and stickiness. Fixes #2563, fixes #6310, fixes #6308.

rix0rrr requested a review from a team May 1, 2020 15:17

rix0rrr self-assigned this May 1, 2020

mergify Bot added the contribution/core This is a PR that came from AWS. label May 1, 2020

rix0rrr added 2 commits May 4, 2020 15:16

Merge remote-tracking branch 'origin/master' into huijbers/elbv2-actions

33a8655

Add new package to metapackages

57a7531

eladb reviewed May 4, 2020

View reviewed changes

rix0rrr added 2 commits May 5, 2020 11:36

Remove accidental certificate

976b18a

API tweaks

b5c7da1

rix0rrr mentioned this pull request May 5, 2020

[BROKEN] feat(elbv2): Supports new types of listener rule conditions #7726

Closed

Bump engine version in package.json

0479616

rix0rrr requested review from a team and eladb May 6, 2020 08:56

nija-at reviewed May 12, 2020

View reviewed changes

rix0rrr requested a review from a team May 14, 2020 09:09

rix0rrr and others added 2 commits May 14, 2020 11:13

Update packages/@aws-cdk/aws-elasticloadbalancingv2-actions/lib/cogni…

1bbf524

…to-action.ts Co-authored-by: Niranjan Jayakar <nija@amazon.com>

Comment updates

fc1f76b

nija-at reviewed May 14, 2020

View reviewed changes

rix0rrr added 2 commits May 14, 2020 15:28

Address review comments

075f60c

Merge remote-tracking branch 'origin/master' into huijbers/elbv2-actions

585b5c8

rix0rrr requested review from a team and nija-at May 14, 2020 13:32

Make actions stable

3fec181

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label May 18, 2020

rix0rrr added 2 commits May 18, 2020 16:47

Merge remote-tracking branch 'origin/master' into huijbers/elbv2-actions

09201ed

Fix tests

468b2e9

This is not great

aa21a3a

Merge branch 'master' into huijbers/elbv2-actions

35da020

Merge branch 'master' into huijbers/elbv2-actions

34e397a

Merge branch 'master' into huijbers/elbv2-actions

0d03153

Merge branch 'master' into huijbers/elbv2-actions

c635d49

Merge branch 'master' into huijbers/elbv2-actions

0e408f0

Merge branch 'master' into huijbers/elbv2-actions

c7665dd

mergify Bot merged commit 2939105 into master May 18, 2020

mergify Bot deleted the huijbers/elbv2-actions branch May 18, 2020 22:01

pahud mentioned this pull request Dec 1, 2025

aws-elasticloadbalancingv2: support NLB weighted target groups #36200

Closed

2 tasks

	* This allows full control of the default Action of the load balancer,
	* This allows full control of the default action of the load balancer,

		@@ -0,0 +1,2 @@
		const baseConfig = require('../../../tools/cdk-build-tools/config/eslintrc');


		Here's an example:

		[Example of using AuthenticateCognitoAction](test/integ.cognito.lit.ts)

	Balancer that the convenience methods don't:
	Balancer that the other two convenience methods don't:

	* Authenticate using an identity provide (IdP) that is compliant with OpenID Connect (OIDC)
	* Authenticate using an identity provider (IdP) that is compliant with OpenID Connect (OIDC)

Conversation

rix0rrr commented May 1, 2020

Commit Message

End Commit Message

Uh oh!

aws-cdk-automation commented May 1, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented May 4, 2020

AWS CodeBuild CI Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented May 5, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented May 5, 2020

AWS CodeBuild CI Report

Uh oh!

nija-at left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nija-at May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented May 14, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented May 14, 2020

AWS CodeBuild CI Report

Uh oh!

nija-at May 12, 2020 •

edited

Loading