fabric8io / kubernetes-client

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

Namespace requires Namespace #2189

Closed Fabian-K closed 4 years ago

Fabian-K commented 4 years ago

Hi,

with 4.10.0 there seems to be an issue with cluster-wide resources. So far, I encountered io.fabric8.kubernetes.client.KubernetesClientException: Namespace not specified. But operation requires namespace. when applying the following yaml:

apiVersion: v1
kind: Namespace
metadata:
  name: istio-operator   

using

k8s.load(<yaml as input stream>).createOrReplace()
Namespace not specified. But operation requires namespace.
io.fabric8.kubernetes.client.KubernetesClientException: Namespace not specified. But operation requires namespace.
    at io.fabric8.kubernetes.client.dsl.base.OperationSupport.checkNamespace(OperationSupport.java:175)
    at io.fabric8.kubernetes.client.dsl.base.OperationSupport.getNamespacedUrl(OperationSupport.java:151)
    at io.fabric8.kubernetes.client.dsl.base.BaseOperation.getCompleteResourceUrl(BaseOperation.java:854)
    at io.fabric8.kubernetes.client.dsl.base.BaseOperation.getMandatory(BaseOperation.java:214)
    at io.fabric8.kubernetes.client.dsl.base.BaseOperation.get(BaseOperation.java:169)
    at io.fabric8.kubernetes.client.handlers.core.v1.NamespaceHandler.reload(NamespaceHandler.java:64)
    at io.fabric8.kubernetes.client.handlers.core.v1.NamespaceHandler.reload(NamespaceHandler.java:40)
    at io.fabric8.kubernetes.client.dsl.internal.NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.createOrReplace(NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java:251)
    at io.fabric8.kubernetes.client.dsl.internal.NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.createOrReplace(NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java:65)

I did not look into this yet.

Best regards, Fabian

Fabian-K commented 4 years ago

Looks like there are 2 NamespaceOperationsImpl: io.fabric8.kubernetes.client.dsl.internal.NamespaceOperationsImpl which has proper isResourceNamespaced and io.fabric8.kubernetes.client.dsl.internal.core.v1.NamespaceOperationsImpl which has not.

The later one is used and referenced in the NamespaceHandler

rohanKanojia commented 4 years ago

@Fabian-K : looks like I introduced this regression. Could you please share a quick PR to fix this?

rohanKanojia commented 4 years ago

umm, leave it. Let me fix this.

Fabian-K commented 4 years ago

Thanks for taking care of it! I think you "just" need to move the manually defined OperationsImpl to the right packages.

In parallel, it is probably worth a try to directly generate them with the right isResourceNamespaced value

rohanKanojia commented 4 years ago

Is their any way to check if a resource is namespaced or not?

rohanKanojia commented 4 years ago

Actually, we have our own *OperationsImpl classes for things much more than isResourceNamespaced . Although I do agree we should refactor our those which are there only for isResourceNamespaced method.

Fabian-K commented 4 years ago

I did some experiments at https://github.com/Fabian-K/kubernetes-client/tree/crd-scoping - tektons ClusterTask is also affected by this.

From the go source code, there is no possibility to tell them apart. But we could easily add this to the config part (https://github.com/Fabian-K/kubernetes-client/blob/6e10b5f575f286f6b0ec39c0de7d3551171ba075/extensions/tekton/generator-v1alpha1/cmd/generate/generate.go#L33-L38)

First I thought about adding an annotation via the json schema - that is however not supported. Next idea was to add a marker interface, in this example "Namespaced" which we can easily add to all namespaced CRD classes. That is already included in the commit for tekton v1alpha1.

Now the only missing part is to include it in the resource-operation.vm (https://github.com/Fabian-K/kubernetes-client/blob/6e10b5f575f286f6b0ec39c0de7d3551171ba075/extensions/tekton/generator-v1alpha1/cmd/generate/generate.go#L33-L38). So far my attempt with return ${model.getImplementsList().stream().anyMatch(i -> Objects.equals(i.getFullyQualifiedName() , "io.fabric8.kubernetes.api.model.Namespaced"))} is failing due to

Fatal error compiling: io.sundr.shaded.org.apache.velocity.exception.ParseErrorException: Lexical error,   Encountered: ">" (62), after : "-" at template[line 71, column 61]

Not sure if the sundr part needs to be enhanced for this or if i´m just using the wrong syntax.

Fabian-K commented 4 years ago

My little experiment is now working, just had to write the resource-operation.vm part a bit lengthier ;)

What do you think of this approaches? The part that I don´t like is the additional interface - somehow an annotation would feel a bit better - but that's a limitation of the json-schema to java part.

On the other hand, the interface should not really hurt, right? What's your opinion on this?

rohanKanojia commented 4 years ago

I'm not sure whether you had a chance to look at my PR or not. I have also introduced a @Namespaced annotation, but I added the code in Annotator class rather than in schema generation code.

Fabian-K commented 4 years ago

Yes, I like it - much better compared to the existing code!

I guess both approaches are very similar. Comparing the two: I like the annotation more compared to the marker interface. On the other hand, I would prefer to maintain the information Cluster vs Namespaced in the go configuration part. I fear that maintaining this information in the Annotation Processor can be forgotten. This can´t occur with go as it is enforced by the compiler there.

rohanKanojia commented 4 years ago

@Fabian-K : I like your approach also but I think we need to fix this and cut a release as soon as possible since people who might be using handlers won't be able to access custom-defined operation classes.

Would you like to file a separate issue for this?

Fabian-K commented 4 years ago

Sure :)

Agree, a "hotfix" release containing your PR would be really helpful. This currently blocks us from upgrading to 4.10