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

auth azure: support access token #3580

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

LiliDeng
Copy link
Collaborator

@LiliDeng LiliDeng commented Jan 3, 2025

managed identity doesn't work with windows container directly for this reason

find a way to auth azure with access token

@LiliDeng LiliDeng requested a review from squirrelsc as a code owner January 3, 2025 16:10
@LiliDeng LiliDeng force-pushed the lildeng/fix_12_25_003 branch 2 times, most recently from d5117f1 to 6e9a2fb Compare January 4, 2025 10:16
@LiliDeng
Copy link
Collaborator Author

LiliDeng commented Jan 5, 2025

During testing, found this token is working for managing azure resources, I am consulting azure python sdk how to handle this.

@squirrelsc
Copy link
Member

During testing, found this token is working for managing azure resources, I am consulting azure python sdk how to handle this.

What does it mean? Do you mean it fails in some scenarios, so you need have more tests or confirmation before merging? If so, please set it to Draft.

@LiliDeng LiliDeng force-pushed the lildeng/fix_12_25_003 branch 2 times, most recently from be05158 to f35f51f Compare January 7, 2025 14:38
@LiliDeng
Copy link
Collaborator Author

LiliDeng commented Jan 7, 2025

During testing, found this token is working for managing azure resources, I am consulting azure python sdk how to handle this.

What does it mean? Do you mean it fails in some scenarios, so you need have more tests or confirmation before merging? If so, please set it to Draft.

Never mind, this one works.

@LiliDeng LiliDeng force-pushed the lildeng/fix_12_25_003 branch from f35f51f to e0d591e Compare January 8, 2025 02:53
@LiliDeng LiliDeng force-pushed the lildeng/fix_12_25_003 branch from e0d591e to b7f2693 Compare January 8, 2025 05:44
@@ -1696,6 +1698,7 @@ def get_or_create_storage_container(
"""
Create a Azure Storage container if it does not exist.
"""
credential = get_static_access_token("AZURE_STORAGE_ACCESS_TOKEN") or credential
Copy link
Member

Choose a reason for hiding this comment

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

Why the env name variable is different? Is it defined by the staroge class or us?

Copy link
Collaborator Author

@LiliDeng LiliDeng Jan 8, 2025

Choose a reason for hiding this comment

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

it is defined by us, the tokens need to be generated based on different scopes.

Copy link
Member

Choose a reason for hiding this comment

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

  1. Is it possible to have one token with all scopes like *?
  2. Define schema to accept tokens, instead of env vars. All LISA vars should be defined in runbook.

Let me know, if it's hard to fulfill above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. I have confirmed with the maintainer of python sdk, he said we have to use different tokens for different scopes.
  2. Use different variables to accept the tokens?

Copy link
Member

Choose a reason for hiding this comment

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

  1. Use different variables to accept the tokens?

As I said, define in schema for them, not directly use env vars. After you defined schema, you can set values by LISA vars, and then assign LISA vars by env vars.

Like you defined access_token in PlatformSchema. Please define similar tokens for other Azure scopes.

@LiliDeng LiliDeng force-pushed the lildeng/fix_12_25_003 branch 3 times, most recently from 9c64209 to 7b135d5 Compare January 8, 2025 10:54
@LiliDeng LiliDeng force-pushed the lildeng/fix_12_25_003 branch from 7b135d5 to fde8170 Compare January 8, 2025 10:59
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