Document boolean behaviour of helm .secrets.existingSecret

[allanrogerr]

Summary

Current functionality

If .secrets.existingSecret is set, then enable the usage of an existing Kubernetes secret to set environment variables for the Tenant.
The existing Kubernetes secret name must be placed under .tenant.configuration.name e.g. existing-minio-env-configuration.

Proposed functionality

Refactor tenant-configuration.yaml to consider boolean .tenant.configSecret.existingSecret, instead of string .secrets.existingSecret.
Current functionality uses counterintuitively the existence of string .secrets.existingSecret when creating a secret with MINIO_ROOT_USER and MINIO_ROOT_PASSWORD, versus using an existing secret with these and similar environment variables.
Proposed functionality:

• uses a clear boolean .tenant.configSecret.existingSecret instead.
• provides warnings when using current functionality
• indicates when the deprecated field will be removed i.e. next minor version
• prevents usage of both functionalities; string .tenant.configSecret.existingSecret, instead of string .secrets.existingSecret

Tests. See https://github.com/allanrogerr/public/wiki/operator-2032

Summary

Tests 1-4: Regression tests of current functionality
Tests 5-9: Integration tests of proposed functionality

[allanrogerr]

Fixes #1795

[allanrogerr]

Fixes #718

[pjuarezd]

I still find it conterintuitive, rather that try to justify odd named fields we could rework them in a way that is a bit less complicated, allow me to share what I think it we should do:

Rework fields

1. Mark .secrets.existingSecret as deprecated. This field is declared as a "secret reference" type, however is only evaluated as a boolean in the template (see line 1), we really do not need the .name field, we only need to know if helm chart will create the secret or not, this evaluation (create the secret or not) can be addressed in other chart field.

operator/helm/tenant/templates/tenant-configuration.yaml

Line 1 in 3359d52

{{- if not .Values.secrets.existingSecret }}

2. Rename .secrets to .tenant.configSecret because:

• Makes more sense to place it under the .tenant field since it only impacts the tenant resource.
• The field is not a list or array, is a single item, and the field values are used to create the MinIO Tenant Configuration secret (see in below template how it is not in a loop)

operator/helm/tenant/templates/tenant-configuration.yaml

Lines 1 to 11 in 3359d52

	{{- if not .Values.secrets.existingSecret }}
	apiVersion: v1
	kind: Secret
	metadata:
	name: {{ dig "secrets" "name" "" (.Values | merge (dict)) }}
	type: Opaque
	stringData:
	config.env: |-
	export MINIO_ROOT_USER={{ .Values.secrets.accessKey | quote }}
	export MINIO_ROOT_PASSWORD={{ .Values.secrets.secretKey | quote }}
	{{- end }}

3. Make field .tenant.configuration Optional, proportioning this field value would indicate:

• That the secret referenced in .tenant.configuration.name already exists or will be create externally from the chart.
• That .tenant.configSecret values will not be proportionated.
• The chart should error if both .tenant.configuration and .tenant.configSecret are provided.

Backward compatibility

Of course doing this change from one version to another would break the installs, the rollout should be in 2 steps:

Step 1 Anounce changes

• Announce the changes in the chart REAME.md.
• Do a bugfix release with compatibility with pre and after fields renames.
• Print warning in the chart so that administrators are aware of the future change:

{{- if .Values.secrets.existingSecret}}
{{- printf "WARNING: '.secrets.existingSecret' is deprecated and will be removed in next minor release (ie: v5.1.0)" | warn }}
{{- end }}

{- if .Values.secrets }}
{{- printf "WARNING: 'secrets' is deprecated and will be removed in next minor release (ie: v5.1.0). Please use 'tenant.configSecret' instead." | warn }}
{{- end }}

Step 2 Slowly rollout breaking changes

Create a minor release with the breaking changes:
We shoud do one or more bugfix releases to allow the community get aware and familiar with the upcoming change, after some releases we will need to do a Minor release, I thik this should be in v5.1.0 and remove compatibility with older fields.

I would like a review from @dvaldivia, @feorlen and @ravindk89, what's your thoughs?

[allanrogerr]

@pjuarezd I truly appreciate the time you spent indicating a better way to document this.

[feorlen]

How much time do you think it would be between deprecation and removal?

Would definitely need to alert everyone (in the readme, PR/commit message, plus Slack) so when customers ask we have something ready to tell them.

[allanrogerr]

I will bring this up again internally.

[allanrogerr]

Documentation to be updated when:

• the new functionality has been released
• the old functionality has disappeared.
• dropping the deprecation references if necessary

The exact time to the next minor release is unknown.

[pjuarezd]

thank @allanrogerr, checking

[ravindk89]

A note - with this setup, lets say we have a user who wants to manage secrets through something like hashicorp vault, where the external secret manager stores the config.env and can create the secret with whatever name they specify.

how does the user specify that here? would it just be

tenant:
    configSecret: my-externally-managed-secret

Where the secret contents matches our requirements of stringData: config.env

[ravindk89]

operator/helm/tenant/values.yaml

Lines 85 to 86 in 3359d52

	configuration:
	name: myminio-env-configuration

We also have this field .tenant.configuration.name - what does this do? @pjuarezd

[allanrogerr]

A note - with this setup, lets say we have a user who wants to manage secrets through something like hashicorp vault, where the external secret manager stores the config.env and can create the secret with whatever name they specify.

how does the user specify that here? would it just be

tenant:
    configSecret: my-externally-managed-secret

Where the secret contents matches our requirements of stringData: config.env

@ravindk89 This can be done currently using the setups in: https://github.com/allanrogerr/public/wiki/operator-2032#test-2
In the proposed solution, this would be possible using the setups in: https://github.com/allanrogerr/public/wiki/operator-2032#test-7
In either case, the name of the existingSecret needs to be specified.

[allanrogerr]

operator/helm/tenant/values.yaml

Lines 85 to 86 in 3359d52

	configuration:
	name: myminio-env-configuration

We also have this field .tenant.configuration.name - what does this do? @pjuarezd

In this particular usecase i.e. creating a tenant using helm, and not the operator ui:

if .tenant.configSecret.existingSecret then
  use secret with name = .tenant.configuration.name for MINIO_ROOT_USER and MINIO_ROOT_PASSWORD
else
  create secret called with name = .configSecret.name containing MINIO_ROOT_USER=.configSecret.accessKey and MINIO_ROOT_PASSWORD=.configSecret.secretKey
  use secret with name = .tenant.configuration.name for MINIO_ROOT_USER and MINIO_ROOT_PASSWORD
fi

[allanrogerr]

Fixes #1795