Allow customize http vhost config using HttpdCustomization.CustomConfigSecret

[stuggi]

This change allows to customize the httpd vhost config using this parameter to specify a secret that contains service config data. The content of each provided snippet gets rendered as a go template and placed into /etc/httpd/conf/httpd_custom_ .
At the end of the vhost config in the default httpd template these custom configs get included using Include conf/httpd_custom_*.

For information on how sections in httpd configuration get merged, check section "How the sections are merged" in https://httpd.apache.org/docs/current/sections.html#merging

All possible parameters which can be use in a template can be looked up in the rendered config-data cm of the service like:

$ oc get cm -n openstack horizon-config-data -o json | jq -r .data.TemplateParameters | base64 -d

or in the running pod of the service in the file:

$ cat /var/lib/config-data/default/TemplateParameters

The content is a versioned dump of the parameters of the service operator, like:

KeystoneEndpointInternal: https://keystone-internal.openstack.svc:5000
Override: false
SSLCertificateFile: /etc/pki/tls/certs/horizon.crt
SSLCertificateKeyFile: /etc/pki/tls/private/horizon.key
ServerName: horizon.openstack.svc
TLS: true
MemcachedServers: memcached-0.memcached.openstack.svc:11212
...

Depends-On: openstack-k8s-operators/lib-common#591
Depends-On: openstack-k8s-operators/lib-common#593

Jira: https://issues.redhat.com/browse/OSPRH-13100

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stuggi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [stuggi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[fmount]

I don't think we need this level of abstraction for an operator where extraMounts are already used to inject files. It would be easier (and keep the interface consistent), to do something like: #510 that we know it would avoid us to introduce any API change in the future. @stuggi @mcgonago wdyt

[openshift-merge-robot]

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[stuggi]

I don't think we need this level of abstraction for an operator where extraMounts are already used to inject files. It would be easier (and keep the interface consistent), to do something like: #510 that we know it would avoid us to introduce any API change in the future. @stuggi @mcgonago wdyt

this is true for api services, which have extraMounts. We added this approach to keystone before it had extraMounts there. I think got added there since then. Do we plan to add extraMounts to all api services? wondering if there should be a single documented way for consistency on how to customize http config for all services and not a mix of using extraMounts or this approach where we do not have them.

[stuggi]

I don't think we need this level of abstraction for an operator where extraMounts are already used to inject files. It would be easier (and keep the interface consistent), to do something like: #510 that we know it would avoid us to introduce any API change in the future. @stuggi @mcgonago wdyt

this is true for api services, which have extraMounts. We added this approach to keystone before it had extraMounts there. I think got added there since then. Do we plan to add extraMounts to all api services? wondering if there should be a single documented way for consistency on how to customize http config for all services and not a mix of using extraMounts or this approach where we do not have them.

@fmount not sure if it is required in other operators, but could you use templates with extraMounts, like in this example openstack-k8s-operators/keystone-operator#525 (comment) , needed for keystone federation. iirc that would be not possible.

[fmount]

I don't think we need this level of abstraction for an operator where extraMounts are already used to inject files. It would be easier (and keep the interface consistent), to do something like: #510 that we know it would avoid us to introduce any API change in the future. @stuggi @mcgonago wdyt

this is true for api services, which have extraMounts. We added this approach to keystone before it had extraMounts there. I think got added there since then. Do we plan to add extraMounts to all api services? wondering if there should be a single documented way for consistency on how to customize http config for all services and not a mix of using extraMounts or this approach where we do not have them.

@fmount not sure if it is required in other operators, but could you use templates with extraMounts, like in this example openstack-k8s-operators/keystone-operator#525 (comment) , needed for keystone federation. iirc that would be not possible.

Right, templating is not possible in that case, but I think we get keystone covered in that case because the feature is already in.
In general, we have a Jira telling to add extraMounts where (and when) needed [2], and it would avoid creating custom interfaces when not needed, and thinking in terms of a ctlplane v2 we can also remove parameters that would be covered by extraMounts.
So while I agree that we should keep consistency, having 2 or 3 different interfaces to do the same thing is probably not the right thing to do. While part of the operators have extraMounts already, I would use that interface where needed unless the need is to do something else, like the keystone use case. I don't see the need of templating here for example, and in general the configMap or the secret mounted via extraMount is something crafted by the human operator.
Templating is useful and very powerful, but we require the human administrator to have knowledge about it [1]. Not that this is a bad thing, but in the long term the idea is to have extraMounts spread across the board (probably with a dedicated CR like Topologies).

[1] openstack-k8s-operators/keystone-operator#525 (comment)
[2] https://issues.redhat.com/browse/RHOSSTRAT-296

[stuggi]

I don't think we need this level of abstraction for an operator where extraMounts are already used to inject files. It would be easier (and keep the interface consistent), to do something like: #510 that we know it would avoid us to introduce any API change in the future. @stuggi @mcgonago wdyt

this is true for api services, which have extraMounts. We added this approach to keystone before it had extraMounts there. I think got added there since then. Do we plan to add extraMounts to all api services? wondering if there should be a single documented way for consistency on how to customize http config for all services and not a mix of using extraMounts or this approach where we do not have them.

@fmount not sure if it is required in other operators, but could you use templates with extraMounts, like in this example openstack-k8s-operators/keystone-operator#525 (comment) , needed for keystone federation. iirc that would be not possible.

Right, templating is not possible in that case, but I think we get keystone covered in that case because the feature is already in. In general, we have a Jira telling to add extraMounts where (and when) needed [2], and it would avoid creating custom interfaces when not needed, and thinking in terms of a ctlplane v2 we can also remove parameters that would be covered by extraMounts. So while I agree that we should keep consistency, having 2 or 3 different interfaces to do the same thing is probably not the right thing to do. While part of the operators have extraMounts already, I would use that interface where needed unless the need is to do something else, like the keystone use case. I don't see the need of templating here for example, and in general the configMap or the secret mounted via extraMount is something crafted by the human operator. Templating is useful and very powerful, but we require the human administrator to have knowledge about it [1]. Not that this is a bad thing, but in the long term the idea is to have extraMounts spread across the board (probably with a dedicated CR like Topologies).

[1] openstack-k8s-operators/keystone-operator#525 (comment) [2] https://issues.redhat.com/browse/RHOSSTRAT-296

do I get it right, what you suggest is to add the include like in https://github.com/openstack-k8s-operators/glance-operator/blob/eee52cfdc598521211804db5f7b16ec80461f750/templates/common/config/httpd.conf#L30 to the vhost template where we have extraMounts, so that a user could add optional static params using this file?

[fmount]

I don't think we need this level of abstraction for an operator where extraMounts are already used to inject files. It would be easier (and keep the interface consistent), to do something like: #510 that we know it would avoid us to introduce any API change in the future. @stuggi @mcgonago wdyt

this is true for api services, which have extraMounts. We added this approach to keystone before it had extraMounts there. I think got added there since then. Do we plan to add extraMounts to all api services? wondering if there should be a single documented way for consistency on how to customize http config for all services and not a mix of using extraMounts or this approach where we do not have them.

@fmount not sure if it is required in other operators, but could you use templates with extraMounts, like in this example openstack-k8s-operators/keystone-operator#525 (comment) , needed for keystone federation. iirc that would be not possible.

Right, templating is not possible in that case, but I think we get keystone covered in that case because the feature is already in. In general, we have a Jira telling to add extraMounts where (and when) needed [2], and it would avoid creating custom interfaces when not needed, and thinking in terms of a ctlplane v2 we can also remove parameters that would be covered by extraMounts. So while I agree that we should keep consistency, having 2 or 3 different interfaces to do the same thing is probably not the right thing to do. While part of the operators have extraMounts already, I would use that interface where needed unless the need is to do something else, like the keystone use case. I don't see the need of templating here for example, and in general the configMap or the secret mounted via extraMount is something crafted by the human operator. Templating is useful and very powerful, but we require the human administrator to have knowledge about it [1]. Not that this is a bad thing, but in the long term the idea is to have extraMounts spread across the board (probably with a dedicated CR like Topologies).
[1] openstack-k8s-operators/keystone-operator#525 (comment) [2] https://issues.redhat.com/browse/RHOSSTRAT-296

do I get it right, what you suggest is to add the include like in https://github.com/openstack-k8s-operators/glance-operator/blob/eee52cfdc598521211804db5f7b16ec80461f750/templates/common/config/httpd.conf#L30 to the vhost template where we have extraMounts, so that a user could add optional static params using this file?

Yeah, although I think it doesn't necessarily need to be within the vhost, but in the generic httpd.conf file.
If nothing is passed (via extraMounts), the current behavior controlled by the service operator (including what we template from the input) is preserved, otherwise we can sideload additional parameters.
This probably doesn't match 1:1 to what this patch propose, which essentially overrides parameters that are processed as golang templates.

[stuggi]

I don't think we need this level of abstraction for an operator where extraMounts are already used to inject files. It would be easier (and keep the interface consistent), to do something like: #510 that we know it would avoid us to introduce any API change in the future. @stuggi @mcgonago wdyt

this is true for api services, which have extraMounts. We added this approach to keystone before it had extraMounts there. I think got added there since then. Do we plan to add extraMounts to all api services? wondering if there should be a single documented way for consistency on how to customize http config for all services and not a mix of using extraMounts or this approach where we do not have them.

@fmount not sure if it is required in other operators, but could you use templates with extraMounts, like in this example openstack-k8s-operators/keystone-operator#525 (comment) , needed for keystone federation. iirc that would be not possible.

Right, templating is not possible in that case, but I think we get keystone covered in that case because the feature is already in. In general, we have a Jira telling to add extraMounts where (and when) needed [2], and it would avoid creating custom interfaces when not needed, and thinking in terms of a ctlplane v2 we can also remove parameters that would be covered by extraMounts. So while I agree that we should keep consistency, having 2 or 3 different interfaces to do the same thing is probably not the right thing to do. While part of the operators have extraMounts already, I would use that interface where needed unless the need is to do something else, like the keystone use case. I don't see the need of templating here for example, and in general the configMap or the secret mounted via extraMount is something crafted by the human operator. Templating is useful and very powerful, but we require the human administrator to have knowledge about it [1]. Not that this is a bad thing, but in the long term the idea is to have extraMounts spread across the board (probably with a dedicated CR like Topologies).
[1] openstack-k8s-operators/keystone-operator#525 (comment) [2] https://issues.redhat.com/browse/RHOSSTRAT-296

do I get it right, what you suggest is to add the include like in https://github.com/openstack-k8s-operators/glance-operator/blob/eee52cfdc598521211804db5f7b16ec80461f750/templates/common/config/httpd.conf#L30 to the vhost template where we have extraMounts, so that a user could add optional static params using this file?

Yeah, although I think it doesn't necessarily need to be within the vhost, but in the generic httpd.conf file. If nothing is passed (via extraMounts), the current behavior controlled by the service operator (including what we template from the input) is preserved, otherwise we can sideload additional parameters. This probably doesn't match 1:1 to what this patch propose, which essentially overrides parameters that are processed as golang templates.

if we just need to be able to customize the global params, yes we could to it in the generic httpd.conf, but if you want to customize/override a vhost specific setting I think we have to load it there, no? similar to this PR

[fmount]

I don't think we need this level of abstraction for an operator where extraMounts are already used to inject files. It would be easier (and keep the interface consistent), to do something like: #510 that we know it would avoid us to introduce any API change in the future. @stuggi @mcgonago wdyt

this is true for api services, which have extraMounts. We added this approach to keystone before it had extraMounts there. I think got added there since then. Do we plan to add extraMounts to all api services? wondering if there should be a single documented way for consistency on how to customize http config for all services and not a mix of using extraMounts or this approach where we do not have them.

@fmount not sure if it is required in other operators, but could you use templates with extraMounts, like in this example openstack-k8s-operators/keystone-operator#525 (comment) , needed for keystone federation. iirc that would be not possible.

Right, templating is not possible in that case, but I think we get keystone covered in that case because the feature is already in. In general, we have a Jira telling to add extraMounts where (and when) needed [2], and it would avoid creating custom interfaces when not needed, and thinking in terms of a ctlplane v2 we can also remove parameters that would be covered by extraMounts. So while I agree that we should keep consistency, having 2 or 3 different interfaces to do the same thing is probably not the right thing to do. While part of the operators have extraMounts already, I would use that interface where needed unless the need is to do something else, like the keystone use case. I don't see the need of templating here for example, and in general the configMap or the secret mounted via extraMount is something crafted by the human operator. Templating is useful and very powerful, but we require the human administrator to have knowledge about it [1]. Not that this is a bad thing, but in the long term the idea is to have extraMounts spread across the board (probably with a dedicated CR like Topologies).
[1] openstack-k8s-operators/keystone-operator#525 (comment) [2] https://issues.redhat.com/browse/RHOSSTRAT-296

do I get it right, what you suggest is to add the include like in https://github.com/openstack-k8s-operators/glance-operator/blob/eee52cfdc598521211804db5f7b16ec80461f750/templates/common/config/httpd.conf#L30 to the vhost template where we have extraMounts, so that a user could add optional static params using this file?

Yeah, although I think it doesn't necessarily need to be within the vhost, but in the generic httpd.conf file. If nothing is passed (via extraMounts), the current behavior controlled by the service operator (including what we template from the input) is preserved, otherwise we can sideload additional parameters. This probably doesn't match 1:1 to what this patch propose, which essentially overrides parameters that are processed as golang templates.

if we just need to be able to customize the global params, yes we could to it in the generic httpd.conf, but if you want to customize/override a vhost specific setting I think we have to load it there, no? similar to this PR

Correct. I can improve my change and add IncludeOptional within the vhost instead of including addiotional config files from the global httpd.conf.
How do you see that? Do you think it would be enough to solve the immediate problem?

[fmount]

I don't think we need this level of abstraction for an operator where extraMounts are already used to inject files. It would be easier (and keep the interface consistent), to do something like: #510 that we know it would avoid us to introduce any API change in the future. @stuggi @mcgonago wdyt

this is true for api services, which have extraMounts. We added this approach to keystone before it had extraMounts there. I think got added there since then. Do we plan to add extraMounts to all api services? wondering if there should be a single documented way for consistency on how to customize http config for all services and not a mix of using extraMounts or this approach where we do not have them.

@fmount not sure if it is required in other operators, but could you use templates with extraMounts, like in this example openstack-k8s-operators/keystone-operator#525 (comment) , needed for keystone federation. iirc that would be not possible.

Right, templating is not possible in that case, but I think we get keystone covered in that case because the feature is already in. In general, we have a Jira telling to add extraMounts where (and when) needed [2], and it would avoid creating custom interfaces when not needed, and thinking in terms of a ctlplane v2 we can also remove parameters that would be covered by extraMounts. So while I agree that we should keep consistency, having 2 or 3 different interfaces to do the same thing is probably not the right thing to do. While part of the operators have extraMounts already, I would use that interface where needed unless the need is to do something else, like the keystone use case. I don't see the need of templating here for example, and in general the configMap or the secret mounted via extraMount is something crafted by the human operator. Templating is useful and very powerful, but we require the human administrator to have knowledge about it [1]. Not that this is a bad thing, but in the long term the idea is to have extraMounts spread across the board (probably with a dedicated CR like Topologies).
[1] openstack-k8s-operators/keystone-operator#525 (comment) [2] https://issues.redhat.com/browse/RHOSSTRAT-296

do I get it right, what you suggest is to add the include like in https://github.com/openstack-k8s-operators/glance-operator/blob/eee52cfdc598521211804db5f7b16ec80461f750/templates/common/config/httpd.conf#L30 to the vhost template where we have extraMounts, so that a user could add optional static params using this file?

Yeah, although I think it doesn't necessarily need to be within the vhost, but in the generic httpd.conf file. If nothing is passed (via extraMounts), the current behavior controlled by the service operator (including what we template from the input) is preserved, otherwise we can sideload additional parameters. This probably doesn't match 1:1 to what this patch propose, which essentially overrides parameters that are processed as golang templates.

if we just need to be able to customize the global params, yes we could to it in the generic httpd.conf, but if you want to customize/override a vhost specific setting I think we have to load it there, no? similar to this PR

Correct. I can improve my change and add IncludeOptional within the vhost instead of including addiotional config files from the global httpd.conf. How do you see that? Do you think it would be enough to solve the immediate problem?

Done [1]

[1] https://github.com/openstack-k8s-operators/glance-operator/pull/797/files#diff-5fa71cf498b52cccaf3cd6618c6ca98f7745d6cf9c1a0005dcd06e0dbb210215

[stuggi]

I don't think we need this level of abstraction for an operator where extraMounts are already used to inject files. It would be easier (and keep the interface consistent), to do something like: #510 that we know it would avoid us to introduce any API change in the future. @stuggi @mcgonago wdyt

this is true for api services, which have extraMounts. We added this approach to keystone before it had extraMounts there. I think got added there since then. Do we plan to add extraMounts to all api services? wondering if there should be a single documented way for consistency on how to customize http config for all services and not a mix of using extraMounts or this approach where we do not have them.

@fmount not sure if it is required in other operators, but could you use templates with extraMounts, like in this example openstack-k8s-operators/keystone-operator#525 (comment) , needed for keystone federation. iirc that would be not possible.

Right, templating is not possible in that case, but I think we get keystone covered in that case because the feature is already in. In general, we have a Jira telling to add extraMounts where (and when) needed [2], and it would avoid creating custom interfaces when not needed, and thinking in terms of a ctlplane v2 we can also remove parameters that would be covered by extraMounts. So while I agree that we should keep consistency, having 2 or 3 different interfaces to do the same thing is probably not the right thing to do. While part of the operators have extraMounts already, I would use that interface where needed unless the need is to do something else, like the keystone use case. I don't see the need of templating here for example, and in general the configMap or the secret mounted via extraMount is something crafted by the human operator. Templating is useful and very powerful, but we require the human administrator to have knowledge about it [1]. Not that this is a bad thing, but in the long term the idea is to have extraMounts spread across the board (probably with a dedicated CR like Topologies).
[1] openstack-k8s-operators/keystone-operator#525 (comment) [2] https://issues.redhat.com/browse/RHOSSTRAT-296

do I get it right, what you suggest is to add the include like in https://github.com/openstack-k8s-operators/glance-operator/blob/eee52cfdc598521211804db5f7b16ec80461f750/templates/common/config/httpd.conf#L30 to the vhost template where we have extraMounts, so that a user could add optional static params using this file?

Yeah, although I think it doesn't necessarily need to be within the vhost, but in the generic httpd.conf file. If nothing is passed (via extraMounts), the current behavior controlled by the service operator (including what we template from the input) is preserved, otherwise we can sideload additional parameters. This probably doesn't match 1:1 to what this patch propose, which essentially overrides parameters that are processed as golang templates.

if we just need to be able to customize the global params, yes we could to it in the generic httpd.conf, but if you want to customize/override a vhost specific setting I think we have to load it there, no? similar to this PR

Correct. I can improve my change and add IncludeOptional within the vhost instead of including addiotional config files from the global httpd.conf. How do you see that? Do you think it would be enough to solve the immediate problem?

Done [1]

[1] https://github.com/openstack-k8s-operators/glance-operator/pull/797/files#diff-5fa71cf498b52cccaf3cd6618c6ca98f7745d6cf9c1a0005dcd06e0dbb210215

sounds good to me if there is no templating required. we can follow up on that when there is the need

[fmount]

I don't think we need this level of abstraction for an operator where extraMounts are already used to inject files. It would be easier (and keep the interface consistent), to do something like: #510 that we know it would avoid us to introduce any API change in the future. @stuggi @mcgonago wdyt

this is true for api services, which have extraMounts. We added this approach to keystone before it had extraMounts there. I think got added there since then. Do we plan to add extraMounts to all api services? wondering if there should be a single documented way for consistency on how to customize http config for all services and not a mix of using extraMounts or this approach where we do not have them.

@fmount not sure if it is required in other operators, but could you use templates with extraMounts, like in this example openstack-k8s-operators/keystone-operator#525 (comment) , needed for keystone federation. iirc that would be not possible.

Right, templating is not possible in that case, but I think we get keystone covered in that case because the feature is already in. In general, we have a Jira telling to add extraMounts where (and when) needed [2], and it would avoid creating custom interfaces when not needed, and thinking in terms of a ctlplane v2 we can also remove parameters that would be covered by extraMounts. So while I agree that we should keep consistency, having 2 or 3 different interfaces to do the same thing is probably not the right thing to do. While part of the operators have extraMounts already, I would use that interface where needed unless the need is to do something else, like the keystone use case. I don't see the need of templating here for example, and in general the configMap or the secret mounted via extraMount is something crafted by the human operator. Templating is useful and very powerful, but we require the human administrator to have knowledge about it [1]. Not that this is a bad thing, but in the long term the idea is to have extraMounts spread across the board (probably with a dedicated CR like Topologies).
[1] openstack-k8s-operators/keystone-operator#525 (comment) [2] https://issues.redhat.com/browse/RHOSSTRAT-296

do I get it right, what you suggest is to add the include like in https://github.com/openstack-k8s-operators/glance-operator/blob/eee52cfdc598521211804db5f7b16ec80461f750/templates/common/config/httpd.conf#L30 to the vhost template where we have extraMounts, so that a user could add optional static params using this file?

Yeah, although I think it doesn't necessarily need to be within the vhost, but in the generic httpd.conf file. If nothing is passed (via extraMounts), the current behavior controlled by the service operator (including what we template from the input) is preserved, otherwise we can sideload additional parameters. This probably doesn't match 1:1 to what this patch propose, which essentially overrides parameters that are processed as golang templates.

if we just need to be able to customize the global params, yes we could to it in the generic httpd.conf, but if you want to customize/override a vhost specific setting I think we have to load it there, no? similar to this PR

Correct. I can improve my change and add IncludeOptional within the vhost instead of including addiotional config files from the global httpd.conf. How do you see that? Do you think it would be enough to solve the immediate problem?

Done [1]
[1] https://github.com/openstack-k8s-operators/glance-operator/pull/797/files#diff-5fa71cf498b52cccaf3cd6618c6ca98f7745d6cf9c1a0005dcd06e0dbb210215

sounds good to me if there is no templating required. we can follow up on that when there is the need

ack, thanks, @mcgonago I think then the agreement is that we can go with #510 as an immediate solution to the problem if @stuggi agrees.