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

Use the public-load-balancer module for frontend #138

Merged
merged 2 commits into from Jan 19, 2021

Conversation

richardTowers
Copy link
Member

@richardTowers richardTowers commented Jan 19, 2021

This reduces a big chunk of duplication, and will make my next
refactoring a little bit easier.

I did some terraform state moves to get a better feel for what this will
actually do. It looks like most of the resources will need replacing, but
for the most part the changes look quite small.

See the terraform plan in concourse for more details on what this will do.

@richardTowers richardTowers force-pushed the use-public-lb-module-for-frontend branch from 29483bc to 84dfd22 Compare January 19, 2021 14:00
This reduces a big chunk of duplication, and will make my next
refactoring a little bit easier.

I did some terraform state moves to get a better feel for what this will
actually do. One thing is that the public-load-balancer module doesn't
restrict the ALB by the office IPs:

```
-/+ resource "aws_security_group_rule" "alb_from_any_https" {
      ~ cidr_blocks              = [ # forces replacement
          - "213.86.153.212/32",
          - "213.86.153.213/32",
          - "213.86.153.214/32",
          - "213.86.153.235/32",
          - "213.86.153.236/32",
          - "213.86.153.237/32",
          - "85.133.67.244/32",
          + "0.0.0.0/0",
        ]
      + description              = "frontend ALB allows requests from CIDRs list over HTTPS"
```

... not sure if we want to sort that out before we merge this.
@richardTowers richardTowers force-pushed the use-public-lb-module-for-frontend branch from 84dfd22 to fd2ecad Compare January 19, 2021 14:02
This is more consistent with what was there before I refactored it to
use the public load balancer module.

In the long run, I don't think frontend needs a public load balancer -
it's just got one for now because we haven't got router running yet. So
having the office IPs passed in directly is fine for now.
@richardTowers richardTowers marked this pull request as ready for review January 19, 2021 14:56
Copy link
Contributor

@sengi sengi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It kills me that office_cidrs_list is spreading again but that's kinda unavoidable for now. Thanks for cleaning this up.

@richardTowers
Copy link
Member Author

It kills me that office_cidrs_list is spreading again but that's kinda unavoidable for now. Thanks for cleaning this up.

I'll be able to remove one set of these variables when I inline modules/apps/frontend. I don't know where we got to with our Network ACL thinking? There was something tricky about it I think?

Anyway, good enough for now :)

@richardTowers richardTowers merged commit 17df4a3 into main Jan 19, 2021
1 check passed
@richardTowers richardTowers deleted the use-public-lb-module-for-frontend branch January 19, 2021 15:29
@sengi
Copy link
Contributor

sengi commented Jan 19, 2021

Yeah, I've only myself to blame for not finishing the ugly network ACL thing to be fair.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants