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

Allow pod environment variables to also be sourced from a secret #946

Merged
merged 8 commits into from
Jul 30, 2020

Conversation

frittentheke
Copy link
Contributor

@frittentheke frittentheke commented Apr 28, 2020

Currently the Operator allows to configure a ConfigMap which key-value pairs are then added to the pod environent variables (https://postgres-operator.readthedocs.io/en/latest/administrator/#custom-pod-environment-variables).
Depending on your clusters RBAC roles and settings, ConfigMaps are usually accessible quite freely (compared to secrets, which can be restricted independently) and having the value exposed in the pod spec adds to this issue of exposing credentials to access all your valuable data.

This PR adds the same functionality, but using a Secret as a source and references to the keys in the secret instead of copying the key-value pairs - like suggested in the two issues #480 and #597

But to allow for a bit more flexibility I changed the order in which the WAL_ and LOG_ envVars are applied. This then allows for those variables to be overruled by those with the same key from the referenced Secret (or ConfigMap). This change applies only to this short "whilelist" of variables. There is still protection to not have someone fiddle with the variables required for the operator to configure the core functionality.

With Spilo taking all its configuration and switches from environment variables, this PR intends to allow for more flexibility, such as to set custom S3 buckets. Also just setting custom (AWS) credentials is now possible (see my issues with this in #858 (comment))

@FxKu
Copy link
Member

FxKu commented Apr 28, 2020

reminds me #481, except this also allows for listing keys. Have you seen it? Maybe things like unit tests could be taken from their addressing the original owner @kupson

@frittentheke
Copy link
Contributor Author

@FxKu lol ... I event replied to that other PR and toally forgot about it.
I absolutely did not want to make this a race for the "best" PR regarding this feature.

But if you agree that sourcing a secret for environment variables is a sensible addition to the operator (noting again, that Spilo just loves env variables - just like @kupson said: #481 (comment))

Also the ability to override the WAL_ and LOG_ parameters are an important addition to be able to let folks configure their own backup targets (we like to call it "bring your own bucket").

I gladly add some unit tests or copy some of @kupson if he does not mind and this PR might get merged.

@FxKu
Copy link
Member

FxKu commented Jun 16, 2020

I would be fine with these changes. Unit test we've talked about are still missing, though ;)

@frittentheke
Copy link
Contributor Author

I would be fine with these changes. Unit test we've talked about are still missing, though ;)

Yeah, really sorry for the delay. I'll get those done soon.

@frittentheke
Copy link
Contributor Author

@FxKu I did some cleaning up and added the unit tests greatly inspired by @kupson and his PR #481

I refrained from adding the hashing of the secrets contents to recognize changes which @kupson did implement. This is a more general problem (kubernetes/kubernetes#22368) in Kubernetes and applies to the ConfigMap which was a valid source for env variables just as well. Until Kubernetes "fixes" this upstream, there are tools like https://github.com/stakater/Reloader which could be added as a sidecar to achieve this functionality without it being a somewhat special and unexpected behavior among Kubernetes uses.

@abdennour
Copy link

Guys! please let's move on and merge this PR.
We are waiting this feature for cloning clusters

@FxKu
Copy link
Member

FxKu commented Jul 14, 2020

@frittentheke thanks for the update. Can you rebase so that we get travis fixed.

3 very minor things I've overlooked so far. Can you add lines also in the configmap and default-config CRD manifests + one entry in the operatorconfigurations.yaml CRD in the helm chart. It's too many places to update, I have to admit.

@frittentheke
Copy link
Contributor Author

@FxKu I added the entries to the example operator config CRD and configmap.
The entries were already present in the values.yaml / values-crd.yaml and are rendered into the corresponding templates as well.

@FxKu
Copy link
Member

FxKu commented Jul 16, 2020

Thanks for fixing 2/3 😃 I should have used links.

@frittentheke
Copy link
Contributor Author

@FxKu urgh, sorry for missing that one ... pretty much the most important one ;-)
It's in now, PTAL

@FxKu
Copy link
Member

FxKu commented Jul 20, 2020

👍

1 similar comment
@Jan-M
Copy link
Member

Jan-M commented Jul 30, 2020

👍

@FxKu FxKu merged commit ece341d into zalando:master Jul 30, 2020
@FxKu
Copy link
Member

FxKu commented Jul 30, 2020

Thanks @frittentheke for your work (and patience). Also special thanks to @kupson for bringing up the idea and writing tests which were obtained here.

@vijay-dcrust
Copy link

@frittentheke thanks for the update. Can you rebase so that we get travis fixed.

3 very minor things I've overlooked so far. Can you add lines also in the configmap and default-config CRD manifests + one entry in the operatorconfigurations.yaml CRD in the helm chart. It's too many places to update, I have to admit.

@frittentheke @FxKu guys, May I know in which version this feature (defining secret var in configmap) will be released.

@haslersn
Copy link

haslersn commented Dec 26, 2020

This requires the secret to be in the same namespace as the Postgres cluster, right? I'm using Postgres clusters in many different namespaces. It would be nice if there could be a single secret in the postgres-operator namespace. Any ideas on how to implement this securely? Maybe, for each Postgres cluster, the postgres-operator could copy the secret from the postgres-operator namespace to the Postgres cluster namespace.

@frittentheke

@kupson
Copy link
Contributor

kupson commented Dec 28, 2020

This might be a little out-of-scope for postgres-operator I think. Sharing/cloning secrets across namespaces is a wider configuration issue with it's own solutions like:

@frittentheke
Copy link
Contributor Author

@vijay-dcrust it's released in 1.6 now https://github.com/zalando/postgres-operator/releases/tag/v1.6.0
@haslersn yes and for good reason. If you share something across all clusters anyways there is not much secrecy. So why not just use a ConfigMap configured via the pod_environment_configmap setting? This can then be global to the operator and to all clusters created.

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

Successfully merging this pull request may close these issues.

7 participants