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

KnativeClient.isSupported doesn't check all of the applicable API Groups #4447

Closed
manusa opened this issue Sep 23, 2022 · 3 comments · Fixed by #4631
Closed

KnativeClient.isSupported doesn't check all of the applicable API Groups #4447

manusa opened this issue Sep 23, 2022 · 3 comments · Fixed by #4631
Assignees
Labels
bug component/extensions Deals with the extensions
Milestone

Comments

@manusa
Copy link
Member

manusa commented Sep 23, 2022

Describe the bug

Initially reported: quarkusio/quarkus#28171
With fix: quarkusio/quarkus#28172

The current implementation of isSupported() for KnativeClient is:

  public boolean isSupported() {
    return hasApiGroup("knative.dev", true);
  }

This is not taking into consideration all of the API groups our models support:

  • serving.knative.dev
  • flows.knative.dev
  • networking.internal.knative.dev
  • etc.

Fabric8 Kubernetes Client version

6.0.0

Steps to reproduce

  • In a cluster with Knative CRDs, invoke the KnativeClient.isSupported method.
  • false is returned instead of true

Expected behavior

true should be returned instead

Suggested fix

The check should not be exact:

  public boolean isSupported() {
    return hasApiGroup("knative.dev", false);
  }
@shawkins
Copy link
Contributor

This change should be applied to DefaultServiceCatalogClient as well.

@manusa manusa modified the milestones: 6.2.0, 6.3.0 Oct 19, 2022
@rohanKanojia rohanKanojia self-assigned this Nov 30, 2022
@rohanKanojia
Copy link
Member

@shawkins : I'm not sure if I understand. From my understanding we only want to use hasApiGroup("<>", true) only when the client is dealing with model types that come from a single apiGroup. All resources in service-catalog module seem to be coming from servicecatalog.k8s.io apiGroup:

packageMapping := map[string]schemagen.PackageInformation{
"github.com/kubernetes-sigs/service-catalog/pkg/apis/servicecatalog/v1beta1": {JavaPackage: "io.fabric8.servicecatalog.api.model", ApiGroup: "servicecatalog.k8s.io", ApiVersion: "v1beta1"},
}

Isn't it working as expected? Or perhaps I've misunderstood the problem.

@shawkins
Copy link
Contributor

shawkins commented Dec 1, 2022

Isn't it working as expected? Or perhaps I've misunderstood the problem.

It is working as expected, but it's more restrictive that what the older check was. Since we don't guarantee a 1-1 relationship with api groups and clients, it's best to have these checks be more general. Should there be another sub group introduced, it will have the same problem as knative.

rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Dec 1, 2022
…I Groups (fabric8io#4447)

+ Extension client interfaces should extend SupportTestingClient rather
  than implementations
+ Disable exact apiGroup match in isSupported method call in
  extension clients which use more than one apiGroup. This effects Istio,
  Knative and Tekton extensions that use more than one apiGroups

Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Dec 2, 2022
…I Groups (fabric8io#4447)

+ Extension client interfaces should extend SupportTestingClient rather
  than implementations
+ Disable exact apiGroup match in isSupported method call in
  extension clients which use more than one apiGroup. This effects Istio,
  Knative and Tekton extensions that use more than one apiGroups

Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Dec 2, 2022
…I Groups (fabric8io#4447)

+ Disable exact apiGroup match in isSupported method call in
  extension clients which use more than one apiGroup. This effects Istio,
  Knative and Tekton extensions that use more than one apiGroups

Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Dec 12, 2022
…I Groups (fabric8io#4447)

+ Disable exact apiGroup match in isSupported method call in
  extension clients which use more than one apiGroup. This effects Istio,
  Knative and Tekton extensions that use more than one apiGroups

Signed-off-by: Rohan Kumar <rohaan@redhat.com>
manusa pushed a commit that referenced this issue Dec 12, 2022
…I Groups (#4447)

+ Disable exact apiGroup match in isSupported method call in
  extension clients which use more than one apiGroup. This effects Istio,
  Knative and Tekton extensions that use more than one apiGroups

Signed-off-by: Rohan Kumar <rohaan@redhat.com>
geoand added a commit to geoand/quarkus that referenced this issue Jan 31, 2023
This can now be done because
fabric8io/kubernetes-client#4447
is included in version 6.3 of the k8s client
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component/extensions Deals with the extensions
Projects
None yet
3 participants