Merge pull request #5937 from dbonfigli/fix-externalgrpc-unimplemented

fix(cloudprovider/exteranlgrpc): properly handle unimplemented methods

Author: k8s-ci-robot

cluster-autoscaler/cloudprovider/externalgrpc/examples/external-grpc-cloud-provider-service/wrapper/wrapper.go

@@ -20,6 +20,8 @@ import (
 	"context"
 	"fmt"
 
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/anypb"
 	apiv1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -113,6 +115,9 @@ func (w *Wrapper) PricingNodePrice(_ context.Context, req *protos.PricingNodePri
 
 	model, err := w.provider.Pricing()
 	if err != nil {
+		if err == cloudprovider.ErrNotImplemented {
+			return nil, status.Error(codes.Unimplemented, err.Error())
+		}
 		return nil, err
 	}
 	reqNode := req.GetNode()
@@ -136,6 +141,9 @@ func (w *Wrapper) PricingPodPrice(_ context.Context, req *protos.PricingPodPrice
 
 	model, err := w.provider.Pricing()
 	if err != nil {
+		if err == cloudprovider.ErrNotImplemented {
+			return nil, status.Error(codes.Unimplemented, err.Error())
+		}
 		return nil, err
 	}
 	reqPod := req.GetPod()
@@ -326,6 +334,9 @@ func (w *Wrapper) NodeGroupTemplateNodeInfo(_ context.Context, req *protos.NodeG
 	}
 	info, err := ng.TemplateNodeInfo()
 	if err != nil {
+		if err == cloudprovider.ErrNotImplemented {
+			return nil, status.Error(codes.Unimplemented, err.Error())
+		}
 		return nil, err
 	}
 	return &protos.NodeGroupTemplateNodeInfoResponse{
@@ -355,6 +366,9 @@ func (w *Wrapper) NodeGroupGetOptions(_ context.Context, req *protos.NodeGroupAu
 	}
 	opts, err := ng.GetOptions(defaults)
 	if err != nil {
+		if err == cloudprovider.ErrNotImplemented {
+			return nil, status.Error(codes.Unimplemented, err.Error())
+		}
 		return nil, err
 	}
 	if opts == nil {

cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_cloud_provider.go

@@ -27,7 +27,9 @@ import (
 	"time"
 
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials"
+	"google.golang.org/grpc/status"
 	"gopkg.in/yaml.v2"
 	apiv1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -158,6 +160,10 @@ func (m *pricingModel) NodePrice(node *apiv1.Node, startTime time.Time, endTime
 		EndTime:   &end,
 	})
 	if err != nil {
+		st, ok := status.FromError(err)
+		if ok && st.Code() == codes.Unimplemented {
+			return 0, cloudprovider.ErrNotImplemented
+		}
 		klog.V(1).Infof("Error on gRPC call PricingNodePrice: %v", err)
 		return 0, err
 	}
@@ -178,6 +184,10 @@ func (m *pricingModel) PodPrice(pod *apiv1.Pod, startTime time.Time, endTime tim
 		EndTime:   &end,
 	})
 	if err != nil {
+		st, ok := status.FromError(err)
+		if ok && st.Code() == codes.Unimplemented {
+			return 0, cloudprovider.ErrNotImplemented
+		}
 		klog.V(1).Infof("Error on gRPC call PricingPodPrice: %v", err)
 		return 0, err
 	}

cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_cloud_provider_test.go

@@ -23,8 +23,11 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/anypb"
 	apiv1 "k8s.io/api/core/v1"
+	"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
 	"k8s.io/autoscaler/cluster-autoscaler/cloudprovider/externalgrpc/protos"
 )
 
@@ -286,6 +289,23 @@ func TestCloudProvider_Pricing(t *testing.T) {
 	_, err = model.NodePrice(apiv1Node3, time.Time{}, time.Time{})
 	assert.Error(t, err)
 
+	// test notImplemented for NodePrice
+	m.On(
+		"PricingNodePrice", mock.Anything, mock.MatchedBy(func(req *protos.PricingNodePriceRequest) bool {
+			return req.Node.Name == "node4"
+		}),
+	).Return(
+		&protos.PricingNodePriceResponse{},
+		status.Error(codes.Unimplemented, "mock error"),
+	)
+
+	apiv1Node4 := &apiv1.Node{}
+	apiv1Node4.Name = "node4"
+
+	_, err = model.NodePrice(apiv1Node4, time.Time{}, time.Time{})
+	assert.Error(t, err)
+	assert.Equal(t, cloudprovider.ErrNotImplemented, err)
+
 	// test correct PodPrice call
 	m.On(
 		"PricingPodPrice", mock.Anything, mock.MatchedBy(func(req *protos.PricingPodPriceRequest) bool {
@@ -333,6 +353,24 @@ func TestCloudProvider_Pricing(t *testing.T) {
 
 	_, err = model.PodPrice(apiv1Pod3, time.Time{}, time.Time{})
 	assert.Error(t, err)
+
+	// test notImplemented for PodPrice
+	m.On(
+		"PricingPodPrice", mock.Anything, mock.MatchedBy(func(req *protos.PricingPodPriceRequest) bool {
+			return req.Pod.Name == "pod4"
+		}),
+	).Return(
+		&protos.PricingPodPriceResponse{},
+		status.Error(codes.Unimplemented, "mock error"),
+	)
+
+	apiv1Pod4 := &apiv1.Pod{}
+	apiv1Pod4.Name = "pod4"
+
+	_, err = model.PodPrice(apiv1Pod4, time.Time{}, time.Time{})
+	assert.Error(t, err)
+	assert.Equal(t, cloudprovider.ErrNotImplemented, err)
+
 }
 
 func TestCloudProvider_GPULabel(t *testing.T) {

cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_node_group.go

@@ -20,6 +20,8 @@ import (
 	"context"
 	"sync"
 
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	apiv1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
@@ -206,6 +208,10 @@ func (n *NodeGroup) TemplateNodeInfo() (*schedulerframework.NodeInfo, error) {
 		Id: n.id,
 	})
 	if err != nil {
+		st, ok := status.FromError(err)
+		if ok && st.Code() == codes.Unimplemented {
+			return nil, cloudprovider.ErrNotImplemented
+		}
 		klog.V(1).Infof("Error on gRPC call NodeGroupTemplateNodeInfo: %v", err)
 		return nil, err
 	}
@@ -269,6 +275,10 @@ func (n *NodeGroup) GetOptions(defaults config.NodeGroupAutoscalingOptions) (*co
 		},
 	})
 	if err != nil {
+		st, ok := status.FromError(err)
+		if ok && st.Code() == codes.Unimplemented {
+			return nil, cloudprovider.ErrNotImplemented
+		}
 		klog.V(1).Infof("Error on gRPC call NodeGroupGetOptions: %v", err)
 		return nil, err
 	}

cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_node_group_test.go

@@ -23,6 +23,8 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	apiv1 "k8s.io/api/core/v1"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
@@ -208,6 +210,27 @@ func TestCloudProvider_TemplateNodeInfo(t *testing.T) {
 	_, err = ng4.TemplateNodeInfo()
 	assert.Error(t, err)
 
+	// test notImplemented
+	m.On(
+		"NodeGroupTemplateNodeInfo", mock.Anything, mock.MatchedBy(func(req *protos.NodeGroupTemplateNodeInfoRequest) bool {
+			return req.Id == "nodeGroup5"
+		}),
+	).Return(
+		&protos.NodeGroupTemplateNodeInfoResponse{
+			NodeInfo: nil,
+		},
+		status.Error(codes.Unimplemented, "mock error"),
+	).Once()
+
+	ng5 := NodeGroup{
+		id:     "nodeGroup5",
+		client: client,
+	}
+
+	_, err = ng5.TemplateNodeInfo()
+	assert.Error(t, err)
+	assert.Equal(t, cloudprovider.ErrNotImplemented, err)
+
 }
 
 func TestCloudProvider_GetOptions(t *testing.T) {
@@ -289,6 +312,26 @@ func TestCloudProvider_GetOptions(t *testing.T) {
 	opts, err = ng3.GetOptions(defaultsOpts)
 	assert.NoError(t, err)
 	assert.Nil(t, opts)
+
+	// test notImplemented
+	m.On(
+		"NodeGroupGetOptions", mock.Anything, mock.MatchedBy(func(req *protos.NodeGroupAutoscalingOptionsRequest) bool {
+			return req.Id == "nodeGroup4"
+		}),
+	).Return(
+		&protos.NodeGroupAutoscalingOptionsResponse{},
+		status.Error(codes.Unimplemented, "mock error"),
+	)
+
+	ng4 := NodeGroup{
+		id:     "nodeGroup4",
+		client: client,
+	}
+
+	_, err = ng4.GetOptions(defaultsOpts)
+	assert.Error(t, err)
+	assert.Equal(t, cloudprovider.ErrNotImplemented, err)
+
 }
 
 func TestCloudProvider_TargetSize(t *testing.T) {

cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.proto

@@ -41,13 +41,13 @@ service CloudProvider {
 
   // PricingNodePrice returns a theoretical minimum price of running a node for
   // a given period of time on a perfectly matching machine.
-  // Implementation optional.
+  // Implementation optional: if unimplemented return error code 12 (for `Unimplemented`)
   rpc PricingNodePrice(PricingNodePriceRequest)
     returns (PricingNodePriceResponse) {}
 
   // PricingPodPrice returns a theoretical minimum price of running a pod for a given
   // period of time on a perfectly matching machine.
-  // Implementation optional.
+  // Implementation optional: if unimplemented return error code 12 (for `Unimplemented`)
   rpc PricingPodPrice(PricingPodPriceRequest)
     returns (PricingPodPriceResponse) {}
 
@@ -103,13 +103,13 @@ service CloudProvider {
   // NodeGroupTemplateNodeInfo returns a structure of an empty (as if just started) node,
   // with all of the labels, capacity and allocatable information. This will be used in
   // scale-up simulations to predict what would a new node look like if a node group was expanded.
-  // Implementation optional.
+  // Implementation optional: if unimplemented return error code 12 (for `Unimplemented`)
   rpc NodeGroupTemplateNodeInfo(NodeGroupTemplateNodeInfoRequest)
     returns (NodeGroupTemplateNodeInfoResponse) {}
 
   // GetOptions returns NodeGroupAutoscalingOptions that should be used for this particular
-  // NodeGroup. Returning a grpc error will result in using default options.
-  // Implementation optional.
+  // NodeGroup.
+  // Implementation optional: if unimplemented return error code 12 (for `Unimplemented`)
   rpc NodeGroupGetOptions(NodeGroupAutoscalingOptionsRequest)
     returns (NodeGroupAutoscalingOptionsResponse) {}
 }
@@ -371,4 +371,3 @@ message NodeGroupAutoscalingOptionsResponse {
   // autoscaling options for the requested node.
   NodeGroupAutoscalingOptions nodeGroupAutoscalingOptions = 1;
 }
-