chore: allow disabling of hydra-maester

[clement-buchart]

Hello,

This is a solution proposal for #122

Let me know if you think this is an acceptable fix, and I can propose the same change for oathkeeper.

If not, let me know what you would expect.

Cheers.

[aeneasr]

How would using maester look like if those charts are separated?

[clement-buchart]

If you want to install

• Hydra with maester : helm install ory/hydra
• Hydra without maester : helm install ory/hydra --set maester.enabled=false
• Maester only : helm install ory/hydra-maester

I think that is what is documented, that's why I didn't change the docs.

[aeneasr]

I think maester takes a couple of settings from the parent chart right now, those values should probably be documented as well and/or automated if possible. We need to set the deployment name for example (iirc).

Is separating the charts the only solution here? I would like to keep them together but if there really is no other way...

[clement-buchart]

I think maester takes a couple of settings from the parent chart right now

I must have missed those, may you point them out ?
I'll have another look, but this is the way we deploy hydra internally and maester, and it works without additional values (for now)

I would like to keep them together but if there really is no other way...

I don't think there is a way to allow for the disabling of maester without making them 2 separate charts, since anything in the charts subfolder WILL be deployed by helm, no matter what.
May you explain why you think they should be the same ?
If so, why having 2 Charts.yaml, why not simply having all the maester resources in hydra chart ?

I think these app have 2 different life-cycles, and should be deployed separately.
In particular, hydra may be deployed multiple times per cluster, but there is not much value in deploying hydra-maester multiple times (it's actually a problem since they'll fight for the CRD definition which is at the cluster scope)

I would have understood if hydra-maester would have been unable to address multiple hydra instances, but since the OAuth2Client CRD defines the target admin API, that's not the case.

Eventually, you may even one day have a HydraInstance CRD that define a hydra instance that hydra-maester would deploy.

[aeneasr]

I must have missed those, may you point them out ?
I'll have another look, but this is the way we deploy hydra internally and maester, and it works without additional values (for now)

I think if you change the fullNameOverride of hydra itself ( #76 ), the names used by maester are no longer correct. We might need to run a test case for this. I might be mistaken but I think that's one of the issues.

I don't think there is a way to allow for the disabling of maester without making them 2 separate charts, since anything in the charts subfolder WILL be deployed by helm, no matter what.

I see, I thought there was a switch to disable that but apparently it's not working as intended.

If so, why having 2 Charts.yaml, why not simply having all the maester resources in hydra chart ? I think these app have 2 different life-cycles, and should be deployed separately.
In particular, hydra may be deployed multiple times per cluster, but there is not much value in deploying hydra-maester multiple times (it's actually a problem since they'll fight for the CRD definition which is at the cluster scope)

Yes, you are right! Let's take your approach then :)

[aeneasr]

It was not published standalone as of now (hydra-maester was), so i didn't add it.
I will.

I was asking because it is already in the PR right? :)

[clement-buchart]

I think if you change the fullNameOverride of hydra itself ( #76 ), the names used by maester are no longer correct. We might need to run a test case for this. I might be mistaken but I think that's one of the issues.

Ok, I understand.
AFAIK the only way to have a value in both charts is to use global values

If you're OK with fullNameOverride becoming global.hydra.fullNameOverride, I can add this to the MR

[aeneasr]

If you're OK with fullNameOverride becoming global.hydra.fullNameOverride, I can add this to the MR

Ah, I see. I don't really know, what's your take on this? I mean we could also just document that right?

Regarding the PR, I think it's pretty much good to go except for the thing with fullNameOverride. I merged your other changes so you'll need to rebase/merge with master :)

[clement-buchart]

Ah, I see. I don't really know, what's your take on this? I mean we could also just document that right?

I think fullNameOverride is a pretty strong convention in the helm community, so going against it is probably not a great idea.

Maybe the best compromise would be to keep fullNameOverride, but have a check in the hydra chart that says "if fullNameOverride is not nil AND maester.enabled is true AND maester.hydraFullNameOverride is nil", fail with the fail function ans a pretty message

This way we keep fullNameOverride where it is expected, and warn potential users at the templating step that it must be set at two places to work effectively with a contextual message.

Obviously with a line in the documentation.

Ok for you ?

[clement-buchart]

I went ahead and implemented my proposal for you to review.

helm template . : OK

helm template . --set fullnameOverride=toto KO

Error: template: hydra/templates/NOTES.txt:1:3: executing "hydra/templates/NOTES.txt" at <include "hydra.check.override.consistency" .>: error calling include: template: hydra/templates/_helpers.tpl:131:3: executing "hydra.check.override.consistency" at <fail "hydra fullname has been overridden, but the new value has not been provided to maester. Set maester.hydraFullnameOverride">: error calling fail: hydra fullname has been overridden, but the new value has not been provided to maester. Set maester.hydraFullnameOverride

helm template . --set fullnameOverride=toto --set maester.enabled=false : OK

helm template . --set fullnameOverride=toto --set maester.hydraFullnameOverride=toto : OK

helm template . --set fullnameOverride=toto --set maester.hydraFullnameOverride=toti : KO

Error: template: hydra/templates/NOTES.txt:1:3: executing "hydra/templates/NOTES.txt" at <include "hydra.check.override.consistency" .>: error calling include: template: hydra/templates/_helpers.tpl:133:3: executing "hydra.check.override.consistency" at <fail (tpl "hydra fullname has been overridden, but a different value was provided to maester. {{ .Values.maester.hydraFullnameOverride }} different of {{ .Values.fullnameOverride }}" .)>: error calling fail: hydra fullname has been overridden, but a different value was provided to maester. toti different of toto

Cheers

[aeneasr]

Yeah wow, good job! I guess this is now ready for a final review? :)

[clement-buchart]

Yes, I think everything's here.

[aeneasr]

👍

[aeneasr]

I'm not entirely sure - do we need the fullNameOverride in oathkeeper as well?

[aeneasr]

@clement-buchart thank you for your great work!

[clement-buchart]

Indeed when using fullnameOverride in oathkeeper, oathkeeper maester wouldn't use the correct configmap name (that issue is present on master)

I added a fix with the same strategy.

@clement-buchart thank you for your great work!

My pleasure, thanks for your responsiveness