Extension development using only api and resource/resourceList implementations #3858

shawkins · 2022-02-14T15:55:58Z

Description

See #3845 - this shows what the ServiceCatalog and Camel-K extensions could look like written only against the api.

The core of the changes are:

to provide Client and Resource adapters to extend from rather than BaseClient and HasMetadataOperation
to make the client, kubernetes client, and namespaced client highly available
add resources and another variant to Client that allows you to specify the expected Resource class type
to add a withItem method (not properly represented yet, it's just tacked in for now) - in several places I'm inferring this was to be called, for example https://github.com/fabric8io/kubernetes-client/pull/3858/files#diff-0c1cdf2689ac477b41f476cf50f20f9f0a5c2f44ea420ea5a8579dbe56a4111cR69 seems to be missing associating the item with the context.

These changes by themselves are not breaking as you could still use a dependency on kubernetes-client to use the existing functionality.

There are some additional cleanup to be pursued:

how we are holding / cleaning up references to handlers

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

also deprecating extra ApiVersionUtil classes moving resource interfaces in service-catalog client from internal to dsl

shawkins · 2022-02-16T18:45:09Z

The next commit refines the initial one:

introduces ExtensibleResource, which allows the ResourceAdapter to easily wrap values that would otherwise not be typed appropriately. The alternative is to bake all of this logic into HasMetadataOperation - which I don't want to do given how overloaded it currently is.

It also makes an initial stab at consolidating the resource (related to #3407) / withItem logic - I'm proposing to change KubernetesClient.resource from returning NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicable to a new interface NamespaceableResource (which I'm proposing to not be Namespaceable as explained in one of the comments). NamespaceableResourceAdapter encapsulates the behavior this exposes. The methods lost from NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicable where already deprecated, so this should be fine for a major version change. ResourceTest has been updated to use an intermediate resource variable as was suggested in the deprecation comment. I did not take this logic fully through yet - the resource list behavior is still based upon the legacy logic - but once that is changed we'll be able to fully remove several interfaces and supporting classes. With those changes, it was helpful to also pull in validation changes mentioned from #3859. @manusa @rohanKanojia let me know if you have any qualms with this. ResourceIT shows the full replacement for resource.deletingExisting().createOrReplace(); as resource.delete(); resource.waitUntilCondition(Objects::isNull...); resource.create(); If this seems like too long of a replacement sequence, there are a couple of options:

un-deprecate and add Recreateable to Resource
direct users to the DeleteAndCreateHelper.
don't consider this a built-in operation - on my first look over the logic when I deprecated Recreateable that seemed sensible - that is you should be using just createOrReplace most of the time and not trying to delete first. If you do require a delete, then there should be a built-in blocking delete, after which you'd call create. A built-in blocking delete would try to do better than Objects::isNull - if the object doesn't have a deletion timestamp, we can throw an exception to exit early.

The withItem method has not been added to an interface. There are a couple of options - add Itemable, or alter Loadable to be Loadable<R, T> to support R withItem. There is also the similarity with the resource entry method - but that one has additional namespace side effects. What makes sense?

Kubernetes.resource now returns NamespaceableResource also adds sanity checks useful for fabric8io#3859 as well

shawkins · 2022-02-17T14:02:26Z

The next commit more fully realizes the resource changes for #3407 - it had seemed like I need to get Resource usage straightened out prior to proceeding with #3845, but now it looks like it's more separable than I thought. In any case I'll go back to that now and clean up the Handler logic. I'll leave switching over the other extension until a later pr.

shawkins · 2022-02-17T16:04:26Z

Rather than make this pr larger, I've added #3875 #3876 to address after this.

The only thing this pr does not do, which would be a nice to have, is for the three argument resources call to validate the Resource class subtype that will eventually be used - however this requires a lot of bookkeeping of the class type, so for now I'm okay with just letting later class cast exceptions be seen. It's not expected that users will call that method directly - they should instead be using other dsl methods.

…n logic also various cleanups and adding some migration docs

shawkins · 2022-02-17T17:07:50Z

@iocanel from my perspective this is ready for review - do you want to leave the WIP on until you complete a review?

shawkins · 2022-02-21T14:53:23Z

I have the changes ready that implement the previous comment (and related cleanups) should we want to go that direction: shawkins#6

shawkins · 2022-02-22T18:35:55Z

@manusa this should be ready for review again. after the merge from master this also includes the removal of KubernetesClient.lists

After this is committed I'll open a pr with the work from shawkins#6 and the additional changes to namespace handling.

extension-api # Conflicts: # extensions/service-catalog/client/src/main/java/io/fabric8/servicecatalog/client/internal/ClusterServiceBrokerOperationsImpl.java # extensions/service-catalog/client/src/main/java/io/fabric8/servicecatalog/client/internal/ClusterServiceClassOperationsImpl.java # extensions/service-catalog/client/src/main/java/io/fabric8/servicecatalog/client/internal/ClusterServicePlanOperationsImpl.java # extensions/service-catalog/client/src/main/java/io/fabric8/servicecatalog/client/internal/ServiceBindingOperationsImpl.java # extensions/service-catalog/client/src/main/java/io/fabric8/servicecatalog/client/internal/ServiceInstanceOperationsImpl.java # extensions/volumesnapshot/client/src/main/java/io/fabric8/volumesnapshot/client/internal/VolumeSnapshotClassOperationsImpl.java # extensions/volumesnapshot/client/src/main/java/io/fabric8/volumesnapshot/client/internal/VolumeSnapshotContentOperationsImpl.java # extensions/volumesnapshot/client/src/main/java/io/fabric8/volumesnapshot/client/internal/VolumeSnapshotOperationsImpl.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/BaseClient.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/BaseOperation.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/GenericKubernetesResourceOperationsImpl.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/osgi/ManagedKubernetesClient.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/internal/DeleteAndCreateHelper.java # kubernetes-client/src/test/java/io/fabric8/kubernetes/client/utils/internal/DeleteAndCreateHelperTest.java # kubernetes-client/src/test/java/io/fabric8/kubernetes/client/utils/internal/PodOperationUtilTest.java also removing KubernetesClient.lists

shawkins · 2022-02-22T22:03:56Z

Thought it might be better just to show the totality of changes, this commit should fail on things that expect an exception on a namespace mismatch. I'll correct that and add migration notes in another commit.

into extension-api # Conflicts: # kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/dsl/ListVisitFromServerGetDeleteRecreateWaitApplicable.java # kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/extended/run/RunOperationsTest.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/BaseKubernetesClient.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/KubernetesListOperationsImpl.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/batch/v1beta1/CronJobOperationsImpl.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/core/v1/BindingOperationsImpl.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/core/v1/ComponentStatusOperationsImpl.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/extended/run/RunOperationsImpl.java # kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/KubernetesListTest.java # openshift-client/src/main/java/io/fabric8/openshift/client/DefaultOpenShiftClient.java also removing generickuberntesclient and applying the same namespacing logic to all items

extension-api # Conflicts: # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/DefaultKubernetesClient.java

shawkins · 2022-02-24T00:23:57Z

The test failures https://github.com/fabric8io/kubernetes-client/runs/5300256746?check_suite_focus=true#step:6:680 are being caused by a new sequence of events such that the wrong apiGroupName is being used. Essentially OperationContext.setApiGroupName is implemented by looking at the item first if non null, then will take the passed in value. The test resources, like the BuildConfig for BuildConfigIT have apiVersion: "v1" - so if you create the context by first associating the invalid item then doing any other operation, it will revert to a null apiGroupName.

My workaround in the next commit is to call updateApiVersion(item) in withItem, but it does look like some of this needs changed - because it appears in the context that the item is the primary place for the apiGroupName, then in the Operation logic it appears that the context (typically without an item the value is coming from the class/rdc or is hardcoded in the XXXOperationsImpl constructor) is primary.

That commit was updated to include an additional change - RouteIT was not setting the currentNamespace, so it was effectively calling inNamespace(null). Prior to these changes it appears that meant "use the default namespace", with these changes it's effectively a usage error meaning "in any namespace", but returning a NonNamespaceOperation. @manusa @rohanKanojia are there any expectations when calling inNamespace(null)?

also updating the test to set the namespace

manusa · 2022-03-01T12:11:02Z

...etes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/KubernetesResourceUtil.java

+  public static void setNamespace(HasMetadata entity, String namespace) {
+    if (entity != null) {
+      ObjectMeta metadata = entity.getMetadata();
+      if (metadata != null) {
+        metadata.setNamespace(namespace);
+      }
+    }
+  }


I haven't checked the purpose of this util.
But was wondering if it might be interesting to initialize metadata instead of just ignoring if it's null:

Suggested change

public static void setNamespace(HasMetadata entity, String namespace) {

if (entity != null) {

ObjectMeta metadata = entity.getMetadata();

if (metadata != null) {

metadata.setNamespace(namespace);

}

}

}

public static void setNamespace(HasMetadata entity, String namespace) {

if (entity != null) {

if (entity.getMetadata() == null) {

entity.setMetadata(new ObjectMeta());

}

entity.getMetadata().setNamespace(namespace);

}

}

This was for consistency with setResourceVersion and in the usage with this pr if there is no objectmetadata the scenario will fail regardless as there would then be no name. However I'm open to anything - you could also consider moving methods like this to HasMetadata under #3625 which was a follow-on to the convenience methods added for finalizers.

It makes sense to me that in scope of this class, both methods should initialize the entity's metadata if necessary.
Maybe we can consider this as a future change.

manusa · 2022-03-01T13:38:49Z

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/OperationSupport.java

+  protected <T> T correctNamespace(T item) {
+    if (!isResourceNamespaced() || this.context.isDefaultNamespace() || !(item instanceof HasMetadata)) {
+      return item;
+    }
+    String itemNs = KubernetesResourceUtil.getNamespace((HasMetadata)item);
+
+    if (Utils.isNotNullOrEmpty(namespace) && Utils.isNotNullOrEmpty(itemNs) && !namespace.equals(itemNs)) {
+      item = Serialization.clone(item);
+      KubernetesResourceUtil.setNamespace((HasMetadata)item, namespace);
+    }
+    return item;
+  }


Not sure if we have an explicit test for this, but full coverage and documentation of conditions should be provided since it's determinant for the Namespace resolution logic.

I'm not sure if this PR also includes the default namespace fallback logic discussed in some meetings.

Not sure if we have an explicit test for this, but full coverage and documentation of conditions should be provided since it's determinant for the Namespace resolution logic.

There's an addition to the migration guide https://github.com/fabric8io/kubernetes-client/pull/3858/files#diff-6cf6f0d1132c6d642456eda0b231e404d9b3743c0c0ff86346055c5f7c78c268R15

And https://github.com/fabric8io/kubernetes-client/blob/cfd501a75f96c8e02e2e554ecdfb4d3dd56f161c/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/NamespacedItemTest.java highlighting the usage of the explicit namespace.

I'm not sure if this PR also includes the default namespace fallback logic discussed in some meetings.

It does not, that issue is if the Config has no default namespace, and still in the 6.0.0 bucket. It is a little related to the discussion about what should inNamespace(null) mean.

And https://github.com/fabric8io/kubernetes-client/blob/cfd501a75f96c8e02e2e554ecdfb4d3dd56f161c/kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/NamespacedItemTest.java highlighting the usage of the explicit namespace.

This is what I was looking for 👍 😃

manusa · 2022-03-01T13:48:38Z

...lient/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java

+  static class ChangeNamespace extends TypedVisitor<ObjectMetaBuilder> {
+
+    private final String explicitNamespace;
+
+    ChangeNamespace(String explicitNamespace) {
+      this.explicitNamespace = explicitNamespace;
+    }
+
+    @Override
+    public void visit(ObjectMetaBuilder builder) {
+      builder.withNamespace(explicitNamespace);
+    }
+  }


There was an idea floating around about having some common reusable visitors or decorators.

This one seems like a good addition, since we have one that does effectively the same in JKube (https://github.com/eclipse/jkube/blob/2d5a744f2d842bb666da840d26111d4ebc2a521f/jkube-kit/enricher/generic/src/main/java/org/eclipse/jkube/enricher/generic/DefaultNamespaceEnricher.java#L91)

@metacosm @iocanel

extension-api # Conflicts: # doc/MIGRATION-v6.md # kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/OpenShiftLoadTest.java

shawkins · 2022-03-01T18:34:39Z

@manusa I've updated with the changes discussed in the meeting. Feel free to squash this.

sonarqubecloud · 2022-03-01T18:42:47Z

SonarCloud Quality Gate failed.

1 Bug
0 Vulnerabilities
0 Security Hotspots
14 Code Smells

31.3% Coverage
0.7% Duplication

shawkins added the wip label Feb 14, 2022

shawkins requested review from rohanKanojia, manusa and iocanel February 14, 2022 15:56

shawkins force-pushed the extension-api branch 2 times, most recently from 6ec6bd1 to f31e881 Compare February 16, 2022 03:18

shawkins added 2 commits February 16, 2022 07:50

possible mechanism for extension based only on api

dba0878

fix fabric8io#3845: adding an api only framework for extensions

f6927d7

also deprecating extra ApiVersionUtil classes moving resource interfaces in service-catalog client from internal to dsl

shawkins force-pushed the extension-api branch 2 times, most recently from 9a9c091 to 61c10f4 Compare February 16, 2022 18:39

shawkins force-pushed the extension-api branch from 61c10f4 to ed141c1 Compare February 16, 2022 18:55

fix fabric8io#3845 fabric8io#3407: refining withItem support

0e6b058

Kubernetes.resource now returns NamespaceableResource also adds sanity checks useful for fabric8io#3859 as well

shawkins force-pushed the extension-api branch from ed141c1 to 0e6b058 Compare February 16, 2022 20:50

shawkins mentioned this pull request Feb 16, 2022

design of resource and resourceList methods #3407

Closed

shawkins force-pushed the extension-api branch 2 times, most recently from ea29ffe to f8689f4 Compare February 17, 2022 16:00

shawkins removed the wip label Feb 17, 2022

shawkins changed the title ~~WIP Extension development using only api~~ Extension development using only api and KubernetesClient resource/resourceList changes Feb 17, 2022

shawkins marked this pull request as ready for review February 17, 2022 16:00

shawkins force-pushed the extension-api branch from f8689f4 to 5568828 Compare February 17, 2022 16:12

iocanel changed the title ~~Extension development using only api and KubernetesClient resource/resourceList changes~~ OPEN WIP Extension development using only api Feb 17, 2022

iocanel added the wip label Feb 17, 2022

fix fabric8io#3845 fabric8io#3407: refining resourceList to use commo…

4af1a5c

…n logic also various cleanups and adding some migration docs

shawkins force-pushed the extension-api branch from 5568828 to 4af1a5c Compare February 17, 2022 17:05

shawkins changed the title ~~OPEN WIP Extension development using only api~~ OPEN WIP Extension development using only api and KubernetesClient resource/resourceList changes Feb 17, 2022

iocanel changed the title ~~OPEN WIP Extension development using only api and KubernetesClient resource/resourceList changes~~ OPEN WIP Extension development using only api Feb 17, 2022

minimizing constructors and other calls to withnamespace

60bd516

unifying item namespace handling

a7dc6d0

shawkins mentioned this pull request Feb 22, 2022

chore: tests in appropriate packages after split #3886

Merged

11 tasks

shawkins force-pushed the extension-api branch 2 times, most recently from d42bac9 to a210718 Compare February 22, 2022 21:55

shawkins force-pushed the extension-api branch from a210718 to 1b9f52f Compare February 23, 2022 02:08

shawkins force-pushed the extension-api branch from 1b9f52f to c30c2d9 Compare February 23, 2022 03:08

Merge branch 'master' of github.com:fabric8io/kubernetes-client into

fd16688

extension-api # Conflicts: # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/DefaultKubernetesClient.java

updating the apiVersion prior to any usage of the item

71be32d

also updating the test to set the namespace

shawkins force-pushed the extension-api branch from 398b7f1 to 71be32d Compare February 24, 2022 03:53

metacosm self-requested a review February 24, 2022 21:22

shawkins mentioned this pull request Feb 25, 2022

fix #3628 removing sorting from lists and templates #3905

Merged

11 tasks

resourceList should accept any HasMetadata collection

cfd501a

shawkins force-pushed the extension-api branch from 3bbb5f1 to cfd501a Compare February 25, 2022 20:49

manusa reviewed Mar 1, 2022

View reviewed changes

shawkins added 2 commits March 1, 2022 12:55

creating a common adapter base and not allowing inNamespace(null)

59dd3b5

Merge branch 'master' of github.com:fabric8io/kubernetes-client into

df20868

extension-api # Conflicts: # doc/MIGRATION-v6.md # kubernetes-tests/src/test/java/io/fabric8/openshift/client/server/mock/OpenShiftLoadTest.java

manusa approved these changes Mar 2, 2022

View reviewed changes

manusa added this to the 6.0.0 milestone Mar 2, 2022

manusa merged commit a617a17 into fabric8io:master Mar 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extension development using only api and resource/resourceList implementations #3858

Extension development using only api and resource/resourceList implementations #3858

shawkins commented Feb 14, 2022 •

edited by iocanel

Loading

shawkins commented Feb 16, 2022 •

edited

Loading

shawkins commented Feb 17, 2022

shawkins commented Feb 17, 2022

shawkins commented Feb 17, 2022

shawkins commented Feb 21, 2022

shawkins commented Feb 22, 2022

shawkins commented Feb 22, 2022

shawkins commented Feb 24, 2022 •

edited

Loading

manusa Mar 1, 2022

shawkins Mar 1, 2022

manusa Mar 1, 2022

manusa Mar 1, 2022

shawkins Mar 1, 2022

manusa Mar 1, 2022

manusa Mar 1, 2022

manusa Mar 1, 2022

shawkins commented Mar 1, 2022

sonarqubecloud bot commented Mar 1, 2022

Extension development using only api and resource/resourceList implementations #3858

Extension development using only api and resource/resourceList implementations #3858

Conversation

shawkins commented Feb 14, 2022 • edited by iocanel Loading

Description

Type of change

Checklist

shawkins commented Feb 16, 2022 • edited Loading

shawkins commented Feb 17, 2022

shawkins commented Feb 17, 2022

shawkins commented Feb 17, 2022

shawkins commented Feb 21, 2022

shawkins commented Feb 22, 2022

shawkins commented Feb 22, 2022

shawkins commented Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawkins commented Mar 1, 2022

sonarqubecloud bot commented Mar 1, 2022

shawkins commented Feb 14, 2022 •

edited by iocanel

Loading

shawkins commented Feb 16, 2022 •

edited

Loading

shawkins commented Feb 24, 2022 •

edited

Loading