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

Listing / loading a CAPI Cluster resource throws an exception #3859

Closed
lion7 opened this issue Feb 15, 2022 · 2 comments · Fixed by #3899
Closed

Listing / loading a CAPI Cluster resource throws an exception #3859

lion7 opened this issue Feb 15, 2022 · 2 comments · Fixed by #3899
Assignees
Milestone

Comments

@lion7
Copy link

lion7 commented Feb 15, 2022

Describe the bug

The KubernetesDeserializer does a lookup for internally defined classes when deserializing JSON/YAML.

I have a CRD where the kind is Cluster (definition can be found here). This gets resolved as class io.fabric8.kubernetes.api.model.Cluster, which is used internally to describe a cluster as part of the kubeconfig (why is this even implementing KubernetesResource?). This class does not match my resource at all, so I expected the deserializer to return a GenericKubernetesResource. Right now this loading behaviour results in all kind of weird exceptions later on.

Code analysis
I noticed that in io.fabric8.kubernetes.internal.KubernetesDeserializer.Mapping#getInternalTypeForName, there is actually a check that the complete apiVersion + kind matches the found class. This check is however only done if there are more than 1 possible classes found, which is not the case here. I would suggest to always do this check to be on the safe side. Part of the code:

            // If only one class found, return it
            if (possibleResults.size() == 1) {
                return possibleResults.get(0);
            } else if (possibleResults.size() > 1) {
              // Compare with apiVersions being compared for set of classes found
              for (Class<? extends KubernetesResource> result : possibleResults) {
                String defaultKeyFromClass = getKeyFromClass(result);
                if (key.equals(defaultKeyFromClass)) {
                  return result;
                }
              }
              return possibleResults.get(0);
            } else {
              return null;
            }

Fabric8 Kubernetes Client version

next (development version)

Steps to reproduce

When listing CAPI cluster resources as generic kubernetes resources, like so

kubernetesClient.genericKubernetesResources("cluster.x-k8s.io/v1beta1", "Cluster").list()

The following exception is thrown:

java.lang.NullPointerException: Cannot invoke "String.length()" because "this.input" is null
	at java.base/java.net.URI$Parser.parse(URI.java:3165)
	at java.base/java.net.URI.<init>(URI.java:623)
	at io.fabric8.kubernetes.client.utils.URLUtils.join(URLUtils.java:89)
	at io.fabric8.kubernetes.client.dsl.base.OperationSupport.getRootUrl(OperationSupport.java:146)
	at io.fabric8.kubernetes.client.dsl.base.OperationSupport.getNamespacedUrl(OperationSupport.java:153)
	at io.fabric8.kubernetes.client.dsl.base.OperationSupport.getNamespacedUrl(OperationSupport.java:164)
	at io.fabric8.kubernetes.client.dsl.base.BaseOperation.list(BaseOperation.java:414)
	at io.fabric8.kubernetes.client.dsl.base.BaseOperation.list(BaseOperation.java:402)
	at io.fabric8.kubernetes.client.dsl.base.BaseOperation.list(BaseOperation.java:83)
	at dev.ruddur.operator.clusterapi.SideroClient.clusterExists(SideroClient.kt:19)

Same goes for loading a resource of this kind:

val s = """
apiVersion: cluster.x-k8s.io/v1beta1
kind: Cluster
metadata:
  name: test
spec:
  clusterNetwork:
    pods:
      cidrBlocks:
        - 10.244.0.0/16
    services:
      cidrBlocks:
        - 10.96.0.0/12
  infrastructureRef:
    apiVersion: infrastructure.cluster.x-k8s.io/v1alpha3
    kind: MetalCluster
    name: test
  controlPlaneRef:
    apiVersion: controlplane.cluster.x-k8s.io/v1alpha3
    kind: TalosControlPlane
    name: test-cp
"""
val r = kubernetesClient.resource(s).get()
println(r)

Which throws:

Exception in thread "main" java.lang.ClassCastException: class io.fabric8.kubernetes.api.model.Cluster cannot be cast to class io.fabric8.kubernetes.api.model.HasMetadata (io.fabric8.kubernetes.api.model.Cluster and io.fabric8.kubernetes.api.model.HasMetadata are in unnamed module of loader 'app')
	at io.fabric8.kubernetes.client.BaseKubernetesClient.resource(BaseKubernetesClient.java:235)
	at dev.ruddur.operator.Main.main(Main.kt:55)

Expected behavior

That I would be able to list and/or create Cluster resources using the GenericKubernetesResource APIs.

Runtime

Kubernetes (vanilla)

Kubernetes API Server version

1.22.3@latest

Environment

Linux

Fabric8 Kubernetes Client Logs

No response

Additional context

Current workaround is to explicitly register this resource as a GenericKubernetesResource:

KubernetesDeserializer.registerCustomKind("cluster.x-k8s.io/v1beta1", "Cluster", GenericKubernetesResource.class)
@shawkins
Copy link
Contributor

shawkins commented Feb 15, 2022

I noticed that in io.fabric8.kubernetes.internal.KubernetesDeserializer.Mapping#getInternalTypeForName, there is actually a check that the complete apiVersion + kind matches the found class.

I think we talked about this a little with #3785 but it didn't end up being needed to resolve that issue. Yes a change is need to prevent this kind of resolution conflict.

The following exception is thrown:
...
Which throws:

The logic will default to using a built-in handler if possible, so it's again resolving to wrong Cluster resource. Since it's not a HasMetadata resource we're generating invalid metadata from its class - the class has neither a Version nor a Group annotation, and the logic in

assumes that if there's a config, it will specify a default apiVersion - which is not required. So in addition to the deserializer change there are a couple of sanity checks needed that will make for better exceptions.

@shawkins shawkins added this to the 6.0.0 milestone Feb 15, 2022
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
Copy link
Contributor

A separate pr will still be needed for the deserializer change.

shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 24, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 24, 2022
@shawkins shawkins linked a pull request Feb 24, 2022 that will close this issue
11 tasks
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 25, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 25, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 25, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 25, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 26, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Mar 1, 2022
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Mar 1, 2022
manusa pushed a commit to manusa/kubernetes-client that referenced this issue Apr 5, 2022
(cherry picked from commit 3c8a074)
Signed-off-by: Marc Nuri <marc@marcnuri.com>
manusa pushed a commit to manusa/kubernetes-client that referenced this issue Apr 5, 2022
(cherry picked from commit 3c8a074)
Signed-off-by: Marc Nuri <marc@marcnuri.com>
manusa pushed a commit that referenced this issue Apr 5, 2022
(cherry picked from commit 3c8a074)
Signed-off-by: Marc Nuri <marc@marcnuri.com>
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

Successfully merging a pull request may close this issue.

2 participants