From bbdc247406aa21d16644828771b377c042bfdeb6 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Sun, 21 Jul 2024 13:48:27 +0200 Subject: [PATCH] aggregator: make linter happy Signed-off-by: Dr. Stefan Schimanski --- .../remote/remote_available_controller.go | 4 +- .../remote_available_controller_test.go | 82 ++++++++++++------- 2 files changed, 55 insertions(+), 31 deletions(-) diff --git a/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/remote/remote_available_controller.go b/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/remote/remote_available_controller.go index baced64a369..146c6a51426 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/remote/remote_available_controller.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/remote/remote_available_controller.go @@ -313,7 +313,7 @@ func (c *AvailableConditionController) sync(key string) error { resp.Body.Close() // we should always been in the 200s or 300s if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { - errCh <- fmt.Errorf("bad status from %v: %v", discoveryURL, resp.StatusCode) + errCh <- fmt.Errorf("bad status from %v: %d", discoveryURL, resp.StatusCode) return } } @@ -324,7 +324,7 @@ func (c *AvailableConditionController) sync(key string) error { select { case err = <-errCh: if err != nil { - results <- fmt.Errorf("failing or missing response from %v: %v", discoveryURL, err) + results <- fmt.Errorf("failing or missing response from %v: %w", discoveryURL, err) return } diff --git a/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/remote/remote_available_controller_test.go b/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/remote/remote_available_controller_test.go index edd330cfe65..d08c9ae1784 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/remote/remote_available_controller_test.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/remote/remote_available_controller_test.go @@ -25,11 +25,9 @@ import ( "testing" "time" - availabilitymetrics "k8s.io/kube-aggregator/pkg/controllers/status/metrics" - "k8s.io/utils/pointer" - v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/dump" v1listers "k8s.io/client-go/listers/core/v1" clienttesting "k8s.io/client-go/testing" @@ -39,11 +37,13 @@ import ( "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/fake" apiregistrationclient "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/typed/apiregistration/v1" listers "k8s.io/kube-aggregator/pkg/client/listers/apiregistration/v1" + availabilitymetrics "k8s.io/kube-aggregator/pkg/controllers/status/metrics" + "k8s.io/utils/ptr" ) const ( - testServicePort = 1234 - testServicePortName = "testPort" + testServicePort int32 = 1234 + testServicePortName = "testPort" ) func newEndpoints(namespace, name string) *v1.Endpoints { @@ -100,13 +100,18 @@ func newRemoteAPIService(name string) *apiregistration.APIService { Service: &apiregistration.ServiceReference{ Namespace: "foo", Name: "bar", - Port: pointer.Int32Ptr(testServicePort), + Port: ptr.To(testServicePort), }, }, } } -func setupAPIServices(apiServices []*apiregistration.APIService) (*AvailableConditionController, *fake.Clientset) { +type T interface { + Fatalf(format string, args ...interface{}) + Errorf(format string, args ...interface{}) +} + +func setupAPIServices(t T, apiServices []runtime.Object) (*AvailableConditionController, *fake.Clientset) { fakeClient := fake.NewSimpleClientset() apiServiceIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) serviceIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) @@ -118,7 +123,9 @@ func setupAPIServices(apiServices []*apiregistration.APIService) (*AvailableCond defer testServer.Close() for _, o := range apiServices { - apiServiceIndexer.Add(o) + if err := apiServiceIndexer.Add(o); err != nil { + t.Fatalf("failed to add APIService: %v", err) + } } c := AvailableConditionController{ @@ -145,7 +152,7 @@ func setupAPIServices(apiServices []*apiregistration.APIService) (*AvailableCond func BenchmarkBuildCache(b *testing.B) { apiServiceName := "remote.group" // model 1 APIService pointing at a given service, and 30 pointing at local group/versions - apiServices := []*apiregistration.APIService{newRemoteAPIService(apiServiceName)} + apiServices := []runtime.Object{newRemoteAPIService(apiServiceName)} for i := 0; i < 30; i++ { apiServices = append(apiServices, newLocalAPIService(fmt.Sprintf("local.group%d", i))) } @@ -154,7 +161,7 @@ func BenchmarkBuildCache(b *testing.B) { for i := 0; i < 100; i++ { services = append(services, newService("foo", fmt.Sprintf("bar%d", i), testServicePort, testServicePortName)) } - c, _ := setupAPIServices(apiServices) + c, _ := setupAPIServices(b, apiServices) b.ReportAllocs() b.ResetTimer() for n := 1; n <= b.N; n++ { @@ -175,7 +182,7 @@ func TestBuildCache(t *testing.T) { name string apiServiceName string - apiServices []*apiregistration.APIService + apiServices []runtime.Object services []*v1.Service endpoints []*v1.Endpoints @@ -184,13 +191,13 @@ func TestBuildCache(t *testing.T) { { name: "api service", apiServiceName: "remote.group", - apiServices: []*apiregistration.APIService{newRemoteAPIService("remote.group")}, + apiServices: []runtime.Object{newRemoteAPIService("remote.group")}, services: []*v1.Service{newService("foo", "bar", testServicePort, testServicePortName)}, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - c, fakeClient := setupAPIServices(tc.apiServices) + c, fakeClient := setupAPIServices(t, tc.apiServices) for _, svc := range tc.services { c.addService(svc) } @@ -210,18 +217,19 @@ func TestSync(t *testing.T) { name string apiServiceName string - apiServices []*apiregistration.APIService + apiServices []runtime.Object services []*v1.Service endpoints []*v1.Endpoints backendStatus int backendLocation string expectedAvailability apiregistration.APIServiceCondition + expectedSyncError string }{ { name: "local", apiServiceName: "local.group", - apiServices: []*apiregistration.APIService{newLocalAPIService("local.group")}, + apiServices: []runtime.Object{newLocalAPIService("local.group")}, backendStatus: http.StatusOK, expectedAvailability: apiregistration.APIServiceCondition{ Type: apiregistration.Available, @@ -233,7 +241,7 @@ func TestSync(t *testing.T) { { name: "no service", apiServiceName: "remote.group", - apiServices: []*apiregistration.APIService{newRemoteAPIService("remote.group")}, + apiServices: []runtime.Object{newRemoteAPIService("remote.group")}, services: []*v1.Service{newService("foo", "not-bar", testServicePort, testServicePortName)}, backendStatus: http.StatusOK, expectedAvailability: apiregistration.APIServiceCondition{ @@ -246,7 +254,7 @@ func TestSync(t *testing.T) { { name: "service on bad port", apiServiceName: "remote.group", - apiServices: []*apiregistration.APIService{newRemoteAPIService("remote.group")}, + apiServices: []runtime.Object{newRemoteAPIService("remote.group")}, services: []*v1.Service{{ ObjectMeta: metav1.ObjectMeta{Namespace: "foo", Name: "bar"}, Spec: v1.ServiceSpec{ @@ -268,7 +276,7 @@ func TestSync(t *testing.T) { { name: "no endpoints", apiServiceName: "remote.group", - apiServices: []*apiregistration.APIService{newRemoteAPIService("remote.group")}, + apiServices: []runtime.Object{newRemoteAPIService("remote.group")}, services: []*v1.Service{newService("foo", "bar", testServicePort, testServicePortName)}, backendStatus: http.StatusOK, expectedAvailability: apiregistration.APIServiceCondition{ @@ -281,7 +289,7 @@ func TestSync(t *testing.T) { { name: "missing endpoints", apiServiceName: "remote.group", - apiServices: []*apiregistration.APIService{newRemoteAPIService("remote.group")}, + apiServices: []runtime.Object{newRemoteAPIService("remote.group")}, services: []*v1.Service{newService("foo", "bar", testServicePort, testServicePortName)}, endpoints: []*v1.Endpoints{newEndpoints("foo", "bar")}, backendStatus: http.StatusOK, @@ -295,7 +303,7 @@ func TestSync(t *testing.T) { { name: "wrong endpoint port name", apiServiceName: "remote.group", - apiServices: []*apiregistration.APIService{newRemoteAPIService("remote.group")}, + apiServices: []runtime.Object{newRemoteAPIService("remote.group")}, services: []*v1.Service{newService("foo", "bar", testServicePort, testServicePortName)}, endpoints: []*v1.Endpoints{newEndpointsWithAddress("foo", "bar", testServicePort, "wrongName")}, backendStatus: http.StatusOK, @@ -309,7 +317,7 @@ func TestSync(t *testing.T) { { name: "remote", apiServiceName: "remote.group", - apiServices: []*apiregistration.APIService{newRemoteAPIService("remote.group")}, + apiServices: []runtime.Object{newRemoteAPIService("remote.group")}, services: []*v1.Service{newService("foo", "bar", testServicePort, testServicePortName)}, endpoints: []*v1.Endpoints{newEndpointsWithAddress("foo", "bar", testServicePort, testServicePortName)}, backendStatus: http.StatusOK, @@ -323,7 +331,7 @@ func TestSync(t *testing.T) { { name: "remote-bad-return", apiServiceName: "remote.group", - apiServices: []*apiregistration.APIService{newRemoteAPIService("remote.group")}, + apiServices: []runtime.Object{newRemoteAPIService("remote.group")}, services: []*v1.Service{newService("foo", "bar", testServicePort, testServicePortName)}, endpoints: []*v1.Endpoints{newEndpointsWithAddress("foo", "bar", testServicePort, testServicePortName)}, backendStatus: http.StatusForbidden, @@ -333,11 +341,12 @@ func TestSync(t *testing.T) { Reason: "FailedDiscoveryCheck", Message: `failing or missing response from`, }, + expectedSyncError: "failing or missing response from", }, { name: "remote-redirect", apiServiceName: "remote.group", - apiServices: []*apiregistration.APIService{newRemoteAPIService("remote.group")}, + apiServices: []runtime.Object{newRemoteAPIService("remote.group")}, services: []*v1.Service{newService("foo", "bar", testServicePort, testServicePortName)}, endpoints: []*v1.Endpoints{newEndpointsWithAddress("foo", "bar", testServicePort, testServicePortName)}, backendStatus: http.StatusFound, @@ -348,11 +357,12 @@ func TestSync(t *testing.T) { Reason: "FailedDiscoveryCheck", Message: `failing or missing response from`, }, + expectedSyncError: "failing or missing response from", }, { name: "remote-304", apiServiceName: "remote.group", - apiServices: []*apiregistration.APIService{newRemoteAPIService("remote.group")}, + apiServices: []runtime.Object{newRemoteAPIService("remote.group")}, services: []*v1.Service{newService("foo", "bar", testServicePort, testServicePortName)}, endpoints: []*v1.Endpoints{newEndpointsWithAddress("foo", "bar", testServicePort, testServicePortName)}, backendStatus: http.StatusNotModified, @@ -362,12 +372,13 @@ func TestSync(t *testing.T) { Reason: "FailedDiscoveryCheck", Message: `failing or missing response from`, }, + expectedSyncError: "failing or missing response from", }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - fakeClient := fake.NewSimpleClientset() + fakeClient := fake.NewSimpleClientset(tc.apiServices...) apiServiceIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) serviceIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) endpointsIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) @@ -398,7 +409,16 @@ func TestSync(t *testing.T) { proxyCurrentCertKeyContent: func() ([]byte, []byte) { return emptyCert(), emptyCert() }, metrics: availabilitymetrics.New(), } - c.sync(tc.apiServiceName) + err := c.sync(tc.apiServiceName) + if tc.expectedSyncError != "" { + if err == nil { + t.Fatalf("%v expected error with %q, got none", tc.name, tc.expectedSyncError) + } else if !strings.Contains(err.Error(), tc.expectedSyncError) { + t.Fatalf("%v expected error with %q, got %q", tc.name, tc.expectedSyncError, err.Error()) + } + } else if err != nil { + t.Fatalf("%v unexpected sync error: %v", tc.name, err) + } // ought to have one action writing status if e, a := 1, len(fakeClient.Actions()); e != a { @@ -445,19 +465,23 @@ func TestUpdateAPIServiceStatus(t *testing.T) { foo := &apiregistration.APIService{Status: apiregistration.APIServiceStatus{Conditions: []apiregistration.APIServiceCondition{{Type: "foo"}}}} bar := &apiregistration.APIService{Status: apiregistration.APIServiceStatus{Conditions: []apiregistration.APIServiceCondition{{Type: "bar"}}}} - fakeClient := fake.NewSimpleClientset() + fakeClient := fake.NewSimpleClientset(foo) c := AvailableConditionController{ apiServiceClient: fakeClient.ApiregistrationV1().(apiregistrationclient.APIServicesGetter), metrics: availabilitymetrics.New(), } - c.updateAPIServiceStatus(foo, foo) + if _, err := c.updateAPIServiceStatus(foo, foo); err != nil { + t.Fatalf("unexpected error: %v", err) + } if e, a := 0, len(fakeClient.Actions()); e != a { t.Error(dump.Pretty(fakeClient.Actions())) } fakeClient.ClearActions() - c.updateAPIServiceStatus(foo, bar) + if _, err := c.updateAPIServiceStatus(foo, bar); err != nil { + t.Fatalf("unexpected error: %v", err) + } if e, a := 1, len(fakeClient.Actions()); e != a { t.Error(dump.Pretty(fakeClient.Actions())) }