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

Further refine type resolving for 6.0 #3993

Closed
shawkins opened this issue Mar 23, 2022 · 8 comments · Fixed by #4039
Closed

Further refine type resolving for 6.0 #3993

shawkins opened this issue Mar 23, 2022 · 8 comments · Fixed by #4039
Assignees
Milestone

Comments

@shawkins
Copy link
Contributor

Is your task related to a problem? Please describe

The KubernetesDeserializer logic relies upon a package list to determine resolving order. It seems like we should not allow this loose resolving of built-in types. The current logic will resolve all of the following to the same class:

kind: Route

kind: Route
apiVersion: v1

kind: Route
apiVersion: route.openshift.io/v1

Describe the solution you'd like

Only support fully specified versions:

kind: Route
apiVersion: route.openshift.io/v1

Describe alternatives you've considered

No response

Additional context

No response

@shawkins
Copy link
Contributor Author

@rohanKanojia @manusa @metacosm I'm not entirely aware of the original motivation for allowing this type of resolving, is there a reason why we can't now just require a full specification? At worst you'll get an error as the item would be passed back as a generic.

@shawkins
Copy link
Contributor Author

Whatever the outcome here will address a bug I left in https://github.com/fabric8io/kubernetes-client/blob/master/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/internal/KubernetesDeserializer.java#L286 - that will only allow things to resolve to the latest version.

@fig666
Copy link

fig666 commented Mar 23, 2022

i don't agree with this comment. during typing phase majority of errors are implemented and produced against focus actions of choice . causing error nodes and defects

@fig666
Copy link

fig666 commented Mar 23, 2022

during the test of indication of knowledge spectations indirect use of modification of system error from malware attacks from hackers.

@shawkins
Copy link
Contributor Author

@fig666 these comments seem unrelated to this issue

@manusa manusa added this to the 6.0.0 milestone Mar 25, 2022
@shawkins shawkins self-assigned this Mar 28, 2022
@shawkins
Copy link
Contributor Author

@manusa @rohanKanojia @metacosm @iocanel as part of this change it would be nice to eliminate the package list down in the KubernetesDeserializer.

Instead of keying by kind, we'll use the full key and always expect exact matches. There are a couple of approaches:

  • have the internal and external (what it's currently doing via KubernetesResourceMappingProvider) explicitly register the model classes.
  • have the internal and external explicitly register a mapping of apiVersion to java package. This doesn't account for situations where the classes may not have that convention or where the kind may not match the class name - such as a conflict with a java reserved word.
  • The last is to switch all deserializer class discovery to jandex - this does require an additional dependency, but it's relatively small. This would roughly look like:
      // for a given classLoader
      Enumeration<URL> indecies = classLoader.getResources("META-INF/jandex.idx");
      while (indecies.hasMoreElements()) {
        URL u = indecies.nextElement();
        URLConnection uc = u.openConnection();
        uc.setUseCaches(false);
        try (InputStream input = uc.getInputStream()) {
          IndexReader reader = new IndexReader(input);
          Index index = reader.read();
          
          List<AnnotationInstance> annotations = index.getAnnotations(DotName.createSimple(Group.class.getName()));
          for (AnnotationInstance a : annotations) {
            DotName name = a.target().asClass().name();
            Class<?> clazz = classLoader.loadClass(name.toString());
            ... getKeyFromClass(clazz);
            // cache the class / key as we are doing now
          }
        }
      }

What are your thoughts on:

  • introducing a runtime rather than just a build-time dependency on jandex
  • requiring the Group annotation for discovery (and in general is there a reason we have separate group and version annotations)
  • having extensions with model classes in a classloader other than the thread context or what has loaded kubernetes-client-api will make them undiscoverable - that is a bit of a disconnect with the extension logic that looks for the extension in the classloader of the provided class. Since this case isn't supported by the KubernetesDeserializer already, do we care about it?

shawkins added a commit to shawkins/kubernetes-client that referenced this issue Mar 30, 2022
@shawkins
Copy link
Contributor Author

shawkins commented Apr 1, 2022

Relates to #2316 - as we will no longer allow it.

@manusa @rohanKanojia @metacosm also related to this if we want to allow for cross version compatibility (when the object model doesn't change) then we need to update the Version annotation to accept an array of versions. The KubernetesDeserializer will know to register the type across all of those versions.

@shawkins
Copy link
Contributor Author

shawkins commented Apr 1, 2022

Double checking kubectl requires a full specification of apiVersion. oc behaves differently. It will accept just version, but only for openshift specific resources.

This works:

kind: Template
apiVersion: v1

This doesn't:

kind: CronJob
apiVersion: v1

It's also not backed by an api server call - this is by convention. If you via a crd add a conflicting kind, if you specify just v1 - it will resolve to the openshift resource. It appears that oc maintains a list of openshift kinds. Such that it will try finding kind.openshift.io/version when only version is specified.

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.

3 participants