fix(elasticloadbalancingv2): can not set sessionTimeout #24457

WinterYukky · 2023-03-05T12:25:49Z

Summary

Application LoadBalancer can not set sessionTimeout on authenticateOidc except in defaultActions.
This PR fixes this bug.

Cause of the bug

This is because the CDK uses the same structures for ListenerRule.AuthenticateOidcConfig and Listener.AuthenticateOidcConfig. These structures have almost the same structure, but validation fails during synthesize because the data type of sessionTimeout is different for String and Integer.

  AuthenticationRequestExtraParams: 
    Key : Value
  AuthorizationEndpoint: String
  ClientId: String
  ClientSecret: String
  Issuer: String
  OnUnauthenticatedRequest: String
  Scope: String
  SessionCookieName: String
- SessionTimeout: String
+ SessionTimeout: Integer
  TokenEndpoint: String
  UseExistingClientSecret: Boolean
  UserInfoEndpoint: String

How to fix?

Add addRuleAction() to register an Action for a ListenerRule so that it can hold both config for Listener and config for ListenerRule. Also, separate renderActions() into one for the Listener (defaultActions) and one for the ListenerRule (actions) and have them use their own configs.

This allows changes to be made without destroying existing published interfaces.

Closes #12843, #21768.

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

TheRealAmazonKendra · 2023-03-09T17:27:40Z

packages/@aws-cdk/aws-elasticloadbalancingv2-actions/.eslintrc.js

@@ -1,2 +1,3 @@
 const baseConfig = require('@aws-cdk/cdk-build-tools/config/eslintrc');
+baseConfig.parserOptions.project = __dirname + '/tsconfig.json';


Can you clarify why this change was made?

The reason behind making this change was that following the error message was displayed during linting.

$ yarn eslint packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-listener.ts yarn run v1.22.19 $ /workspaces/aws-cdk/node_modules/.bin/eslint packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-listener.ts ============= WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree. You may find that it works just fine, or you may not. SUPPORTED TYPESCRIPT VERSIONS: >=3.3.1 <4.5.0 YOUR TYPESCRIPT VERSION: 4.9.5 Please only submit bug reports when using the officially supported version. ============= /workspaces/aws-cdk/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-listener.ts 0:0 error Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser. The file does not match your project config: packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-listener.ts. The file must be included in at least one of the projects provided ✖ 1 problem (1 error, 0 warnings) error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Please focus on errors, not warnings. This issue fixed by #7880 . However, @aws-cdk/aws-elasticloadbalancingv2-actions is not included then, so I changed it.

Ah, OK. Thanks for letting me know. No problems with this change.

Thank you @TheRealAmazonKendra !
However, all .eslintrc.js has removed from individual packages by #24376 🫥. Naturally, I erased it from this PR.

aws-cdk-automation · 2023-04-01T00:08:52Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

…x/elbv2/can-not-set-session-timeout

Pull request has been modified.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

TheRealAmazonKendra · 2023-04-05T04:27:26Z

Will take another look at this tomorrow.

corymhall

I can't believe this is necessary (why cloudformation why), but nice solution!

mergify · 2023-04-19T18:04:29Z

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

gitpod-io · 2023-04-20T11:36:22Z

Pull request has been modified.

mergify · 2023-04-20T12:00:07Z

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

aws-cdk-automation · 2023-04-20T13:39:12Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 80f6264
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-04-20T13:39:36Z

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

WinterYukky · 2023-04-22T04:12:23Z

Thank you @corymhall for reviewing this PR 🥰

EysaN · 2023-05-11T16:04:40Z

could someone please clarify the implementation of the fix here because I am still seeing the same behavior in v2.79.0 (build e4a2c01) python 3.11.0

action = elbv2.ListenerAction.authenticate_oidc(
     issuer="dummy",
     authorization_endpoint="dummy",
     token_endpoint="dummy",
     user_info_endpoint="dummy",
     client_id="dummy",
     client_secret=SecretValue.secrets_manager("dummy"),
     next=elbv2.ListenerAction.forward([target_group]),
     session_cookie_name="dummy",
     session_timeout=Duration.seconds(14400),
     scope='dummy',
     on_unauthenticated_request=elbv2.UnauthenticatedAction.AUTHENTICATE
 )

WinterYukky · 2023-05-11T18:12:38Z

Thank you @EysaN for your reporting.

This fix makes it possible to apply configuration to different structures by retaining the respective information (numbers and strings) for the ListenerRule.authenticateOIDCConfig and Listener.authenticateOIDCConfig structures.

I've tried creating a simple Python (v3.11.3) example with v2.79.0 and it seems like I can Synth without a problem. Can you give me a minimal reproduction to reproduce?

from aws_cdk import (
    Duration,
    SecretValue,
    Stack,
    aws_ec2 as ec2,
    aws_elasticloadbalancingv2 as elbv2,
)
from constructs import Construct

class ExampleStack(Stack):

    def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)

        vpc = ec2.Vpc(self, "Vpc")
        lb = elbv2.ApplicationLoadBalancer(self, "Lb", 
            vpc=vpc,
        )
        action = elbv2.ListenerAction.authenticate_oidc(
            issuer="dummy",
            authorization_endpoint="dummy",
            token_endpoint="dummy",
            user_info_endpoint="dummy",
            client_id="dummy",
            client_secret=SecretValue.secrets_manager("dummy"),
            next=elbv2.ListenerAction.fixed_response(200,
                content_type="text/plain",
                message_body="Authenticated",
            ),
            session_cookie_name="dummy",
            session_timeout=Duration.seconds(14400),
            scope='dummy',
            on_unauthenticated_request=elbv2.UnauthenticatedAction.AUTHENTICATE
        )
        listener = lb.add_listener("Listener", 
            protocol=elbv2.ApplicationProtocol.HTTP,
            default_action=action,
        )
        listener.add_action("Action", 
            priority=1,
            conditions=[elbv2.ListenerCondition.path_patterns(["/page*"])],
            action=action,
        )

f1nl0wt3ch · 2023-05-17T07:38:38Z

Is that something with OIDC, the timeout will be set in String format? I am still facing with this issue with sdk version 2. Anyone solved this problem with typescript?!

const authAction = new elbv2_actions.AuthenticateCognitoAction({ userPool: userpool, next: elbv2.ListenerAction.forward([serviceTg]), userPoolClient: userPoolClient, sessionCookieName: 'dummy', sessionTimeout: Duration.hours(1), scope: 'openid', onUnauthenticatedRequest: elbv2.UnauthenticatedAction.DENY, userPoolDomain: userPoolDomain, })

WinterYukky · 2023-05-17T08:55:40Z

@f1nl0wt3ch
Yes, AWS CDK is written in TypeScript so it works TypeScript. What are you using the AWS CDK version? This feature was released since v.277.0

I could synth the following code.

import { Duration, SecretValue, Stack, StackProps } from "aws-cdk-lib";
import { Vpc } from "aws-cdk-lib/aws-ec2";
import {
  ApplicationLoadBalancer,
  ListenerAction,
  ListenerCondition,
} from "aws-cdk-lib/aws-elasticloadbalancingv2";
import { Construct } from "constructs";

export class ExampleStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    const vpc = new Vpc(this, "Vpc");
    const alb = new ApplicationLoadBalancer(this, "Alb", {
      vpc,
    });
    const listener = alb.addListener("Listener", {
      port: 80,
      defaultAction: ListenerAction.authenticateOidc({
        authorizationEndpoint: "Dummy",
        clientId: "Dummy",
        clientSecret: SecretValue.unsafePlainText("Dummy"),
        issuer: "Dummy",
        tokenEndpoint: "Dummy",
        userInfoEndpoint: "Dummy",
        sessionTimeout: Duration.days(1),
        next: ListenerAction.fixedResponse(200),
      }),
    });
    listener.addAction("AdditionalOidcAuthenticationAction", {
      priority: 1,
      conditions: [ListenerCondition.pathPatterns(["/page*"])],
      action: ListenerAction.authenticateOidc({
        authorizationEndpoint: "Dummy",
        clientId: "Dummy",
        clientSecret: SecretValue.unsafePlainText("Dummy"),
        issuer: "Dummy",
        tokenEndpoint: "Dummy",
        userInfoEndpoint: "Dummy",
        sessionTimeout: Duration.days(1),
        next: ListenerAction.fixedResponse(200),
      }),
    });
  }
}

When you using elbv2_actions.AuthenticateCognitoAction, it also were tested to can deploy on integ test.
https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/framework-integ/test/aws-elasticloadbalancingv2-actions/test/integ.cognito.ts#L141-L160

f1nl0wt3ch · 2023-05-17T09:37:02Z

Hi @WinterYukky !

Thank for your answer. I am using v.2.14.0. Let me try again.

fix(elbv2): can not set timeout

9e4ffcc

github-actions bot added the valued-contributor [Pilot] contributed between 6-12 PRs to the CDK label Mar 5, 2023

aws-cdk-automation requested a review from a team March 5, 2023 12:26

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Mar 5, 2023

WinterYukky changed the title ~~fix(elbv2): can not set sessionTimeout~~ fix(elasticloadbalancingv2): can not set sessionTimeout Mar 5, 2023

aws-cdk-automation previously requested changes Mar 5, 2023

View reviewed changes

fix(elasticloadbalancingv2-actions): can not set sessionTimeout

da595a4

fix(elbv2): can not set sessionTimeout

5bfbf78

WinterYukky force-pushed the fix/elbv2/can-not-set-session-timeout branch from 4d20a55 to 5bfbf78 Compare March 8, 2023 12:38

Merge branch 'main' into fix/elbv2/can-not-set-session-timeout

0119844

TheRealAmazonKendra previously requested changes Mar 28, 2023

View reviewed changes

WinterYukky added 3 commits April 1, 2023 04:25

Merge branch 'main' of https://github.com/WinterYukky/aws-cdk into fi…

8da684f

…x/elbv2/can-not-set-session-timeout

Merge branch 'main' of https://github.com/WinterYukky/aws-cdk into fi…

66f8c30

…x/elbv2/can-not-set-session-timeout

chore: move to aws-cdk-lib

e163a98

aws-cdk-automation previously requested changes Apr 1, 2023

View reviewed changes

WinterYukky added 2 commits April 1, 2023 15:48

chore: fix import path

d34475a

test: fix integ test

df24c64

corymhall previously approved these changes Apr 19, 2023

View reviewed changes

Merge branch 'main' into fix/elbv2/can-not-set-session-timeout

3684d97

corymhall self-assigned this Apr 20, 2023

corymhall approved these changes Apr 20, 2023

View reviewed changes

Merge branch 'main' into fix/elbv2/can-not-set-session-timeout

80f6264

mergify bot merged commit cefbb33 into aws:main Apr 20, 2023

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(elasticloadbalancingv2): can not set sessionTimeout #24457

fix(elasticloadbalancingv2): can not set sessionTimeout #24457

WinterYukky commented Mar 5, 2023 •

edited

Loading

aws-cdk-automation left a comment

TheRealAmazonKendra Mar 9, 2023

WinterYukky Mar 29, 2023 •

edited

Loading

TheRealAmazonKendra Apr 5, 2023

WinterYukky Apr 5, 2023

aws-cdk-automation commented Apr 1, 2023

aws-cdk-automation left a comment

TheRealAmazonKendra commented Apr 5, 2023

corymhall left a comment

mergify bot commented Apr 19, 2023

gitpod-io bot commented Apr 20, 2023

mergify bot commented Apr 20, 2023

aws-cdk-automation commented Apr 20, 2023

mergify bot commented Apr 20, 2023

WinterYukky commented Apr 22, 2023

EysaN commented May 11, 2023

WinterYukky commented May 11, 2023 •

edited

Loading

f1nl0wt3ch commented May 17, 2023

WinterYukky commented May 17, 2023

f1nl0wt3ch commented May 17, 2023 •

edited

Loading

		@@ -1,2 +1,3 @@
		const baseConfig = require('@aws-cdk/cdk-build-tools/config/eslintrc');
		baseConfig.parserOptions.project = __dirname + '/tsconfig.json';

fix(elasticloadbalancingv2): can not set sessionTimeout #24457

fix(elasticloadbalancingv2): can not set sessionTimeout #24457

Conversation

WinterYukky commented Mar 5, 2023 • edited Loading

Summary

Cause of the bug

How to fix?

aws-cdk-automation left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Mar 9, 2023

Choose a reason for hiding this comment

WinterYukky Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

TheRealAmazonKendra Apr 5, 2023

Choose a reason for hiding this comment

WinterYukky Apr 5, 2023

Choose a reason for hiding this comment

aws-cdk-automation commented Apr 1, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra commented Apr 5, 2023

corymhall left a comment

Choose a reason for hiding this comment

mergify bot commented Apr 19, 2023

gitpod-io bot commented Apr 20, 2023

mergify bot commented Apr 20, 2023

aws-cdk-automation commented Apr 20, 2023

AWS CodeBuild CI Report

mergify bot commented Apr 20, 2023

WinterYukky commented Apr 22, 2023

EysaN commented May 11, 2023

WinterYukky commented May 11, 2023 • edited Loading

f1nl0wt3ch commented May 17, 2023

WinterYukky commented May 17, 2023

f1nl0wt3ch commented May 17, 2023 • edited Loading

WinterYukky commented Mar 5, 2023 •

edited

Loading

WinterYukky Mar 29, 2023 •

edited

Loading

WinterYukky commented May 11, 2023 •

edited

Loading

f1nl0wt3ch commented May 17, 2023 •

edited

Loading