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

[Azure][Storage] Update Error response for accessing private container without access #3807

Conversation

landscapepainter
Copy link
Collaborator

@landscapepainter landscapepainter commented Aug 2, 2024

This updates the temporary solution implemented at #3791

Error was incorrectly handled when the user attempted to access a private container without access. This PR fixes with correct error response.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    • Manually try to use private container without access as a source from task.yaml raises:
sky.exceptions.StorageBucketGetError: Failed to access existing bucket 'test'. This is likely because it is a private bucket you do not have access to.
To fix:
  1. If you are trying to create a new bucket: use a different name.
  2. If you are trying to connect to an existing bucket: make sure your cloud credentials have access to it.
  • All smoke tests:
    • pytest tests/test_smoke.py::TestStorageWithCredentials --azure
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

@landscapepainter landscapepainter marked this pull request as draft August 2, 2024 05:29
@landscapepainter landscapepainter marked this pull request as ready for review August 4, 2024 20:35
@landscapepainter
Copy link
Collaborator Author

@romilbhardwaj This is ready for a look!

Comment on lines 2509 to 2518
is_private = (True if
container_client.get_container_properties().get(
'public_access', None) is None else False)
# when user attempts to use private container without
# access rights
if self.resource_group_name is None and is_private:
with ux_utils.print_exception_no_traceback():
raise exceptions.StorageBucketGetError(
_BUCKET_FAIL_TO_CONNECT_MESSAGE.format(
name=self.name))
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed offline, we may want to retain some checks similar to this for the case where the storage account is accessible but some of the containers in the account are private.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for catching this. Reverted the removal!

@landscapepainter
Copy link
Collaborator Author

@romilbhardwaj This is ready for another look!

Copy link
Collaborator

@romilbhardwaj romilbhardwaj left a comment

Choose a reason for hiding this comment

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

Thanks @landscapepainter! LGTM if tests pass.

@landscapepainter landscapepainter added this pull request to the merge queue Sep 5, 2024
Merged via the queue into skypilot-org:master with commit 455e065 Sep 5, 2024
20 checks passed
@landscapepainter landscapepainter deleted the azure-fix-access-private-container-wo-access branch September 5, 2024 05:10
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