From 855a1c17131f92fca6de33279a02eca3893ca374 Mon Sep 17 00:00:00 2001 From: Devan Goodwin Date: Tue, 18 Jul 2017 11:52:42 -0300 Subject: [PATCH] Fix unused Secret export logic. The strategy used for the secret store defined custom export logic, and had accompanying unit tests. However the secret storage did not actually wire this up by setting an ExportStrategy and thus the code was never used in the real world. This change fixes the missing assignment and adds testing at a higher level to ensure any uses of the generic registry.Store that we expect to have an ExportStrategy do, and no others. Several other strategies in the RBAC package also appeared to have unwired Export logic, however their implementations were all empty leading me to believe that these are not considered exportable. The empty methods have now been removed. --- pkg/master/BUILD | 4 + pkg/master/master_test.go | 59 ++++++++++++++ .../certificates/storage/storage.go | 1 + pkg/registry/core/secret/storage/storage.go | 1 + pkg/registry/rbac/clusterrole/strategy.go | 4 - .../rbac/clusterrolebinding/strategy.go | 4 - pkg/registry/rbac/role/strategy.go | 4 - pkg/registry/rbac/rolebinding/strategy.go | 4 - pkg/registry/registrytest/BUILD | 2 + pkg/registry/registrytest/validate.go | 79 +++++++++++++++++++ .../pkg/registry/generic/registry/store.go | 29 +++++++ .../apiserver/pkg/registry/rest/rest.go | 4 +- 12 files changed, 178 insertions(+), 17 deletions(-) create mode 100644 pkg/registry/registrytest/validate.go diff --git a/pkg/master/BUILD b/pkg/master/BUILD index 54de9a3e371..6d3cf8c61b0 100644 --- a/pkg/master/BUILD +++ b/pkg/master/BUILD @@ -124,6 +124,9 @@ go_test( "//pkg/client/clientset_generated/internalclientset/fake:go_default_library", "//pkg/generated/openapi:go_default_library", "//pkg/kubelet/client:go_default_library", + "//pkg/registry/certificates/rest:go_default_library", + "//pkg/registry/core/rest:go_default_library", + "//pkg/registry/registrytest:go_default_library", "//pkg/version:go_default_library", "//vendor/github.com/go-openapi/loads:go_default_library", "//vendor/github.com/go-openapi/spec:go_default_library", @@ -134,6 +137,7 @@ go_test( "//vendor/k8s.io/api/autoscaling/v1:go_default_library", "//vendor/k8s.io/api/batch/v1:go_default_library", "//vendor/k8s.io/api/batch/v2alpha1:go_default_library", + "//vendor/k8s.io/api/certificates/v1beta1:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/api/extensions/v1beta1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", diff --git a/pkg/master/master_test.go b/pkg/master/master_test.go index dc7d6a1af9f..40e3abeefe1 100644 --- a/pkg/master/master_test.go +++ b/pkg/master/master_test.go @@ -30,6 +30,7 @@ import ( autoscalingapiv1 "k8s.io/api/autoscaling/v1" batchapiv1 "k8s.io/api/batch/v1" batchapiv2alpha1 "k8s.io/api/batch/v2alpha1" + certificatesapiv1beta1 "k8s.io/api/certificates/v1beta1" apiv1 "k8s.io/api/core/v1" extensionsapiv1beta1 "k8s.io/api/extensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -56,6 +57,9 @@ import ( "k8s.io/kubernetes/pkg/apis/extensions" "k8s.io/kubernetes/pkg/apis/rbac" kubeletclient "k8s.io/kubernetes/pkg/kubelet/client" + certificatesrest "k8s.io/kubernetes/pkg/registry/certificates/rest" + corerest "k8s.io/kubernetes/pkg/registry/core/rest" + "k8s.io/kubernetes/pkg/registry/registrytest" kubeversion "k8s.io/kubernetes/pkg/version" "github.com/stretchr/testify/assert" @@ -112,6 +116,61 @@ func setUp(t *testing.T) (*etcdtesting.EtcdTestServer, Config, *assert.Assertion return server, *config, assert.New(t) } +// TestLegacyRestStorageStrategies ensures that all Storage objects which are using the generic registry Store have +// their various strategies properly wired up. This surfaced as a bug where strategies defined Export functions, but +// they were never used outside of unit tests because the export strategies were not assigned inside the Store. +func TestLegacyRestStorageStrategies(t *testing.T) { + _, _, masterCfg, _ := newMaster(t) + storageProvider := corerest.LegacyRESTStorageProvider{ + StorageFactory: masterCfg.StorageFactory, + ProxyTransport: masterCfg.ProxyTransport, + KubeletClientConfig: masterCfg.KubeletClientConfig, + EventTTL: masterCfg.EventTTL, + ServiceIPRange: masterCfg.ServiceIPRange, + ServiceNodePortRange: masterCfg.ServiceNodePortRange, + LoopbackClientConfig: masterCfg.GenericConfig.LoopbackClientConfig, + } + + _, apiGroupInfo, err := storageProvider.NewLegacyRESTStorage(masterCfg.GenericConfig.RESTOptionsGetter) + if err != nil { + t.Errorf("failed to create legacy REST storage: %v", err) + } + + // Any new stores with export logic will need to be added here: + exceptions := registrytest.StrategyExceptions{ + // Only these stores should have an export strategy defined: + HasExportStrategy: []string{ + "secrets", + "limitRanges", + "nodes", + "podTemplates", + }, + } + + strategyErrors := registrytest.ValidateStorageStrategies(apiGroupInfo.VersionedResourcesStorageMap["v1"], exceptions) + for _, err := range strategyErrors { + t.Error(err) + } +} + +func TestCertificatesRestStorageStrategies(t *testing.T) { + _, _, masterCfg, _ := newMaster(t) + certStorageProvider := certificatesrest.RESTStorageProvider{} + apiGroupInfo, _ := certStorageProvider.NewRESTStorage(masterCfg.APIResourceConfigSource, masterCfg.GenericConfig.RESTOptionsGetter) + + exceptions := registrytest.StrategyExceptions{ + HasExportStrategy: []string{ + "certificatesigningrequests", + }, + } + + strategyErrors := registrytest.ValidateStorageStrategies( + apiGroupInfo.VersionedResourcesStorageMap[certificatesapiv1beta1.SchemeGroupVersion.Version], exceptions) + for _, err := range strategyErrors { + t.Error(err) + } +} + func newMaster(t *testing.T) (*Master, *etcdtesting.EtcdTestServer, Config, *assert.Assertions) { etcdserver, config, assert := setUp(t) diff --git a/pkg/registry/certificates/certificates/storage/storage.go b/pkg/registry/certificates/certificates/storage/storage.go index 372717f3ee3..715dbbe848c 100644 --- a/pkg/registry/certificates/certificates/storage/storage.go +++ b/pkg/registry/certificates/certificates/storage/storage.go @@ -45,6 +45,7 @@ func NewREST(optsGetter generic.RESTOptionsGetter) (*REST, *StatusREST, *Approva CreateStrategy: csrregistry.Strategy, UpdateStrategy: csrregistry.Strategy, DeleteStrategy: csrregistry.Strategy, + ExportStrategy: csrregistry.Strategy, } options := &generic.StoreOptions{RESTOptions: optsGetter} if err := store.CompleteWithOptions(options); err != nil { diff --git a/pkg/registry/core/secret/storage/storage.go b/pkg/registry/core/secret/storage/storage.go index 8dc5592eed0..2827bdb1614 100644 --- a/pkg/registry/core/secret/storage/storage.go +++ b/pkg/registry/core/secret/storage/storage.go @@ -42,6 +42,7 @@ func NewREST(optsGetter generic.RESTOptionsGetter) *REST { CreateStrategy: secret.Strategy, UpdateStrategy: secret.Strategy, DeleteStrategy: secret.Strategy, + ExportStrategy: secret.Strategy, } options := &generic.StoreOptions{RESTOptions: optsGetter, AttrFunc: secret.GetAttrs} if err := store.CompleteWithOptions(options); err != nil { diff --git a/pkg/registry/rbac/clusterrole/strategy.go b/pkg/registry/rbac/clusterrole/strategy.go index 7d4e766a880..301418a1411 100644 --- a/pkg/registry/rbac/clusterrole/strategy.go +++ b/pkg/registry/rbac/clusterrole/strategy.go @@ -93,7 +93,3 @@ func (strategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.O func (strategy) AllowUnconditionalUpdate() bool { return true } - -func (s strategy) Export(ctx genericapirequest.Context, obj runtime.Object, exact bool) error { - return nil -} diff --git a/pkg/registry/rbac/clusterrolebinding/strategy.go b/pkg/registry/rbac/clusterrolebinding/strategy.go index 383be8bd2d9..e1d83ecf4ab 100644 --- a/pkg/registry/rbac/clusterrolebinding/strategy.go +++ b/pkg/registry/rbac/clusterrolebinding/strategy.go @@ -93,7 +93,3 @@ func (strategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.O func (strategy) AllowUnconditionalUpdate() bool { return true } - -func (s strategy) Export(ctx genericapirequest.Context, obj runtime.Object, exact bool) error { - return nil -} diff --git a/pkg/registry/rbac/role/strategy.go b/pkg/registry/rbac/role/strategy.go index da80adc4ed7..619cfeed125 100644 --- a/pkg/registry/rbac/role/strategy.go +++ b/pkg/registry/rbac/role/strategy.go @@ -93,7 +93,3 @@ func (strategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.O func (strategy) AllowUnconditionalUpdate() bool { return true } - -func (s strategy) Export(ctx genericapirequest.Context, obj runtime.Object, exact bool) error { - return nil -} diff --git a/pkg/registry/rbac/rolebinding/strategy.go b/pkg/registry/rbac/rolebinding/strategy.go index 0abeb7ed76c..9d4781a50c3 100644 --- a/pkg/registry/rbac/rolebinding/strategy.go +++ b/pkg/registry/rbac/rolebinding/strategy.go @@ -93,7 +93,3 @@ func (strategy) ValidateUpdate(ctx genericapirequest.Context, obj, old runtime.O func (strategy) AllowUnconditionalUpdate() bool { return true } - -func (s strategy) Export(ctx genericapirequest.Context, obj runtime.Object, exact bool) error { - return nil -} diff --git a/pkg/registry/registrytest/BUILD b/pkg/registry/registrytest/BUILD index a485cae0130..d6a0239b1cf 100644 --- a/pkg/registry/registrytest/BUILD +++ b/pkg/registry/registrytest/BUILD @@ -17,11 +17,13 @@ go_library( "node.go", "service.go", "shortNamesProvider.go", + "validate.go", ], tags = ["automanaged"], deps = [ "//pkg/api:go_default_library", "//pkg/api/testapi:go_default_library", + "//pkg/util/slice:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/internalversion:go_default_library", diff --git a/pkg/registry/registrytest/validate.go b/pkg/registry/registrytest/validate.go new file mode 100644 index 00000000000..40b92270020 --- /dev/null +++ b/pkg/registry/registrytest/validate.go @@ -0,0 +1,79 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package registrytest + +import ( + "fmt" + "k8s.io/apiserver/pkg/registry/generic/registry" + "k8s.io/apiserver/pkg/registry/rest" + "k8s.io/kubernetes/pkg/util/slice" +) + +// ValidateStorageStrategies ensures any instances of the generic registry.Store in the given storage map +// have expected strategies defined. +func ValidateStorageStrategies(storageMap map[string]rest.Storage, exceptions StrategyExceptions) []error { + errs := []error{} + + // Used to ensure we saw all the expected exceptions: + hasExportExceptionsSeen := []string{} + + for k, storage := range storageMap { + switch t := storage.(type) { + case registry.GenericStore: + // At this point it appears all uses of the generic registry store should have a create, update, and + // delete strategy set: + if t.GetCreateStrategy() == nil { + errs = append(errs, fmt.Errorf("store for type [%v] does not have a CreateStrategy", k)) + } + if t.GetUpdateStrategy() == nil { + errs = append(errs, fmt.Errorf("store for type [%v] does not have an UpdateStrategy", k)) + } + if t.GetDeleteStrategy() == nil { + errs = append(errs, fmt.Errorf("store for type [%v] does not have a DeleteStrategy", k)) + } + + // Check that ExportStrategy is set if applicable: + if slice.ContainsString(exceptions.HasExportStrategy, k, nil) { + hasExportExceptionsSeen = append(hasExportExceptionsSeen, k) + if t.GetExportStrategy() == nil { + errs = append(errs, fmt.Errorf("store for type [%v] does not have an ExportStrategy", k)) + } + } else { + // By default we expect Stores to not have additional export logic: + if t.GetExportStrategy() != nil { + errs = append(errs, fmt.Errorf("store for type [%v] has an unexpected ExportStrategy", k)) + } + } + + } + } + + // Ensure that we saw all our expected exceptions: + for _, expKey := range exceptions.HasExportStrategy { + if !slice.ContainsString(hasExportExceptionsSeen, expKey, nil) { + errs = append(errs, fmt.Errorf("no generic store seen for expected ExportStrategy: %v", expKey)) + } + } + + return errs +} + +// StrategyExceptions carries information on what exceptions to default strategy expectations are expected. +type StrategyExceptions struct { + // HasExportStrategy is a list of the resource keys whose store should have a custom export strategy. + HasExportStrategy []string +} diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go index 3b514376cf5..fd352123627 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go @@ -52,6 +52,14 @@ import ( // object. type ObjectFunc func(obj runtime.Object) error +// GenericStore interface can be used for type assertions when we need to access the underlying strategies. +type GenericStore interface { + GetCreateStrategy() rest.RESTCreateStrategy + GetUpdateStrategy() rest.RESTUpdateStrategy + GetDeleteStrategy() rest.RESTDeleteStrategy + GetExportStrategy() rest.RESTExportStrategy +} + // Store implements pkg/api/rest.StandardStorage. It's intended to be // embeddable and allows the consumer to implement any non-generic functions // that are required. This object is intended to be copyable so that it can be @@ -177,6 +185,7 @@ type Store struct { var _ rest.StandardStorage = &Store{} var _ rest.Exporter = &Store{} var _ rest.TableConvertor = &Store{} +var _ GenericStore = &Store{} const OptimisticLockErrorMsg = "the object has been modified; please apply your changes to the latest version and try again" @@ -233,6 +242,26 @@ func (e *Store) NewList() runtime.Object { return e.NewListFunc() } +// GetCreateStrategy implements GenericStore. +func (e *Store) GetCreateStrategy() rest.RESTCreateStrategy { + return e.CreateStrategy +} + +// GetUpdateStrategy implements GenericStore. +func (e *Store) GetUpdateStrategy() rest.RESTUpdateStrategy { + return e.UpdateStrategy +} + +// GetDeleteStrategy implements GenericStore. +func (e *Store) GetDeleteStrategy() rest.RESTDeleteStrategy { + return e.DeleteStrategy +} + +// GetExportStrategy implements GenericStore. +func (e *Store) GetExportStrategy() rest.RESTExportStrategy { + return e.ExportStrategy +} + // List returns a list of items matching labels and field according to the // store's PredicateFunc. func (e *Store) List(ctx genericapirequest.Context, options *metainternalversion.ListOptions) (runtime.Object, error) { diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go index e7e9259a9aa..99a2fbe0f18 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go @@ -86,7 +86,9 @@ type Lister interface { List(ctx genericapirequest.Context, options *metainternalversion.ListOptions) (runtime.Object, error) } -// Exporter is an object that knows how to strip a RESTful resource for export +// Exporter is an object that knows how to strip a RESTful resource for export. A store should implement this interface +// if export is generally supported for that type. Errors can still be returned during the actual Export when certain +// instances of the type are not exportable. type Exporter interface { // Export an object. Fields that are not user specified (e.g. Status, ObjectMeta.ResourceVersion) are stripped out // Returns the stripped object. If 'exact' is true, fields that are specific to the cluster (e.g. namespace) are