Support reading environment from secret/configmap #2295
Conversation
ramondeklein
force-pushed
the
fix-env-2279
branch
2 times, most recently
from
d8aebf1 to
5c02e7b
Compare
ramondeklein
force-pushed
the
fix-env-2279
branch
from
5c02e7b to
c244afe
Compare
|
well... if tests are broken, somenthing is broken, please take a look @ramondeklein |
There was a problem hiding this 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"`
| type EnvVarSource struct { | ||
| // Selects a key of a ConfigMap. | ||
| // +optional | ||
| ConfigMapKeyRef *ConfigMapKeySelector `json:"configMapKeyRef,omitempty" protobuf:"bytes,3,opt,name=configMapKeyRef"` |
There was a problem hiding this comment.
This could be corev1.ConfigMapKeySelector, I don't see why we would not honor the Optional flag that is being removed
| ConfigMapKeyRef *ConfigMapKeySelector `json:"configMapKeyRef,omitempty" protobuf:"bytes,3,opt,name=configMapKeyRef"` | |
| ConfigMapKeyRef *corev1.ConfigMapKeySelector `json:"configMapKeyRef,omitempty" protobuf:"bytes,3,opt,name=configMapKeyRef"` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, I just skipped it. I'll rework to support Optional and use corev1.ConfigMapKeySelector (same for secret).
| cli.StringFlag{ | ||
| Name: "config-name", | ||
| Value: "", | ||
| Usage: "secret being watched", | ||
| }, |
There was a problem hiding this comment.
This is now read from the tenant specification.
| type EnvVarSource struct { | ||
| // Selects a key of a ConfigMap. | ||
| // +optional | ||
| ConfigMapKeyRef *ConfigMapKeySelector `json:"configMapKeyRef,omitempty" protobuf:"bytes,3,opt,name=configMapKeyRef"` |
There was a problem hiding this comment.
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.
@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
force-pushed
the
fix-env-2279
branch
from
c244afe to
9225dc8
Compare
Thank you for fixing the bug breaking the test @ramondeklein! |
|
Paused implementation, because it looks like #2253 is trying to restart pods anyway. This needs further discussion... |
|
Operator v7 will revert to old behaviour, so not needed anymore. |
This PR implements the following changes:
DEV_NAMESPACEto the namespace where the tenant is located).Fixes #2279.