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

Instance creation and configuration api should be refactored #121

Open
kke opened this issue May 14, 2019 · 5 comments
Open

Instance creation and configuration api should be refactored #121

kke opened this issue May 14, 2019 · 5 comments
Labels

Comments

@kke
Copy link
Contributor

kke commented May 14, 2019

Oddities / annoyances:

  • K8s:Client.new should probably not require transport as an argument.
  • K8s::Client.config takes a config and passes it to Transport.config. I think it's a misleading method name.
  • K8s::Transport.config same as above, returns an instance of K8s::Transport, I would expect it to return a config instead of accepting one.
  • Transport.new takes auth token/username/password, but there's K8s::Config::UserAuthProvider, maybe Transport.new should actually take an instance of some kind of Authentication class
  • Transport is a mixture of abstraction for Excon and generic methods.
  • The ! methods such as api_groups! and api_resources! are unintuitive. Perhaps api_groups(force_refresh: true) or something would be better.
  • Method visibility in general has been ignored completely, there are no private methods. (I didn't find any methods that should be private except maybe the Util mixin should use module_function)
  • K8s::Resource.from_files reads a single file or all files in a directory. I would expect it to take an array of paths or IOs.
  • There's a million methods for creating a client instance.
  • The client is a bit cumbersome to mock/stub because of the long method chains. It should probably have some kind of test mode / mock transport.

Other than that, it's actually pretty great.

@kke kke added the chore label May 14, 2019
@kke
Copy link
Contributor Author

kke commented May 14, 2019

Also:

Do we actually need the Dry::Struct / Dry::Types for anything? Couldn't everything just be K8s::Resource( which is < RecursiveOpenStruct).

@kke
Copy link
Contributor Author

kke commented Aug 8, 2019

Trying to think how it should work.

K8s::Config

This should actually be something like K8s::KubeConfig. It's not a configuration for K8s::Client. It's for parsing kubeconfig files and finding the necessary values for constructing options for a transport.

K8s::Transport

  • It's supposed to be an abstraction. It should be generic. The Excon backend should perhaps be K8s::Transport::Excon. While at it, fix Transport passes extra params to Excon #57
  • K8s::Transport.config(<K8s::Config>) should be K8s::KubeConfig.new(...).context.transport
  • K8s::Transport.token_from_auth_provider(<K8s::Config::AuthProvider>) and K8s::Transport.token_from_exec(<K8s::Config::UserExec>) should come automatically from the above mentioned K8s::KubeConfig.new(...).context.transport
  • K8s::Transport#version (and K8s::Client.version) is the same as K8s::Client.new.api('version'). They should be removed, perhaps replaced with K8s::Client#api_version` which would return a string, not a resource.
  • K8s::Transport.new options should be something like: (server:, token: nil, ca: nil, username: nil, password: nil, client_cert: nil, client_key: nil, client_key_pass: nil, insecure_tls_skip_verify: false, logger: nil)

K8s::Stack

Should maybe be in another gem, such as k8s-client-stack as it's sort of custom.

K8s::Client

  • K8s::Client.config and K8s::Client.in_cluster_config should be dropped
  • K8s::Client.new should take (kubeconfig = nil, transport: nil, namespace: nil, insecure_tls_skip_verify: false, logger: nil, **transport_options). If transport is set, non-empty transport_options should raise. If transport is nil and transport_options is empty, configuration autodiscovery (the current K8s::Client.autoconfig) should be attempted with `logger: logger, insecure_skip_tls_verify: insecure_skip_tls_verify). If autoconfig fails, it should raise.

The user should mostly have to do:

client = K8s::Client.new
client.api('version').gitVersion
client = K8s::Client.new('~/.kube/config', context: 'foo@foofoo')
client = K8s::Client.new(server: 'https://localhost:6443', ca: 'AbCFCFGH==', token: 'abcd')
begin 
  client = K8s::Client.new
rescue K8s::Error::SSL
  client = K8s::Client.new(insecure_skip_tls_verify: true)
end

The user should not have to do anything like:

K8s::Client.new(transport: K8s::Transport.config(K8s::Config.load_file('/etc/kubernetes/admin.conf')))

K8s::Client.config(K8s::Config.load_file('/etc/kubernetes/admin.conf'))

The interface to create a client instance should be K8s::Client.new. Optionally, a shortcut for it could be K8s.client:

K8s.client(insecure_skip_tls_verify: true) do |client|
  puts client.api('version').gitVersion
end

TODO: try to split this into bite sized issues

@kke kke changed the title The API is a bit fragmented Instance creation and configuration api should be refactored Aug 8, 2019
@cben
Copy link

cben commented Aug 8, 2019

Some points to remember About token_from_auth_provider, token_from_exec:

  1. In some cases it's bearer token, in some it's a TLS client cert. At least exec provider is generic, you don't know ahead of time which you'll get. So shouldnt have token in name, and return type should admit both.
    Lately I'm thinking whether auth providers can be modeled cleanly as a KubeConfig -> KubeConfig transform?

  2. they might expire and require renewal. Thus user should not be involved with obtaining a token once and passing it to Client, but Client could call a method on each request to obtain fresh credentials, renewing if necessary.
    This will become more pressing with Transition ServiceAccount admission controller to improved service account token volumes kubernetes/kubernetes#70679.

  3. Worse, some providers don't have expiration dates, so one gets an auth expired error and should renew and retry 😞

  4. Is it useful to create multiple Clients with same config, sharing some single object that'll obtain / renew auth?
    If yes, this might need Client.new(SomethingDoingAuth.nSomethingDoingAuth.new(...)) rather simple Client.new.Client.new. (but it can be optional advanced use case)

See also notes on ManageIQ/kubeclient#393

@kke
Copy link
Contributor Author

kke commented Aug 9, 2019

Perhaps the authentication modules should be implemented as some sort of http client library middleware things.

@kke
Copy link
Contributor Author

kke commented Aug 9, 2019

Also, adding to #121 (comment)

  • There's no sane way to use Transport.new that would actually work outside of specs. All of the cert configuration is done in Transport.config which builds the options hash.

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

No branches or pull requests

2 participants