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