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.

Sign up for GitHub

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

Jump to bottom

Set Load Balancer internal/external per process instead of per application #1082

Closed
wants to merge 1 commit into from
Closed

Set Load Balancer internal/external per process instead of per application #1082

wants to merge 1 commit into from

Conversation

iserko
Copy link
Contributor

@iserko iserko commented May 5, 2017 •
edited
Loading

Introduces EMPIRE_X_EXPOSURE which needs to be public.

If that environment variable is not set, we fallback to the default selection via application (where you use domain-add).

Added this explanation in the Procfile doc.

Fixes #1076

Things working and tested:

  • Creating a new application using Procfile work as specified (one internal and one external ALB) https://github.com/iserko/flask_test/blob/f369c115fd41a0086d364f38b693a353963c041d/Procfile ✅
  • Modifying a running application by setting this environment variable and deploying ✅ (new ALB/ELB gets created and the old one is deleted)
  • When using old Procfile format doing env set EMPIRE_X_EXPOSURE=public creates a public ALB/ELB
  • Migrating from old Procfile with an internal ALB/ELB to new Procfile format (LB is not recreated)
  • Migrating from old Procfile with an external ALB/ELB to new Procfile format (LB is not recreated)

The biggest thing is that this setting shouldn't be considered as harmless. If you change it ... your load balancer WILL get recreated.

@ejholmes Thoughts?

ejholmes
ejholmes suggested changes May 5, 2017
View reviewed changes
Copy link
Contributor

@ejholmes ejholmes left a comment

Choose a reason for hiding this comment

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

Modifying a running application by setting this environment variable and deploying ... ⚠️ Breaks CloudFormation as a Target Group can only be assigned to one Load Balancer, so we'd need to create a separate Target Group?

Probably what we should do is add a prefix to the resource name for the target group that includes the LB scheme. So instead of the resource name being webTargetGroup, it would be webInternalTargetGroup or webExternalTargetGroup.

scheduler/cloudformation/template.go Outdated
@@ -54,6 +69,20 @@ func loadBalancerType(app *twelvefactor.Manifest, process *twelvefactor.Process)
return classicLoadBalancer
}

func schemeExposure(app *twelvefactor.Manifest, process *twelvefactor.Process) string {
env := twelvefactor.Env(app, process)
if v, ok := env["EMPIRE_X_EXPOSURE"]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move this check into https://github.com/remind101/empire/blob/master/releases.go#L456, where it can just set the appropriate Exposure on the twelvefactor.Process.

I've been intending on moving all of the EMPIRE_X_* checks from this file and into releases.go so this would be a good start.

@iserko iserko force-pushed the by_process_exposure branch 7 times, most recently from 8a595e4 to 0b5bee5 Compare June 13, 2017 18:42
@iserko
Copy link
Contributor Author

iserko commented Jun 13, 2017

@ejholmes I've just rebased and pushed additional changes.

The TargetGroup prefix was really easy to put it, so that wasn't the problem.
The biggest problem was moving the EMPIRE_X_EXPOSURE var into releases.go
At that point the app and process Environments aren't merged yet (it happens in twelvefactor.go). So I exposed the merge function by capitalizing it and using it in releases.go.

Please let me know if you have comments either via Github or if you have time in the next few hours I'll be online via Slack

@iserko iserko force-pushed the by_process_exposure branch 3 times, most recently from 4789bca to bb15edd Compare June 13, 2017 19:18
ejholmes
ejholmes reviewed Jun 13, 2017
View reviewed changes
Copy link
Contributor

@ejholmes ejholmes left a comment

Choose a reason for hiding this comment

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

Awesome! I'll try to play around with this later today/tomorrow to check how renaming the target group behaves.

procfile/README.md Outdated
Name | Default value | Available options | Description
-----|---------------|-------------------|------------
`EMPIRE_X_LOAD_BALANCER_TYPE` | `elb` | `alb`, `elb`| Determines whether you will use an ALB or ELB
`EMPIRE_X_EXPOSURE` | `private` | `private`, `public` | Sets whether your ALB or ELB will be public (internet-facing) or private (internal), the default is private, however if you have used the deprecated `domain-add` command then the load balancer will become public. **If you change this setting, the load balancer will be recreated as soon as you deploy**
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that we've tried to do with the extended Procfile is maintain some level of "parity" with docker-compose.yml, so that concepts just kinda translate over (e.g. ports).

In docker-compose.yaml, you can attach a container to a "network" (like external or internal), which I think maps pretty nicely to how we use internal/external ELB's. I wonder if we should just use a networks key to determine whether the process should be exposed external/internal.

For example:

web:
  command: ./bin/web
  networks:
    - external # Expose this process with an "external" ELB.
admin:
  command: ./bin/web
  networks:
    - internal # Expose this process with an "internal" ELB

Just a thought, totally fine with starting with some EMPIRE_X env vars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that idea too

releases.go
mergedEnv := twelvefactor.Merge(appEnv, env)

isExternal := release.App.Exposure == exposePublic
if v, ok := mergedEnv["EMPIRE_X_EXPOSURE"]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we should add some tests similar to https://github.com/remind101/empire/blob/master/tests/empire/empire_test.go#L77-L180 that exercises this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep definitely makes sense. Let me have a stab at it

procfile/README.md Outdated
@@ -78,3 +78,13 @@ This allows you to set process specific environment variables. If these are set
environment:
EMPIRE_X_LOAD_BALANCER_TYPE: "alb"
```

Supported environment variables that can either be set via `emp set` for the whole application or
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for documenting these!

@iserko
Set Load Balancer internal/external per process instead of per applic…
2ddd969 
…ation

Introduces EMPIRE_X_EXPOSURE which needs to be either `internal` or `internet-facing` (kept AWS terminology here).

If that environment variable is not set, we fallback to the default selection via application (where you use `domain-add`).

Fixes remind101#1076
@iserko iserko force-pushed the by_process_exposure branch from bb15edd to 2ddd969 Compare August 2, 2017 06:35
@iserko iserko closed this Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejholmes ejholmes ejholmes requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@iserko @ejholmes