-
-
Notifications
You must be signed in to change notification settings - Fork 241
Feat: custom security groups #4027
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
5caf274
3be52da
5d690f0
8434813
95b5e44
6d49ee0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ | |
| ComputeWithPrivateGatewaySupport, | ||
| ComputeWithPrivilegedSupport, | ||
| ComputeWithReservationSupport, | ||
| ComputeWithSecurityGroupSupport, | ||
| ComputeWithVolumeSupport, | ||
| generate_unique_gateway_instance_name, | ||
| generate_unique_instance_name, | ||
|
|
@@ -124,6 +125,7 @@ class AWSCompute( | |
| ComputeWithGatewaySupport, | ||
| ComputeWithPrivateGatewaySupport, | ||
| ComputeWithVolumeSupport, | ||
| ComputeWithSecurityGroupSupport, | ||
| Compute, | ||
| ): | ||
| def __init__( | ||
|
|
@@ -329,12 +331,27 @@ def create_instance( | |
| instance_type=instance_offer.instance.name, | ||
| image_config=self.config.os_images, | ||
| ) | ||
| security_group_id = self._create_security_group( | ||
| ec2_client=ec2_client, | ||
| region=instance_offer.region, | ||
| project_id=project_name, | ||
| vpc_id=vpc_id, | ||
| ) | ||
| security_group_id = instance_config.security_group | ||
| if security_group_id is None and self.config.security_group_ids is not None: | ||
| security_group_id = self.config.security_group_ids.get(instance_offer.region) | ||
| if security_group_id is None and self.config.security_group_name is not None: | ||
| security_group_id = aws_resources.get_security_group_id_by_name( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getting security group should be cached like self._create_security_group and other methods – otherwise every AZ attempt re-issues an identical describe_security_groups call. |
||
| ec2_client=ec2_client, | ||
| name=self.config.security_group_name, | ||
| vpc_id=vpc_id, | ||
| ) | ||
| if security_group_id is None: | ||
| raise ComputeError( | ||
| f"Security group '{self.config.security_group_name}' not found in" | ||
| f" VPC {vpc_id} (region {instance_offer.region})" | ||
| ) | ||
| if security_group_id is None: | ||
| security_group_id = self._create_security_group( | ||
| ec2_client=ec2_client, | ||
| region=instance_offer.region, | ||
| project_id=project_name, | ||
| vpc_id=vpc_id, | ||
| ) | ||
| try: | ||
| response = ec2_resource.create_instances( # pyright: ignore[reportAttributeAccessIssue] | ||
| **aws_resources.create_instances_struct( | ||
|
|
@@ -363,9 +380,18 @@ def create_instance( | |
| ) | ||
| except botocore.exceptions.ClientError as e: | ||
| logger.warning("Got botocore.exceptions.ClientError: %s", e) | ||
| if e.response["Error"]["Code"] == "InvalidParameterValue": | ||
| error_code = e.response["Error"]["Code"] | ||
| if error_code == "InvalidParameterValue": | ||
| msg = e.response["Error"].get("Message", "") | ||
| raise ComputeError(f"Invalid AWS request: {msg}") | ||
| if error_code == "InvalidGroup.NotFound": | ||
| # A misconfigured security group (e.g. wrong VPC/region) is not a | ||
| # capacity issue, so surface it clearly instead of retrying other AZs. | ||
| msg = e.response["Error"].get("Message", "") | ||
| raise ComputeError( | ||
| f"Security group not found for instance in region" | ||
| f" {instance_offer.region}: {msg}" | ||
| ) | ||
| continue | ||
| instance = response[0] | ||
| # wait_until_running() is only needed so that instance is immediately ready for volume attach. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,7 @@ def validate_config(self, config: AWSBackendConfigWithCreds, default_creds_enabl | |
| raise_invalid_credentials_error(fields=[["creds"]]) | ||
| self._check_config_tags(config) | ||
| self._check_config_iam_instance_profile(session, config) | ||
| self._check_config_security_group(config) | ||
| self._check_config_vpc(session, config) | ||
|
|
||
| def create_backend( | ||
|
|
@@ -146,6 +147,19 @@ def _check_config_iam_instance_profile( | |
| f"Failed to check IAM instance profile {config.iam_instance_profile}" | ||
| ) | ||
|
|
||
| def _check_config_security_group(self, config: AWSBackendConfigWithCreds): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth checking security group existence in configurator (as opposed to provisioning-time only) so that users get early error feedback, similar to vpc/subnet checks. |
||
| if config.security_group_ids is None: | ||
| return | ||
| regions = config.regions if config.regions is not None else DEFAULT_REGIONS | ||
| unknown_regions = [r for r in config.security_group_ids if r not in regions] | ||
| if unknown_regions: | ||
| raise ServerClientError( | ||
| msg=( | ||
| f"`security_group_ids` specifies regions not in `regions`: {unknown_regions}." | ||
| " This is likely a typo — remove the extra keys or add them to `regions`" | ||
| ) | ||
| ) | ||
|
|
||
| def _check_config_vpc(self, session: Session, config: AWSBackendConfigWithCreds): | ||
| allocate_public_ip = config.public_ips if config.public_ips is not None else True | ||
| use_default_vpcs = config.default_vpcs if config.default_vpcs is not None else True | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to use the same wording across backends for consistency (with backend-specific terminology) . Something like this:
By default,
dstackcreates and manages its own security group per project that allows: SSH ingress (TCP port 22) from0.0.0.0/0, unrestricted egress, and all traffic within the group so that multi-node clusters work out of the box.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made an attempt to update the wording/phrasing. Feel free to modify as you see fit!