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.

Sign up for GitHub

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

Jump to bottom

Replace the unsafe gvk to gvr conversion with restmapper. #3775

Merged
merged 1 commit into from
Nov 18, 2021
Merged

Replace the unsafe gvk to gvr conversion with restmapper. #3775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion cmd/kubeapps-apis/plugins/resources/v1alpha1/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ func init() {
// RegisterWithGRPCServer enables a plugin to register with a gRPC server
// returning the server implementation.
func RegisterWithGRPCServer(s grpc.ServiceRegistrar, configGetter core.KubernetesConfigGetter, clustersConfig kube.ClustersConfig, pluginConfigPath string) (interface{}, error) {
svr := NewServer(configGetter)
svr, err := NewServer(configGetter)
if err != nil {
return nil, err
}
v1alpha1.RegisterResourcesServiceServer(s, svr)
return svr, nil
}
Expand Down
62 changes: 56 additions & 6 deletions cmd/kubeapps-apis/plugins/resources/v1alpha1/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,16 @@ import (
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/runtime/serializer"
"k8s.io/apimachinery/pkg/watch"
"k8s.io/client-go/discovery"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
"k8s.io/client-go/restmapper"
log "k8s.io/klog/v2"

"github.com/kubeapps/kubeapps/cmd/apprepository-controller/pkg/client/clientset/versioned/scheme"
"github.com/kubeapps/kubeapps/cmd/kubeapps-apis/core"
pkgsGRPCv1alpha1 "github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/core/packages/v1alpha1"
"github.com/kubeapps/kubeapps/cmd/kubeapps-apis/gen/plugins/resources/v1alpha1"
Expand All @@ -49,9 +54,45 @@ type Server struct {
// corePackagesClientGetter holds a function to obtain the core.packages.v1alpha1
// client. It is similarly initialised in NewServer() below.
corePackagesClientGetter func() (pkgsGRPCv1alpha1.PackagesServiceClient, error)

// We keep a restmapper to cache discovery of REST mappings from GVK->GVR.
restMapper meta.RESTMapper

// kindToResource is a function to convert a GVK to GVR. Can be replaced
// in tests with a dummy version using the unsafe helpers while the real
// implementation queries the k8s API for a REST mapper.
kindToResource func(meta.RESTMapper, schema.GroupVersionKind) (schema.GroupVersionResource, error)
}

// createRESTMapper returns a rest mapper configured with the APIs of the
// local k8s API server. This is used to convert between the GroupVersionKinds
// of the resource references to the GroupVersionResource used by the API server.
func createRESTMapper() (meta.RESTMapper, error) {
Comment on lines +67 to +70
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nuts... what an extra amount of coding for performing just a conversion...I guess I lack some K8s internal knowledge here, but I don't know why we need this kind of runtime information.
Thanks a lot for coming up with this solution (I don't know if the restmapper might be useful for additional things as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It comes down to the fact that a client (kubectl or in our case our client-go client) cannot safely make assumptions about the mapping between an object (the yaml manifest with the GroupVersionKind) and the resource endpoint in the API. The only safe way to determine the mapping is to query the api server for the API information, which is what this now does.

config, err := rest.InClusterConfig()
if err != nil {
return nil, err
}
// To use the config with RESTClientFor, extra fields are required.
// See https://github.com/kubernetes/client-go/issues/657#issuecomment-842960258
config.GroupVersion = &schema.GroupVersion{Group: "", Version: "v1"}
config.NegotiatedSerializer = serializer.WithoutConversionCodecFactory{CodecFactory: scheme.Codecs}
client, err := rest.RESTClientFor(config)
if err != nil {
return nil, err
}
discoveryClient := discovery.NewDiscoveryClient(client)
groupResources, err := restmapper.GetAPIGroupResources(discoveryClient)
if err != nil {
return nil, err
}
return restmapper.NewDiscoveryRESTMapper(groupResources), nil
}

func NewServer(configGetter core.KubernetesConfigGetter) *Server {
func NewServer(configGetter core.KubernetesConfigGetter) (*Server, error) {
mapper, err := createRESTMapper()
if err != nil {
return nil, err
}
return &Server{
clientGetter: func(ctx context.Context, cluster string) (kubernetes.Interface, dynamic.Interface, error) {
if configGetter == nil {
Expand Down Expand Up @@ -79,7 +120,15 @@ func NewServer(configGetter core.KubernetesConfigGetter) *Server {
}
return pkgsGRPCv1alpha1.NewPackagesServiceClient(conn), nil
},
}
restMapper: mapper,
kindToResource: func(mapper meta.RESTMapper, gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) {
mapping, err := mapper.RESTMapping(gvk.GroupKind())
if err != nil {
return schema.GroupVersionResource{}, err
}
return mapping.Resource, nil
},
}, nil
}

// GetResources returns the resources for an installed package.
Expand Down Expand Up @@ -142,10 +191,11 @@ func (s *Server) GetResources(r *v1alpha1.GetResourcesRequest, stream v1alpha1.R
return status.Errorf(codes.Internal, "unable to parse group version from %q: %s", ref.ApiVersion, err.Error())
}
gvk := groupVersion.WithKind(ref.Kind)
// TODO(minelson): Find alternative to UnsafeGuessKindToResource.
// Looks to involve restmapper.NewDeferredDiscoveryRESTMapper(discovery.NewDiscoveryClient(c))
// See https://pkg.go.dev/k8s.io/client-go/restmapper#NewDeferredDiscoveryRESTMapper
gvr, _ := meta.UnsafeGuessKindToResource(gvk)

gvr, err := s.kindToResource(s.restMapper, gvk)
if err != nil {
return status.Errorf(codes.Internal, "unable to map group-kind %v to resource: %s", gvk.GroupKind(), err.Error())
}

if !r.GetWatch() {
resource, err := dynamicClient.Resource(gvr).Namespace(namespace).Get(stream.Context(), ref.GetName(), metav1.GetOptions{})
Expand Down
8 changes: 8 additions & 0 deletions cmd/kubeapps-apis/plugins/resources/v1alpha1/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,10 @@ import (
"google.golang.org/grpc/test/bufconn"
apps "k8s.io/api/apps/v1"
core "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/dynamic"
dynfake "k8s.io/client-go/dynamic/fake"
"k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -105,6 +107,12 @@ func getResourcesClient(t *testing.T, objects ...runtime.Object) (v1alpha1.Resou
corePackagesClientGetter: func() (pkgsGRPCv1alpha1.PackagesServiceClient, error) {
return pkgsGRPCv1alpha1.NewPackagesServiceClient(conn), nil
},
// For testing, define a kindToResource converter that doesn't require
// a rest mapper.
kindToResource: func(mapper meta.RESTMapper, gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) {
gvr, _ := meta.UnsafeGuessKindToResource(gvk)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, at least the unsafe thing is still useful :P

return gvr, nil
},
})

go func() {
Expand Down