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

(aws_elasticloadbalancingv2): authenticate_oidc SessionTimeout requires different type if used in Listener or ListenerRule #21768

Closed
peterfranzen opened this issue Aug 25, 2022 · 8 comments
Assignees
Labels
@aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@peterfranzen
Copy link

peterfranzen commented Aug 25, 2022

Describe the bug

I'm creating an elbv2 ListenerAction with CDK v2.38.1 as follows:

action=elbv2.ListenerAction.authenticate_oidc(
                    authorization_endpoint="https://example.com/",
                    client_id= client_id,
                    client_secret=client_secret,
                    issuer="https://my.issuer",
                    token_endpoint="https://my/token",
                    user_info_endpoint="https://my/userinfo",
                    session_timeout=Duration.minutes(60),
                    next=elbv2.ListenerAction.redirect(...)

No matter what I put in for the session_timeout parameter it gives an error. If I use an int or a string then it asks for a Duration, but if I use a Duration I get an error saying it "should be a number".

Expected Behavior

Allow me to use a Duration (e.g. Duration.minutes(60)) object for session_timeout.

Current Behavior

When I use a Duration object (e.g. Duration.minutes(60)) I get the following error:

authenticateOidcConfig: supplied properties not correct for "AuthenticateOidcConfigProperty" sessionTimeout: "3600" should be a number.

Reproduction Steps

Use a Duration object in the sesttion_timeout parameter field in a elbv2.ListenerAction.authenticate_oidc.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.38.1

Framework Version

No response

Node.js Version

n/a

OS

MacOS

Language

Python

Language Version

No response

Other information

No response

@peterfranzen peterfranzen added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 25, 2022
@github-actions github-actions bot added the @aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 label Aug 25, 2022
@peterwoodworth
Copy link
Contributor

Hey @peterfranzen, I'm unable to reproduce this with Python on the same version. Can you try reinstalling your packages, or are you able to reproduce this on a fresh project? Thanks.

Here's my code which synthesizes

        elbv2.ListenerAction.authenticate_oidc(
          authorization_endpoint='asdfasdf',
          client_id='asfasdf',
          client_secret=secrets.Secret(self, 'Secret').secret_value,
          issuer='asdfas',
          token_endpoint='asdgseg',
          user_info_endpoint='fsdgaeg',
          next=elbv2.ListenerAction.forward([group]),
          session_timeout=Duration.minutes(60)
        )

@peterwoodworth peterwoodworth added needs-reproduction This issue needs reproduction. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Aug 25, 2022
@peterfranzen
Copy link
Author

peterfranzen commented Aug 25, 2022

Thanks, I isolated this a bit more and I have a little more context. When I create just that construct alone it does work with no errors.

However, I'm creating a number of elbv2.ApplicationListenerRules in a for loop. When I exclude the session_timeout variable the loop works and each of my ListenerRules synth just fine, but when I do include the Duration object then I get the error. The following does not work:

for rule in ruleList:
    AppRedirectRule = elbv2.ApplicationListenerRule(self, rule['rulename'],
                ...
                action=elbv2.ListenerAction.authenticate_oidc(
                    authorization_endpoint='asdfasdf',
                    client_id='asfasdf',
                    client_secret=secrets.Secret(self, 'Secret').secret_value,
                    issuer='asdfas',
                    token_endpoint='asdgseg',
                    user_info_endpoint='fsdgaeg',
                    next=elbv2.ListenerAction.forward([group]),
                    session_timeout=Duration.minutes(60)
            )

Am I approaching creating this wrong? Is there something about Duration that doesn't allow it to be used within a loop? The Duration error comes up even if the loop only runs once.

@peterwoodworth
Copy link
Contributor

Ok I think I figured out what's going wrong

We take in the Duration passed by the user, convert it to seconds, and pass it as a string here

sessionTimeout: options.sessionTimeout?.toSeconds().toString(),

CloudFormation specifies that Listener.AuthenticateOidcConfig takes in a string for SessionTimeout

"SessionTimeout": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-elasticloadbalancingv2-listener-authenticateoidcconfig.html#cfn-elasticloadbalancingv2-listener-authenticateoidcconfig-sessiontimeout",
"PrimitiveType": "String",
"Required": false,
"UpdateType": "Mutable"
},

However CloudFormation specifies that ListenerRule.AuthenticateOidcConfig takes in a number

"SessionTimeout": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-elasticloadbalancingv2-listenerrule-authenticateoidcconfig.html#cfn-elasticloadbalancingv2-listenerrule-authenticateoidcconfig-sessiontimeout",
"PrimitiveType": "Integer",
"Required": false,
"UpdateType": "Mutable"
},

We are trying to pass in this duration as a string into ListenerRule.AuthenticateOidcConfig, which will break because it is expecting a number. We need to ensure that this is generated as a number if used in a ListenerRule

@peterwoodworth peterwoodworth added p2 effort/small Small work item – less than a day of effort and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. needs-reproduction This issue needs reproduction. labels Aug 25, 2022
@peterwoodworth peterwoodworth changed the title (aws_elasticloadbalancingv2): authenticate_oidc SessionTimeout not accepting Duration object (aws_elasticloadbalancingv2): authenticate_oidc SessionTimeout requires different type if used in Listener or ListenerRule Aug 25, 2022
@blamarao
Copy link

Same issue as here -> #12843 ??

@mwebber
Copy link

mwebber commented Oct 11, 2022

@blamarao Yes, this looks identical to #12843, which was reported against CDK v1.
It is still present in CDK 2.43.1

mergify bot pushed a commit that referenced this issue Apr 20, 2023
## 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](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-elasticloadbalancingv2-listenerrule-authenticateoidcconfig.html) and [Listener.AuthenticateOidcConfig](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-elasticloadbalancingv2-listener-authenticateoidcconfig.html). These structures have almost the same structure, but validation fails during synthesize because the data type of `sessionTimeout` is different for String and Integer.

```diff
  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*
@WinterYukky
Copy link
Contributor

@peterwoodworth
This issue is fixed at v2.77.0. Could you close this issue?

@peterwoodworth
Copy link
Contributor

thanks!

@github-actions
Copy link

github-actions bot commented May 1, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

No branches or pull requests

6 participants