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.

Sign up for GitHub

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

Jump to bottom

Support reading environment from secret/configmap #2295

Closed

ramondeklein wants to merge 6 commits into minio:master from ramondeklein:fix-env-2279

Contributor

ramondeklein commented Aug 26, 2024 •

edited

Loading

This PR implements the following changes:

Not needed to set configuration secret on the command-line anymore (is read from the tenant configuration)
Allow environment variables to be read from configmap/secret (with immediate update).
Improve "unquoting" of environment variables (fixes Error: First path segment in URL cannot contain colon #2281).
Safer method to overwrite the configuration file.
Update CRD to remove the unsupported mappings (field reference and resource field reference).
Allow running sidecar locally (by setting DEV_NAMESPACE to the namespace where the tenant is located).
Added unit test that checks writing configuration file based on configmap/secret.

Fixes #2279.

ramondeklein self-assigned this

ramondeklein force-pushed the fix-env-2279 branch 2 times, most recently from d8aebf1 to 5c02e7b Compare

August 27, 2024 11:30

ramondeklein requested review from pjuarezd, dvaldivia, cniackz and cesnietor

August 27, 2024 11:48

ramondeklein marked this pull request as ready for review

August 27, 2024 11:48

ramondeklein requested a review from jiuker

August 27, 2024 12:01

ramondeklein force-pushed the fix-env-2279 branch from 5c02e7b to c244afe Compare

August 27, 2024 12:51

Member

pjuarezd commented Aug 27, 2024

well... if tests are broken, somenthing is broken, please take a look @ramondeklein

pjuarezd requested changes

View reviewed changes

Member

pjuarezd left a comment

So, If I am understanding the code right, sidecar will no longer watch the .spec.config.name secret and will solely get the env variables from a configMap or a secret mentioned in spec.env (on a side note: my suggestions is to use spec.envFrom)

In that case shouldn't the field .spec.configuration be completelly removed form TenantSpec?

        // *Optional* +
	//
	// Specify a secret that contains additional environment variable configurations to be used for the MinIO pools.
	// The secret is expected to have a key named config.env containing all exported environment variables for MinIO+
	// +optional
	Configuration *corev1.LocalObjectReference `json:"configuration,omitempty"`

https://github.com/minio/operator/pull/2295/files#diff-4b966c49c9d32f55033009bd92b32bd169a03fb267a736d3dc5da3619965bc1fR346-R351

pkg/apis/minio.min.io/v2/types.go Show resolved Hide resolved

pkg/apis/minio.min.io/v2/types.go Show resolved Hide resolved

pkg/apis/minio.min.io/v2/types.go

+              type EnvVarSource struct {
+              	// Selects a key of a ConfigMap.
+              	// +optional
+              	ConfigMapKeyRef *ConfigMapKeySelector `json:"configMapKeyRef,omitempty" protobuf:"bytes,3,opt,name=configMapKeyRef"`

Member

pjuarezd Aug 27, 2024

This could be corev1.ConfigMapKeySelector, I don't see why we would not honor the Optional flag that is being removed

Suggested change

      
            	ConfigMapKeyRef *ConfigMapKeySelector `json:"configMapKeyRef,omitempty" protobuf:"bytes,3,opt,name=configMapKeyRef"`
          
            	ConfigMapKeyRef *corev1.ConfigMapKeySelector `json:"configMapKeyRef,omitempty" protobuf:"bytes,3,opt,name=configMapKeyRef"`

Contributor Author

ramondeklein Aug 29, 2024

I did that at first, but code is more complicated, because when the value is not optional we need to generate an error and abort the creation. It adds a lot of code. The current implementation just skips the environment variable (so optional is always enabled) if it cannot find the source.

There is no valid use-case to allow optional/non-optional support (AFAIK), so I prefered to use the simple code.

Member

pjuarezd Aug 29, 2024

How do we notify that a env variable could not be mounted to the user?, we just silently remove it?

optional could solve this problem, even if means more code, if optional: false then we should error out somewhere, if optional: true we can silently drop the env variable, as the user stated that it is fine to run MinIO without it.

Contributor Author

ramondeklein Aug 30, 2024

Yes, I just skipped it. I'll rework to support Optional and use corev1.ConfigMapKeySelector (same for secret).

pkg/apis/minio.min.io/v2/types.go Show resolved Hide resolved

pkg/apis/minio.min.io/v2/types.go Show resolved Hide resolved

sidecar/pkg/sidecar/sidecar_utils.go Show resolved Hide resolved

ramondeklein commented

View reviewed changes

sidecar/cmd/sidecar/sidecar.go

Comment on lines -39 to -43

-              		cli.StringFlag{
-              			Name:  "config-name",
-              			Value: "",
-              			Usage: "secret being watched",
-              		},

Contributor Author

ramondeklein Aug 26, 2024

This is now read from the tenant specification.

pkg/apis/minio.min.io/v2/types.go Show resolved Hide resolved

pkg/apis/minio.min.io/v2/types.go

+              type EnvVarSource struct {
+              	// Selects a key of a ConfigMap.
+              	// +optional
+              	ConfigMapKeyRef *ConfigMapKeySelector `json:"configMapKeyRef,omitempty" protobuf:"bytes,3,opt,name=configMapKeyRef"`

Contributor Author

ramondeklein Aug 29, 2024

I did that at first, but code is more complicated, because when the value is not optional we need to generate an error and abort the creation. It adds a lot of code. The current implementation just skips the environment variable (so optional is always enabled) if it cannot find the source.

There is no valid use-case to allow optional/non-optional support (AFAIK), so I prefered to use the simple code.

pkg/apis/minio.min.io/v2/types.go Show resolved Hide resolved

pkg/apis/minio.min.io/v2/types.go Show resolved Hide resolved

sidecar/pkg/sidecar/sidecar_utils.go Show resolved Hide resolved

Contributor Author

ramondeklein commented Aug 29, 2024

well... if tests are broken, somenthing is broken, please take a look @ramondeklein

@pjuarezd I've looked into this and all tests seem to run fine locally. Could it be that they time out when running in GitHub actions? I don't really get why this PR results in issues and some others seem to work fine...

ramondeklein added 6 commits

August 29, 2024 19:56


          Allow running locally

76f0dd2


          support envFrom in sidecar (minio#2279)

77208c4


          remove unsupported fields

498d10e


          update and add unit tests

dcb4c57


          fixed quoting

6517b73


          allow sidecar to access configmaps

9225dc8

ramondeklein force-pushed the fix-env-2279 branch from c244afe to 9225dc8 Compare

August 29, 2024 17:57

ramondeklein requested a review from pjuarezd

August 29, 2024 18:10

Member

pjuarezd commented Aug 29, 2024

well... if tests are broken, somenthing is broken, please take a look @ramondeklein

Thank you for fixing the bug breaking the test @ramondeklein!

ramondeklein mentioned this pull request

Update environment in MinioJob #2302

Closed

Contributor Author

ramondeklein commented Aug 30, 2024

Paused implementation, because it looks like #2253 is trying to restart pods anyway. This needs further discussion...

ramondeklein marked this pull request as draft

August 30, 2024 11:37

Contributor Author

ramondeklein commented Oct 25, 2024

Operator v7 will revert to old behaviour, so not needed anymore.

ramondeklein closed this

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

Reviewers

dvaldivia Awaiting requested review from dvaldivia

cniackz Awaiting requested review from cniackz

cesnietor Awaiting requested review from cesnietor

jiuker Awaiting requested review from jiuker

pjuarezd Awaiting requested review from pjuarezd

Labels

None yet