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

export newTokenProvider function for library usage #371

Closed

Conversation

erhancagirici
Copy link

Exports the token/newTokenProvider() function for library usage.

In Crossplane Helm Provider, we are interested in authenticating to AKS non-interactively when supplied with AKS kubeconfigs that has execProvider section relying on kubelogin and user interaction.
Using the CLI is currently not an option and we aim to handle this programmatically in go code. We checked the execCredentialPlugin.Do(), however it outputs to stdout with no chance to configure and ultimately we are interested in obtaining the token provider only, not the converted kubeconfig.

Feedback is appreciated, thanks!

Signed-off-by: Erhan Cagirici <erhan@upbound.io>
@@ -15,7 +15,7 @@ type TokenProvider interface {
Token() (adal.Token, error)
}

func newTokenProvider(o *Options) (TokenProvider, error) {
func NewTokenProvider(o *Options) (TokenProvider, error) {
Copy link
Member

Choose a reason for hiding this comment

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

shall we add some docs to describe these public methods / structs / interfaces?

Copy link
Member

Choose a reason for hiding this comment

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

I also have a concern on the options exposed. Things like this:

IsLegacy bool
Timeout time.Duration
TokenCacheDir string
tokenCacheFile string
might be more for internal usage.

Copy link

Choose a reason for hiding this comment

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

This is how we are planning to use and it works!

I believe it is fair to expect users not using options that they are not interested in / don't care, or could they be hidden with follow up PRs if you really want to hide them 🤔

Copy link
Member

Choose a reason for hiding this comment

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

So instead of exposing the GetToken from pkg directly, how about we convert to a structure like this:

  1. rename pkg to internal, this can avoid library usage to import the internal implementation.
  2. export a minimum GetTokenOptions struct at the package root level.
  3. export the GetToken function at the package root level, and let client use like this: kubelogin.GetToken(kubelogin.GetTokenOptions{}). In the GetToken, we convert the exported GetTokenOptions to the internal options object and invoke the internal GetToken call.
  4. future library usage goes to package root level, cli usage stays the same.

kubelogin was not designed to be consumed as library at this first place, so I think it would be better to hide most of the things first so we can avoid unexpected dependencies.

@weinong @turkenh @erhancagirici WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this proposal to give us a clean separation between library interface and cli interface.

Copy link
Member

Choose a reason for hiding this comment

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

hey all, I created a proposal for above changes: #373 @turkenh / @erhancagirici Can you take a look and let us know your thoughts? If no blocker, we will start the implementations and migrations, thanks!

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3e83c88) 64.77% compared to head (48fc079) 64.77%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #371   +/-   ##
=======================================
  Coverage   64.77%   64.77%           
=======================================
  Files          23       23           
  Lines        1766     1766           
=======================================
  Hits         1144     1144           
  Misses        549      549           
  Partials       73       73           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bcho
Copy link
Member

bcho commented Dec 20, 2023

Hi, we have merged the library changes to the master branch. I have submitted a pr for updating the usage in crossplane side: crossplane-contrib/provider-helm#207 Let's move the discussion there, hope we can finalize the required setup there. I will close this PR as it doesn't apply any more.

@bcho bcho closed this Dec 20, 2023
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.

5 participants