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

Adding custom token support #5272

Merged

Conversation

akashkeshari
Copy link
Contributor


This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
The precondition is to put your code inside this repository and upgrade the version in the pull request but do not modify src/index.json.

Changes in the PR:

  1. Adding custom AAD token support which does not require az login. User can directly pass the AAD access token to the CLI and execute the connectedk8s commands
  2. Adding a flag for overriding container log path in the connect and update command

Akash Keshari and others added 30 commits November 12, 2019 12:59
@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Oct 10, 2022

Just out of curiosity, may I ask is it safe to put tokens into the environment variables? + CLI Authentication expert @jiasli help review this PR

@akashkeshari
Copy link
Contributor Author

Just out of curiosity, may I ask is it safe to put tokens into the environment variables? + CLI Authentication expert @jiasli help review this PR

@zhoxing-ms , wanted to clarify one thing from our side. the custom token support in connectedk8s CLI will only be consumed by internal partners. They have a use case where they want to use the managed identity token for executing the connectedk8s operations. Also, in these scenarios, the connectedk8s CLI will be running inside a container and not on user's machine, so we're fine with setting it as environment variable

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Oct 10, 2022

the custom token support in connectedk8s CLI will only be consumed by internal partners. They have a use case where they want to use the managed identity token for executing the connectedk8s operations. Also, in these scenarios, the connectedk8s CLI will be running inside a container and not on user's machine, so we're fine with setting it as environment variable

@akashkeshari Got it, thanks~

@zhoxing-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Comment on lines +112 to +116
def signed_session(self, session=None):
session = session or requests.Session()
header = "{} {}".format('Bearer', self.access_token)
session.headers['Authorization'] = header
return session
Copy link
Member

Choose a reason for hiding this comment

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

This is only for Track 1 SDK. Most likely you won't need it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we require this since the 'GraphRbacManagementClient' still calls this function. When this function was not implemented , I saw the following error : ''AccessTokenCredential' object has no attribute 'signed_session''

Copy link
Member

Choose a reason for hiding this comment

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

GraphRbacManagementClient is the old Track 1 SDK for AD Graph. Consider using MS Graph client: https://github.com/Azure/azure-cli/blob/dev/doc/microsoft_graph_client.md

@akashkeshari akashkeshari marked this pull request as draft November 21, 2022 07:55
@akashkeshari akashkeshari marked this pull request as ready for review February 2, 2023 18:42
@zhoxing-ms zhoxing-ms merged commit e99e6b7 into Azure:main Feb 6, 2023
@azclibot
Copy link
Collaborator

azclibot commented Feb 6, 2023

[Release] Update index.json for extension [ connectedk8s ] : https://dev.azure.com/azclitools/internal/_build/results?buildId=32118&view=results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants