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

Relax required permissions for custom VPC (GCP) #2944

Merged
merged 14 commits into from
Jan 5, 2024

Conversation

davidwagnerkc
Copy link
Contributor

We are unable to assign firewall create and delete permissions to the SkyPilot service account due to our security policies. This relaxes the required permissions so that anyone bringing their own VPC with a properly setup firewall is not required to have those sensitive permissions assigned.

There are a few documentation and comment changes along these lines. In addition to this added storage.objects.update to GCS minimal permissions documentation as we ran into an issue overwriting files without it.

Tested sky check before and after. As well as sky launch.

Before the change, tested on new instance at tip of main earlier today 1.4 a70057c

Checking credentials to enable clouds for SkyPilot.
  GCP: disabled
    Reason: The following permissions are not enabled for the current GCP identity (skypilot-v1@crypto-lodge-387815.iam.gserviceaccount.com [project_id=crypto-lodge-387815]):
    {'compute.firewalls.delete', 'compute.firewalls.create'}
    For more details, visit: https://skypilot.readthedocs.io/en/latest/cloud-setup/cloud-permissions/gcp.html

After the change after updating my fork to a70057c:

Checking credentials to enable clouds for SkyPilot.
  GCP: enabled

I ran into an issue with sky launch where the proper permission were not in place and provisioning failed in trying to create and change IAM permissions which we do not give the service account permissions to do. This turned out that while sky check would read permissions from sky/skylet/providers/gcp/constants.py where my change was made sky launch was reading them from sky/provision/gcp/constants.py. I copy and pasted this file, but ideally this duplication of constants would be removed in the code base. I am not sure if there are plans to do so, but I did not attempt a unification in this PR.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • 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

Copy link
Collaborator

@Michaelvll Michaelvll 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 submitting the PR @davidwagnerkc! This is awesome and unblock the important use case where the user's VPC already has the firewall setup. : )
We are aware of the duplication of the permission check in the providers and it will be fixed in #2943. The code looks good to me (we can refactor the permission list construction out of constants.py in another PR).

docs/source/cloud-setup/cloud-permissions/gcp.rst Outdated Show resolved Hide resolved
davidwagnerkc and others added 2 commits January 4, 2024 20:44
Co-authored-by: Zhanghao Wu <zhanghao.wu@outlook.com>
Co-authored-by: Zhanghao Wu <zhanghao.wu@outlook.com>
@davidwagnerkc
Copy link
Contributor Author

@Michaelvll I've never actually seen the GitHub suggestion with commit changes button, but cool feature. I went ahead and committed the two suggestions you had, I hope that is okay. If there is anything else I can do here just let me know.

@Michaelvll Michaelvll merged commit d4e0f5b into skypilot-org:master Jan 5, 2024
19 checks passed
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.

2 participants