8000 fix: adding the discovery servicename to the statefulset by shawkins · Pull Request #39419 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: adding the discovery servicename to the statefulset #39419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 6, 2025
Merged

fix: adding the discovery servicename to the statefulset #39419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .../src/main/java/org/keycloak/operator/controllers/KeycloakDeploymentDependentResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ private StatefulSet createBaseDeployment(Keycloak keycloakCR, Context<Keycloak>
.editOrNewSpec().withImagePullSecrets(keycloakCR.getSpec().getImagePullSecrets()).endSpec()
.endTemplate()
.withReplicas(keycloakCR.getSpec().getInstances())
.withServiceName(KeycloakDiscoveryServiceDependentResource.getName(keycloakCR))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it rather use the main service?

Suggested change
.withServiceName(KeycloakDiscoveryServiceDependentResource.getName(keycloakCR))
.withServiceName(KeycloakServiceDependentResource.getServiceName(keycloakCR))

Copy lin 8000 k
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's supposed to be a headless service.

Copy link
Contributor
@vmuzikar vmuzikar May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the docs it does not seem so?

Additionally it says:

This service must exist before the StatefulSet

Despite it now works with StS created before the Service, can we really rely on it as that behaviour might change to align with the spec?

Copy link
Contributor Author
@shawkins shawkins May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the docs it does not seem so?

I'm referencing https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#stable-network-id

I suppose the language there does say "can", not "must" be headless.

Despite it now works with StS created before the Service, can we really rely on it as that behaviour might change to align with the spec?

This behavior / validation has been poorly defined for quite some time. kubernetes/kubernetes#69608 was logged years ago.

Even if it doesn't "work" at some point in the furture, the worst case behavior would be the statefulset not scaling up until the service is created - since these are peer dependent resources, that would be a narrow window.

I'm leary of adding any kind of reconciliation condition for this kind of behavior - instead if you are worried about not eventually seeing some kind of ephemeral error, then I'd opt instead for Dependent.dependsOn

Copy link
Contributor
@vmuzikar vmuzikar May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm referencing https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#stable-network-id

I suppose the language there does say "can", not "must" be headless.

I see. Well, it says headless service so probably there's no going around it. However, it feels a bit odd to me that the KC Pod's DNS records would be derived from the Discovery Service. Moreover, what if we switch to the JDBC ping in the future, so that Service is not needed anymore?

But it's not a blocker for me, I don't have a better idea. We might create a dedicated headlessService just for this purpose but that seems like an overkill at this point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it doesn't "work" at some point in the furture, the worst case behavior would be the statefulset not scaling up until the service is created - since these are peer dependent resources, that would be a narrow window.

Makes sense 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might create a dedicated headlessService just for this purpose but that seems like an overkill at this point.

I think we'd just leave the discovery service in place. There's not much harm / risk and users may have already adopted using it for their own purposes as well.

.endSpec();

var specBuilder = baseDeploymentBuilder.editSpec().editTemplate().editOrNewSpec();
Expand Down
5 changes: 4 additions & 1 deletion operator/src/test/java/org/keycloak/operator/testsuite/unit/PodTemplateTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import io.fabric8.kubernetes.api.model.VolumeMount;
import io.fabric8.kubernetes.api.model.apps.StatefulSet;
import io.fabric8.kubernetes.api.model.apps.StatefulSetBuilder;
import io.fabric8.kubernetes.api.model.apps.StatefulSetSpec;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.utils.Serialization;
import io.javaoperatorsdk.operator.api.reconciler.Context;
Expand Down Expand Up @@ -405,10 +406,12 @@ public void testDefaults() {
PodTemplateSpec additionalPodTemplate = null;

// Act
var podTemplate = getDeployment(additionalPodTemplate).getSpec().getTemplate();
StatefulSetSpec spec = getDeployment(additionalPodTemplate).getSpec();
var podTemplate = spec.getTemplate();
var container = podTemplate.getSpec().getContainers().get(0);

// Assert
assertThat(spec.getServiceName()).isEqualTo("instance-discovery");
assertNotNull(container);
assertThat(container.getArgs()).doesNotContain(KeycloakDeploymentDependentResource.OPTIMIZED_ARG);
assertThat(container.getArgs()).contains("-Djgroups.bind.address=$(POD_IP)");
Expand Down
Loading
0