Ability to set pod environment variables on cluster resource by dmvolod · Pull Request #1794 · zalando/postgres-operator

dmvolod · 2022-02-22T05:59:04Z

Fixes #1566
Closes #1209
Closes #1422
Closes #907
Closes #1807

Please pay attention, that this PR implementing an individual []EnvVar option to the cluster CR and has higher priority to the cluster pod_environment_secret and pod_environment_configmap options.

TODO (after approach approve)

Doc changes
Unit testing

dmvolod · 2022-02-28T14:07:06Z

@FxKu could you please to have a look at the fix approach. If it will be suitable, I will add docs and unit-test for it. Thanks in advance.

cdmikechen · 2022-03-04T13:25:44Z

In CRD, Is there possible to add only some environment variables to pod? for example

spec:
  env:
    - name: ENV_1
      value: VALUE_1

dmvolod · 2022-03-04T13:34:22Z

In CRD, Is there possible to add only some environment variables to pod? for example

Not sure, that's good approach as will need to support both setup for predefined ConfigMap/Secret and current one. Common way much better and didn't overload CR if it's not needed

dmvolod · 2022-03-17T18:18:14Z

@FxKu could you please to find a time and have a look at the fix approach. If it will be suitable, I will add docs and unit-test for it. Thanks in advance.

FxKu · 2022-03-21T07:16:27Z

Hey @dmvolod, while it looks elegant at first, I don't think it should be solved like this because only one way is possible - global config map or the need to add config maps for each cluster. The latter will be tedious if you need cluster-specific variables only for a handful of clusters while the rest of your fleet should be configured in the same way. Better have env variables from cluster-specific config maps / secrets to overwrite global ones - like we do it with other options, too. Probably, partly overwriting (in case of same env variables) will be most convenient.

dmvolod · 2022-03-21T07:29:19Z

Hey @dmvolod, while it looks elegant at first, I don't think it should be solved like this because only one way is possible - global config map or the need to add config maps for each cluster. The latter will be tedious if you need cluster-specific variables only for a handful of clusters while the rest of your fleet should be configured in the same way. Better have env variables from cluster-specific config maps / secrets to overwrite global ones - like we do it with other options, too. Probably, partly overwriting (in case of same env variables) will be most convenient.

Thanks for feedback, @FxKu . I'm also agree with approach to have a ConfigMap/Secret parameter for each cluster individually if needed and present. If you agree with this approach I will add these options to the CRD and implement logic to overwrite global parameters if CR parameters are present and find in the cluster ConfigMap/Secret.

cdmikechen · 2022-03-22T12:36:15Z

I‘m looking forward to the progress of this PR.
I have similar needs at present. We need to deploy multiple Postgres to different namespaces and back up to different minio services.

FxKu · 2022-03-23T08:29:33Z

@cdmikechen note that you can already run multiple operators next to each other configured differently. They could either watch only one namespace or you define a CONTROLLER_ID in deployment and manifests to specify ownership over clusters.

cdmikechen · 2022-03-23T09:05:46Z

@cdmikechen note that you can already run multiple operators next to each other configured differently. They could either watch only one namespace or you define a CONTROLLER_ID in deployment and manifests to specify ownership over clusters.

@FxKu
Thanks~ It looks like a good solution for my needs. I used to deploy only one operator to manage all namespaces, which will really bring me some troubles.
I'll try it later and see if there will be any other problems.

dmvolod · 2022-03-25T05:24:37Z

@FxKu please have a look at the new approach with individual []EnvVar option again and has a higher priority to the global cluster options. Thanks in advance.

gc-jro · 2022-03-28T11:14:11Z

Hi @dmvolod, thanks for this PR.

Do you still plan to add the possibility to refer to a cluster specific secret for environment variables in the postgresql custom resource?

I’ve a slightly different set of requirements, where we maintain our cluster configuration in Git, but aren’t allowed to store confidential information (like wal-g backup settings) in plain text. To circumvent this, we use SealedSecrets [1] to store the settings encrypted in Git. In our k8s cluster the SealedSecrets get decrypted into normal secrets that can be used by the cluster. I’ve also the requirement that we’ve multiple postgresql clusters in the same namespace, and the need that they use different wal-g backup settings.

Therefore, it would be great if in addition to use individual []EnvVar settings in the postgresql CR, there would be an option to specify an individual secret in the postgresql CR that would be used for populating environment variables.
If you aren’t planning to add this option that’s totally fine, then I’ll ask my boss to get some time to patch the functionality in myself.

[1] https://github.com/bitnami-labs/sealed-secrets

dmvolod · 2022-03-29T07:53:55Z

Hi @gc-jro !

Yes, sure. Could you please to have a look at the EnvVar struct which is including EnvVarSource field and supports referencing to the Secret (SecretKeyRef).
https://github.com/kubernetes/api/blob/master/core/v1/types.go#L1977-L2015

What do you think about it?

gc-jro · 2022-03-29T10:53:58Z

Ah I see. Thanks a lot for clearing that up for me. I really like it, it'll solve all my problems 😄

I'll just have to define something like

apiVersion: v1
kind: Secret
metadata:
  name: wal-g-envs
  namespace: yyy
data:
  BACKUP_NUM_TO_RETAIN: xxx
  ...
---
apiVersion: "acid.zalan.do/v1"
kind: postgresql
metadata:
  name: wal01-postgres-cluster
  namespace: yyy
spec:
  teamId: "wal01"
  volume:
    size: 20Gi
  env:
    - name: BACKUP_NUM_TO_RETAIN
      valueFrom:
        secretKeyRef:
          key: BACKUP_NUM_TO_RETAIN
          name: wal-g-envs
  ...

to forward the values from the secret to the database. Very neat.

Thank you very much. I can't wait to give it a spin once its merged into master.

FxKu · 2022-03-31T15:28:55Z

Thank @dmvolod ! Like the new approach much better. As you already pointed out, docs and unit test are the remaining bits to get this merged.

FxKu · 2022-04-06T12:00:32Z

I got it working now. See my gist: https://gist.github.com/FxKu/0b4e0641f1c846a2a0edbc2c6ee6a0cc
Because I changed the expected list of []v1.EnvVar in TestPodEnvironmentConfigMapVariables the comparison with getPodEnvironmentConfigMapVariables result did not work anymore. But maybe it should be a test on its own together with fetching variables for the secrets and test clearly what overrides what (and also make it clearer in the docs, but I can help there).

dmvolod · 2022-04-06T12:08:42Z

I got it working now. See my gist: https://gist.github.com/FxKu/0b4e0641f1c846a2a0edbc2c6ee6a0cc
Because I changed the expected list of []v1.EnvVar in TestPodEnvironmentConfigMapVariables the comparison with getPodEnvironmentConfigMapVariables result did not work anymore. But maybe it should be a test on its own together with fetching variables for the secrets and test clearly what overrides what (and also make it clearer in the docs, but I can help there).

Thanks for test, @FxKu
Probably, we should move EnvVar logic to the generateSpiloPodEnvVars and utilize another tests for that. What do you think about it?

FxKu · 2022-04-06T12:19:34Z

+		}
+	}
+	return false
+}


Actually, it might be even better to do isEnvVarPresent in generateSpiloPodEnvVars, too, instead of appending duplicate variables and resolving it later with deduplicateEnvVars.

Fix logic, please review.

FxKu · 2022-04-07T08:33:29Z

+			envVarConstant: "WAL_S3_BUCKET",
+			envVarValue:    "custom-s3-bucket",
+		},
+	}


custom variables should not be pre-pended so this addition is also obsolete.

This is approach from the following point

A custom variable called like a hard-coded variable that can be overridden (like WAL_S3_BUCKET)

FxKu · 2022-04-07T08:38:33Z

+					},
+				},
+			},
+		},


The test should incl. 3 kinds of variables - like in my gist.

A custom variable called like a hard-coded variable that cannot be overridden (I uses KUBERNETES_SCOPE_LABEL)

A custom variable called like a hard-coded variable that can be overridden (like WAL_S3_BUCKET)

And a custom variable that's not in the list of hard-coded variables. And there we can test, if a variable from c.Spec.Env overwrites on from environment ConfigMap and/or Secret.

Good point, thanks will implement this options.

Implemented tests, please review.

FxKu · 2022-04-07T09:14:24Z

Ok aside from missing unit tests there's one problem that has to be fixed. Right now we only append env vars and rely on deduplicateEnvVars to come up with the right set in the end. That is, it will take only the first mention of a variable into account except when the variable starts with WAL_ or LOG_. Then it will take the last mention.

This logic will not work anymore now that we have a third layer, the cluster-specific Env.

It MUST NOT override our hard-coded set of variables from generateSpiloPodEnvVars up until this line. Only append after that.
MUST override the PodEnvironmentConfigMap and/or Secret. So we append before that. But then WAL_ or LOG_ settings from PodEnvironmentConfigMap and/or Secret would override c.Spec.Env because of the logic in deduplicateEnvVars.

That's why I suggested that we should not simply append to the list of envVars but only do so if the key does not exists yet. So basically what you partly did with isEnvVarPresent. But instead of adding this check everywhere, we better write an append function for EnvVar lists that incl. this check. Finally, deduplicateEnvVars can be removed.

dmvolod · 2022-04-07T10:14:48Z

That's why I suggested that we should not simply append to the list of envVars but only do so if the key does not exists yet. So basically what you partly did with isEnvVarPresent. But instead of adding this check everywhere, we better write an append function for EnvVar lists that incl. this check. Finally, deduplicateEnvVars can be removed.

Does it means that we need to utilize new appendEnvVars function everywhere inside the generateSpiloPodEnvVars and related functions?

FxKu · 2022-04-07T11:08:11Z

Yes, inside generateSpiloPodEnvVars. The flow should be:

envVars := []v1.EnvVar{...}
append spiloCompathWalPathList
append c.Spec.Env
fetch from getPodEnvironmentSecretVariables and append
fetch from getPodEnvironmentConfigMapVariables and append
fetch from generateCloneEnvironment and append
fetch from generateStandbyEnvironment and append
append WAL_ LOG_ AZURE_ etc. variables

I don't see why anything related to spilo env vars should happen outside of generateSpiloPodEnvVars. Noticed that deduplicateEnvVars is also used for sidecars. So maybe it can stay then.

dmvolod · 2022-04-07T15:19:35Z

Yes, inside generateSpiloPodEnvVars. The flow should be:

envVars := []v1.EnvVar{...}

append spiloCompathWalPathList

append c.Spec.Env

fetch from getPodEnvironmentSecretVariables and append

fetch from getPodEnvironmentConfigMapVariables and append

fetch from generateCloneEnvironment and append

fetch from generateStandbyEnvironment and append

append WAL_ LOG_ AZURE_ etc. variables

I don't see why anything related to spilo env vars should happen outside of generateSpiloPodEnvVars. Noticed that deduplicateEnvVars is also used for sidecars. So maybe it can stay then.

Thanks for idea, please have a look at the code. Added functions and changed from plain append to the new one.

FxKu · 2022-04-11T08:15:09Z

👍

sdudoladov · 2022-04-11T08:15:55Z

👍

FxKu · 2022-04-11T08:23:07Z

Thanks @dmvolod for your contribution. Good collaboration!

dmvolod · 2022-04-11T08:24:48Z

Thanks @FxKu and @sdudoladov for review and ideas.

dmvolod requested review from CyberDem0n, FxKu, Jan-M and sdudoladov as code owners February 22, 2022 05:59

FxKu mentioned this pull request Feb 25, 2022

Retry when getting the pod_environment_secret #1777

Merged

dmvolod force-pushed the issue-1566 branch from f1b3705 to b984589 Compare February 26, 2022 10:46

dmvolod force-pushed the issue-1566 branch from b984589 to cad915c Compare March 2, 2022 04:47

dmvolod mentioned this pull request Mar 2, 2022

Copy pod_environment_secret to the cluster namespaces #1789

Closed

FxKu reviewed Mar 21, 2022

View reviewed changes

Comment thread pkg/cluster/k8sres.go Outdated

FxKu added this to the 1.9 milestone Mar 24, 2022

dmvolod force-pushed the issue-1566 branch from cad915c to a2f30a1 Compare March 25, 2022 05:20

dmvolod force-pushed the issue-1566 branch from a2f30a1 to 4cb4952 Compare March 25, 2022 09:21

dmvolod changed the title ~~Ability to set pod_environment_secret and pod_environment_configmap on cluster resource~~ Ability to set pod environment variables on cluster resource Mar 30, 2022

dmvolod force-pushed the issue-1566 branch from 4cb4952 to 4966ec5 Compare March 30, 2022 06:33

dmvolod force-pushed the issue-1566 branch from bc38f98 to 20e580c Compare April 1, 2022 19:28

FxKu removed this from the 1.9 milestone Apr 4, 2022

Return test and EnvVar logic back

cb403c4

FxKu reviewed Apr 6, 2022

View reviewed changes

Comment thread pkg/cluster/k8sres.go Outdated

FxKu reviewed Apr 6, 2022

View reviewed changes

Comment thread pkg/cluster/k8sres.go Outdated

FxKu reviewed Apr 6, 2022

View reviewed changes

Comment thread pkg/cluster/k8sres.go Outdated

FxKu reviewed Apr 6, 2022

View reviewed changes

Comment thread pkg/cluster/k8sres.go Outdated

FxKu reviewed Apr 6, 2022

View reviewed changes

New Env logic and unit test suite

c87273b

dmvolod force-pushed the issue-1566 branch from 10703e1 to c87273b Compare April 6, 2022 14:13

FxKu reviewed Apr 7, 2022

View reviewed changes

Comment thread pkg/cluster/k8sres.go Outdated

FxKu reviewed Apr 7, 2022

View reviewed changes

Add appendEnvVars function and fix core logic

e3f7816

dmvolod added 2 commits April 7, 2022 18:59

Remove deduplicateEnvVars function

47c0ea3

Move ENABLE_WAL_PATH_COMPAT to the generateSpiloPodEnvVars function

c853dab

sdudoladov merged commit 9bcb25a into zalando:master Apr 11, 2022

FxKu mentioned this pull request Apr 13, 2022

Disable WAL archiving #908

Closed

jehof mentioned this pull request Jul 22, 2022

allow ability to adjust patroni log setting (#1203) #1857

Open

eberlep mentioned this pull request Aug 19, 2022

Allow to temporarily disable WAL archiving fi-ts/postgreslet#415

Open

FxKu mentioned this pull request Jan 4, 2024

[feature request] ability to restore from wal-e backup to specific point of time #569

Closed

FxKu mentioned this pull request Apr 24, 2024

PodEnvironmentConfigMap per cluster #1025

Closed

Uh oh!

Conversation

dmvolod commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmvolod commented Feb 28, 2022

Uh oh!

cdmikechen commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmvolod commented Mar 4, 2022

Uh oh!

dmvolod commented Mar 17, 2022

Uh oh!

FxKu commented Mar 21, 2022

Uh oh!

Uh oh!

dmvolod commented Mar 21, 2022

Uh oh!

cdmikechen commented Mar 22, 2022

Uh oh!

FxKu commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdmikechen commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmvolod commented Mar 25, 2022

Uh oh!

gc-jro commented Mar 28, 2022

Uh oh!

dmvolod commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gc-jro commented Mar 29, 2022

Uh oh!

FxKu commented Mar 31, 2022

Uh oh!

FxKu commented Apr 6, 2022

Uh oh!

dmvolod commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FxKu Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

dmvolod Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FxKu Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

dmvolod Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

FxKu Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

dmvolod Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

dmvolod Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

FxKu commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmvolod commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FxKu commented Apr 7, 2022

Uh oh!

dmvolod commented Apr 7, 2022

Uh oh!

FxKu commented Apr 11, 2022

dmvolod commented Feb 22, 2022 •

edited

Loading

cdmikechen commented Mar 4, 2022 •

edited

Loading

FxKu commented Mar 23, 2022 •

edited

Loading

cdmikechen commented Mar 23, 2022 •

edited

Loading

dmvolod commented Mar 29, 2022 •

edited

Loading

dmvolod commented Apr 6, 2022 •

edited

Loading

FxKu commented Apr 7, 2022 •

edited

Loading

dmvolod commented Apr 7, 2022 •

edited

Loading