fabric8io / kubernetes-client

Java client for Kubernetes & OpenShift
http://fabric8.io
Apache License 2.0
3.43k stars 1.46k forks source link

CustomResourceFluentImpl produces a shallow copy #3076

Closed shawkins closed 3 years ago

shawkins commented 3 years ago

In the CustomResourceFluentImpl, there are methods such as:

    public A withMetadata(ObjectMeta metadata) {
        this.metadata=metadata; return (A) this;
    }

which are invoked when a reference object is passed to the builder constructor.

In core classes the implementation looks like:

    public A withMetadata(ObjectMeta metadata) {
        _visitables.get("metadata").remove(this.metadata);
        if (metadata!=null){ this.metadata= new ObjectMetaBuilder(metadata); _visitables.get("metadata").add(this.metadata);} return (A) this;
    }

which does produce a deep copy.

shawkins commented 3 years ago

cc @iocanel

iocanel commented 3 years ago

It seems that ObjectMeta is not recognized as bildable.

Indeed it seems that there should be a @BuildableReference(ObjectMeta.class) defined as is the case with all model types:

https://github.com/fabric8io/kubernetes-client/blob/master/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/CustomResource.java#L69

manusa commented 3 years ago

CustomResource should be annotated like any other Resource (from modules different to kubernetes-model-core):

@Buildable(editableEnabled = false, validationEnabled = false, generateBuilderPackage = false, lazyCollectionInitEnabled = false, builderPackage = "io.fabric8.kubernetes.api.builder", refs = {
    @BuildableReference(io.fabric8.kubernetes.api.model.ObjectMeta.class),
    @BuildableReference(LabelSelector.class),
    @BuildableReference(Container.class),
    @BuildableReference(PodTemplateSpec.class),
    @BuildableReference(ResourceRequirements.class),
    @BuildableReference(IntOrString.class),
    @BuildableReference(ObjectReference.class),
    @BuildableReference(LocalObjectReference.class),
    @BuildableReference(PersistentVolumeClaim.class)
})

The list should match: https://github.com/fabric8io/kubernetes-client/blob/4eee56eb02cc3880435bcf54aacd86c735205013/kubernetes-model-generator/kubernetes-model-jsonschema2pojo/src/main/java/io/fabric8/kubernetes/jsonschema2pojo/KubernetesTypeAnnotator.java#L41-L50

rohanKanojia commented 3 years ago

@shawkins : Are you interested in submitting a fix for this issue?

shawkins commented 3 years ago

Are you interested in submitting a fix for this issue?

I am not yet familiar with this part of the code. It would be better if I just followed what fix was applied.

rohanKanojia commented 3 years ago

@shawkins : I think Ioannis and Marc are saying to add explicit BuildableReference annotation for ObjectMeta:

diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/CustomResource.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/CustomResource.java
index 5f078a0fe..1bb42c26a 100644
--- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/CustomResource.java
+++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/CustomResource.java
@@ -34,6 +34,8 @@ import io.fabric8.kubernetes.model.annotation.ShortNames;
 import io.fabric8.kubernetes.model.annotation.Version;
 import io.sundr.builder.annotations.Buildable;
 import java.util.Optional;
+
+import io.sundr.builder.annotations.BuildableReference;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

@@ -66,8 +68,10 @@ import org.slf4j.LoggerFactory;
   "spec",
   "status"
 })
-@Buildable(builderPackage = "io.fabric8.kubernetes.api.builder", editableEnabled = false)
 @JsonInclude(Include.NON_NULL)
+@Buildable(editableEnabled = false, validationEnabled = false, generateBuilderPackage = false, lazyCollectionInitEnabled = false, builderPackage = "io.fabric8.kubernetes.api.builder", refs = {
+  @BuildableReference(io.fabric8.kubernetes.api.model.ObjectMeta.class),
+})
 public abstract class CustomResource<S, T> implements HasMetadata {
   private static final Logger LOG = LoggerFactory.getLogger(CustomResource.class);
shawkins commented 3 years ago

Does this also mean that anyone expecting this cloning behavior will need to add refs for spec and status classes for whatever extends CustomResource as well?

rohanKanojia commented 3 years ago

Are you using sundrio for CustomResource builder generation? I think this @BuildableReference is only used to refer to those classes which are not available at compile time. If you see all core Kubernetes resource types (Pod, Secret etc) don't have this since ObjectMeta is also compiled in kubernetes-model-core

shawkins commented 3 years ago

Are you using sundrio for CustomResource builder generation?

Yes

We have custom resource:

@Buildable(
        builderPackage = "io.fabric8.kubernetes.api.builder",
        refs = @BuildableReference(CustomResource.class),
        editableEnabled = false
)
@Group("managedkafka.bf2.org")
@Version("v1alpha1")
public class MyResource extends CustomResource<MyResourceSpec, MyResourceStatus> implements Namespaced {

Which generates a MyResourceFluentImpl:

public MyResourceFluentImpl(MyResource instance) {
        this.withMetadata(instance.getMetadata());

        this.withSpec(instance.getSpec());

        this.withStatus(instance.getStatus());

        this.withKind(instance.getKind());

        this.withApiVersion(instance.getApiVersion());
    }

All of the with methods are implemented in CustomResourceFluentImpl. This change will address the metadata - but I'm not quite seeing how spec and status will work. Even if I add BuildableReference for those on MyResource, the MyResourceFluentImpl is still calling the CustomResourceFluentImpl.

metacosm commented 3 years ago

Relates to #2807.

shawkins commented 3 years ago

Captured upstream as https://github.com/sundrio/sundrio/issues/248

After the pr based upon @rohanKanojia suggested change above is committed, then we can close this issue.

manusa commented 3 years ago

3232, closing issue as the rest should be addressed upstream (if possible).