[autoscaler] AWS Autoscaler support for EC2 security group rule configuration by pdames · Pull Request #7873 · ray-project/ray

pdames · 2020-04-02T20:02:44Z

Why are these changes needed?

These changes support common enterprise network security compliance use cases to ensure that inbound AWS EC2 instance connections are restricted to a known set of internal subnets. Although custom security groups or network ACLs may currently be configured to restrict SSH access to internal subnets, their creation requires:

Learning the current inbound connection rule set required for a Ray cluster and associated CLI tools to function properly (which may change over time).
Creating a custom security group and/or network ACL prior to running Ray cluster setup (via either a manual or automated out-of-band process).
Specifying the custom security group ID and/or subnet ID associated with the network ACL in the Ray autoscaler config file (which are only known after creating a custom security group or Network ACL)
In general, I believe that we should decouple AWS autoscaler users from these responsibilities by allowing them to specify common security group configuration overrides to apply during default security group bootstrapping. To that end, these changes expose new AWS autoscaler configuration settings that allow users to specify custom inbound head/worker node security group rules to apply (via authorization of new rules or revocation of old rules) during config bootstrapping. Please refer to the included "example-security-groups.yaml" file for additional details. Also note that, if no custom security group rule config is specified, then prior default security group bootstrap behavior is maintained, namely:
Inbound rules permitting external connections to previously bootstrapped security groups are not modified.
Newly bootstrapped security groups allow all network traffic between head and worker nodes as well as external SSH connections from 0.0.0.0/0.

These changes also fix a bug which allowed users to specify separate subnet ID configurations for AWS head and worker nodes, but then ignored the configured head node subnet ID during cluster security group bootstrapping. Previously, we would always bootstrap one security group for both head and worker nodes and always choose the VPC associated with a worker's subnet ID (thus failing to create a separate head node security group whenever its subnet ID configuration resolved to a different VPC). Now, separate security groups will be created for head and worker nodes whenever their respective subnet ID configurations resolve to separate VPCs.

It also may be worth noting that additional tests and documentation updates are expected to follow this pull request, but I wanted to collect early reviewer opinions now instead of opening a more complete (but monolithic) pull request later.

Related issue number

#8420

Checks

[X ] I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://ray.readthedocs.io/en/latest/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested (please justify below)

AmplabJenkins · 2020-04-02T20:10:07Z

Can one of the admins verify this patch?

AmplabJenkins · 2020-04-02T21:01:13Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24160/
Test FAILed.

AmplabJenkins · 2020-04-03T09:08:09Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24185/
Test PASSed.

richardliaw · 2020-04-03T23:59:44Z

ci/travis/install-dependencies.sh

why do we need this?

I see, it's for boto tests later. These tests don't actually require creds right?

Correct - the tests are stubbed/mocked out and don't interact with any real AWS account.

richardliaw · 2020-04-06T20:08:45Z

hey @allenyin55, do you mind reviewing this PR?

AmplabJenkins · 2020-04-07T02:04:12Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24332/
Test FAILed.

allenyin55 · 2020-04-07T19:53:54Z

hey @allenyin55, do you mind reviewing this PR?

Might be tough this week, but I'll try to make some time for this.

… inbound rule configuration and separate head/worker subnets.

…ult SSH inbound rules, and correct misleading AWS autoscaler example and test comments.

…ir method signature.

…ules, revoke unspecified rules by default, and revised security group config example.

pdames · 2020-04-09T17:17:32Z

FYI, the latest update of this pull request includes changes to deduplicate equivalent, but unequal, rules as mentioned in #7844. Essentially, the rules boil down to simply resolving case-sensitivity differences between prefix and group list IDs, as well as canonicalizing IPv4 and IPv6 addresses. The canonicalization rules rely on fairly standard Java IP address libraries together with a few custom extensions that ultimately produce the same canonical string representations that we can get using https://docs.python.org/3/library/ipaddress.html:

canonical_cidr_ip_string = str(ipaddress.ip_network(cidr_ip_string, False))

richardliaw · 2020-04-09T17:35:02Z

This PR is just exploding; can you split some of this up into separate PRs?

AmplabJenkins · 2020-04-09T18:19:39Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/24465/
Test PASSed.

pdames · 2020-04-09T18:30:16Z

This PR is just exploding; can you split some of this up into separate PRs?

I'm open to whatever makes your job easiest. There are a few ways that I could see us split it up going forward:

Break the most recent changes for deduping equivalent but unequal rules and automatic revocation of all unlisted rules out into a separate PR (although it should be noted that represents a backwards-incompatible change, and that most of the new code volume here is from tests).
Include only non-test, non-example changes in this PR. Include tests and examples in 1 or 2 subsequent PRs (helps break up focus by area, although I worry a bit about pushing code without accompanying tests to protect it from regresions).
Leave it as it is for now, and simply ensure that any subsequent revisions to the logic, tests, or examples here go into new PRs.

Let me know if you have a preference (or if the preference perhaps isn't listed here).

pdames force-pushed the aws-autoscaler-security-groups branch from 207a0aa to d0172ee Compare April 3, 2020 07:48

pdames mentioned this pull request Apr 3, 2020

[autoscaler] Allowing users to provide extra configs for AWS #7844

Merged

6 tasks

richardliaw reviewed Apr 3, 2020

View reviewed changes

pdames force-pushed the aws-autoscaler-security-groups branch from d0172ee to 57688b0 Compare April 7, 2020 01:18

pdames added 4 commits April 9, 2020 00:20

[autoscaler] AWS Autoscaler support for idempotent EC2 security group…

c03bede

… inbound rule configuration and separate head/worker subnets.

[autoscaler] Reuse AWS autoscaler default channels when crafting defa…

b09133a

…ult SSH inbound rules, and correct misleading AWS autoscaler example and test comments.

[autoscaler] Update AWS autoscaler tests to use new AWS config key_pa…

8239ea5

…ir method signature.

[autoscaler] Support deduping equivalent but unequal security group r…

acc44d1

…ules, revoke unspecified rules by default, and revised security group config example.

pdames force-pushed the aws-autoscaler-security-groups branch from 57688b0 to acc44d1 Compare April 9, 2020 16:50

pdames mentioned this pull request Apr 21, 2020

[autoscaler] Allow users to disable the cluster config cache #8117

Merged

6 tasks

pdames mentioned this pull request May 8, 2020

[autoscaler] Honor separate head and worker node subnet IDs #8374

Merged

6 tasks

ericl closed this Sep 21, 2020

Conversation

pdames commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

AmplabJenkins commented Apr 2, 2020

Uh oh!

AmplabJenkins commented Apr 2, 2020

Uh oh!

AmplabJenkins commented Apr 3, 2020

Uh oh!

richardliaw Apr 3, 2020

Choose a reason for hiding this comment

Uh oh!

richardliaw Apr 4, 2020

Choose a reason for hiding this comment

Uh oh!

pdames Apr 4, 2020

Choose a reason for hiding this comment

Uh oh!

richardliaw commented Apr 6, 2020

Uh oh!

AmplabJenkins commented Apr 7, 2020

Uh oh!

allenyin55 commented Apr 7, 2020

Uh oh!

pdames commented Apr 9, 2020

Uh oh!

richardliaw commented Apr 9, 2020

Uh oh!

AmplabJenkins commented Apr 9, 2020

Uh oh!

pdames commented Apr 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pdames commented Apr 2, 2020 •

edited

Loading