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

Don't promise that Config#context is what acts on exec:/auth-provider: #400

Merged
merged 2 commits into from
Mar 3, 2019

Conversation

cben
Copy link
Collaborator

@cben cben commented Mar 1, 2019

Before I release, I want to retract the promise I added in the last commit of #394 (0cbf5db), subsequently repeated in #396.

Motivation:

Following #392 (comment), I want to let Config and Config::Context expose the underlying data, because why not.
Ideally for that, Config#context would become a passive function, and then the active work of executing exec: command / auth-provider: network calls would be delayed until you call Config::Context#auth_options (?)
See below why that's not as simple, but I don't want to tie my hands now.

Plus this all is likely to change with auto auth renewal (#393).
Should figure that plan before making changes. Maybe reserving this freedom will help there too?

@motymichaely @timothysmith0609 @jeremywadsack @rhysm, do you think this is reasonable or am I inconveniencing people?

Why can't simply move the active work from Config#context to Config::Context#auth_options

  1. exec: may return client certificate instead of token, and that goes into ssl_options.
    I think we don't support that now, but should. UPDATE: adding in Add support for client-type ExecCredentials #453

  2. Might be problematic change with current usage patterns:
    Users that need several Client objects (for different api groups), may well be doing:

    context = config.context(...)  # once
    client1 = Kubeclient::Client.new(... context.auth_options, ...)
    client2 = Kubeclient::Client.new(... context.auth_options, ...)

    If I simply make auth_options do the work, it'll then obtain many tokens.

however, could have Context obtain auth at most once, but delayed until auth_options / ssl_options are called?

cben added 2 commits March 1, 2019 02:21
This was bad advice. Config::Context#api_version returns the apiVersion
of the kubeconfig file itself. It is almost certainly 'v1' but has nothing
to do with server API version you want use (depending on api group)!
Retracts promise made in 0cbf5db

Motivation:
I want to let Config and Config::Context expose the underlying data,
because why not.  Ideally for that, Config#context would become a
passive function, moving the work to Config::Context#auth_options (?)

There are open questions about this, and probably should figure out a
plan around auth renewal (ManageIQ#393) first.   Anyway, don't want my hands tied.
@jeremywadsack
Copy link
Contributor

To be honest I'm not really following the problem here. But from what I think I understand here are some comments:

Lazy auth sounds fine to me. It seems that would make renewal easier. Especially if the auth is checked on each API call and has a return if authorized? && !expired? guard.

Regarding this:

context = config.context(...)  # once
client1 = Kubeclient::Client.new(... context.auth_options, ...)
client2 = Kubeclient::Client.new(... context.auth_options, ...)

This has been a persistent struggle for me, because the config includes the api_version which is different depending on the client. (Discussed in #208 (comment))

I like this solution (#393 (comment)):

Conclusion: we want a new object responsible for auth (not giving it a name yet).
Many Client objects will be able to point to a single auth object.

And I agree that separating api_version from the configuration is a good idea — it's related to the client, not the .kube/config file or the authentication.

Are we conflating authentication and configuration here as well? Should those concepts be separated?

@cben
Copy link
Collaborator Author

cben commented Mar 1, 2019

Are we conflating authentication and configuration here as well? Should those concepts be separated?

Yes! That's the root issue. Config started life as give-me-some-fixed-options data object, then grew actively-obtain-auth powers that we hid inside the existing interface... There is no explicit "obtain auth now" message.

Now, in the future where Client will ask some "auth object" for valid auth, things are better, and the whole question when auth is obtained falls away (whenever necessary).
I'm deliberately being vague, I don't know yet if "auth object" will be some new class(es) or Config::Context itself...

I'm probably overthinking this. Renewing auth currently is impractically painful, and doubt anybody actually does it :)

@jeremywadsack
Copy link
Contributor

I would strongly support separating configuration and authentication. It was a struggle for me to understand/implement the GCP options because if the confusion between the two. I think that Config should be informational (unless there's a reason for configuration to reload in some environments) but mostly it holds the data. There should be a separate process/class that uses Config and handle authentication. In fact Config should define the strategy (perhaps a factory). At least that's my humble opinion. It's your gem. :)

@cben cben merged commit 49046a5 into ManageIQ:master Mar 3, 2019
@tsontario
Copy link
Contributor

I would strongly support separating configuration and authentication

I agree with this as well. I like the idea of separating out the actual config with the details of a given client instance (e.g. api_version). Lazy auth is also 👍 for me, too.

As everyone correctly points out, the issue now is how to actually model these elements. On an intuitive level, I would expect Config#context to not perform any authentication, and instead return the contents of the context object only. We could then move out our default handling for auth_options, etc. to a separate method, keeping the defaults we've written but allowing for finer-grained control if needed. The result of this could be a Client object that has a greater understanding of its authentication/authorization attributes. This is basically the same idea as @jeremywadsack elucidated above.

Currently, I don't see a way of implementing this without breaking the API :( but the tradeoff does seem to be worth it.

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.

3 participants