Merge pull request #831 from red-hat-storage/sync_us--master

Syncing latest changes from upstream master for rook

Documentation/Helm-Charts/operator-chart.md

@@ -157,6 +157,7 @@ The following table lists the configurable parameters of the rook-operator chart
 | `logLevel` | Global log level for the operator. Options: `ERROR`, `WARNING`, `INFO`, `DEBUG` | `"INFO"` |
 | `monitoring.enabled` | Enable monitoring. Requires Prometheus to be pre-installed. Enabling will also create RBAC rules to allow Operator to create ServiceMonitors | `false` |
 | `nodeSelector` | Kubernetes [`nodeSelector`](https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#nodeselector) to add to the Deployment. | `{}` |
+| `obcAllowAdditionalConfigFields` | Many OBC additional config fields may be risky for administrators to allow users control over. The safe and default-allowed fields are 'maxObjects' and 'maxSize'. Other fields should be considered risky. To allow all additional configs, use this value:   "maxObjects,maxSize,bucketMaxObjects,bucketMaxSize,bucketPolicy,bucketLifecycle,bucketOwner" | "maxObjects,maxSize" |
 | `obcProvisionerNamePrefix` | Specify the prefix for the OBC provisioner in place of the cluster namespace | `ceph cluster namespace` |
 | `operatorPodLabels` | Custom pod labels for the operator | `{}` |
 | `priorityClassName` | Set the priority class for the rook operator deployment if desired | `nil` |

Documentation/Storage-Configuration/Object-Storage-RGW/ceph-object-bucket-claim.md

@@ -91,11 +91,25 @@ If both `bucketName` and `generateBucketName` are blank or omitted then the stor
 
     * `maxObjects`: The maximum number of objects in the bucket as a quota on the user account automatically created for the bucket.
     * `maxSize`: The maximum size of the bucket as a quota on the user account automatically created for the bucket. Please note minimum recommended value is 4K.
-    * `bucketMaxObjects`: The maximum number of objects in the bucket as an individual bucket quota. This is useful when the bucket is shared among multiple users.
-    * `bucketMaxSize`: The maximum size of the bucket as an individual bucket quota.
-    * `bucketPolicy`: A raw JSON format string that defines an AWS S3 format the bucket policy. If set, the policy string will override any existing policy set on the bucket and any default bucket policy that the bucket provisioner potentially would have automatically generated.
-    * `bucketLifecycle`: A raw JSON format string that defines an AWS S3 format bucket lifecycle configuration. Note that the rules must be sorted by `ID` in order to be idempotent.
-    * `bucketOwner`: The name of a pre-existing ceph rgw user account that will own the bucket. A `CephObjectStoreUser` resource may be used to create an ceph rgw user account. If the bucket already exists and is owned by a different user, the bucket will be re-linked to the specified user.
+    * `bucketMaxObjects`: (disabled by default) The maximum number of objects in the bucket as an individual bucket quota. This is useful when the bucket is shared among multiple users.
+    * `bucketMaxSize`: (disabled by default) The maximum size of the bucket as an individual bucket quota.
+    * `bucketPolicy`: (disabled by default) A raw JSON format string that defines an AWS S3 format the bucket policy. If set, the policy string will override any existing policy set on the bucket and any default bucket policy that the bucket provisioner potentially would have automatically generated.
+    * `bucketLifecycle`: (disabled by default) A raw JSON format string that defines an AWS S3 format bucket lifecycle configuration. Note that the rules must be sorted by `ID` in order to be idempotent.
+    * `bucketOwner`: (disabled by default)  The name of a pre-existing ceph rgw user account that will own the bucket. A `CephObjectStoreUser` resource may be used to create an ceph rgw user account. If the bucket already exists and is owned by a different user, the bucket will be re-linked to the specified user.
+
+Several OBC `additionalConfig` fields are disabled by default. Default-disabled additional config
+fields may be risky for administrators to allow users control over, and they should be enabled only
+with caution.
+
+The default allowed fields are `maxObjects` and `maxSize`. These are designed to fit into the OBC
+framework's original design goals. Other fields can be allowed but exert control outside of OBC's
+original design goals and should be considered risky. At best, users may be able to break their own
+OBCs in unexpected ways. At worst, users may brick the whole S3 object store for all users
+(`bucketPolicy` in particular). Administrators should take care to enable features only when they
+are personally willing to take on the risks.
+
+OBC `additionalConfig` fields can be enabled and disabled using the the `rook-ceph-operator-config`
+configmap value `ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS`.
 
 ### OBC Custom Resource after Bucket Provisioning
 

PendingReleaseNotes.md

@@ -2,5 +2,14 @@
 
 ## Breaking Changes
 
+Object:
+
+- Some ObjectBucketClaim options were added in Rook v1.16 that allowed more control over buckets.
+    These controls allow users to self-serve their own S3 policies, which many administrators might
+    consider a risk, depending on their environment. Rook has taken steps to ensure potentially risky
+    configurations are disabled by default to ensure the safest off-the-shelf configurations.
+    Administrators who wish to allow users to use the full range of OBC configurations must use the
+    new `ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS` to enable users to set potentially risky options.
+    See https://github.com/rook/rook/pull/15376 for more information.
 
 ## Features

deploy/charts/rook-ceph/templates/configmap.yaml

@@ -14,6 +14,9 @@ data:
 {{- end }}
 {{- if .Values.obcProvisionerNamePrefix }}
   ROOK_OBC_PROVISIONER_NAME_PREFIX: {{ .Values.obcProvisionerNamePrefix | quote }}
+{{- end }}
+{{- if .Values.obcAllowAdditionalConfigFields }}
+  ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS: {{ .Values.obcAllowAdditionalConfigFields | quote }}
 {{- end }}
   ROOK_CEPH_ALLOW_LOOP_DEVICES: {{ .Values.allowLoopDevices | quote }}
   ROOK_ENABLE_DISCOVERY_DAEMON: {{ .Values.enableDiscoveryDaemon | quote }}

deploy/charts/rook-ceph/values.yaml

@@ -655,6 +655,13 @@ enableOBCWatchOperatorNamespace: true
 # @default -- `ceph cluster namespace`
 obcProvisionerNamePrefix:
 
+# -- Many OBC additional config fields may be risky for administrators to allow users control over.
+# The safe and default-allowed fields are 'maxObjects' and 'maxSize'.
+# Other fields should be considered risky. To allow all additional configs, use this value:
+#   "maxObjects,maxSize,bucketMaxObjects,bucketMaxSize,bucketPolicy,bucketLifecycle,bucketOwner"
+# @default -- "maxObjects,maxSize"
+obcAllowAdditionalConfigFields: "maxObjects,maxSize"
+
 monitoring:
   # -- Enable monitoring. Requires Prometheus to be pre-installed.
   # Enabling will also create RBAC rules to allow Operator to create ServiceMonitors

deploy/examples/operator-openshift.yaml

@@ -541,6 +541,12 @@ data:
   # Custom prefix value for the OBC provisioner instead of ceph cluster namespace, do not set on existing cluster
   # ROOK_OBC_PROVISIONER_NAME_PREFIX: "custom-prefix"
 
+  # Many OBC additional config fields may be risky for administrators to allow users control over.
+  # The safe and default-allowed fields are 'maxObjects' and 'maxSize'.
+  # Other fields should be considered risky. To allow all additional configs, use this value:
+  #   "maxObjects,maxSize,bucketMaxObjects,bucketMaxSize,bucketPolicy,bucketLifecycle,bucketOwner"
+  # ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS: "maxObjects,maxSize" # default allowed configs
+
   # Whether to start the discovery daemon to watch for raw storage devices on nodes in the cluster.
   # This daemon does not need to run if you are only going to create your OSDs based on StorageClassDeviceSets with PVCs.
   ROOK_ENABLE_DISCOVERY_DAEMON: "false"

deploy/examples/operator.yaml

@@ -491,6 +491,12 @@ data:
   # Custom prefix value for the OBC provisioner instead of ceph cluster namespace, do not set on existing cluster
   # ROOK_OBC_PROVISIONER_NAME_PREFIX: "custom-prefix"
 
+  # Many OBC additional config fields may be risky for administrators to allow users control over.
+  # The safe and default-allowed fields are 'maxObjects' and 'maxSize'.
+  # Other fields should be considered risky. To allow all additional configs, use this value:
+  #   "maxObjects,maxSize,bucketMaxObjects,bucketMaxSize,bucketPolicy,bucketLifecycle,bucketOwner"
+  # ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS: "maxObjects,maxSize" # default allowed configs
+
   # Whether to start the discovery daemon to watch for raw storage devices on nodes in the cluster.
   # This daemon does not need to run if you are only going to create your OSDs based on StorageClassDeviceSets with PVCs.
   ROOK_ENABLE_DISCOVERY_DAEMON: "false"

pkg/operator/ceph/controller.go

@@ -134,6 +134,7 @@ func (r *ReconcileConfig) reconcile(request reconcile.Request) (reconcile.Result
 	opcontroller.SetAllowLoopDevices(r.config.Parameters)
 	opcontroller.SetEnforceHostNetwork(r.config.Parameters)
 	opcontroller.SetRevisionHistoryLimit(r.config.Parameters)
+	opcontroller.SetObcAllowAdditionalConfigFields(r.config.Parameters)
 
 	logger.Infof("%s done reconciling", controllerName)
 	return reconcile.Result{}, nil

pkg/operator/ceph/controller/controller_utils.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"slices"
 	"strconv"
 	"strings"
 	"time"
@@ -54,6 +55,9 @@ const (
 	enforceHostNetworkSettingName  string = "ROOK_ENFORCE_HOST_NETWORK"
 	enforceHostNetworkDefaultValue string = "false"
 
+	obcAllowAdditionalConfigFieldsSettingName  string = "ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS"
+	obcAllowAdditionalConfigFieldsDefaultValue string = "maxObjects,maxSize"
+
 	revisionHistoryLimitSettingName string = "ROOK_REVISION_HISTORY_LIMIT"
 
 	// UninitializedCephConfigError refers to the error message printed by the Ceph CLI when there is no ceph configuration file
@@ -90,6 +94,9 @@ var (
 	// loopDevicesAllowed indicates whether loop devices are allowed to be used
 	loopDevicesAllowed          = false
 	revisionHistoryLimit *int32 = nil
+
+	// allowed OBC additional config fields
+	obcAllowAdditionalConfigFields = strings.Split(obcAllowAdditionalConfigFieldsDefaultValue, ",")
 )
 
 func DiscoveryDaemonEnabled(data map[string]string) bool {
@@ -159,6 +166,15 @@ func RevisionHistoryLimit() *int32 {
 	return revisionHistoryLimit
 }
 
+func SetObcAllowAdditionalConfigFields(data map[string]string) {
+	strval := k8sutil.GetValue(data, obcAllowAdditionalConfigFieldsSettingName, obcAllowAdditionalConfigFieldsDefaultValue)
+	obcAllowAdditionalConfigFields = strings.Split(strval, ",")
+}
+
+func ObcAdditionalConfigKeyIsAllowed(configField string) bool {
+	return slices.Contains(obcAllowAdditionalConfigFields, configField)
+}
+
 // canIgnoreHealthErrStatusInReconcile determines whether a status of HEALTH_ERR in the CephCluster can be ignored safely.
 func canIgnoreHealthErrStatusInReconcile(cephCluster cephv1.CephCluster, controllerName string) bool {
 	// Get a list of all the keys causing the HEALTH_ERR status.

pkg/operator/ceph/controller/controller_utils_test.go

@@ -246,3 +246,45 @@ func TestIsReadyToReconcile(t *testing.T) {
 		assert.False(t, clusterExists)
 	})
 }
+
+func TestObcAllowAdditionalConfigFields(t *testing.T) {
+	tests := []struct {
+		name           string
+		data           map[string]string
+		shouldAllow    []string
+		shouldDisallow []string
+	}{
+		{"not set", map[string]string{},
+			[]string{"maxObjects", "maxSize"}, // default allowlist is unlikely to need changing EVER
+			[]string{"bucketMaxObjects", "bucketMaxSize", "bucketPolicy", "bucketLifecycle", "bucketOwner", "random"}},
+		{"set to empty", map[string]string{"ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS": ""},
+			[]string{}, // admin can allow no quota options if desired
+			[]string{"maxObjects", "maxSize", "bucketMaxObjects", "bucketMaxSize", "bucketPolicy", "bucketLifecycle", "bucketOwner", "random"}},
+		{"set to default", map[string]string{"ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS": "maxObjects,maxSize"},
+			[]string{"maxObjects", "maxSize"},
+			[]string{"bucketMaxObjects", "bucketMaxSize", "bucketPolicy", "bucketLifecycle", "bucketOwner", "random"}},
+		{"all quota fields", map[string]string{"ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS": "maxObjects,maxSize,bucketMaxObjects,bucketMaxSize"},
+			[]string{"maxObjects", "maxSize", "bucketMaxObjects", "bucketMaxSize"},
+			[]string{"bucketPolicy", "bucketLifecycle", "bucketOwner", "random"}},
+		{"all fields", map[string]string{"ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS": "maxObjects,maxSize,bucketMaxObjects,bucketMaxSize,bucketPolicy,bucketLifecycle,bucketOwner"},
+			[]string{"maxObjects", "maxSize", "bucketMaxObjects", "bucketMaxSize", "bucketPolicy", "bucketLifecycle", "bucketOwner"},
+			[]string{"random"}},
+		// this mechanism doesn't do any field checking - that isn't it's job - it merely handles
+		// allow-listing essentially arbitrary config keys
+		{"all fields including unknown", map[string]string{"ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS": "maxObjects,maxSize,bucketMaxObjects,bucketMaxSize,bucketPolicy,bucketLifecycle,bucketOwner,random"},
+			[]string{"maxObjects", "maxSize", "bucketMaxObjects", "bucketMaxSize", "bucketPolicy", "bucketLifecycle", "bucketOwner", "random"},
+			[]string{"otherRandom"}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			SetObcAllowAdditionalConfigFields(tt.data)
+
+			for _, toAllow := range tt.shouldAllow {
+				assert.True(t, ObcAdditionalConfigKeyIsAllowed(toAllow))
+			}
+			for _, toDisallow := range tt.shouldDisallow {
+				assert.False(t, ObcAdditionalConfigKeyIsAllowed(toDisallow))
+			}
+		})
+	}
+}

pkg/operator/ceph/object/bucket/provisioner_test.go

@@ -31,6 +31,7 @@ import (
 	rookclient "github.com/rook/rook/pkg/client/clientset/versioned/fake"
 	"github.com/rook/rook/pkg/clusterd"
 	"github.com/rook/rook/pkg/daemon/ceph/client"
+	opcontroller "github.com/rook/rook/pkg/operator/ceph/controller"
 	"github.com/rook/rook/pkg/operator/ceph/object"
 	"github.com/rook/rook/pkg/operator/test"
 	"github.com/stretchr/testify/assert"
@@ -500,48 +501,84 @@ func TestProvisioner_additionalConfigSpecFromMap(t *testing.T) {
 	})
 
 	t.Run("maxObjects field should be set", func(t *testing.T) {
+		opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{})
+
 		spec, err := additionalConfigSpecFromMap(map[string]string{"maxObjects": "2"})
 		assert.NoError(t, err)
 		var i int64 = 2
 		assert.Equal(t, additionalConfigSpec{maxObjects: &i}, *spec)
 	})
 
 	t.Run("maxSize field should be set", func(t *testing.T) {
+		opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{})
+
 		spec, err := additionalConfigSpecFromMap(map[string]string{"maxSize": "3"})
 		assert.NoError(t, err)
 		var i int64 = 3
 		assert.Equal(t, additionalConfigSpec{maxSize: &i}, *spec)
 	})
 
 	t.Run("bucketMaxObjects field should be set", func(t *testing.T) {
+		opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{"ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS": "bucketMaxObjects"})
+		defer opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{})
+
 		spec, err := additionalConfigSpecFromMap(map[string]string{"bucketMaxObjects": "4"})
 		assert.NoError(t, err)
 		assert.Equal(t, additionalConfigSpec{bucketMaxObjects: &(&struct{ i int64 }{4}).i}, *spec)
 	})
 
 	t.Run("bucketMaxSize field should be set", func(t *testing.T) {
+		opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{"ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS": "bucketMaxSize"})
+		defer opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{})
+
 		spec, err := additionalConfigSpecFromMap(map[string]string{"bucketMaxSize": "5"})
 		assert.NoError(t, err)
 		assert.Equal(t, additionalConfigSpec{bucketMaxSize: &(&struct{ i int64 }{5}).i}, *spec)
 	})
 
 	t.Run("bucketPolicy field should be set", func(t *testing.T) {
+		opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{"ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS": "bucketPolicy"})
+		defer opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{})
+
 		spec, err := additionalConfigSpecFromMap(map[string]string{"bucketPolicy": "foo"})
 		assert.NoError(t, err)
 		assert.Equal(t, additionalConfigSpec{bucketPolicy: &(&struct{ s string }{"foo"}).s}, *spec)
 	})
 
 	t.Run("bucketLifecycle field should be set", func(t *testing.T) {
+		opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{"ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS": "bucketLifecycle"})
+		defer opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{})
+
 		spec, err := additionalConfigSpecFromMap(map[string]string{"bucketLifecycle": "foo"})
 		assert.NoError(t, err)
 		assert.Equal(t, additionalConfigSpec{bucketLifecycle: &(&struct{ s string }{"foo"}).s}, *spec)
 	})
 
 	t.Run("bucketOwner field should be set", func(t *testing.T) {
+		opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{"ROOK_OBC_ALLOW_ADDITIONAL_CONFIG_FIELDS": "bucketOwner"})
+		defer opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{})
+
 		spec, err := additionalConfigSpecFromMap(map[string]string{"bucketOwner": "foo"})
 		assert.NoError(t, err)
 		assert.Equal(t, additionalConfigSpec{bucketOwner: &(&struct{ s string }{"foo"}).s}, *spec)
 	})
+
+	t.Run("fields disallowed by default", func(t *testing.T) {
+		opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{})
+
+		for _, configKey := range []string{"bucketMaxObjects", "bucketMaxSize", "bucketPolicy", "bucketLifecycle", "bucketOwner"} {
+			_, err := additionalConfigSpecFromMap(map[string]string{configKey: "foo"})
+			assert.Error(t, err)
+		}
+	})
+
+	t.Run("does not fail on empty map", func(t *testing.T) {
+		opcontroller.SetObcAllowAdditionalConfigFields(map[string]string{})
+
+		spec, err := additionalConfigSpecFromMap(map[string]string{})
+		assert.NoError(t, err)
+		assert.Equal(t, additionalConfigSpec{}, *spec)
+	})
 }
 
 func numberOfCallsWithValue(substr string, strs []string) int {