add labels and annotations to service (httpSpec) #39925
Conversation
closes keycloak#23283 Signed-off-by: Gilvan Filho <gfilho@redhat.com>
| @JsonPropertyDescription("Annotations to be appended to the Service object") | ||
| Map<String, String> annotations; | ||
|
|
||
| @JsonPropertyDescription("Labels to be appended to the Service object") | ||
| Map<String, String> labels; |
There was a problem hiding this comment.
I'd argue we should probably add a service spec instead of putting it under http. But leaving the room for @shawkins to chime in as well.
There was a problem hiding this comment.
I thought about this but I think everything inside spec.http is about service layer so I don't have sure if make sense have a service spec. I mean, if we had a service spec where will we put httpEnabled, httpPort and so on?
There was a problem hiding this comment.
The existing http spec is a good enough place for this - it's ok to let the cr have some abstraction. If we go down the path of adding a service stanza arguably that will need to be split between the discovery and regular service.
|
Another thought is whether it really makes sense to add labels and annotations specifically to Services. Whether not have it covered in a generic manner for all Operator managed resources (see #15395). @shawkins WDYT? @gilvansfilho Can you share the use case you are implementing this for? |
The main motivation is to be possible to add With this PR we reduce the steps needs to have |
Given some of the other openshift specific logic in the operator, the alternative here is to directly support reencrypt declaratively just for openshift. If in the near-term we're looking to stop using a cluster role, we'll already have to use a route to auto-determine the hostname. If we don't want to go down this path, then opening up the labels and annotations like this seems fine. If we do eventually add a more global control for custom metadata I would expect that to be applied first, then the more specific metadata overlayed on that. |
|
If the only motivation for having dedicated labels and annotations setting for services is the reencrypt, I'd rather go down the path of a first class support for reencrypt. We generally try to avoid adding arbitrary fields to the CR if we can have a better UX by adding a support for specific use cases. |
The exisitng issue for this is #20128 Getting this supported end-to-end is not trivial, and of course initially openshift specific. There are two cases - reencrypt using a serving certificate, and reencrypt using the http key / cert. We'll also need to have the key / cert for the route - there isn't currently a spot for this on the CR, unless you want to somehow make the http key / cert apply which is also discussed on #34777 These cases would look like: Re-encrypt implied: Re-encrypt using serving certificate would need some way to indciate the annotation needs added. That would either be a mode for the ingress, or something on the http spec to indicate a key / cert should be generated: |
|
At Openshift to use |
I'd vote for rather using the cert generated for the service via the annotation.
That's a good question. Would there be a real life use case where you'd need different cert and key for Keycloak and the Route in re-encrypt case? Maybe when you don't trust Keycloak so much and don't want to give it the TLS private key that will be also used externally? :D Maybe we could have something like: spec:
http:
tlsSecret: keycloak-tls
ingress:
tls:
secret: my-other-secret
generated: true|false # when set to true, secret must be left blank; currently works only on OpenShift |
I too. But keep in mind that cert generated via the annotation is for the pod (service layer), not for the route. For the route openshift allows to provide a custom cert or use cluster default cert. For
So the |
Ok, so we have either an implicit reencrypt where both tlsSecrets are specified, or an explicit reencrypt mode where both tlsSecrets are optional.
Are you good with an ingress mode or similar flag? Also I vote to proceed with the additional metadata support proposed in this pr, and tackle the openshift specific reencrypt support under the other issue. |
Sure To provide first class support to Openshift reencrypt routes I think we need below scenarios...
Scenario 1: Reencrypt with cluster domain and serving certificate for podOperator will have to:
With that we have:
Scenario 2: Reencrypt with cluster domain and custom certificate for podUser must provide:
Operator will have to:
With that we have:
Scenario 3: Reencrypt with custom domain certificate and serving certificate for serviceUser must provide:
Operator will have to:
With that we have:
Scenario 4: Reencrypt with custom domain certificate and custom certificate for podUser must provide:
Operator will have to:
User must provide With that we have:
I guess all exposed above will require a lot of work.
So, I also vote to proceed with this PR, allowing to inject custom labels and annotatios to services, So one who is trying to use Then will need to manually create a |
|
@gilvansfilho our understanding is the same. Scenario 4 is what I was thinking doesn't need to explicitly specify the mode as reencrypt is implied. Can you move / add the previous comment to #20128? |
|
@gilvansfilho Thank you for providing the analysis!
Thinking about it more considering the scenarios, I'm gonna agree with @shawkins here. I don't even think we need OpenShift specific support. We could just expose the TLS secret field in the ingress spec and document some use cases (the scenarios above) that will use the service annotations and/or the TLS secret fields. WDYT? |
|
Agree :) |
There was a problem hiding this comment.
LGTM, thank you @gilvansfilho
There was a problem hiding this comment.
LGTM, nice work @gilvansfilho.
Let's address the reencrypt scenario by adding TLS secret to Ingress in #20128.
closes #23283