Tenant Helm chart: existingSecret does not make sense currently

[stephan2012]

Is your feature request related to a problem? Please describe.

Looking at the tenant Helm chart, the description for the existingSecret parameter states

Set the value for existingSecret to use a pre created secret and dont create default one

However, the only consequence of setting a value here is that it disables creating a Kubernetes Secret. Somewhat counterintuitively, the templates do not pick it up to provide the root credentials. Instead, the root credentials have to be passed “manually”.

Describe the solution you'd like
Incorporate a given secret name into the configuration.

Describe alternatives you've considered
Alternatively, replace the Secret name with a toggle switch because it de-facto acts as such currently. This introduces a breaking change, of course.

Additional context
N/A

Kindly let me know if you are interested in a PR.

[jiuker]

No. ExistingSecret if not set, operator will create a secret like that config

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

Line 1 in cf4d30f

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

from your config.

[stephan2012]

@jiuker You closed this issue probably a bit too early. I unsuccessfully tested setting an existingSecret a few weeks ago, but the operator complained about the missing root user credentials.

[jiuker]

operator/helm/tenant/values.yaml

Lines 292 to 307 in 09c2011

	## Use an extraResources template section to include additional Kubernetes resources
	## with the Helm deployment.
	## Example: the following creates the config secret together with the tenant:
	#secrets:
	# existingSecret: custom-env-configuration
	#extraResources:
	# - |
	# apiVersion: v1
	# kind: Secret
	# type: Opaque
	# metadata:
	# name: {{ dig "secrets" "existingSecret" "" (.Values | merge (dict)) }}
	# stringData:
	# config.env: |-
	# export MINIO_ROOT_USER='minio'
	# export MINIO_ROOT_PASSWORD='minio123'

Have you tried this part? @stephan2012

[jessebot]

TLDR;

I got it working by passing in my secret name to the tenant.configuration.name helm parameter.

values.yaml:

secrets:
  # this field is more like a boolean. if it's empty, minio creates a secret
  # if it's NOT empty, minio doesn't create a secret, but does not use this secret
 existingSecret: default-tenant-env-config
 
# this is where the secret name actually gets defined
tenant:
  configuration:
    name:  default-tenant-env-config

Example secret:

apiVersion: v1
kind: Secret
type: Opaque
metadata:
  name: default-tenant-env-config
  namespace: minio
data:
  config.env: TUlOSU9fUk9PVF9VU0VSPW1pbmlvYWRtaW4KICAgICAgICBNSU5JT19ST09UX1BBU1NXT1JEPUQ2bVluelB1NDZPdFJGZ1M3OHR0Vk5SOHFwcFJJZmQ1

Example secret value decoded:

$ kubectl get secret default-tenant-env-config -n minio -o yaml | yq .data[] | base64 --decode
MINIO_ROOT_USER=minioadmin
        MINIO_ROOT_PASSWORD=D6mYnzPu46OtRFgS78ttVNR8qppRIfd5

further addressing of the tenant helm chart

@jiuker this value does not current get used anywhere:

operator/helm/tenant/values.yaml

Line 9 in d653dc1

# existingSecret: random-env-configuration

I ran the following from the helm/tenant directory and it shows that nowhere does the helm chart actually use this value other than to not create the default secret:

$ ag existingSecret
values.yaml
8:  ## Set the value for existingSecret to use a pre created secret and dont create default one
9:  # existingSecret: random-env-configuration
297:#  existingSecret: custom-env-configuration
304:#      name: {{ dig "secrets" "existingSecret" "" (.Values | merge (dict)) }}

templates/tenant-configuration.yaml
1:{{- if not .Values.secrets.existingSecret }}

If it's not going to be used and is just a boolean, it should be set to a boolean with a comment explaining that you need to set the tenant.configuration.name.

The most common reason to use an existingSecret to is avoid plain text credentials in the values file for security sake, so setting the plain text root credentials in tenant.extraResources isn't an option for environments that need to comply with certain infosec certifications.

If I use the tenant.configuration.name for the name of my existing Secret, it kinda works, but it still tries to split on the "=" and so you end up with a root user of something like export MINIO_ROOT_USER and a password of 'minio' export MINIO_ROOT_PASSWORD='minio123' .

Could you point me to where in the code base here this existing Secret gets parsed? Is it here?:

operator/pkg/controller/tenants.go

Lines 40 to 45 in d653dc1

	configFromFile := miniov2.ParseRawConfiguration(minioConfigurationSecret.Data["config.env"])
	for key, val := range configFromFile {
	tenantConfiguration[key] = val
	}
	}
	return tenantConfiguration, nil

Because if that's the case, it looks like the export bit is not necessary in either a automatically created or existingSecret:

operator/pkg/apis/minio.min.io/v2/helper.go

Lines 1053 to 1072 in d653dc1

	func ParseRawConfiguration(configuration []byte) (config map[string][]byte) {
	config = map[string][]byte{}
	if configuration != nil {
	scanner := bufio.NewScanner(strings.NewReader(string(configuration)))
	for scanner.Scan() {
	ekv, err := parsEnvEntry(scanner.Text())
	if err != nil {
	klog.Errorf("Error parsing tenant configuration: %v", err.Error())
	continue
	}
	if ekv.Skip {
	// Skips empty lines
	continue
	}
	config[ekv.Key] = []byte(ekv.Value)
	if ekv.Key == "MINIO_ROOT_USER" || ekv.Key == "MINIO_ACCESS_KEY" {
	config["accesskey"] = config[ekv.Key]
	} else if ekv.Key == "MINIO_ROOT_PASSWORD" || ekv.Key == "MINIO_SECRET_KEY" {
	config["secretkey"] = config[ekv.Key]
	}

here's the export bit being trimmed:

operator/pkg/apis/minio.min.io/v2/helper.go

Line 1025 in d653dc1

envTokens := strings.SplitN(strings.TrimSpace(strings.TrimPrefix(envEntry, "export")), envSeparator, 2)

I think the best way to accept and parse secrets going forward is to use key/value pairs from a secret that looks like this:

apiVersion: v1
kind: Secret
type: Opaque
metadata:
  name: my-existing-secret
stringData:
  MINIO_ROOT_USER: 'minio'
  MINIO_ROOT_PASSWORD: 'minio123'

This makes it easier to template and a bit more Kubernetes native the secret as in-line shell commands are bit combersome. It also makes it so that you can use them as env vars for pods, deployments and statefulsets a bit easier like this: https://kubernetes.io/docs/tasks/inject-data-application/distribute-credentials-secure/#define-container-environment-variables-using-secret-data

[reitermarkus]

@jiuker this value does not current get used anywhere:

Yes, in the current master, values.yaml contains existingSecret at the top level

operator/helm/tenant/values.yaml

Lines 35 to 36 in 856c422

	existingSecret:
	name: myminio-env-configuration

while the template expects it to be at secrets.existingSecret

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

Line 1 in 856c422

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

But yeah, definitely agree with @jessebot that this should be refactored to use valueFrom or envFrom style configuration.

[jyousefpour1]

+1 refactoring for valueFrom or envFrom

[allanrogerr]

Reading thru this, the single issue is reframing existingSecret as a boolean, with clarifying documentation. I will take this up with the team.

Any other refactoring should be considered in a separate feature request.

[allanrogerr]

Fixed by #1795