KEYCLOAK-12929 external database support with uri #137
Conversation
There was a problem hiding this comment.
@pb82 This is fantastic work, thank you. Two small changes from my review. Will verify once these are made.
pb82
force-pushed
the
KEYCLOAK-12929
branch
2 times, most recently
from
a2151ab
to
78aed12
Compare
|
@pb82 Kubernetes Service manual mentions this:
So if my understanding is correct - they say if this is a DNS name, we should use In the same manual I also found this:
So it seems for the IP addresses, we could use Did you try this options? I'm asking because the solution you proposed seems pretty complicated with lots of corner cases. If we could get rid of the Endpoints object and implement everything in a single Service (just decide in the runtime, whether it needs a Selector, a |
|
I have verified this PR works with an external AWS RDS Postgres instance |
|
@slaskawi I tried using |
There was a problem hiding this comment.
Great work Peter!
I added a few comments and there two things I would like to you to change:
- Always add
SERVICE_PORTandSERVICE_HOSTvariables if possible. Generating datastores for RHSSO is very fragile (and overly too complicated). Adding something conditionally may lead to problems with reproducing bugs. I would like to avoid it, if possible. - Both
PostgresqlServiceandgetPostgresqlServiceDesiredStatetakeboolas the third parameter. In both of the cases, this look awkward. Is it possible to reduce this number of parameters and get rid of it? Maybe it should be embedded into those functions?
| if clusterState.DatabaseSecret == nil { | ||
| return | ||
| } | ||
| if model.IsIP(clusterState.DatabaseSecret.Data[model.DatabaseSecretExternalAddressProperty]) { |
There was a problem hiding this comment.
Perhaps we could use methods from util.go here?
There was a problem hiding this comment.
IsIP is actually just a wrapper around a Go stdlib function. Or do you mean we should move this into the util package?
| // set up an endpoints object for the service to send traffic. An externalName | ||
| // type service won't work in this case. For more details, see https://cloud.google.com/blog/products/gcp/kubernetes-best-practices-mapping-external-services | ||
| desired.AddAction(i.getPostgresqlServiceEndpointsDesiredState(clusterState, cr)) | ||
| desired.AddAction(i.getPostgresqlServiceDesiredState(clusterState, cr, false)) |
There was a problem hiding this comment.
I don't like passing this true or false (3rd argument) to getPostgresqlServiceDesiredState. It looks a bit awkward - could we somehow infer this argument from the CR inside getPostgresqlServiceDesiredState?
There was a problem hiding this comment.
The problem is, true or false here do not enable/disable external database mode but service type. We could infer them by checking for IP again in the service model. Or we could have two models, one for the default clusterIP service and one for the externalName service.
There was a problem hiding this comment.
Personally, I would implement 2 models. I think it would be a bit cleaner this way.
But I'll leave the final decision to you. I'm fine either way.
There was a problem hiding this comment.
I think having two models would complicate it even more: now we would have a cluster state that either knows about the two models or needs to be able to distinguish between what kind of model is required.
|
@slaskawi it's rebased now |
|
ping @slaskawi , rebased again |
JIRA ID
https://issues.redhat.com/browse/KEYCLOAK-12929
ping @davidffrench
Additional Information
The Keycloak Operator allows connecting to external (outside of the cluster) databases by providing a pre-populated database secret. This only worked when specifying the IP Address of the external database. This is a problem because the IP address can change. We want to support both, IP and hostname.
Unfortunately IP adresses and hostnames have to be treated in different ways: Hostnames are supported by externalName type services. But this type of service does not support IP addresses and so we have to rely on default ClusterIP type services and an Endpoints object to support that use case. This PR checks if the pre-existing database secret contains an IP or a hostname and chooses the reconciliation strategy accordingly.
Verification Steps
A number of scenarios have to be verified. For every scenario, make sure that the deployment succeeds and that there are no errors in the log. Feel free to ask me for the connection details / db secret of an external database if you need one.
Checklist: