Custom TLS secrets #690
Custom TLS secrets #690
Conversation
zimbatm
requested review from
CyberDem0n,
FxKu,
Jan-M,
RafiaSabih,
avaczi,
erthalion and
sdudoladov
as code owners
|
The topic of having proper certs for the Postgres pots comes up regularly. So far my idea was more a minimalist CA, where a secret or AWS KMS encrypted value contains the private key to sign local certs and DB host names. I have to read into what you are trying to do here. And then it is questionable who provisions what, does the operator provision things more merely take care of mounting/referencing the right things in the statefulset. Thanks for bringing this up! |
|
Hey @Jan-M, thanks for taking a look :) On our side we have the cert-manager operator deployed to handles the certificates. It introduces new types of resources such as the The idea of this PR at the moment is to add a new If I understand properly what you mentioned, your idea would be to have the postgres-operator trigger the certificate generation. The upside would be that ceritifcates would be generate on a per-host basis instead of on a per-cluster basis. But the downside is that now the postgres-operator has to integrate more tightly to either cert-manager or another operator. |
|
Nope, I do not think the operator should start dealing with TLS cert and issueing. So in favor of supporting necessary things to inject and or provide TLS certs to Postgres pods. I just ment we had an idea once about a "CA" for local clusters. Can we not have the secret name as a format string in the config and then some global and per cluster enable disable? |
|
Okay this PR is working now. I still need to handle certificate rotation somehow, and add more tests and documentation.
I'm not sure what the advantage would be. Since both the Certificate and Postgresql resources are created by the user, he is in the best position to define a name and make the connection explicit. Otherwise he would have to ask the administrator what secretName mapping has been configured on the operator. |
|
For certificate rotation, this could be handled automatically in the spilo container: zalando/spilo#373 |
There was a problem hiding this comment.
if the fsgroup is required for the TLS feature to work, it should be spelled out.
There was a problem hiding this comment.
good idea. I already have a check in the code but I will add a note in the documentation as well.
|
Alright, this is ready for another review |
|
now with tests! |
|
once zalando/spilo#373 is merged I also want to add support for CA certificates |
|
@zimbatm so we merged your PR in Spilo. Can you update this PR - so that we might include it in the next release. |
|
thanks @FxKu! It looks like coverall wants me to write even more tests. Do you have a recommendation at what place I can write tests for pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go ? |
|
At the moment, I see some overlap with #736 and also with the current additional secret mount implementation. If it would be possible to specify an array additional volumes, I guess both features could be covered with it. On the other hand, we also aim for low complexity of the cluster manifest. Just one flag |
|
okay I will take a look tomorrow. Today I also added configuration to pass a CA file (not yet pushed) that will make the implementation more complete and also allow for checking client certificates. |
|
This PR is ready to me. It's tested and it's working for me (aside from the master build failures). I think the other PR is nice but also opens the deployment to much more variance. Here the nice thing is that the TLS setup is delegated to the controller and has a specific implementation, whereas the other PR allows to build a custom TLS scheme on top with a initContainer, custom Volume and maybe a sidecar container to handle key rotation. So the out-of-the-box experience is probably worse as the user has to figure out how to tie everything together. I believe that using a secret volume is the right call to store the TLS certificates. |
|
Any chance this can be merged? |
|
Is there anything I can do to get this merged? |
I'm afraid no, unfortunately it take so long on our side. But I have this PR in my plan. |
|
Well that's life. As long as you're not blocked by me :) |
|
Btw, @zimbatm could you merge master once into this PR? Or make it available to push to this branch for operator maintainers. We can start fixing travis tests here. |
|
@erthalion: rebased! I am looking into how to give you write access to the branch. |
Apparently there should be some toggle already |
|
It looks like this option is disabled when the PR is coming from an org instead of an account. Do you want me to open a new PR coming from my account? |
|
just went ahead and opened #798 |
Interesting. Nope, let's keep it like it is right now. If I would like to suggest something, I can fork a PR fork :) |
|
Closing as superseded by newer PR |
Hi,
I am looking for early feedback on this PR. The goal right now is to talk about the design and if this is something Zalando would be interested in merging, plus an associated roadmap of things that would need to happen. If we could do that it would be much appreciated!
The main motivation is to be able to specify a custom TLS certificate from k8s secrets ( #633 ). Right now it only supports static secrets but down the road I would also like to support reloading the secrets whenever they change to allow proper key rotation and integration with cert-manager.
Cheers,
z