Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: import kubelogin library mode #207
base: main
Are you sure you want to change the base?
feat: import kubelogin library mode #207
Changes from all commits
52daba5046e36ba41be6fFile filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erhancagirici I am less sure how crossplane consumes this rest config: are we expecting the input config is returned from an AAD enabled AKS cluster? Because using kubelogin's SP login method, it's required to have
server-idset.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcho
yes, this codepath here is executed when the user specifies an
identitysection in the provider configuration, alongside the cluster credentials (kubeconfig). Specifying theidentitysection (credentials for the SP) assumes user's intention that the kubeconfig cannot work standalone and this is an Azure AAD enabled AKS cluster kubeconfig withexecProvidersection. If user does not specify anyidentityblock in the provider configuration, then we assume that the kubeconfig can work standalone, therefore do not visit this codepath at all, and do not wrap the rest config i.e no kubelogin invocation.So, it is OK to assume that we have a AAD-enabled AKS kubeconfig, hence with
execProvidersection and a--server-idpresent here.Regarding the general intention here: we aim to non-interactively authenticate to the k8s cluster, given a kubeconfig with interactive login. Therefore we would like to parse
execProvidersection that haskubelogincommand and arguments, and build the necessary list of options for SP auth (we currently support only SP auth ). This I think, in a way, corresponds to manually handling thekubelogin convert. Then obtain a token to authenticate and remove theexecProvideraltogether.Actually, my initial intention was to offload the argument parsing to
kubeloginitself somehow, so that we don't maintain kubelogin flags here and we obtain CLI options directly. Then extract/reuse some likeserver-id,environmentto build new options for SP auth, combining them with the externally-provided SP credentials.AFAIU, kubelogin library mode changes does not expose CLI flags, so we need to still handle the CLI options -> library options parsing + conversions (which is fine btw).
So this is kind of an hybrid use case, where we would like to still
kubeloginwith CLI params but as a library. It is a matter of at which level we integrate to kubelogin.Thanks a ton for working on this, the kubelogin changes are very much appreciated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for your review, I agree with you on the flags parsing pain here. I created an issue to track it: Azure/kubelogin#394 and will see how to expose it in library mode. We can follow up after the change in kubelogin side.