Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Env configuration issues #1556

Closed
D1StrX opened this issue Apr 10, 2023 · 13 comments · Fixed by #1663
Closed

Env configuration issues #1556

D1StrX opened this issue Apr 10, 2023 · 13 comments · Fixed by #1663

Comments

@D1StrX
Copy link

D1StrX commented Apr 10, 2023

Expected Behavior

Don't create minio1-env-configuration, when different configuration is pointing to another secret.
Also, the 'env-configuration' shouldn't be a hardcoded thing, which is now.

Current Behavior

When the default secret config is the helm chart is disabled, it still creates the minio1-env-configuration secret. While in the helm chart the configuration clearly points to the custom pre-created secret.

# secrets:
#   name: minio1-env-configuration
#   # MinIO root user and password
#   accessKey: minio
#   secretKey: minio123
...
  configuration:
    name: <something>-env-configuration

Possible Solution

Helm chart should contains;
Option A: existingSecret:
Option B: Just don't create the secret minio1-env-configuration, when the configuration points to another secret.

Steps to Reproduce (for bugs)

  1. Pre-create a secret containing`the info below, and converted to base64 as one string:
export MINIO_ROOT_USER="<some_username>"
export MINIO_ROOT_PASSWORD="<some_password>"
apiVersion: v1
kind: Secret
metadata:
  name: <secret_name>
  namespace: <namespace>
type: Opaque
data:
  config.env: <base64 content as one string>
  1. Deploy a tenant, where the secrets: part and details is commented
helm install <release_name> minio/tenant --namespace <namespace> --values .\values.yml --version 4.5.8

Context

Regression

No

Your Environment

  • Version used (minio-operator): 4.5.8 (cannot upgrade due Upgrade operator 4.5.8 -> 5.0.x breaks tenants #1544)
  • Environment name and version (e.g. kubernetes v1.17.2): 1.26.3
  • Server type and version: on-prem
  • Operating System and version (uname -a): Ubuntu 22.04
  • Link to your deployment file:
@shtripat
Copy link
Contributor

I am unable to reproduce this. Followed the below steps -

  1. Create a kind cluster
  2. Deploy minio-operator v4.5.8
$ wget https://github.com/minio/operator/raw/master/helm-releases/operator-4.5.8.tgz
$ tar xvf operator-4.5.8.tgz
$ helm install --namespace minio-operator --create-namespace minio-operator ./operator
  1. Created a file mysecret.yml
apiVersion: v1
kind: Secret
metadata:
  name: my-minio-env-configuration
  namespace: tenant-ns
type: Opaque
data:
  config.env: ZXhwb3J0IE1JTklPX1JPT1RfVVNFUj1taW5pbwpleHBvcnQgTUlOSU9fUk9PVF9QQVNTV09SRD1taW5pbzEyMwo=

where

$ echo "ZXhwb3J0IE1JTklPX1JPT1RfVVNFUj1taW5pbwpleHBvcnQgTUlOSU9fUk9PVF9QQVNTV09SRD1taW5pbzEyMwo=" | base64 -d
export MINIO_ROOT_USER=minio
export MINIO_ROOT_PASSWORD=minio123
  1. Create a tenant namespace and created the above secret
$ kubectl create ns tenant-ns
$ kubectl apply -f mysecret.yml
$ kubectl get secrets -n tenant-ns
NAME                              TYPE                 DATA   AGE
my-minio-env-configuration        Opaque               1      20m
  1. Updated the tenant helm charts values.yml and commented the secrets section as below
$ wget https://github.com/minio/operator/raw/master/helm-releases/tenant-4.5.8.tgz
$ tar xvf tenant-4.5.8.tgz
$ cat ./tenant/values.yml
## Secret with default environment variable configurations to be used by MinIO Tenant.
## Not recommended for production deployments! Create the secret manually instead.
#secrets:
#  name: minio1-env-configuration
#  # MinIO root user and password
#  accessKey: minio
#  secretKey: minio123

## MinIO Tenant Definition
tenant:
  # Tenant name
  name: minio1
  ## Registry location and Tag to download MinIO Server image
  image:
    repository: quay.io/minio/minio
    tag: RELEASE.2023-01-12T02-06-16Z
    pullPolicy: IfNotPresent
  ## Customize any private registry image pull secret.
  ## currently only one secret registry is supported
  imagePullSecret: { }
  ## If a scheduler is specified here, Tenant pods will be dispatched by specified scheduler.
  ## If not specified, the Tenant pods will be dispatched by default scheduler.
  scheduler: { }
  ## Secret name that contains additional environment variable configurations.
  ## The secret is expected to have a key named config.env containing environment variables exports.
  configuration:
    name: my-minio-env-configuration
  ## Specification for MinIO Pool(s) in this Tenant.
  pools:
  1. Created the tenant
$ helm install --namespace tenant-ns tenant-ns ./tenant
  1. Wait for few seconds and tenant pods come online
$ kubectl get pods -n tenant-ns
NAME                                     READY   STATUS    RESTARTS      AGE
minio1-log-0                             1/1     Running   0             21m
minio1-log-search-api-6fd56d8668-dglns   1/1     Running   5 (19m ago)   21m
minio1-pool-0-0                          1/1     Running   0             21m
minio1-pool-0-1                          1/1     Running   0             21m
minio1-pool-0-2                          1/1     Running   0             21m
minio1-pool-0-3                          1/1     Running   0             21m
minio1-prometheus-0                      2/2     Running   0             17m
  1. Check secrets in the tenant namespace
$ kubectl get secrets -n tenant-ns
NAME                              TYPE                 DATA   AGE
minio1-log-secret                 Opaque               4      22m
minio1-tls                        Opaque               2      23m
my-minio-env-configuration        Opaque               1      26m
operator-tls                      Opaque               1      23m
operator-webhook-secret           Opaque               3      23m
sh.helm.release.v1.tenant-ns.v1   helm.sh/release.v1   1      24m

I see only manually created secret my-minio-env-configuration and not a default one minio1-env-configuration

@D1StrX
Copy link
Author

D1StrX commented Jun 22, 2023

The secret you used is the default secret my-minio-env-configuration. When using a different name and defining that in the values file, for example different-minio-env-configuration, it does use the custom secret. But also/still (re)create the my-minio-env-configuration. That last part shouldn't happen.

@shtripat
Copy link
Contributor

May be I am not following the whole thing here. This time I create a totally new secret with name different-minio-env-config with values

export MINIO_ROOT_USER=testuser
export MINIO_ROOT_PASSWORD=testuser123

and created tenant now. Dont see default re-created secret though

$ kubectl get secrets -A
NAMESPACE        NAME                                   TYPE                                  DATA   AGE
kube-system      bootstrap-token-abcdef                 bootstrap.kubernetes.io/token         6      14m
minio-operator   console-sa-secret                      kubernetes.io/service-account-token   3      13m
minio-operator   operator-tls                           Opaque                                2      13m
minio-operator   sh.helm.release.v1.minio-operator.v1   helm.sh/release.v1                    1      13m
tenant-ns        different-minio-env-config             Opaque                                1      10m
tenant-ns        minio1-log-secret                      Opaque                                4      3m46s
tenant-ns        minio1-tls                             Opaque                                2      4m49s
tenant-ns        operator-tls                           Opaque                                1      4m45s
tenant-ns        operator-webhook-secret                Opaque                                3      4m54s
tenant-ns        sh.helm.release.v1.tenant-ns.v1        helm.sh/release.v1                    1      5m1s

$ kubectl get secrets/different-minio-env-config -n tenant-ns -oyaml | yq '.metadata.annotations."kubectl.kubernetes.io/last-applied-configuration"' | jq '.data."config.env"' | sed 's/"//g' | base64 -d
export MINIO_ROOT_USER=testuser
export MINIO_ROOT_PASSWORD=testuser123

Feel free to share few screens shots that can help.

@D1StrX
Copy link
Author

D1StrX commented Jun 23, 2023

my values file for example has this:

## Secret with default environment variable configurations to be used by MinIO Tenant.
## Not recommended for production deployments! Create the secret manually instead.
# secrets:
#   name: minio1-env-configuration
#   # MinIO root user and password
#   accessKey: minio
#   secretKey: minio123

## MinIO Tenant Definition
tenant:
  # Tenant name
  name: mindev
  ## Registry location and Tag to download MinIO Server image
  image:
    repository: quay.io/minio/minio
    tag: RELEASE.2023-04-20T17-56-55Z
    pullPolicy: IfNotPresent
  ## Customize any private registry image pull secret.
  ## currently only one secret registry is supported
  imagePullSecret: {}
  ## If a scheduler is specified here, Tenant pods will be dispatched by specified scheduler.
  ## If not specified, the Tenant pods will be dispatched by default scheduler.
  scheduler: {}
  ## Secret name that contains additional environment variable configurations.
  ## The secret is expected to have a key named config.env containing environment variables exports.
  configuration:
    name: random-env-configuration
...

Commands ran:

  • kubectl apply -f <secret.yml>
  • helm install mindev minio/tenant --namespace mindev --values .\values.yml
    Minio does use the username/password from secret.yml. But still creates myenv as shown here:
    Before deployment:
    image
    After helm deployment
    image

This is on a fresh install. Same happens when doing an helm upgrade.
Ran:

k delete secrets -n mindev myminio-env-configuration
secret "myminio-env-configuration" deleted

helm upgrade mindev minio/tenant --namespace mindev --values .\values.yml
image

So I would think that this is something specific to the MinIO tenant Helm chart?

@shtripat
Copy link
Contributor

Let me check this out @D1StrX . Steps look fine to me. Will simulate and keep posted.

@D1StrX
Copy link
Author

D1StrX commented Jun 26, 2023

Thanks. One different question regarding the predefined bucket creation values. Does the operator support defining policies and all bucket features like the minio/minio tenant repo?

@shtripat
Copy link
Contributor

shtripat commented Jun 26, 2023

myminio-env-configuration

@D1StrX what all helm repos added to your setup? Can you paste output of helm repo list command.
Also try the same scenario by downloading the operator and tenant helm releases locally from https://github.com/minio/operator/tree/master/helm-releases/operator-4.5.8.tgz and https://github.com/minio/operator/tree/master/helm-releases/tenant-4.5.8.tgz ?

@shtripat
Copy link
Contributor

Thanks. One different question regarding the predefined bucket creation values. Does the operator support defining policies and all bucket features like the minio/minio tenant repo?

@cniackz ^

@D1StrX
Copy link
Author

D1StrX commented Jun 26, 2023

myminio-env-configuration

@D1StrX what all hel repos added to your setup? Can you paste output of helm repo list command.
Also try the same scenario by downloading the operator and tenant helm releases locally from https://github.com/minio/operator/tree/master/helm-releases/operator-4.5.8.tgz and https://github.com/minio/operator/tree/master/helm-releases/tenant-4.5.8.tgz ?

I used helm repo add minio https://operator.min.io/ from the minio-operator documentation. Otherwise helm install ... minio/tenant ... wouldn't work.

I could try a release locally. I hope v5.0.5 can be used as well? (But the Issue is also in 4.5.8, its not version specific)

@shtripat
Copy link
Contributor

I can see the problem now. If you use charts from helm repo with local values.yml it does create myminio-env-configuration secret even if its commented in values.yml. The same problem doesn't happen if charts downloaded and used. Its not an issue as long as custom created secret used in values.yml for tenant. Just that an extra secret getting created by charts.

@shtripat
Copy link
Contributor

@D1StrX after some debugging and reading, I figure out that
As the base helm chart in helm repo already contains a values.yaml with below entry

secrets:
  name: myminio-env-configuration
  # MinIO root user and password
  accessKey: minio
  secretKey: minio123

the default secret myminio-env-configuration would still get created. Overwriting values using local values.yaml does work but just by commenting locally it's not going to stop base values.yaml to create the secrets.

While installing a chart, Helm combines all the values from different sources and then applies.

The order of precedence for values in helm are

  1. Explicit value set using --set during install has highest
  2. Values specified using --values
  3. Values mentioned in base values.yaml

So, here nothing is un-expected and if you want to avoid creation of default secret, download the chart, update the values.yaml and run it locally using command helm install <release-name> --namespace <NS> ./tenant
Hope this clarifies!

@D1StrX
Copy link
Author

D1StrX commented Jun 27, 2023

It does clarify. But that is against the point of Helm right? Chart values should be set based on userinput. If input equals something, use that. If empty, use default value.

This is something that almost everyone does. They have 2 fields for user/password and a helm option useExsitingSecret. If user/password: "" -> use useExsitingSecret: "value"

Could you change it in this behaviour? Instead of commenting it, a value "" can be used, which looks for the existing secret value.

secrets:
  name: myminio-env-configuration
  # MinIO root user and password
  accessKey: ""
  secretKey: ""
  ExistingSecret: "different-credentials-secret"

@D1StrX
Copy link
Author

D1StrX commented Jul 20, 2023

@pjuarezd I have further optimized the approach of above. Completely eliminating the need of:

tenant:
...
  configuration:
    name: <myminio-env-configuration>

It checks if secrets.ExistingSecret is populated. If yes, use that. If not, use secrets.name.
Created a PR #1693

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants