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

ecs-patterns: Simple http -> https redirect #8488

Closed
2 tasks
MrArnoldPalmer opened this issue Jun 11, 2020 · 13 comments · Fixed by #9341
Closed
2 tasks

ecs-patterns: Simple http -> https redirect #8488

MrArnoldPalmer opened this issue Jun 11, 2020 · 13 comments · Fixed by #9341
Assignees
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on.

Comments

@MrArnoldPalmer
Copy link
Contributor

Add a property to ecs-patterns constructs to automatically setup an http->https redirect on the service's load balancer.

Use Case

User's with publicly accessible services using https usually want to automatically redirect clients from http to https.

As the result of discussion within #5583, it is apparent that setting this up could be much easier. Since this is a super common pattern and one that we should encourage for publicly accessible services, we should make it as easy as possible in the high level patterns constructs.

Proposed Solution

Add an optional property redirectToHttps or forceHttps that when set to true will create a listener on port 80 that redirects to 443. This option should only be able to be set to true when a certificate is provided. This should be added to all ecs-patterns constructs that create load balancers.

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@MrArnoldPalmer MrArnoldPalmer added effort/small Small work item – less than a day of effort needs-triage This issue or PR still needs to be triaged. @aws-cdk/aws-ecs-patterns Related to ecs-patterns library labels Jun 11, 2020
@SomayaB SomayaB added feature-request A feature should be added or improved. and removed needs-triage This issue or PR still needs to be triaged. labels Jun 19, 2020
@logemann
Copy link

+1 for this idea which i also had and accidentally opened a new FR ;-)

@hoegertn
Copy link
Contributor

Is there any progress on this issue? Is somebody working on implementing this?

@MrArnoldPalmer
Copy link
Contributor Author

@hoegertn not that I know of. Open to PRs if you're interested.

@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Jul 30, 2020
@khalidx
Copy link

khalidx commented Aug 2, 2020

Hey! I think this already works... let me know if I'm wrong!

Here's how I implemented HTTP -> HTTPS redirects on the load balancer (single file implementation, < 60 lines, should be easy to follow): https://github.com/khalidx/origin/blob/master/src/cdk.ts

I'll inline the implementation here for reference:

    const vpc = new ec2.Vpc(this, 'InfrastructureVpc', { maxAzs: 3 })

    const cluster = new ecs.Cluster(this, 'InfrastructureCluster', { vpc })

    const service = new ecs_patterns.ApplicationLoadBalancedFargateService(this, 'ServerService', {
      cluster,
      desiredCount: 2,
      cpu: 256,
      memoryLimitMiB: 1024,
      taskImageOptions: {
        image: ecs.ContainerImage.fromAsset(join(__dirname, '../'), { exclude: [ 'node_modules', 'dist', 'exec', 'cdk.out' ] })
      },
      publicLoadBalancer: true,
      domainName: configuration.subdomian,
      domainZone: route53.HostedZone.fromHostedZoneAttributes(this, 'InfrastructureZone', configuration.hostedZone),
      certificate: acm.Certificate.fromCertificateArn(this, 'InfrastructureCertificate', configuration.certificate)
    })

    service
    .loadBalancer
    .addListener('HttpListener', { protocol: elbv2.ApplicationProtocol.HTTP, port: 80 })
    .addRedirectResponse('HttpRedirect', { statusCode: 'HTTP_301', protocol: elbv2.ApplicationProtocol.HTTPS, port: '443' })

By the way, check out https://github.com/khalidx/origin for a boilerplate starter project that deploys a node express API as an HTTPS exposed service using CDK. It also generates a node module, native binaries, and Docker image for your API, so that it can run anywhere!

@hoegertn
Copy link
Contributor

hoegertn commented Aug 2, 2020

Yes this is a valid approach but the feature requested and implemented by me is to have this with a single config option to save the user from creating the additional LB listener.

@khalidx
Copy link

khalidx commented Aug 2, 2020

Yes this is a valid approach but the feature requested and implemented by me is to have this with a single config option to save the user from creating the additional LB listener.

Ah, I see @hoegertn! That makes sense. So instead of doing service.loadBalancer the configuration would instead be on the ecs_patterns.ApplicationLoadBalancedFargateService object.

@mergify mergify bot closed this as completed in #9341 Aug 17, 2020
mergify bot pushed a commit that referenced this issue Aug 17, 2020
closes #8488

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
misterjoshua pushed a commit to misterjoshua/aws-cdk that referenced this issue Aug 19, 2020
closes aws#8488

----

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

@MrArnoldPalmer This PR #9341 seems to be a good start, but I am running into the issue where domainZone and domainName are still required props, even if I am passing in a certificate that I have already provisioned.

@hoegertn
Copy link
Contributor

Imho they are required to create the Route53 entries.

@iwarshak
Copy link

@hoegertn No disagreement as to why it's there, just pointing this out to @MrArnoldPalmer and this comment: #5583 (comment)

@merland
Copy link

merland commented May 24, 2021

@MrArnoldPalmer This PR #9341 seems to be a good start, but I am running into the issue where domainZone and domainName are still required props, even if I am passing in a certificate that I have already provisioned.

@iwarshak @hoegertn This seems like the same usecase I was working with here. I needed to do it without involving domainZone and domainName. I did succeed but the approach is very ugly.

Hopefully the same result can be achieved with nicer code now, a year later. However this feature request is still open, so maybe not...

@hoegertn
Copy link
Contributor

I think this should work now with commit e6c85e4

So since 1.100.0

@namedgraph
Copy link

I don't understand how @khalidx suggestion is supposed to work if NetworkLoadBalancedEc2Service by default creates an HTTP listener on port 80?
https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs-patterns/lib/base/network-load-balanced-service-base.ts#L350
Shouldn't the target properties use props.listenerPort instead of hardcoding port 80?

    const targetProps = {
      port: props.listenerPort,
    };

Anyway when I try to access that listener like this:

NetworkLoadBalancedEc2Service ec2Service = NetworkLoadBalancedEc2Service.Builder.create()....
ec2Service.getListener().???

the only methods available are addAction, addTargetGroups, addTargets, not addRedirectResponse.

@alejandrodau
Copy link

It seems this is now possible using the addRedirect method

https://docs.aws.amazon.com/cdk/api/v1/docs/aws-elasticloadbalancingv2-readme.html#:~:text=If%20you%20do%20not%20provide%20any%20options%20for%20this%20method%2C%20it%20redirects%20HTTP%20port%2080%20to%20HTTPS%20port%20443.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.