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.

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Support reading environment from secret/configmap #2295

wants to merge 6 commits into from
+877 −195

Conversation

ramondeklein
Copy link
Contributor

@ramondeklein 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 ramondeklein self-assigned this Aug 26, 2024
@ramondeklein ramondeklein force-pushed the fix-env-2279 branch 2 times, most recently from d8aebf1 to 5c02e7b Compare August 27, 2024 11:30
@ramondeklein ramondeklein marked this pull request as ready for review August 27, 2024 11:48
@pjuarezd
Copy link
Member

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

pjuarezd
pjuarezd requested changes Aug 27, 2024
View reviewed changes
Copy link
Member

@pjuarezd pjuarezd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
ramondeklein commented Aug 29, 2024
View reviewed changes
sidecar/cmd/sidecar/sidecar.go
Comment on lines -39 to -43
cli.StringFlag{
Name: "config-name",
Value: "",
Usage: "secret being watched",
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@ramondeklein
Copy link
Contributor Author

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...

@pjuarezd
Copy link
Member

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

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

@ramondeklein ramondeklein mentioned this pull request Aug 30, 2024
Open
@ramondeklein
Copy link
Contributor Author

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

@ramondeklein ramondeklein marked this pull request as draft August 30, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvaldivia dvaldivia Awaiting requested review from dvaldivia

@cniackz cniackz Awaiting requested review from cniackz

@cesnietor cesnietor Awaiting requested review from cesnietor

@jiuker jiuker Awaiting requested review from jiuker

@pjuarezd pjuarezd Awaiting requested review from pjuarezd

Requested changes must be addressed to merge this pull request.

Assignees

@ramondeklein ramondeklein

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@ramondeklein @pjuarezd