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

(ALB + Cognito): Missing connection between SecurityGroup of ALB and SecurityGroup of service. #12994

Closed
JPLemelin opened this issue Feb 11, 2021 · 6 comments · Fixed by #21939
Assignees
Labels
bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@JPLemelin
Copy link

I'm using ALB and ECS Service (Fargate in my case) splitted in different stack at TargetGroup level (like this)

When i'm using action of Forward in listner everything is fine 👌

let action = elbv2.ListenerAction.forward([targetgroup])
listner.addAction(`${serviceName}-listner-action`, {
  priority: serviceListnerPriority,
  hostHeader: 'sample.exemple.com',
  action: action
})

When i'm using action of AuthenticateCognitoAction before the Forward action
(I'm using cognito auth in from of alb, similar to this)

let action = elbv2.ListenerAction.forward([targetgroup])

action = new elbv2Actions.AuthenticateCognitoAction({
  userPool: userPool,
  userPoolClient: userPoolClientAlb,
  userPoolDomain: userPoolDomain,
  next: action,
})

listner.addAction(`${serviceName}-listner-action`, {
  priority: serviceListnerPriority,
  hostHeader: 'sample.exemple.com',
  action: action
})

and in my service stack the service.attachToApplicationTargetGroup(props.albTargetGroup) will not generate the AWS::EC2::SecurityGroupIngress to accept connection from ALB to the service.

Reproduction Steps

What did you expect to happen?

In the service stack, I expect a resource type AWS::EC2::SecurityGroupIngress who will refer to the ALB SecurityGroup

What actually happened?

In the service stack, the resource AWS::EC2::SecurityGroupIngress is missing !

Environment

  • CDK CLI Version : 1.85.0 (build 5f44668)
  • Framework Version:
  • Node.js Version: 12.20.1
  • OS : MacOSX
  • Language (Version): TypeScript (3.9.7)

Other

Currently i'm bypassing the problem by exporting albSG from ALB Stack and manually adding connection between them

service.connections.allowFrom(props.albSG, ec2.Port.tcp(80), 'Allow connection from ALB')

PS: If it's necessary, I can also build a full CDK project with this bug.


This is 🐛 Bug Report

@JPLemelin JPLemelin added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 11, 2021
@njlynch
Copy link
Contributor

njlynch commented Mar 19, 2021

@JPLemelin - Thanks for the bug report, and for the detailed information.

Nothing is immediately jumping out at me here. If possible, I'd love to take you up on your offer to build a quick reproducible project with this bug. It would help a lot on being able to quickly hone in on what's going wrong here.

@njlynch njlynch added effort/medium Medium work item – several days of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Mar 19, 2021
@JPLemelin
Copy link
Author

@njlynch I added a repo here https://github.com/JPLemelin/aws-cdk-issue-12994

I also add output folder (cdk.out) of synth to compare difference between stack:

  • service-sample-without-cognito-prod who are only using forward action, stack file: cdk.out/service-sample-without-cognito-prod.template.json
  • service-sample-prod who are using AuthenticateCognitoAction then forward action, stack file: cdk.out/service-sample-prod.template.json
  • service-sample-patched-prod same as previous, but with patched SecurityGroup, stack file: cdk.out/service-sample-patched-prod.template.json

The main diff betweem service-sample-prod <=> service-sample-without-cognito-prod are:

  • AWS::EC2::SecurityGroupIngress
  • AWS::EC2::SecurityGroupEgress
+    "serviceSecurityGroupfrombaseInfraprodalbsg2960D4B48057A0CF64": {
+      "Type": "AWS::EC2::SecurityGroupIngress",
+      "Properties": {
+        "IpProtocol": "tcp",
+        "Description": "Load balancer to target",
+        "FromPort": 80,
+        "GroupId": {
+          "Fn::GetAtt": [
+            "serviceSecurityGroupF051F0EB",
+            "GroupId"
+          ]
+        },
+        "SourceSecurityGroupId": {
+          "Fn::ImportValue": "baseInfra-prod:ExportsOutputFnGetAttalbsg40B076C4GroupId00309A0E"
+        },
+        "ToPort": 80
+      },
+      "Metadata": {
+        "aws:cdk:path": "service-sample-without-cognito-prod/service/SecurityGroup/from baseInfraprodalbsg2960D4B4:80"
+      }
+    },
+    "serviceSecurityGroupbaseInfraprodalbsg2960D4B480from1B1F44EE": {
+      "Type": "AWS::EC2::SecurityGroupEgress",
+      "Properties": {
+        "GroupId": {
+          "Fn::ImportValue": "baseInfra-prod:ExportsOutputFnGetAttalbsg40B076C4GroupId00309A0E"
+        },
+        "IpProtocol": "tcp",
+        "Description": "Load balancer to target",
+        "DestinationSecurityGroupId": {
+          "Fn::GetAtt": [
+            "serviceSecurityGroupF051F0EB",
+            "GroupId"
+          ]
+        },
+        "FromPort": 80,
+        "ToPort": 80
+      },
+      "Metadata": {
+        "aws:cdk:path": "service-sample-without-cognito-prod/service/SecurityGroup/baseInfraprodalbsg2960D4B4:80 from"
+      }
+    },

@mmoulton
Copy link

I'm experiencing the same issue.

I have not been able to fully trace the issue yet, but I'm wondering @njlynch, do you know if the registerConnectable cascades appropriately since the ApplicationTargetGroup ends up being forwarded to as the next target of a AuthenticateCognitoAction. My first pass through the code seems like bind is never called on the next action anywhere in ListenerAction.

@github-actions
Copy link

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 17, 2022
@JPLemelin
Copy link
Author

JPLemelin commented Jun 17, 2022

I update the repo with CDK v2 https://github.com/JPLemelin/aws-cdk-issue-12994/tree/cdk-v2

The issue still present under CDK v2.28.1

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 17, 2022
@corymhall corymhall self-assigned this Aug 15, 2022
@mergify mergify bot closed this as completed in #21939 Sep 8, 2022
mergify bot pushed a commit that referenced this issue Sep 8, 2022
)

When you add an action to a listener the `bind` method is called, and one of the things that is typically done is to configure security group ingress. When you chain actions together, i.e.

```ts
listener.addAction('first-action', {
  action: ListenerAction.authenticateOidc({
    next: ListenerAction.forward([secondAction]),
    ...,
  }),
});
```

Bind is never called for the second action (i.e. `next`) which means the security group ingress rules are not created.

This PR updates the `ListenerAction.bind` method to call `bind` for any `next` action that is configured.

fixes #12994


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Sep 8, 2022

⚠️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.

Kruspe pushed a commit to DavidSchwarz2/aws-cdk that referenced this issue Sep 13, 2022
…#21939)

When you add an action to a listener the `bind` method is called, and one of the things that is typically done is to configure security group ingress. When you chain actions together, i.e.

```ts
listener.addAction('first-action', {
  action: ListenerAction.authenticateOidc({
    next: ListenerAction.forward([secondAction]),
    ...,
  }),
});
```

Bind is never called for the second action (i.e. `next`) which means the security group ingress rules are not created.

This PR updates the `ListenerAction.bind` method to call `bind` for any `next` action that is configured.

fixes aws#12994


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants