Merge pull request juju#18541 from wallyworld/remove-k8s-operator-sto…

…rage

juju#18541

We no longer support specifying operator storage for k8s charms since this was only required for Juju prior to 2.8 and only for older operator charms. This PR removes the model config attributes and associated processing when performing add-k8s etc. It's essentially all code deletion except for the items below.

When migrating an older model, we also want to not import the deprecated model config attribute. There's also other attributes which fall into this category, eg secret-backend, logging-output etc. So the import service for model config has extra logic added - it loads up the model defaults and extracts the attribute names. Any incoming attributes not in this list are excluded from the import.

The other extra change is to ensure workload storage is validated against the k8s cluster at deploy time. This was being done for operator storage but not workload storage, which was an omission.

## QA steps

We need to check that add-k8s correctly probes for storage and sets cloud config. This can be done on microk8s

Without explicitly specifying workload storage
```
microk8s config > ~/.kube/config
juju add-k8s mtest
k8s substrate "microk8s/localhost" added as cloud "mtest".
You can now bootstrap to this cloud by running 'juju bootstrap mtest'.

juju show-cloud mtest
...
config:
 workload-storage: microk8s-hostpath
...
```

Specifying a non default storage class - create a storage class called "test" on the cluster.
```
juju add-k8s mtest2 --storage test
k8s substrate "microk8s/localhost" added as cloud "mtest2" with storage provisioned
by the existing "test" storage class.
You can now bootstrap to this cloud by running 'juju bootstrap mtest2'

juju show-cloud mtest2
...
config:
 workload-storage: test
...
```

deploy a charm
```
juju deploy mysql-k8s --channel=8.0
```
or
```
juju deploy mysql-k8s --channel=8.0 --storage database=test
```

migrate a 3.6 model
```
juju40 bootstrap microk8s c40
juju bootstrap microk8s c36
juju add-model foo
juju model-config workload-storage=foo
juju migrate foo c40
juju switch c40:foo
juju model-config
```

## Documentation changes

@tmihoc we'll need to update the doc to remove any references to "operator-storage" config on k8s clouds.

## Links

**Jira card:** [JUJU-6352](https://warthogs.atlassian.net/browse/JUJU-6352)



[JUJU-6352]: https://warthogs.atlassian.net/browse/JUJU-6352?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

apiserver/facades/client/application/application.go

@@ -23,7 +23,6 @@ import (
 	apiservererrors "github.com/juju/juju/apiserver/errors"
 	"github.com/juju/juju/apiserver/facade"
 	"github.com/juju/juju/caas"
-	k8s "github.com/juju/juju/caas/kubernetes/provider"
 	k8sconstants "github.com/juju/juju/caas/kubernetes/provider/constants"
 	corebase "github.com/juju/juju/core/base"
 	corecharm "github.com/juju/juju/core/charm"
@@ -102,7 +101,6 @@ type APIBase struct {
 
 type CaasBrokerInterface interface {
 	ValidateStorageClass(ctx context.Context, config map[string]interface{}) error
-	Version() (*version.Number, error)
 }
 
 func newFacadeBase(stdCtx context.Context, ctx facade.ModelContext) (*APIBase, error) {
@@ -454,23 +452,17 @@ func (c caasDeployParams) precheck(
 		return fmt.Errorf("getting model config: %w", err)
 	}
 
-	// For older charms, operator-storage model config is mandatory.
-	if k8s.RequireOperatorStorage(c.charm) {
-		storageClassName, _ := cfg.AllAttrs()[k8sconstants.OperatorStorageKey].(string)
-		if storageClassName == "" {
-			return errors.New(
-				"deploying this Kubernetes application requires a suitable storage class.\n" +
-					"None have been configured. Set the operator-storage model config to " +
-					"specify which storage class should be used to allocate operator storage.\n" +
-					"See https://discourse.charmhub.io/t/getting-started/152.",
-			)
+	workloadStorageClass, _ := cfg.AllAttrs()[k8sconstants.WorkloadStorageKey].(string)
+	for storageName, cons := range c.storage {
+		if cons.Pool == "" && workloadStorageClass == "" {
+			return errors.Errorf("storage pool for %q must be specified since there's no model default storage class", storageName)
 		}
-		sp, err := charmStorageParams(ctx, "", storageClassName, cfg, "", storageService, registry)
+		sp, err := charmStorageParams(ctx, "", workloadStorageClass, cfg, cons.Pool, storageService, registry)
 		if err != nil {
-			return errors.Annotatef(err, "getting operator storage params for %q", c.applicationName)
+			return errors.Annotatef(err, "getting workload storage params for %q", c.applicationName)
 		}
 		if sp.Provider != string(k8sconstants.StorageProviderType) {
-			poolName := cfg.AllAttrs()[k8sconstants.OperatorStorageKey]
+			poolName := cfg.AllAttrs()[k8sconstants.WorkloadStorageKey]
 			return errors.Errorf(
 				"the %q storage pool requires a provider type of %q, not %q", poolName, k8sconstants.StorageProviderType, sp.Provider)
 		}
@@ -479,17 +471,6 @@ func (c caasDeployParams) precheck(
 		}
 	}
 
-	workloadStorageClass, _ := cfg.AllAttrs()[k8sconstants.WorkloadStorageKey].(string)
-	for storageName, cons := range c.storage {
-		if cons.Pool == "" && workloadStorageClass == "" {
-			return errors.Errorf("storage pool for %q must be specified since there's no model default storage class", storageName)
-		}
-		_, err := charmStorageParams(ctx, "", workloadStorageClass, cfg, cons.Pool, storageService, registry)
-		if err != nil {
-			return errors.Annotatef(err, "getting workload storage params for %q", c.applicationName)
-		}
-	}
-
 	return nil
 }
 

apiserver/facades/controller/caasapplicationprovisioner/provisioner_test.go

@@ -815,7 +815,6 @@ func (s *CAASApplicationProvisionerSuite) TestCharmStorageParamsPoolNotFound(c *
 
 func (s *CAASApplicationProvisionerSuite) fakeModelConfig() (*envconfig.Config, error) {
 	attrs := coretesting.FakeConfig()
-	attrs["operator-storage"] = "k8s-storage"
 	attrs["agent-version"] = "2.6-beta3.666"
 	return envconfig.New(envconfig.UseDefaults, attrs)
 }

apiserver/facades/controller/caasmodeloperator/mock_test.go

@@ -78,7 +78,6 @@ func (m *mockModel) UUID() string {
 
 func (m *mockModel) ModelConfig(_ context.Context) (*config.Config, error) {
 	attrs := coretesting.FakeConfig()
-	attrs["operator-storage"] = "k8s-storage"
 	attrs["agent-version"] = "2.6-beta3"
 	return config.New(config.UseDefaults, attrs)
 }

caas/kubernetes/metadata.go

@@ -51,7 +51,6 @@ type ClusterMetadataChecker interface {
 // ClusterMetadata defines metadata about a cluster.
 type ClusterMetadata struct {
 	WorkloadStorageClass *storagev1.StorageClass
-	OperatorStorageClass *storagev1.StorageClass
 	Cloud                string
 	Regions              set.Strings
 }

caas/kubernetes/provider/base_test.go

@@ -170,7 +170,6 @@ func (s *BaseSuite) SetUpTest(c *gc.C) {
 	// init config for each test for easier changing config inside test.
 	cfg, err := config.New(config.UseDefaults, coretesting.FakeConfig().Merge(coretesting.Attrs{
 		config.NameKey:                  "test",
-		k8sconstants.OperatorStorageKey: "",
 		k8sconstants.WorkloadStorageKey: "",
 	}))
 	c.Assert(err, jc.ErrorIsNil)
@@ -499,7 +498,6 @@ func (s *fakeClientSuite) SetUpTest(c *gc.C) {
 
 	s.cfg, err = config.New(config.UseDefaults, coretesting.FakeConfig().Merge(coretesting.Attrs{
 		config.NameKey:                  "test",
-		k8sconstants.OperatorStorageKey: "",
 		k8sconstants.WorkloadStorageKey: "",
 	}))
 	c.Assert(err, jc.ErrorIsNil)

caas/kubernetes/provider/bootstrap_test.go

@@ -69,7 +69,6 @@ func (s *bootstrapSuite) SetUpTest(c *gc.C) {
 
 	cfg, err := config.New(config.UseDefaults, coretesting.FakeConfig().Merge(coretesting.Attrs{
 		config.NameKey:                  "controller-1",
-		k8sconstants.OperatorStorageKey: "",
 		k8sconstants.WorkloadStorageKey: "",
 	}))
 	c.Assert(err, jc.ErrorIsNil)

caas/kubernetes/provider/cloud.go

@@ -84,19 +84,10 @@ func UpdateKubeCloudWithStorage(k8sCloud cloud.Cloud, storageParams KubeCloudSto
 		}
 	}
 
-	// We at least expected operator storage to be available to successfully use
-	// this cloud.
-	if clusterMetadata.OperatorStorageClass == nil {
-		return cloud.Cloud{}, &environs.PreferredStorageNotFound{
-			Message: "no preferred operator storage found in Kubernetes cluster",
-		}
-	}
-
 	if k8sCloud.Config == nil {
 		k8sCloud.Config = make(map[string]interface{})
 	}
 
-	k8sCloud.Config[k8sconstants.OperatorStorageKey] = clusterMetadata.OperatorStorageClass.Name
 	k8sCloud.Config[k8sconstants.WorkloadStorageKey] = ""
 	if clusterMetadata.WorkloadStorageClass != nil {
 		k8sCloud.Config[k8sconstants.WorkloadStorageKey] = clusterMetadata.WorkloadStorageClass.Name
@@ -140,7 +131,7 @@ func (p kubernetesEnvironProvider) FinalizeCloud(ctx environs.FinalizeCloudConte
 	cld.AuthTypes = k8scloud.SupportedAuthTypes()
 
 	// if storage is already defined there is no need to query the cluster
-	if opStorage, ok := cld.Config[k8sconstants.OperatorStorageKey]; ok && opStorage != "" {
+	if opStorage, ok := cld.Config[k8sconstants.WorkloadStorageKey]; ok && opStorage != "" {
 		return cld, nil
 	}
 

caas/kubernetes/provider/cloud_test.go

@@ -19,7 +19,6 @@ import (
 
 	"github.com/juju/juju/caas"
 	k8s "github.com/juju/juju/caas/kubernetes"
-	k8scloud "github.com/juju/juju/caas/kubernetes/cloud"
 	"github.com/juju/juju/caas/kubernetes/provider"
 	k8sutils "github.com/juju/juju/caas/kubernetes/provider/utils"
 	jujucloud "github.com/juju/juju/cloud"
@@ -45,9 +44,9 @@ var defaultK8sCloud = jujucloud.Cloud{
 var defaultClusterMetadata = &k8s.ClusterMetadata{
 	Cloud:   k8s.K8sCloudMicrok8s,
 	Regions: set.NewStrings(k8s.Microk8sRegion),
-	OperatorStorageClass: &storagev1.StorageClass{
+	WorkloadStorageClass: &storagev1.StorageClass{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "operator-sc",
+			Name: "workload-sc",
 		},
 	},
 }
@@ -104,37 +103,7 @@ func (s *cloudSuite) TestFinalizeCloudMicrok8s(c *gc.C) {
 		SkipTLSVerify:   true,
 		Endpoint:        "http://1.1.1.1:8080",
 		HostCloudRegion: fmt.Sprintf("%s/%s", k8s.K8sCloudMicrok8s, k8s.Microk8sRegion),
-		Config:          map[string]interface{}{"operator-storage": "operator-sc", "workload-storage": ""},
-		Regions:         []jujucloud.Region{{Name: k8s.Microk8sRegion, Endpoint: "http://1.1.1.1:8080"}},
-	})
-}
-
-func (s *cloudSuite) TestFinalizeCloudMicrok8sAlreadyStorage(c *gc.C) {
-	preparedCloud := jujucloud.Cloud{
-		Name:            k8s.K8sCloudMicrok8s,
-		Type:            jujucloud.CloudTypeKubernetes,
-		AuthTypes:       []jujucloud.AuthType{jujucloud.UserPassAuthType},
-		CACertificates:  []string{""},
-		Endpoint:        "http://1.1.1.1:8080",
-		HostCloudRegion: fmt.Sprintf("%s/%s", k8s.K8sCloudMicrok8s, k8s.Microk8sRegion),
-		Config:          map[string]interface{}{"operator-storage": "something-else", "workload-storage": ""},
-		Regions:         []jujucloud.Region{{Name: k8s.Microk8sRegion, Endpoint: "http://1.1.1.1:8080"}},
-	}
-
-	p := s.getProvider()
-	cloudFinalizer := p.(environs.CloudFinalizer)
-
-	ctx := mockContext{Context: context.Background()}
-	cloud, err := cloudFinalizer.FinalizeCloud(&ctx, preparedCloud)
-	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(cloud, jc.DeepEquals, jujucloud.Cloud{
-		Name:            k8s.K8sCloudMicrok8s,
-		Type:            jujucloud.CloudTypeKubernetes,
-		AuthTypes:       k8scloud.SupportedAuthTypes(),
-		CACertificates:  []string{""},
-		Endpoint:        "http://1.1.1.1:8080",
-		HostCloudRegion: fmt.Sprintf("%s/%s", k8s.K8sCloudMicrok8s, k8s.Microk8sRegion),
-		Config:          map[string]interface{}{"operator-storage": "something-else", "workload-storage": ""},
+		Config:          map[string]interface{}{"workload-storage": "workload-sc"},
 		Regions:         []jujucloud.Region{{Name: k8s.Microk8sRegion, Endpoint: "http://1.1.1.1:8080"}},
 	})
 }

caas/kubernetes/provider/constants/storage.go

@@ -28,10 +28,6 @@ const (
 	// WorkloadStorageKey is the model config attribute used to specify
 	// the storage class for provisioning workload storage.
 	WorkloadStorageKey = "workload-storage"
-
-	// OperatorStorageKey is the model config attribute used to specify
-	// the storage class for provisioning operator storage.
-	OperatorStorageKey = "operator-storage"
 )
 
 // QualifiedStorageClassName returns a qualified storage class name.

caas/kubernetes/provider/k8s.go

@@ -387,7 +387,7 @@ Please bootstrap again and choose a different controller name.`, controllerName)
 	// The namespace will be set to controller-name in newcontrollerStack.
 
 	// do validation on storage class.
-	_, err = k.validateOperatorStorage(ctx)
+	_, err = k.validateControllerWorkloadStorage(ctx)
 	return errors.Trace(err)
 }
 
@@ -417,10 +417,10 @@ func (k *kubernetesClient) EnsureImageRepoSecret(ctx context.Context, imageRepo
 	return errors.Trace(err)
 }
 
-func (k *kubernetesClient) validateOperatorStorage(ctx context.Context) (string, error) {
-	storageClass, _ := k.Config().AllAttrs()[constants.OperatorStorageKey].(string)
+func (k *kubernetesClient) validateControllerWorkloadStorage(ctx context.Context) (string, error) {
+	storageClass, _ := k.Config().AllAttrs()[constants.WorkloadStorageKey].(string)
 	if storageClass == "" {
-		return "", errors.NewNotValid(nil, "config without operator-storage value not valid.\nRun juju add-k8s to reimport your k8s cluster.")
+		return "", errors.NewNotValid(nil, "config without workload-storage value not valid.\nRun juju add-k8s to reimport your k8s cluster.")
 	}
 	_, err := k.getStorageClass(ctx, storageClass)
 	return storageClass, errors.Trace(err)
@@ -437,7 +437,8 @@ func (k *kubernetesClient) Bootstrap(
 		return nil, errors.NotSupportedf("set base for bootstrapping to kubernetes")
 	}
 
-	storageClass, err := k.validateOperatorStorage(ctx)
+	// Validate workload storage is available to the controller.
+	storageClass, err := k.validateControllerWorkloadStorage(ctx)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}

caas/kubernetes/provider/k8s_test.go

@@ -245,7 +245,7 @@ func (s *K8sBrokerSuite) TestSetConfig(c *gc.C) {
 	c.Assert(err, jc.ErrorIsNil)
 }
 
-func (s *K8sBrokerSuite) TestBootstrapNoOperatorStorage(c *gc.C) {
+func (s *K8sBrokerSuite) TestBootstrapNoWorkloadStorage(c *gc.C) {
 	ctrl := s.setupController(c)
 	defer ctrl.Finish()
 
@@ -260,15 +260,15 @@ func (s *K8sBrokerSuite) TestBootstrapNoOperatorStorage(c *gc.C) {
 	_, err := s.broker.Bootstrap(ctx, callCtx, bootstrapParams)
 	c.Assert(err, gc.NotNil)
 	msg := strings.Replace(err.Error(), "\n", "", -1)
-	c.Assert(msg, gc.Matches, "config without operator-storage value not valid.*")
+	c.Assert(msg, gc.Matches, "config without workload-storage value not valid.*")
 }
 
 func (s *K8sBrokerSuite) TestBootstrap(c *gc.C) {
 	ctrl := s.setupController(c)
 	defer ctrl.Finish()
 
-	// Ensure the broker is configured with operator storage.
-	s.setupOperatorStorageConfig(c)
+	// Ensure the broker is configured with workload storage.
+	s.setupWorkloadStorageConfig(c)
 
 	ctx := envtesting.BootstrapContext(context.Background(), c)
 	callCtx := envcontext.WithoutCredentialInvalidator(ctx)
@@ -284,10 +284,7 @@ func (s *K8sBrokerSuite) TestBootstrap(c *gc.C) {
 		},
 	}
 	gomock.InOrder(
-		// Check the operator storage exists.
 		s.mockStorageClass.EXPECT().Get(gomock.Any(), "test-some-storage", v1.GetOptions{}).
-			Return(nil, s.k8sNotFoundError()),
-		s.mockStorageClass.EXPECT().Get(gomock.Any(), "some-storage", v1.GetOptions{}).
 			Return(sc, nil),
 	)
 	result, err := s.broker.Bootstrap(ctx, callCtx, bootstrapParams)
@@ -300,10 +297,10 @@ func (s *K8sBrokerSuite) TestBootstrap(c *gc.C) {
 	c.Assert(err, jc.ErrorIs, errors.NotSupported)
 }
 
-func (s *K8sBrokerSuite) setupOperatorStorageConfig(c *gc.C) {
+func (s *K8sBrokerSuite) setupWorkloadStorageConfig(c *gc.C) {
 	cfg := s.broker.Config()
 	var err error
-	cfg, err = cfg.Apply(map[string]interface{}{"operator-storage": "some-storage"})
+	cfg, err = cfg.Apply(map[string]interface{}{"workload-storage": "some-storage"})
 	c.Assert(err, jc.ErrorIsNil)
 	err = s.broker.SetConfig(context.Background(), cfg)
 	c.Assert(err, jc.ErrorIsNil)
@@ -313,8 +310,8 @@ func (s *K8sBrokerSuite) TestPrepareForBootstrap(c *gc.C) {
 	ctrl := s.setupController(c)
 	defer ctrl.Finish()
 
-	// Ensure the broker is configured with operator storage.
-	s.setupOperatorStorageConfig(c)
+	// Ensure the broker is configured with workload storage.
+	s.setupWorkloadStorageConfig(c)
 
 	sc := &storagev1.StorageClass{
 		ObjectMeta: v1.ObjectMeta{