Add --ec2-security-group flag support #112

serialx · 2016-04-25T12:00:00Z

This PR makes the following changes:

Adds --ec2-security-group flag support

I tested this PR by actually launching multiple clusters that we use everyday. :)

Fixes #72

BTW, having great fun using flintrock. We've replaced our internal production spark-ec2 scripts to flintrock. We are testing it right now. Great software!

nchammas · 2016-05-02T01:55:26Z

Thank you for the kind words @serialx, and sorry about the delay in reviewing this. I will try to look at it next weekend.

nchammas · 2016-05-08T15:56:04Z

flintrock/config.yaml.template

@@ -23,6 +23,9 @@ providers:
    # vpc-id: <id>
    # subnet-id: <id>
    # placement-group: <name>
+    # security-groups:
+    #   - sg-group1


I was thinking these would be security group names, not IDs. I think that's more user-friendly. Do you agree?

nchammas · 2016-05-08T16:03:19Z

Thanks for the clean PR, @serialx.

In addition to the line comments, there was one other minor issue: The error message when the user provides a nonexistent security group is confusing.

botocore.exceptions.ClientError: An error occurred (InvalidParameterValue) when calling
the RunInstances operation: Value () for parameter groupId is invalid. The value cannot be empty

If possible, I would prefer it if we could tell the user "Security group 'group-name' does not exist in VPC 'vpc-id'." Is there a relatively straightforward way of doing that?

serialx · 2016-06-23T08:33:57Z

Thank you for your detailed feedback. I've been testing out the group name approach and have not been successful with this error:

Traceback (most recent call last):
  File "/usr/local/bin/flintrock", line 9, in <module>
    load_entry_point('Flintrock==0.5.0.dev0', 'console_scripts', 'flintrock')()
  File "/usr/local/lib/python3.5/site-packages/flintrock/flintrock.py", line 880, in main
    cli(obj={})
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/flintrock/flintrock.py", line 331, in launch
    instance_initiated_shutdown_behavior=ec2_instance_initiated_shutdown_behavior)
  File "/usr/local/lib/python3.5/site-packages/flintrock/ec2.py", line 46, in wrapper
    res = func(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/flintrock/ec2.py", line 611, in launch
    InstanceInitiatedShutdownBehavior=instance_initiated_shutdown_behavior)
  File "/usr/local/lib/python3.5/site-packages/boto3/resources/factory.py", line 481, in do_action
    response = action(self, *args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/boto3/resources/action.py", line 83, in __call__
    response = getattr(parent.meta.client, operation_name)(**params)
  File "/usr/local/lib/python3.5/site-packages/botocore/client.py", line 228, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/usr/local/lib/python3.5/site-packages/botocore/client.py", line 486, in _make_api_call
    raise ClientError(parsed_response, operation_name)
botocore.exceptions.ClientError: An error occurred (InvalidParameterCombination) when calling the RunInstances operation: The parameter groupName cannot be used with the parameter subnet

I guess this is not supported on VPC environments.

serialx · 2016-06-23T08:39:05Z

About the non-existent security group I get this error:

There was a problem with the launch. Cleaning up...
Traceback (most recent call last):
  File "/usr/local/bin/flintrock", line 9, in <module>
    load_entry_point('Flintrock==0.5.0.dev0', 'console_scripts', 'flintrock')()
  File "/usr/local/lib/python3.5/site-packages/flintrock/flintrock.py", line 880, in main
    cli(obj={})
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/flintrock/flintrock.py", line 331, in launch
    instance_initiated_shutdown_behavior=ec2_instance_initiated_shutdown_behavior)
  File "/usr/local/lib/python3.5/site-packages/flintrock/ec2.py", line 46, in wrapper
    res = func(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/flintrock/ec2.py", line 611, in launch
    InstanceInitiatedShutdownBehavior=instance_initiated_shutdown_behavior)
  File "/usr/local/lib/python3.5/site-packages/boto3/resources/factory.py", line 481, in do_action
    response = action(self, *args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/boto3/resources/action.py", line 83, in __call__
    response = getattr(parent.meta.client, operation_name)(**params)
  File "/usr/local/lib/python3.5/site-packages/botocore/client.py", line 228, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/usr/local/lib/python3.5/site-packages/botocore/client.py", line 486, in _make_api_call
    raise ClientError(parsed_response, operation_name)
botocore.exceptions.ClientError: An error occurred (InvalidGroup.NotFound) when calling the RunInstances operation: The security group 'sg-12341234' does not exist in VPC 'vpc-**********'

Would this suffice? Maybe the botocore became more friendly on error cases. :)

nchammas · 2016-06-26T23:06:40Z

Regarding this error:

botocore.exceptions.ClientError: An error occurred (InvalidParameterCombination) when calling the 
RunInstances operation: The parameter groupName cannot be used with the parameter subnet

My intention was that we would let users pass in security group names, but wherever necessary we would translate them into security group IDs to satisfy AWS's API.

That lets us separate the user-interface from what we have to pass to AWS.

Does that make sense?

serialx · 2016-06-30T11:20:48Z

Totally got your message. Revised the commit to fully support group names. :)

Also, groups which are not found will issue the following error message.

Traceback (most recent call last):
  File "/usr/local/bin/flintrock", line 9, in <module>
    load_entry_point('Flintrock==0.5.0.dev0', 'console_scripts', 'flintrock')()
  File "/usr/local/lib/python3.5/site-packages/flintrock/flintrock.py", line 880, in main
    cli(obj={})
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.5/site-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/flintrock/flintrock.py", line 331, in launch
    instance_initiated_shutdown_behavior=ec2_instance_initiated_shutdown_behavior)
  File "/usr/local/lib/python3.5/site-packages/flintrock/ec2.py", line 46, in wrapper
    res = func(*args, **kwargs)
  File "/usr/local/lib/python3.5/site-packages/flintrock/ec2.py", line 566, in launch
    security_groups=security_groups)
  File "/usr/local/lib/python3.5/site-packages/flintrock/ec2.py", line 322, in get_ec2_security_groups
    groups=non_found_groups))
Exception: Error security group not found: {'not-found-sg'}

nchammas · 2016-07-11T16:02:02Z

flintrock/ec2.py

+    """
+    ec2 = boto3.resource(service_name='ec2', region_name=region)
+
+    # Resolve security group names


I think we can drop all the comments like this one since the code is pretty self-explanatory.

In general, I prefer to add comments to explain why something was done when it may not be obvious, as opposed to what was done which is usually clear enough from the code.

nchammas · 2016-07-11T16:07:02Z

@serialx - Apologies for taking so long to review this. It's summertime over here and I've been spending much less time at the keyboard than usual. 😄

This PR looks pretty good to me and I'd like to get it in soon. My only remaining concern is that we are allowing the user to mix specifying security groups by name and by ID. I'd prefer it if we allowed security groups to be specified only by name. It makes the interface and code a bit simpler.

Is that OK with you? Is there some case where someone might really need to specify the security group by ID and not by name?

serialx · 2016-07-12T12:21:54Z

I think supporting only group names is fine. Changed the commit including the the changes you mentioned in the comment.

nchammas · 2016-07-14T00:38:48Z

Thanks @serialx. Merging this in.

* master: 0.6.0 dev begins add some minor steps update standalone version in example this is 0.5.0 upgrade dependencies (nchammas#128) use latest Amazon Linux AMI rephrase note about future Windows support remove note about squashing PR commits up default Spark version to 1.6.2 add CHANGES for spark download source and additional security groups rename some internals related to security groups Resolve nchammas#72 add --ec2-security-group flag support (nchammas#112) added HADOOP_LIBEXEC_DIR env var (nchammas#127) Add option to download Spark from a custom URL (nchammas#125) add custom Hadoop URL change; reformat Markdown links

nchammas reviewed May 8, 2016
View reviewed changes

serialx force-pushed the ec2-security-group branch from 19bede3 to 942badd Compare June 23, 2016 07:12

serialx force-pushed the ec2-security-group branch from bb36c00 to 593695d Compare June 30, 2016 11:19

nchammas reviewed Jul 11, 2016
View reviewed changes

serialx force-pushed the ec2-security-group branch from 593695d to 4813b6b Compare July 12, 2016 12:21

serialx force-pushed the ec2-security-group branch from 4813b6b to da9826e Compare July 12, 2016 12:22

Resolve nchammas#72 add --ec2-security-group flag support

e3226a2

serialx force-pushed the ec2-security-group branch from da9826e to e3226a2 Compare July 12, 2016 12:23

nchammas merged commit da95a09 into nchammas:master Jul 14, 2016

This was referenced Jul 23, 2016

Use a access_origins parameters for the security groups #105

Closed

Give users an easy way to authorize new addresses to access a cluster #38

Closed

pyup-vuln-bot mentioned this pull request Oct 18, 2016

Changelog flintrock version 0.5.0 pyupio/safety-db#714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --ec2-security-group flag support #112

Add --ec2-security-group flag support #112

serialx commented Apr 25, 2016

nchammas commented May 2, 2016

nchammas May 8, 2016

nchammas commented May 8, 2016 •

edited

Loading

serialx commented Jun 23, 2016

serialx commented Jun 23, 2016

nchammas commented Jun 26, 2016

serialx commented Jun 30, 2016

nchammas Jul 11, 2016

nchammas commented Jul 11, 2016

serialx commented Jul 12, 2016

nchammas commented Jul 14, 2016

Add --ec2-security-group flag support #112

Add --ec2-security-group flag support #112

Conversation

serialx commented Apr 25, 2016

nchammas commented May 2, 2016

nchammas May 8, 2016

Choose a reason for hiding this comment

nchammas commented May 8, 2016 • edited Loading

serialx commented Jun 23, 2016

serialx commented Jun 23, 2016

nchammas commented Jun 26, 2016

serialx commented Jun 30, 2016

nchammas Jul 11, 2016

Choose a reason for hiding this comment

nchammas commented Jul 11, 2016

serialx commented Jul 12, 2016

nchammas commented Jul 14, 2016

nchammas commented May 8, 2016 •

edited

Loading