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.

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

[AWS] Fix fetching AZ when describe zones permission does not exist in all regions #2463

Merged
merged 12 commits into from
Aug 30, 2023

Conversation

Michaelvll
Copy link
Collaborator

@Michaelvll Michaelvll commented Aug 25, 2023

Fixes #2451

Current logging for sky check:

$ sky check                                                                                                                                                                                          
Checking credentials to enable clouds for SkyPilot.
AWS: [WARNING] Missing availability zone mappings for the following enabled regions:
Regions                                                       Reason                                                                            
['us-east-1', 'ca-central-1', 'ap-southeast-2', 'ap-          Failed to retrieve availability zones. Please ensure that the                     
northeast-2', 'ap-northeast-1', 'ap-south-1', 'eu-north-1',   `ec2:DescribeAvailabilityZones` action is enabled for your AWS account in IAM.    
'us-west-2', 'eu-west-1', 'eu-west-3', 'us-east-2', 'ap-      Ref: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_DescribeAvailabi  
northeast-3', 'eu-central-1', 'ap-southeast-1', 'eu-west-2']  lityZones.html.                                                                   
  AWS: enabled          
  Azure: enabled          
  GCP: enabled          
  IBM: enabled          
  Kubernetes: enabled          
  Lambda: enabled          
  OCI: enabled          
  SCP: enabled          
  Cloudflare (for R2 object store): enabled 

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    • sky check with an AWS account with the following IAM policy (only allow the permission for a single region:
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "VisualEditor0",
            "Effect": "Deny",
            "Action": "ec2:DescribeAvailabilityZones",
            "Resource": "*"
        },
        {
            "Sid": "VisualEditor1",
            "Effect": "Allow",
            "Action": "ec2:DescribeAvailabilityZones",
            "Resource": "*",
            "Condition": {
                "StringEquals": {
                    "aws:RequestedRegion": "us-west-1"
                }
            }
        }
    ]
}
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

@TRT-BradleyB
Copy link

This works for me now! re. #2451

Copy link
Member

@concretevitamin concretevitamin left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @Michaelvll! Some minor comments.

sky/exceptions.py Outdated Show resolved Hide resolved
@@ -215,3 +215,35 @@ class ClusterOwnerIdentityMismatchError(Exception):
class NoCloudAccessError(Exception):
"""Raised when all clouds are disabled."""
pass


class AWSAzFetchingError(Exception):
Copy link
Member

Choose a reason for hiding this comment

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

(not feeling strongly) nit: AwsAzFetchingError

https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I slightly prefer the AWS, as the AWSNodeProvider from ray code already applies this, but feel free to push back.

sky/clouds/service_catalog/data_fetchers/fetch_aws.py Outdated Show resolved Hide resolved
sky/clouds/service_catalog/data_fetchers/fetch_aws.py Outdated Show resolved Hide resolved
@Michaelvll
Copy link
Collaborator Author

Latest UX:

$ sky check                                                                                                                
Checking credentials to enable clouds for SkyPilot.
AWS: [WARNING] Missing availability zone mappings for the following enabled regions:
Region                                                      Reason                                                                            
ap-southeast-2, ap-northeast-2, us-west-2, ap-south-1,      Failed to retrieve availability zones. Please ensure that the                     
us-east-2, eu-west-2, eu-west-1, eu-central-1, us-east-1,   `ec2:DescribeAvailabilityZones` action is enabled for your AWS account in IAM.    
ap-northeast-1, ap-northeast-3, eu-west-3, ap-southeast-1,  Ref: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_DescribeAvailabi  
eu-north-1, ca-central-1                                    lityZones.html.                                                                   
  AWS: enabled   

Copy link
Member

@concretevitamin concretevitamin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Michaelvll.

sky/clouds/service_catalog/data_fetchers/fetch_aws.py Outdated Show resolved Hide resolved
@@ -136,7 +136,7 @@ def _get_availability_zones(region: str) -> pd.DataFrame:
with ux_utils.print_exception_no_traceback():
raise exceptions.AWSAzFetchingError(
region,
reason=exceptions.AWSAzFetchingError.Reason.CREDENTIAL
reason=exceptions.AWSAzFetchingError.Reason.AUTHENTICATION
Copy link
Member

Choose a reason for hiding this comment

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

How about just adhere to the name, AUTH_FAILURE?

sky/clouds/service_catalog/data_fetchers/fetch_aws.py Outdated Show resolved Hide resolved
@Michaelvll Michaelvll merged commit bce08e5 into master Aug 30, 2023
@Michaelvll Michaelvll deleted the fix-aws-without-describe-zones branch August 30, 2023 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sky check run failing on availability zones permission, but works via aws cli
3 participants