From bebb314a16da25b3c4b6ea4a7a4f09181448e9f2 Mon Sep 17 00:00:00 2001 From: Alexander Schwartz Date: Wed, 10 Jul 2024 19:27:06 +0200 Subject: [PATCH] Use port name instead of port number for the ingress (#30925) Also adding a retry if server-side-apply fails Closes #30924 Signed-off-by: Alexander Schwartz Co-authored-by: Steven Hawkins --- .../KeycloakIngressDependentResource.java | 40 +++++++++++++++++-- .../integration/KeycloakIngressTest.java | 2 +- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakIngressDependentResource.java b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakIngressDependentResource.java index 5d97c2c8c0..01d28b4415 100644 --- a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakIngressDependentResource.java +++ b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakIngressDependentResource.java @@ -18,6 +18,7 @@ package org.keycloak.operator.controllers; import io.fabric8.kubernetes.api.model.networking.v1.Ingress; import io.fabric8.kubernetes.api.model.networking.v1.IngressBuilder; +import io.fabric8.kubernetes.client.KubernetesClientException; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource; @@ -25,6 +26,7 @@ import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDep import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition; import io.quarkus.logging.Log; +import org.jboss.logging.Logger; import org.keycloak.operator.Constants; import org.keycloak.operator.Utils; import org.keycloak.operator.crds.v2alpha1.deployment.Keycloak; @@ -40,6 +42,8 @@ import static org.keycloak.operator.crds.v2alpha1.CRDUtils.isTlsConfigured; @KubernetesDependent(labelSelector = Constants.DEFAULT_LABELS_AS_STRING) public class KeycloakIngressDependentResource extends CRUDKubernetesDependentResource { + private static final Logger LOG = Logger.getLogger(KeycloakIngressDependentResource.class.getName()); + public static class EnabledCondition implements Condition { @Override public boolean isMet(DependentResource dependentResource, Keycloak primary, @@ -48,6 +52,8 @@ public class KeycloakIngressDependentResource extends CRUDKubernetesDependentRes } } + private static final ThreadLocal USE_SSA = new ThreadLocal<>(); + public KeycloakIngressDependentResource() { super(Ingress.class); } @@ -56,11 +62,39 @@ public class KeycloakIngressDependentResource extends CRUDKubernetesDependentRes return Optional.ofNullable(keycloak.getSpec().getIngressSpec()).map(IngressSpec::isIngressEnabled).orElse(true); } + @Override + public Ingress update(Ingress actual, Ingress desired, Keycloak primary, Context context) { + try { + return super.update(actual, desired, primary, context); + } catch (KubernetesClientException e) { + if (e.getCode() == 422) { + // This attempts to recover from errors like "Failure executing: PATCH" / "Invalid value: cannot set both port name & port number." + // when the server side apply fails. + try { + LOG.info("Failed to update Ingress with server-side apply, retrying with client-side", e); + USE_SSA.set(false); + return super.update(actual, desired, primary, context); + } finally { + USE_SSA.remove(); + } + } + throw e; + } + } + + @Override + protected boolean useSSA(Context context) { + if (Boolean.FALSE.equals(USE_SSA.get())) { + return false; + } + return super.useSSA(context); + } + @Override public Ingress desired(Keycloak keycloak, Context context) { var annotations = new HashMap(); boolean tlsConfigured = isTlsConfigured(keycloak); - var port = KeycloakServiceDependentResource.getServicePort(tlsConfigured, keycloak); + var portName = tlsConfigured ? Constants.KEYCLOAK_HTTPS_PORT_NAME : Constants.KEYCLOAK_HTTP_PORT_NAME; if (tlsConfigured) { annotations.put("nginx.ingress.kubernetes.io/backend-protocol", "HTTPS"); @@ -86,7 +120,7 @@ public class KeycloakIngressDependentResource extends CRUDKubernetesDependentRes .withNewService() .withName(KeycloakServiceDependentResource.getServiceName(keycloak)) .withNewPort() - .withNumber(port) + .withName(portName) .endPort() .endService() .endDefaultBackend() @@ -98,7 +132,7 @@ public class KeycloakIngressDependentResource extends CRUDKubernetesDependentRes .withNewService() .withName(KeycloakServiceDependentResource.getServiceName(keycloak)) .withNewPort() - .withNumber(port) + .withName(portName) .endPort() .endService() .endBackend() diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakIngressTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakIngressTest.java index 6ee4056ed9..34faf82425 100644 --- a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakIngressTest.java +++ b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakIngressTest.java @@ -238,7 +238,7 @@ public class KeycloakIngressTest extends BaseOperatorTest { assertEquals("HTTPS", i.getMetadata().getAnnotations().get("nginx.ingress.kubernetes.io/backend-protocol")); assertEquals("passthrough", i.getMetadata().getAnnotations().get("route.openshift.io/termination")); assertEquals("true", i.getMetadata().getAnnotations().get("haproxy.router.openshift.io/disable_cookies")); - assertEquals(Constants.KEYCLOAK_HTTPS_PORT, i.getSpec().getDefaultBackend().getService().getPort().getNumber()); + assertEquals(Constants.KEYCLOAK_HTTPS_PORT_NAME, i.getSpec().getDefaultBackend().getService().getPort().getName()); }); // Delete the ingress