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

Support multiple apis, detect kind/apiVersion #241

Closed
wants to merge 14 commits into from
Closed

Support multiple apis, detect kind/apiVersion #241

wants to merge 14 commits into from

Conversation

danajp
Copy link

@danajp danajp commented Mar 28, 2017

EDIT: overview of alternatives: #208 (comment)

Fixes #208

This is WIP. There are pretty much no tests. I'm pretty sure it doesn't actually work. At this point I'm just looking for feedback on the approach I'm taking.

This PR does a couple things:

  1. It supports all the kubernetes apiserver apis in a single Kubeclient::Client if you do not pass an api version to the constructor.
    We can support backwards compatibility by doing the following:

    • if a single api version is passed to the constructor, we'll only use that api
    • if no api version is passed, we'll discover and use all the apis
  2. It adds new create, update, patch and delete methods which accept a Kubeclient::Resource instance with apiVersion and kind keys and uses those to determine which api group and resource to use to create/update/delete the entity.

To get there, the api discovery code has been refactored to support this. The new classes are

  • Kubeclient::Common::Apis - handles discovery of the api groups
  • Kubeclient::Common::Api - a single api group, handles discovery of entities in that group
  • Kubeclient::Common::Entity - a single api entity. This replaces the OpenStructs that were used to store entities before

@danajp danajp changed the title Kind api version from entity config Support multiple apis, detect kind/apiVersion Mar 28, 2017
@simon3z
Copy link
Collaborator

simon3z commented Mar 28, 2017

@moolitayer @cben please review.

@moolitayer
Copy link
Collaborator

@danajp can you please post an example showing new/modified interfaces this PR will expose

@danajp
Copy link
Author

danajp commented Mar 29, 2017

Sure, here you go @moolitayer, I'm not sure all of these actually work yet.

config = Kubeclient::Config.read('/home/dana/.kube/config')

# --- constructor changes --------------------------------------------
# older interface, supports /api/v1 only by default
core_client = Kubeclient::Client.new(
  config.context.api_endpoint,
  :ssl_options => config.context.ssl_options,
  :auth_options => config.context.auth_options
)

# older interface, supports only /apis/batch/v1
batch_client = Kubeclient::Client.new(
  config.context.api_endpoint + '/apis',
  'batch/v1',
  :ssl_options => config.context.ssl_options,
  :auth_options => config.context.auth_options
)

# new interface, supports all apis in a single instance
all_client = Kubeclient::Client.new(
  config.context.api_endpoint,
  nil,
  :ssl_options => config.context.ssl_options,
  :auth_options => config.context.auth_options
)

# --- entity CRUD changes --------------------------------------------
service = Kubeclient::Resource.new
service.kind = 'Service'
service.apiVersion = 'v1'
service.metadata = {}
service.metadata.name = 'foo'
service.metadata.namespace = 'default'
service.spec = {}
service.spec.ports = [
  { :port => 3000, :targetPort => 'http-server', :protocol => 'TCP' }
]

# old api unchanged
all_client.create_service(service)
all_client.update_service(service)
all_client.patch_service(service.metadata.name, service, service.metadata.namespace)
all_client.delete_service(service.metadata.name, service.metadata.namespace)

# new api
all_client.create(service)
all_client.update(service)
all_client.patch(service)
all_client.delete(service)

@moolitayer
Copy link
Collaborator

I've been seeing lots of code that would benefit from having an interface addition of:

client.create(resource|json)
client.update(resource|json)
client.patch(resource|json)
client.delete(resource|json)

@cben @abonas @simon3z would you tentatively agree to this as an interface ADDITION ?

If so maybe we can start off with a PR doing only that?
I'm asking since some other issues in this PR would need additional hashing out, and empirically shorter PRs get reviewed and merged faster.

@abonas
Copy link
Member

abonas commented Apr 23, 2017

Some comments from briefly looking at this PR:

  1. I have concerns regarding having now more than one way of doing the same thing (or very similar things) in the client.
  2. the data required in the entities that will be fed to CRUD methods will now differ (requiring api version in the new method, for example), but it will remain the exact same objects/entities - it can be confusing to the gem users. Also, if the user of this gem doesn't persist (like miq) the api version, then this user is limited to the old set of methods. Trying to "feed" the entities to the new CRUD methods will lead to exceptions, and won't be very intuitive.
  3. Naming - Apis is not a word :). ApiGroup/ApiGroups imo sounds better. Also, API is an abbreviation, so it's probably better to capitalize it. (Api vs API)

@danajp
Copy link
Author

danajp commented Sep 10, 2018

Closing this PR as we've pretty much abandoned this approach in our project in favor of something similar to #332.

We've got a wrapper around kubeclient that implements a method like this:

def get_client(api_version, kind)
  # ...
end

And then 99% of the time, we've already got a Resource instance that we use to generate the method names:

resource = Kubeclient::Resource.new(:kind => "Service", :apiVersion => "v1")
# ...
KubeclientWrapper
  .get_client(resource.apiVersion, resource.kind)
  .send("create_#{resource.kind.underscore}")

@danajp danajp closed this Sep 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow different resources to use different apiVersion
5 participants