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

Add AWS VPN support #2261

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add AWS VPN support #2261

wants to merge 7 commits into from

Conversation

mjz-smu
Copy link
Contributor

@mjz-smu mjz-smu commented May 21, 2020

Notes:

  • adds aws provider support for vpn_service.
  • aws vpn requires ike v.1 currently.
  • aws vpn gateways can't connect to other aws vpn gateways.
  • example config:
iperf:
  description: Run iperf over vpn
  flags:
    iperf_sending_thread_count: 5
#    use_vpn: True
    vpn_service_gateway_count: 1
  vpn_service:
    tunnel_count: 1
    shared_key: FEEDDAD
    ike_version: 1
    routing_type: static
  vm_groups:
    vm_1:
      cloud: AWS
      cidr: 10.0.1.0/24
      vm_spec:
        AWS:
            machine_type: t2.micro
            zone: us-west-1b
    vm_2:
      cloud: AWS
      cidr: 192.168.1.0/24
      vm_spec:
        AWS:
            #machine_type: m5.large
            machine_type: t2.micro
            zone: us-east-2b

dlott
dlott suggested changes May 26, 2020
View reviewed changes
perfkitbenchmarker/configs/vpn_tests/005_iperf_vpn_gcp2aws.yml Outdated
@@ -0,0 +1,28 @@
# VPN iperf config.
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a specific test, not general functionality?

Copy link

Choose a reason for hiding this comment

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

Oops, removed that file.

perfkitbenchmarker/providers/aws/aws_emr.py
@@ -95,6 +95,7 @@ class AwsEMR(spark_service.BaseSparkService):
CLOUD = providers.AWS
SPARK_SAMPLE_LOCATION = '/usr/lib/spark/lib/spark-examples.jar'
SERVICE_NAME = 'emr'
cidr = None # placeholder for vpn service
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a plan to integrate here?

Copy link

Choose a reason for hiding this comment

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

No. EMR cluster passes itself to aws_network.AwsNetwork.GetNetwork(self) which expects zone and cidr attributes. This probably should have gone into [#1986] but didn't present problems until the aws network code was involved (this PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the correct fix is for EMR to instead call aws_network.AwsNetwork.GetNetworkFromNetworkSpec and just build the network spec like

PerfKitBenchmarker/perfkitbenchmarker/providers/aws/aws_dpb_emr.py

Line 84 in 64eeb3d

self.network = aws_network.AwsNetwork.GetNetworkFromNetworkSpec(

perfkitbenchmarker/providers/aws/aws_network.py Outdated
flags.DEFINE_string(
'aws_subnet', None,
'The static AWS subnet id to use. Default creates a new one')
'aws_vpc',
Copy link
Contributor

Choose a reason for hiding this comment

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

There seem to be a lot of ?auto? formatting changes in here. Can you revert (or split those out to a separate pull request) so it's easier to review?

Copy link

Choose a reason for hiding this comment

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

Fixed. Is there a .style.yapf that works for this codebase?

Copy link
Contributor

Choose a reason for hiding this comment

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

sadly no, but I'm told that chromium style is closest.

perfkitbenchmarker/providers/aws/aws_network.py Outdated
@@ -65,12 +78,35 @@ def __init__(self):
self.firewall_icmp_set = set()
self._lock = threading.Lock()

# @TODO: merge this not in rebase
Copy link
Contributor

Choose a reason for hiding this comment

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

Still TODO?

Copy link

Choose a reason for hiding this comment

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

Wasn't sure if it should be separate from this PR. Fixed in [cd82eda] which is the last commit on this push, so I can pop it off and resubmit if it doesn't belong here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given the size of this, it'd be great to resubmit this standalone (and any other pieces we can break off).

cwilkes
cwilkes reviewed May 26, 2020
View reviewed changes
perfkitbenchmarker/providers/aws/aws_network.py Outdated


class AwsVPNGW(network.BaseVpnGateway):
CLOUD = providers.AWS
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move all the AWS VPN related code (both existing and new) into a new file
aws_vpn_network.py
as that will make it a lot easier to diff. We did that with https://github.com/GoogleCloudPlatform/PerfKitBenchmarker/blob/master/perfkitbenchmarker/providers/aws/aws_placement_group.py which made it a lot easier to review.

Copy link

Choose a reason for hiding this comment

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

Fixed.

dlott
dlott reviewed Nov 16, 2020
View reviewed changes
perfkitbenchmarker/providers/aws/aws_network.py
@@ -85,8 +89,8 @@ def AllowIcmp(self, vm):
authorize_cmd = util.AWS_PREFIX + [
'ec2',
'authorize-security-group-ingress',
'--region=%s' % vm.region,
'--group-id=%s' % vm.group_id,
'--region=%s' % region or vm.region,
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 use the values from entry here rather than duplicating the logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

and inline source = cidr or '0.0.0.0/0'

perfkitbenchmarker/providers/aws/aws_emr.py
@@ -95,6 +95,7 @@ class AwsEMR(spark_service.BaseSparkService):
CLOUD = providers.AWS
SPARK_SAMPLE_LOCATION = '/usr/lib/spark/lib/spark-examples.jar'
SERVICE_NAME = 'emr'
cidr = None # placeholder for vpn service
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the correct fix is for EMR to instead call aws_network.AwsNetwork.GetNetworkFromNetworkSpec and just build the network spec like

PerfKitBenchmarker/perfkitbenchmarker/providers/aws/aws_dpb_emr.py

Line 84 in 64eeb3d

self.network = aws_network.AwsNetwork.GetNetworkFromNetworkSpec(

perfkitbenchmarker/providers/aws/aws_network.py
self.subnet = None
self.vpc_peering = None
self.cidr = None
Copy link
Contributor

Choose a reason for hiding this comment

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

move to the else in if spec.cidr to have single assignment.

perfkitbenchmarker/providers/aws/aws_vpn_network.py Outdated
'create-vpn-gateway',
'--region=%s' % self.region,
'--availability-zone=%s' % self.az,
'--type=%s' % 'ipsec.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this change? Why not just make a constant?

perfkitbenchmarker/providers/aws/aws_vpn_network.py Outdated
# }
def _Create(self):
"""Creates the VPN gateway."""
'''
Copy link
Contributor

Choose a reason for hiding this comment

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

please include link to documentation.

perfkitbenchmarker/providers/aws/aws_vpn_network.py Outdated
'--options=%s' % self._getCnxnOpts()
]
response, _ = util.IssueRetryableCommand(create_cmd)
response_xml = response[response.find("<"):response.rfind(">") +
Copy link
Contributor

Choose a reason for hiding this comment

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

needs more comments about formats and parsing. Can you parse as Json and then xml rather than this manual stuff?

perfkitbenchmarker/providers/aws/aws_vpn_network.py Outdated
response_json = re.sub(r'<.*>', '', response)
response_json = json.loads(response_json)
self.id = response_json["VpnConnection"]["VpnConnectionId"]
# self.ip = response_json["VpnConnection"]["VgwTelemetry"][0][
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

perfkitbenchmarker/providers/aws/aws_vpn_network.py Outdated
]
logging.info("Get VPN Cnxn Options<tun_opts>: %s" % str(opts))
# https://docs.aws.amazon.com/cli/latest/userguide/cli-using-param.html
logging.info(
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

perfkitbenchmarker/providers/aws/aws_vpn_network.py Outdated
str(json.dumps(json.dumps(opts))[1:-1].replace('\\',
''))
)
return json.dumps(json.dumps(opts))[1:-1].replace('\\', '')
Copy link
Contributor

Choose a reason for hiding this comment

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

needs comments about encodings. prefer using standard parsers.

perfkitbenchmarker/providers/aws/aws_vpn_network.py Outdated
'--customer-gateway-ids=%s' % self.id
]
response, _ = util.IssueRetryableCommand(cmd)
# logging.log(logging.INFO, response)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matt-smu matt-smu matt-smu left review comments

@dlott dlott dlott requested changes

@cwilkes cwilkes cwilkes left review comments

@shyamsabhaya shyamsabhaya Awaiting requested review from shyamsabhaya

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mjz-smu @cwilkes @dlott @matt-smu @googlebot