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

Provide a way to write extensions without a compile dependency on kubernetes-client #3845

Closed
shawkins opened this issue Feb 11, 2022 · 5 comments

Comments

@shawkins
Copy link
Contributor

shawkins commented Feb 11, 2022

Is your task related to a problem? Please describe

There are a couple of follow-on issues after the split:
extensions use -client internal stuff, and in general have a compile dependency on the client. Affected classes:
./service-catalog/client/src/main/java/io/fabric8/servicecatalog/client/internal/ClusterServiceBrokerOperationsImpl.java
./service-catalog/client/src/main/java/io/fabric8/servicecatalog/client/internal/ClusterServiceClassOperationsImpl.java
./service-catalog/client/src/main/java/io/fabric8/servicecatalog/client/internal/ClusterServicePlanOperationsImpl.java
./service-catalog/client/src/main/java/io/fabric8/servicecatalog/client/internal/ServiceBindingOperationsImpl.java
./service-catalog/client/src/main/java/io/fabric8/servicecatalog/client/internal/ServiceInstanceOperationsImpl.java
./volumesnapshot/client/src/main/java/io/fabric8/volumesnapshot/client/internal/VolumeSnapshotClassOperationsImpl.java
./volumesnapshot/client/src/main/java/io/fabric8/volumesnapshot/client/internal/VolumeSnapshotContentOperationsImpl.java
./volumesnapshot/client/src/main/java/io/fabric8/volumesnapshot/client/internal/VolumeSnapshotOperationsImpl.java

Describe the solution you'd like

The extensions are making use of HasMetadataOperation, but it requires a lot of boiler plate.

VolumeSnapshotResource and VolumeSnapshotContentResource are just marker interfaces for later use. The rest seem to just mix-in behavior, which could be handled by a proxy based framework. The only part of that which isn't straight forward is the withIem on the context. As far as I can see all of the calls using withItem(null) aren't actually needed. ClusterServicePlanOperationsImpl.instantiateAnd does create a ServiceInstanceResource scoped to that particular item. There is no public way to do that.

Describe alternatives you've considered

No response

Additional context

No response

@shawkins
Copy link
Contributor Author

shawkins commented Feb 11, 2022

Wrt to the extensions there are three options:

  • just let them depend on the client for now
  • split them as well. Their client code is using BaseClient and Handlers - this requires few code changes, but introduces new modules
  • make them work off of just the api. After looking into this more, it's unfortunately a lot of work. Being able to extend HasMetadataOperation and directly use the context grant a lot of freedom to what you can do with extensions. It seems possible though to make this work.

With the first of the second options we could reconsider if the client dsl.base should be moved to dsl.internal in #3844 as those classes are needed by the extension implementations.

@manusa @rohanKanojia do you have a preference?

@shawkins
Copy link
Contributor Author

To explain the third option a little more, please see https://github.com/fabric8io/kubernetes-client/compare/master...shawkins:extension-api?expand=1

Since proxies seem like a bad idea, you would need to at a minimum introduce base ClientAdapter and ResourceAdapter classes, provide a way to register custom ResourceAdapters via the ExtensionAdapter, and make the KubernetesClient or just Client highly available.

The possible pr above is not complete, but it does show what this roughly looks like. Instead of a BaseClient, extensions are based off of the ClientAdapter https://github.com/fabric8io/kubernetes-client/compare/master...shawkins:extension-api?expand=1 and register their handlers via a method on ExtensionAdapter https://github.com/fabric8io/kubernetes-client/compare/master...shawkins:extension-api?expand=1#diff-4c7752f30e81306dade39ca6e74eb9de194ec68f3085dcc6163bc6af703d71b6R58 - the backing logic uses ResourcedHasMetadataOperation to convert those ResourceAdapters into HasMetadataOperations, which the handler logic already understands. This cuts out on a lot of the boiler plate and internal constructs used by the clients and resource/operation classes. I do see that the extension / handler logic isn't quite where it needs to be as we can load extensions from several class loaders - but I'm not yet registering the Handlers as a side effect and there's no removal of handlers when an extension is removed.

There are two new methods needed on Client - https://github.com/fabric8io/kubernetes-client/compare/master...shawkins:extension-api?expand=1#diff-d919f06771631aa4daf8bb36a88f11e9d6becfe9e917355a5cf45e6a3f808d7fR94 a two argument resources, which is the same as the current KubernetesClient.resources. And a three argument one, which allows to specify the Resource subclass expected (that will also be straight forward to implement - it would be good if it did type checking of expected Resource subclass).

This means that ClientAdapter subclasses will call one on those resource methods when providing an api hook - https://github.com/fabric8io/kubernetes-client/compare/master...shawkins:extension-api?expand=1#diff-f1c5df836493e8e17da0294febe096975fcea06b09f1dfed66749c93f084a023R66

And Resource/Operation customizations can call those resource methods, or use adapt to get to api calls https://github.com/fabric8io/kubernetes-client/compare/master...shawkins:extension-api?expand=1#diff-0c1cdf2689ac477b41f476cf50f20f9f0a5c2f44ea420ea5a8579dbe56a4111cR41

The last bit of missing functionality is a withItem call mentioned above. That could be implemented today with appropriate namespace handling then a call to load(Serialization.toYaml(item)) - so it seems best to actually add a withItem either to Loadable or similar location.

@shawkins
Copy link
Contributor Author

shawkins commented Feb 14, 2022

Sorry, I've updated the branch so that it's actually working, so the line numbers are not correct in the previous comment.

@shawkins
Copy link
Contributor Author

@manusa in the PR I'm proposing to modify the extension modules to have a compile dependency on -api and a runtime dependency on kubernetes-client. From a user perspective this means that if you for example have an existing dependency to camel-k-client, that everything will keep working for you - you don't need to further modify your dependencies. The assumptions are:

  • and you are using the constructors on DefaultCamelKClient
  • you are unlikely to also need any of the direct access in kubernetes-client

If you are instead using DefaultKubernetesClient then adapting to CamelKClient, you'll need a compile dependency on kubernetes-client (or to change your code to use the DefaultKubernetesClient constructors, or whatever the new building mechanism is).

So if we want to discount the possibility that we're allowing some users to not have to further manipulate their dependencies, or we just want consistency with main -api and -client, then it's fine to change this runtime dependency into just a test dependency and document that they'll need add an appropriately scoped dependency to kubernetes-client if they don't already have one.

@shawkins
Copy link
Contributor Author

Spawned #3866 as a separate issue to leave this one to represent the extension development.

shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 16, 2022
also removing extra ApiVersionUtil classes
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 16, 2022
also deprecating extra ApiVersionUtil classes
moving resource interfaces in service-catalog client from internal to
dsl
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 16, 2022
Kubernetes.resource now returns NamespaceableResource

also adds sanity checks useful for fabric8io#3859 as well
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 16, 2022
Kubernetes.resource now returns NamespaceableResource

also adds sanity checks useful for fabric8io#3859 as well
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 16, 2022
Kubernetes.resource now returns NamespaceableResource

also adds sanity checks useful for fabric8io#3859 as well
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 16, 2022
Kubernetes.resource now returns NamespaceableResource

also adds sanity checks useful for fabric8io#3859 as well
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 17, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 17, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 17, 2022
…n logic

also various cleanups and adding some migration docs
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 17, 2022
…n logic

also various cleanups and adding some migration docs
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 17, 2022
…n logic

also various cleanups and adding some migration docs
@shawkins shawkins changed the title Other issues with splitting api and impl Provide a way to write extensions without a compile dependency on kubernetes-client Feb 17, 2022
@manusa manusa closed this as completed in f6927d7 Mar 2, 2022
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

No branches or pull requests

1 participant