Merge pull request #49321 from dgoodwin/export-wiring

Automatic merge from submit-queue (batch tested with PRs 49615, 49321, 49982, 49788, 50355)

Fix unused Secret export logic.

**What this PR does / why we need it**:

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.

**Which issue this PR fixes**: fixes #49042

**Release note**:

```release-note
```

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",

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)
 

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 {

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 {

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
-}

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
-}

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
-}

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
-}

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",

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
+}

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) {

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