K8SPSMDB-780: Unsafe improvements #1504

egegunes · 2024-04-06T11:34:56Z

CHANGE DESCRIPTION

These changes attempt to fix the overloaded allowUnsafeConfigurations
flag.

In previous implementation, allowUnsafeConfigurations wasn't just
allow unsafe configuration but make everything unsafe by disabling TLS,
allowing backups in unhealthy clusters, etc... without user's explicit
intent.

With these changes, we decouple those things from the unsafe flag and
remove all implicit behaviors. We introduce a new section called
unsafeFlags:

unsafeFlags:
  tls: false
  replsetSize: false
  mongosSize: false
  terminationGracePeriod: false
  backupIfUnhealthy: false

Starting from v1.16.0, allowUnsafeConfigurations is deprecated and
won't have any affect.

TLS Mode

This decoupling required a special attention to the TLS configuration.
Before these changes only way to disable TLS is setting
allowUnsafeConfigurations to true. Now, we introduce a new field:

spec:
  tls:
    mode: disabled

This field accepts the following values: disabled, allowTLS,
preferTLS and requireTLS.

If user sets mode to disabled, the operator will throw an error: TLS must be enabled. Set spec.unsafeFlags.tls to true to disable this check.

Since the use of TLS flags and reconciling TLS secrets depends on
tls.mode field, we need to block users to set net.tls.mode in custom
MongoDB configuration. If user sets a custom configuration like:

spec:
  replsets:
  - name: rs0
    size: 3
    configuration: |
      net:
        tls:
          mode: allowTLS

the operator will throw an error: tlsMode must be set using spec.tls.mode.

CHECKLIST

Jira

Is the Jira ticket created and referenced properly?
Does the Jira ticket have the proper statuses for documentation (Needs Doc) and QA (Needs QA)?
Does the Jira ticket link to the proper milestone (Fix Version field)?

Tests

Is an E2E test/test case added for the new feature/change?
Are unit tests added where appropriate?
Are OpenShift compare files changed for E2E tests (compare/*-oc.yml)?

Config/Logging/Testability

Are all needed new/changed options added to default YAML files?
Did we add proper logging messages for operator actions?
Did we ensure compatibility with the previous version or cluster upgrade process?
Does the change support oldest and newest supported MongoDB version?
Does the change support oldest and newest supported Kubernetes version?

github-actions · 2024-04-06T11:35:17Z

build/ps-entry.sh

+		"$checkArg" | "$checkArg"=*)
+			return 0
+			;;


[shfmt] _{reported by reviewdog 🐶}

Suggested change

"$checkArg" | "$checkArg"=*)

return 0

;;

"$checkArg" | "$checkArg"=*)

return 0

;;

github-actions · 2024-04-06T11:35:17Z

build/ps-entry.sh

+		"$checkArg")
+			echo "$1"
+			return 0
+			;;
+		"$checkArg"=*)
+			echo "${arg#"$checkArg"=}"
+			return 0
+			;;


[shfmt] _{reported by reviewdog 🐶}

Suggested change

"$checkArg")

echo "$1"

return 0

;;

"$checkArg"=*)

echo "${arg#"$checkArg"=}"

return 0

;;

"$checkArg")

echo "$1"

return 0

;;

"$checkArg"=*)

echo "${arg#"$checkArg"=}"

return 0

;;

github-actions · 2024-04-06T11:35:17Z

build/ps-entry.sh

+		"$ensureNoArg")
+			shift # also skip the value
+			continue
+			;;
+		"$ensureNoArg"=*)
+			# value is already included
+			continue
+			;;


[shfmt] _{reported by reviewdog 🐶}

Suggested change

"$ensureNoArg")

shift # also skip the value

continue

;;

"$ensureNoArg"=*)

# value is already included

continue

;;

"$ensureNoArg")

shift # also skip the value

continue

;;

"$ensureNoArg"=*)

# value is already included

continue

;;

github-actions · 2024-04-06T11:35:17Z

build/ps-entry.sh

+			*.sh | *.js) # this should match the set of files we check for below
+				shouldPerformInitdb="$f"
+				break
+				;;


[shfmt] _{reported by reviewdog 🐶}

Suggested change

*.sh | *.js) # this should match the set of files we check for below

shouldPerformInitdb="$f"

break

;;

*.sh | *.js) # this should match the set of files we check for below

shouldPerformInitdb="$f"

break

;;

github-actions · 2024-04-06T11:35:17Z

build/ps-entry.sh

+			*.sh)
+				echo "$0: running $f"
+				# shellcheck source=/dev/null
+				. "$f"
+				;;
+			*.js)
+				echo "$0: running $f"
+				"${mongo[@]}" "$MONGO_INITDB_DATABASE" "$f"
+				echo
+				;;
+			*) echo "$0: ignoring $f" ;;


[shfmt] _{reported by reviewdog 🐶}

Suggested change

*.sh)

echo "$0: running $f"

# shellcheck source=/dev/null

. "$f"

;;

*.js)

echo "$0: running $f"

"${mongo[@]}" "$MONGO_INITDB_DATABASE" "$f"

echo

;;

*) echo "$0: ignoring $f" ;;

*.sh)

echo "$0: running $f"

# shellcheck source=/dev/null

. "$f"

;;

*.js)

echo "$0: running $f"

"${mongo[@]}" "$MONGO_INITDB_DATABASE" "$f"

echo

;;

*) echo "$0: ignoring $f" ;;

These changes attempt to fix the overloaded `allowUnsafeConfigurations` flag. In previous implementation, `allowUnsafeConfigurations` wasn't just allow unsafe configuration but make everything unsafe by disabling TLS, allowing backups in unhealthy clusters, etc... without user's explicit intent. With these changes, we decouple those things from the unsafe flag and remove all implicit behaviors. We introduce a new section called `unsafeFlags`: ``` unsafeFlags: tls: false replsetSize: false mongosSize: false terminationGracePeriod: false backupIfUnhealthy: false ``` Starting from `v1.16.0`, `allowUnsafeConfigurations` is deprecated and won't have any affect. **TLS Mode** This decoupling required a special attention to the TLS configuration. Before these changes only way to disable TLS is setting `allowUnsafeConfigurations` to true. Now, we introduce a new field: ``` spec: tls: mode: disabled ``` This field accepts the following values: `disabled`, `allowTLS`, `preferTLS` and `requireTLS`. If user sets mode to `disabled`, the operator will throw an error: `TLS must be enabled. Set spec.unsafeFlags.tls to true to disable this check.` Since the use of TLS flags and reconciling TLS secrets depends on `tls.mode` field, we need to block users to set `net.tls.mode` in custom MongoDB configuration. If user sets a custom configuration like: ``` spec: replsets: - name: rs0 size: 3 configuration: | net: tls: mode: allowTLS ``` the operator will throw an error: `tlsMode must be set using spec.tls.mode`.

github-actions · 2024-04-09T11:05:40Z

e2e-tests/scaling/run

+cat_config "$conf_dir/$cluster.yml" |
+	yq eval '.spec.replsets[0].expose.enabled=true' |
+	yq eval '.spec.replsets[0].expose.exposeType="ClusterIP"' |
+	kubectl_bin apply -f -


[shfmt] _{reported by reviewdog 🐶}

Suggested change

cat_config "$conf_dir/$cluster.yml" |

yq eval '.spec.replsets[0].expose.enabled=true' |

yq eval '.spec.replsets[0].expose.exposeType="ClusterIP"' |

kubectl_bin apply -f -

cat_config "$conf_dir/$cluster.yml" \

| yq eval '.spec.replsets[0].expose.enabled=true' \

| yq eval '.spec.replsets[0].expose.exposeType="ClusterIP"' \

| kubectl_bin apply -f -

github-actions · 2024-04-09T16:09:49Z

e2e-tests/scaling/run

+cat_config "$conf_dir/$cluster.yml" |
+	yq eval '.spec.unsafeFlags.replsetSize=true' |
+	yq eval '.spec.replsets[0].expose.enabled=true' |
+	yq eval '.spec.replsets[0].expose.exposeType="ClusterIP"' |
+	kubectl_bin apply -f -


[shfmt] _{reported by reviewdog 🐶}

Suggested change

cat_config "$conf_dir/$cluster.yml" |

yq eval '.spec.unsafeFlags.replsetSize=true' |

yq eval '.spec.replsets[0].expose.enabled=true' |

yq eval '.spec.replsets[0].expose.exposeType="ClusterIP"' |

kubectl_bin apply -f -

cat_config "$conf_dir/$cluster.yml" \

| yq eval '.spec.unsafeFlags.replsetSize=true' \

| yq eval '.spec.replsets[0].expose.enabled=true' \

| yq eval '.spec.replsets[0].expose.exposeType="ClusterIP"' \

| kubectl_bin apply -f -

pooknull · 2024-04-16T16:10:21Z

pkg/psmdb/container.go

+		} else {
+			args = append(args, "--clusterAuthMode=x509")
+		}
+	} else if (cr.CompareVersion("1.16.0") >= 0 && cr.Spec.Unsafe.TLS) || (cr.CompareVersion("1.16.0") < 0 && cr.Spec.UnsafeConf) {


What do you think about creating a cr.UnsafeTLSDisabled() method to replace this long condition check?

pooknull · 2024-04-16T16:11:54Z

pkg/psmdb/pmm.go

-	if cr.CompareVersion("1.13.0") >= 0 && !cr.Spec.UnsafeConf {
+	if cr.CompareVersion("1.13.0") >= 0 {


Why it was removed?

good catch, we need to check if tls enabled there: 6badccf

inelpandzic · 2024-04-17T08:45:09Z

pkg/apis/psmdb/v1/psmdb_defaults_test.go

@@ -226,7 +226,7 @@ func TestSetSafeDefault(t *testing.T) {
 	cr := &api.PerconaServerMongoDB{
 		ObjectMeta: metav1.ObjectMeta{Name: "psmdb-mock", Namespace: "psmdb"},
 		Spec: api.PerconaServerMongoDBSpec{
-			CRVersion: version.Version,
+			CRVersion: "1.15.0",


I think we should add or update this unit tests to cover these new unsafe flags for 1.16.0.

hors

@egegunes I think we need to comment it by default https://github.com/percona/percona-server-mongodb-operator/pull/1504/files#diff-66e2e64739b6c9fbb1f1f10431f083ab8ab642cf5049f8033e0bcae5afd1ea1aR29-R34

JNKPercona · 2024-04-17T15:46:53Z

Test name	Status
arbiter	passed
balancer	passed
custom-replset-name	passed
cross-site-sharded	passed
data-at-rest-encryption	passed
data-sharded	passed
demand-backup	passed
demand-backup-eks-credentials	passed
demand-backup-physical	passed
demand-backup-physical-sharded	passed
demand-backup-sharded	passed
expose-sharded	passed
ignore-labels-annotations	passed
init-deploy	passed
finalizer	passed
ldap	passed
ldap-tls	passed
limits	passed
liveness	passed
mongod-major-upgrade	passed
mongod-major-upgrade-sharded	passed
monitoring-2-0	passed
multi-cluster-service	passed
non-voting	passed
one-pod	passed
operator-self-healing-chaos	passed
pitr	passed
pitr-sharded	passed
pitr-physical	passed
pvc-resize	passed
recover-no-primary	passed
rs-shard-migration	passed
scaling	passed
scheduled-backup	passed
security-context	passed
self-healing-chaos	passed
service-per-pod	passed
serviceless-external-nodes	passed
smart-update	passed
split-horizon	passed
storage	passed
tls-issue-cert-manager	passed
upgrade	passed
upgrade-consistency	passed
upgrade-consistency-sharded-tls	passed
upgrade-sharded	passed
users	passed
version-service	passed
We run 48 out of 48

commit: 660460a
image: perconalab/percona-server-mongodb-operator:PR-1504-660460a7

pull-request-size bot added the size/XL 500-999 lines label Apr 6, 2024

github-actions bot reviewed Apr 6, 2024

View reviewed changes

egegunes force-pushed the K8SPSMDB-780 branch 2 times, most recently from 10ecc42 to 52ddcce Compare April 8, 2024 09:36

egegunes force-pushed the K8SPSMDB-780 branch from 52ddcce to 3377f1e Compare April 8, 2024 10:13

fix tests

7ee44f2

pull-request-size bot added size/XXL 1000+ lines and removed size/XL 500-999 lines labels Apr 8, 2024

fix tests

83a7ba4

github-actions bot reviewed Apr 9, 2024

View reviewed changes

fix tests

0504898

github-actions bot reviewed Apr 9, 2024

View reviewed changes

egegunes added 2 commits April 15, 2024 10:54

Merge branch 'main' into K8SPSMDB-780

096cb60

fix tests

330245a

egegunes added this to the v1.16.0 milestone Apr 15, 2024

egegunes added 5 commits April 16, 2024 10:46

Merge branch 'main' into K8SPSMDB-780

84f504c

fix tests

b166c22

fix pvc-resize

743a11e

fix custom-replset-name

5875749

Merge branch 'main' into K8SPSMDB-780

5cac03b

egegunes marked this pull request as ready for review April 16, 2024 15:55

egegunes requested review from tplavcic, nmarukovich, ptankov, hors, inelpandzic and pooknull as code owners April 16, 2024 15:55

pooknull reviewed Apr 16, 2024

View reviewed changes

address review comments

6badccf

egegunes requested a review from pooknull April 17, 2024 08:05

inelpandzic reviewed Apr 17, 2024

View reviewed changes

hors reviewed Apr 17, 2024

View reviewed changes

egegunes added 2 commits April 17, 2024 15:29

fix cluster deletion

3bd2f42

comment unsafe flags

660460a

hors self-requested a review April 17, 2024 15:49

hors approved these changes Apr 17, 2024

View reviewed changes

hors merged commit c325d88 into main Apr 17, 2024
16 checks passed

hors deleted the K8SPSMDB-780 branch April 17, 2024 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

K8SPSMDB-780: Unsafe improvements #1504

K8SPSMDB-780: Unsafe improvements #1504

egegunes commented Apr 6, 2024 •

edited by jira bot

Loading

github-actions bot Apr 6, 2024

github-actions bot Apr 6, 2024

github-actions bot Apr 6, 2024

github-actions bot Apr 6, 2024

github-actions bot Apr 6, 2024

github-actions bot Apr 9, 2024

github-actions bot Apr 9, 2024

pooknull Apr 16, 2024

egegunes Apr 16, 2024

pooknull Apr 16, 2024

egegunes Apr 16, 2024

inelpandzic Apr 17, 2024

hors left a comment

JNKPercona commented Apr 17, 2024

		if cr.CompareVersion("1.13.0") >= 0 && !cr.Spec.UnsafeConf {
		if cr.CompareVersion("1.13.0") >= 0 {

K8SPSMDB-780: Unsafe improvements #1504

K8SPSMDB-780: Unsafe improvements #1504

Conversation

egegunes commented Apr 6, 2024 • edited by jira bot Loading

CHANGE DESCRIPTION

CHECKLIST

github-actions bot Apr 6, 2024

Choose a reason for hiding this comment

github-actions bot Apr 6, 2024

Choose a reason for hiding this comment

github-actions bot Apr 6, 2024

Choose a reason for hiding this comment

github-actions bot Apr 6, 2024

Choose a reason for hiding this comment

github-actions bot Apr 6, 2024

Choose a reason for hiding this comment

github-actions bot Apr 9, 2024

Choose a reason for hiding this comment

github-actions bot Apr 9, 2024

Choose a reason for hiding this comment

pooknull Apr 16, 2024

Choose a reason for hiding this comment

egegunes Apr 16, 2024

Choose a reason for hiding this comment

pooknull Apr 16, 2024

Choose a reason for hiding this comment

egegunes Apr 16, 2024

Choose a reason for hiding this comment

inelpandzic Apr 17, 2024

Choose a reason for hiding this comment

hors left a comment

Choose a reason for hiding this comment

JNKPercona commented Apr 17, 2024

egegunes commented Apr 6, 2024 •

edited by jira bot

Loading