From a4c9330683c55c9bbebece2ad305f939e725aff0 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Mon, 26 Oct 2020 21:49:57 -0700 Subject: [PATCH] Populate ClusterIPs on read Old stored services will not have the `clusterIPs` field when read back without this. This includes some renaming for clarity and expanded comments, and a new test for default on read. --- pkg/registry/core/service/storage/storage.go | 48 +++--- .../core/service/storage/storage_test.go | 158 +++++++++++++++++- pkg/registry/core/service/strategy.go | 9 +- pkg/registry/core/service/strategy_test.go | 2 +- 4 files changed, 189 insertions(+), 28 deletions(-) diff --git a/pkg/registry/core/service/storage/storage.go b/pkg/registry/core/service/storage/storage.go index 56fc7e04058bd..ba7d86980aa54 100644 --- a/pkg/registry/core/service/storage/storage.go +++ b/pkg/registry/core/service/storage/storage.go @@ -25,17 +25,17 @@ import ( "k8s.io/apiserver/pkg/registry/generic" genericregistry "k8s.io/apiserver/pkg/registry/generic/registry" "k8s.io/apiserver/pkg/registry/rest" + utilfeature "k8s.io/apiserver/pkg/util/feature" api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/printers" printersinternal "k8s.io/kubernetes/pkg/printers/internalversion" printerstorage "k8s.io/kubernetes/pkg/printers/storage" "k8s.io/kubernetes/pkg/registry/core/service" registry "k8s.io/kubernetes/pkg/registry/core/service" + svcreg "k8s.io/kubernetes/pkg/registry/core/service" netutil "k8s.io/utils/net" - - utilfeature "k8s.io/apiserver/pkg/util/feature" - "k8s.io/kubernetes/pkg/features" ) type GenericREST struct { @@ -85,7 +85,7 @@ func NewGenericREST(optsGetter generic.RESTOptionsGetter, serviceCIDR net.IPNet, } } genericStore := &GenericREST{store, primaryIPFamily, secondaryFamily} - store.Decorator = genericStore.defaultServiceOnRead // default on read + store.Decorator = genericStore.defaultOnRead return genericStore, &StatusREST{store: &statusStore}, nil } @@ -126,35 +126,35 @@ func (r *StatusREST) Update(ctx context.Context, name string, objInfo rest.Updat return r.store.Update(ctx, name, objInfo, createValidation, updateValidation, false, options) } -// defaults fields that were not previously set on read. becomes an -// essential part of upgrading a service -func (r *GenericREST) defaultServiceOnRead(obj runtime.Object) error { - if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) { - return nil - } - +// defaultOnRead sets interlinked fields that were not previously set on read. +// We can't do this in the normal defaulting path because that same logic +// applies on Get, Create, and Update, but we need to distinguish between them. +// +// This will be called on both Service and ServiceList types. +func (r *GenericREST) defaultOnRead(obj runtime.Object) error { service, ok := obj.(*api.Service) if ok { - return r.defaultAServiceOnRead(service) + return r.defaultOnReadService(service) } serviceList, ok := obj.(*api.ServiceList) if ok { - return r.defaultServiceList(serviceList) + return r.defaultOnReadServiceList(serviceList) } - // this was not an object we can default + // This was not an object we can default. This is not an error, as the + // caching layer can pass through here, too. return nil } -// defaults a service list -func (r *GenericREST) defaultServiceList(serviceList *api.ServiceList) error { +// defaultOnReadServiceList defaults a ServiceList. +func (r *GenericREST) defaultOnReadServiceList(serviceList *api.ServiceList) error { if serviceList == nil { return nil } for i := range serviceList.Items { - err := r.defaultAServiceOnRead(&serviceList.Items[i]) + err := r.defaultOnReadService(&serviceList.Items[i]) if err != nil { return err } @@ -163,12 +163,22 @@ func (r *GenericREST) defaultServiceList(serviceList *api.ServiceList) error { return nil } -// defaults a single service -func (r *GenericREST) defaultAServiceOnRead(service *api.Service) error { +// defaultOnReadService defaults a single Service. +func (r *GenericREST) defaultOnReadService(service *api.Service) error { if service == nil { return nil } + // We might find Services that were written before ClusterIP became plural. + // We still want to present a consistent view of them. + // NOTE: the args are (old, new) + svcreg.NormalizeClusterIPs(nil, service) + + // The rest of this does not apply unless dual-stack is enabled. + if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) { + return nil + } + if len(service.Spec.IPFamilies) > 0 { return nil // already defaulted } diff --git a/pkg/registry/core/service/storage/storage_test.go b/pkg/registry/core/service/storage/storage_test.go index 0fef9c32431a3..271e47c27acf3 100644 --- a/pkg/registry/core/service/storage/storage_test.go +++ b/pkg/registry/core/service/storage/storage_test.go @@ -18,6 +18,7 @@ package storage import ( "net" + "reflect" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -322,6 +323,155 @@ func makeServiceList() (undefaulted, defaulted *api.ServiceList) { return undefaulted, defaulted } +func TestServiceDefaultOnRead(t *testing.T) { + // Helper makes a mostly-valid Service. Test-cases can tweak it as needed. + makeService := func(tweak func(*api.Service)) *api.Service { + svc := &api.Service{ + ObjectMeta: metav1.ObjectMeta{Name: "svc", Namespace: "ns"}, + Spec: api.ServiceSpec{ + Type: api.ServiceTypeClusterIP, + ClusterIP: "1.2.3.4", + ClusterIPs: []string{"1.2.3.4"}, + }, + } + if tweak != nil { + tweak(svc) + } + return svc + } + // Helper makes a mostly-valid ServiceList. Test-cases can tweak it as needed. + makeServiceList := func(tweak func(*api.ServiceList)) *api.ServiceList { + list := &api.ServiceList{ + Items: []api.Service{{ + ObjectMeta: metav1.ObjectMeta{Name: "svc", Namespace: "ns"}, + Spec: api.ServiceSpec{ + Type: api.ServiceTypeClusterIP, + ClusterIP: "1.2.3.4", + ClusterIPs: []string{"1.2.3.4"}, + }, + }}, + } + if tweak != nil { + tweak(list) + } + return list + } + + testCases := []struct { + name string + input runtime.Object + expectErr bool + expect runtime.Object + }{{ + name: "no change v4", + input: makeService(nil), + expect: makeService(nil), + }, { + name: "missing clusterIPs v4", + input: makeService(func(svc *api.Service) { + svc.Spec.ClusterIPs = nil + }), + expect: makeService(nil), + }, { + name: "no change v6", + input: makeService(func(svc *api.Service) { + svc.Spec.ClusterIP = "2000::" + svc.Spec.ClusterIPs = []string{"2000::"} + }), + expect: makeService(func(svc *api.Service) { + svc.Spec.ClusterIP = "2000::" + svc.Spec.ClusterIPs = []string{"2000::"} + }), + }, { + name: "missing clusterIPs v6", + input: makeService(func(svc *api.Service) { + svc.Spec.ClusterIP = "2000::" + svc.Spec.ClusterIPs = nil + }), + expect: makeService(func(svc *api.Service) { + svc.Spec.ClusterIP = "2000::" + svc.Spec.ClusterIPs = []string{"2000::"} + }), + }, { + name: "list, no change v4", + input: makeServiceList(nil), + expect: makeServiceList(nil), + }, { + name: "list, missing clusterIPs v4", + input: makeServiceList(func(list *api.ServiceList) { + list.Items[0].Spec.ClusterIPs = nil + }), + expect: makeService(nil), + }, { + name: "not Service or ServiceList", + input: &api.Pod{}, + expectErr: false, + }} + + for _, tc := range testCases { + makeStorage := func(t *testing.T) (*GenericREST, *etcd3testing.EtcdTestServer) { + etcdStorage, server := registrytest.NewEtcdStorage(t, "") + restOptions := generic.RESTOptions{ + StorageConfig: etcdStorage, + Decorator: generic.UndecoratedStorage, + DeleteCollectionWorkers: 1, + ResourcePrefix: "services", + } + + _, cidr, err := net.ParseCIDR("10.0.0.0/24") + if err != nil { + t.Fatalf("failed to parse CIDR") + } + + serviceStorage, _, err := NewGenericREST(restOptions, *cidr, false) + if err != nil { + t.Fatalf("unexpected error from REST storage: %v", err) + } + return serviceStorage, server + } + t.Run(tc.name, func(t *testing.T) { + storage, server := makeStorage(t) + defer server.Terminate(t) + defer storage.Store.DestroyFunc() + + tmp := tc.input.DeepCopyObject() + err := storage.defaultOnRead(tmp) + if err != nil && !tc.expectErr { + t.Errorf("unexpected error: %v", err) + } + if err == nil && tc.expectErr { + t.Errorf("unexpected success") + } + + svc, ok := tmp.(*api.Service) + if !ok { + list, ok := tmp.(*api.ServiceList) + if !ok { + return + } + svc = &list.Items[0] + } + + exp, ok := tc.expect.(*api.Service) + if !ok { + list, ok := tc.expect.(*api.ServiceList) + if !ok { + return + } + exp = &list.Items[0] + } + + // Verify fields we know are affected + if svc.Spec.ClusterIP != exp.Spec.ClusterIP { + t.Errorf("clusterIP: expected %v, got %v", exp.Spec.ClusterIP, svc.Spec.ClusterIP) + } + if !reflect.DeepEqual(svc.Spec.ClusterIPs, exp.Spec.ClusterIPs) { + t.Errorf("clusterIPs: expected %v, got %v", exp.Spec.ClusterIPs, svc.Spec.ClusterIPs) + } + }) + } +} + func TestServiceDefaulting(t *testing.T) { makeStorage := func(t *testing.T, primaryCIDR string, isDualStack bool) (*GenericREST, *StatusREST, *etcd3testing.EtcdTestServer) { etcdStorage, server := registrytest.NewEtcdStorage(t, "") @@ -455,15 +605,15 @@ func TestServiceDefaulting(t *testing.T) { } copyUndefaultedList := undefaultedServiceList.DeepCopy() - // run for each service + // run for each Service for i, svc := range copyUndefaultedList.Items { - storage.defaultServiceOnRead(&svc) + storage.defaultOnRead(&svc) compareSvc(svc, defaultedServiceList.Items[i]) } copyUndefaultedList = undefaultedServiceList.DeepCopy() - // run as a servicr list - storage.defaultServiceOnRead(copyUndefaultedList) + // run as a ServiceList + storage.defaultOnRead(copyUndefaultedList) for i, svc := range copyUndefaultedList.Items { compareSvc(svc, defaultedServiceList.Items[i]) } diff --git a/pkg/registry/core/service/strategy.go b/pkg/registry/core/service/strategy.go index 96e34c1c11905..c84e0315ce9ea 100644 --- a/pkg/registry/core/service/strategy.go +++ b/pkg/registry/core/service/strategy.go @@ -97,7 +97,7 @@ func (strategy svcStrategy) PrepareForCreate(ctx context.Context, obj runtime.Ob service := obj.(*api.Service) service.Status = api.ServiceStatus{} - normalizeClusterIPs(nil, service) + NormalizeClusterIPs(nil, service) dropServiceDisabledFields(service, nil) } @@ -107,7 +107,7 @@ func (strategy svcStrategy) PrepareForUpdate(ctx context.Context, obj, old runti oldService := old.(*api.Service) newService.Status = oldService.Status - normalizeClusterIPs(oldService, newService) + NormalizeClusterIPs(oldService, newService) dropServiceDisabledFields(newService, oldService) dropTypeDependentFields(newService, oldService) trimFieldsForDualStackDowngrade(newService, oldService) @@ -224,8 +224,9 @@ func (serviceStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtim return validation.ValidateServiceStatusUpdate(obj.(*api.Service), old.(*api.Service)) } -// normalizeClusterIPs adjust clusterIPs based on ClusterIP -func normalizeClusterIPs(oldSvc *api.Service, newSvc *api.Service) { +// NormalizeClusterIPs adjust clusterIPs based on ClusterIP. This must not +// consider any other fields. +func NormalizeClusterIPs(oldSvc, newSvc *api.Service) { // In all cases here, we don't need to over-think the inputs. Validation // will be called on the new object soon enough. All this needs to do is // try to divine what user meant with these linked fields. The below diff --git a/pkg/registry/core/service/strategy_test.go b/pkg/registry/core/service/strategy_test.go index f437719962108..38e14c8cbec7f 100644 --- a/pkg/registry/core/service/strategy_test.go +++ b/pkg/registry/core/service/strategy_test.go @@ -704,7 +704,7 @@ func TestNormalizeClusterIPs(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - normalizeClusterIPs(tc.oldService, tc.newService) + NormalizeClusterIPs(tc.oldService, tc.newService) if tc.newService == nil { t.Fatalf("unexpected new service to be nil")