From 6b0e1f87f9b42a000e1327f9430b9c198cbe9a85 Mon Sep 17 00:00:00 2001 From: Steven Hawkins Date: Mon, 21 Aug 2023 13:34:59 -0400 Subject: [PATCH] converts the ingress logic to a conditional dependent resource (#22221) Closes #22206 --- .../controllers/KeycloakController.java | 16 +--- ... => KeycloakIngressDependentResource.java} | 74 +++++++------------ .../integration/KeycloakIngressTest.java | 21 +++--- .../testsuite/unit/IngressLogicTest.java | 73 ++++++------------ 4 files changed, 63 insertions(+), 121 deletions(-) rename operator/src/main/java/org/keycloak/operator/controllers/{KeycloakIngress.java => KeycloakIngressDependentResource.java} (67%) diff --git a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakController.java b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakController.java index 41bf7e21ad..536be4d905 100644 --- a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakController.java +++ b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakController.java @@ -18,7 +18,6 @@ package org.keycloak.operator.controllers; import io.fabric8.kubernetes.api.model.Service; import io.fabric8.kubernetes.api.model.apps.StatefulSet; -import io.fabric8.kubernetes.api.model.networking.v1.Ingress; import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.api.config.informer.InformerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Context; @@ -52,6 +51,7 @@ import static io.javaoperatorsdk.operator.api.reconciler.Constants.WATCH_CURRENT @ControllerConfiguration(namespaces = WATCH_CURRENT_NAMESPACE, dependents = { @Dependent(type = KeycloakAdminSecretDependentResource.class), + @Dependent(type = KeycloakIngressDependentResource.class, reconcilePrecondition = KeycloakIngressDependentResource.EnabledCondition.class), @Dependent(type = KeycloakServiceDependentResource.class, useEventSourceWithName = "serviceSource"), @Dependent(type = KeycloakDiscoveryServiceDependentResource.class, useEventSourceWithName = "serviceSource") }) @@ -86,22 +86,13 @@ public class KeycloakController implements Reconciler, EventSourceInit .withOnUpdateFilter(new MetadataAwareOnUpdateFilter<>()) .build(); - InformerConfiguration ingressesIC = InformerConfiguration - .from(Ingress.class) - .withLabelSelector(Constants.DEFAULT_LABELS_AS_STRING) - .withNamespaces(namespace) - .withSecondaryToPrimaryMapper(Mappers.fromOwnerReference()) - .withOnUpdateFilter(new MetadataAwareOnUpdateFilter<>()) - .build(); - EventSource statefulSetEvent = new InformerEventSource<>(statefulSetIC, context); EventSource servicesEvent = new InformerEventSource<>(servicesIC, context); - EventSource ingressesEvent = new InformerEventSource<>(ingressesIC, context); Map sources = new HashMap<>(); sources.put("serviceSource", servicesEvent); sources.putAll(EventSourceInitializer.nameEventSources(statefulSetEvent, - ingressesEvent, watchedSecrets.getWatchedSecretsEventSource())); + watchedSecrets.getWatchedSecretsEventSource())); return sources; } @@ -126,9 +117,6 @@ public class KeycloakController implements Reconciler, EventSourceInit kcDeployment.createOrUpdateReconciled(); kcDeployment.updateStatus(statusAggregator); - var kcIngress = new KeycloakIngress(client, kc); - kcIngress.createOrUpdateReconciled(); - var status = statusAggregator.build(); Log.info("--- Reconciliation finished successfully"); diff --git a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakIngress.java b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakIngressDependentResource.java similarity index 67% rename from operator/src/main/java/org/keycloak/operator/controllers/KeycloakIngress.java rename to operator/src/main/java/org/keycloak/operator/controllers/KeycloakIngressDependentResource.java index 5a8fd26d37..3e95ccb215 100644 --- a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakIngress.java +++ b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakIngressDependentResource.java @@ -16,10 +16,13 @@ */ package org.keycloak.operator.controllers; -import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.networking.v1.Ingress; import io.fabric8.kubernetes.api.model.networking.v1.IngressBuilder; -import io.fabric8.kubernetes.client.KubernetesClient; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.CRUDKubernetesDependentResource; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependent; +import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition; import org.keycloak.operator.Constants; import org.keycloak.operator.crds.v2alpha1.deployment.Keycloak; @@ -30,32 +33,27 @@ import java.util.Optional; import static org.keycloak.operator.crds.v2alpha1.CRDUtils.isTlsConfigured; -public class KeycloakIngress extends OperatorManagedResource { +@KubernetesDependent(labelSelector = Constants.DEFAULT_LABELS_AS_STRING) +public class KeycloakIngressDependentResource extends CRUDKubernetesDependentResource { - private final Ingress existingIngress; - private final Keycloak keycloak; - - public KeycloakIngress(KubernetesClient client, Keycloak keycloakCR) { - super(client, keycloakCR); - this.keycloak = keycloakCR; - this.existingIngress = fetchExistingIngress(); - } - - @Override - protected Optional getReconciledResource() { - IngressSpec ingressSpec = keycloak.getSpec().getIngressSpec(); - if (ingressSpec != null && !ingressSpec.isIngressEnabled()) { - if (existingIngress != null && existingIngress.hasOwnerReferenceFor(keycloak)) { - deleteExistingIngress(); - } - return Optional.empty(); - } else { - return Optional.of(newIngress()); + public static class EnabledCondition implements Condition { + @Override + public boolean isMet(DependentResource dependentResource, Keycloak primary, + Context context) { + return isIngressEnabled(primary); } } - private Ingress newIngress() { - // set default annotations + public KeycloakIngressDependentResource() { + super(Ingress.class); + } + + public static boolean isIngressEnabled(Keycloak keycloak) { + return Optional.ofNullable(keycloak.getSpec().getIngressSpec()).map(IngressSpec::isIngressEnabled).orElse(true); + } + + @Override + public Ingress desired(Keycloak keycloak, Context context) { var annotations = new HashMap(); boolean tlsConfigured = isTlsConfigured(keycloak); var port = KeycloakServiceDependentResource.getServicePort(tlsConfigured, keycloak); @@ -73,8 +71,10 @@ public class KeycloakIngress extends OperatorManagedResource { Ingress ingress = new IngressBuilder() .withNewMetadata() - .withName(getName()) - .withNamespace(getNamespace()) + .withName(getName(keycloak)) + .withNamespace(keycloak.getMetadata().getNamespace()) + .addToLabels(Constants.DEFAULT_LABELS) + .addToLabels(OperatorManagedResource.updateWithInstanceLabels(null, keycloak.getMetadata().getName())) .addToAnnotations(annotations) .endMetadata() .withNewSpec() @@ -84,21 +84,18 @@ public class KeycloakIngress extends OperatorManagedResource { .withName(KeycloakServiceDependentResource.getServiceName(keycloak)) .withNewPort() .withNumber(port) - .withName("") // for SSA to clear the name if already set .endPort() .endService() .endDefaultBackend() .addNewRule() .withNewHttp() .addNewPath() - .withPath("") .withPathType("ImplementationSpecific") .withNewBackend() .withNewService() .withName(KeycloakServiceDependentResource.getServiceName(keycloak)) .withNewPort() .withNumber(port) - .withName("") // for SSA to clear the name if already set .endPort() .endService() .endBackend() @@ -116,22 +113,7 @@ public class KeycloakIngress extends OperatorManagedResource { return ingress; } - protected void deleteExistingIngress() { - client.resource(existingIngress).delete(); - } - - protected Ingress fetchExistingIngress() { - return client - .network() - .v1() - .ingresses() - .inNamespace(getNamespace()) - .withName(getName()) - .get(); - } - - @Override - public String getName() { - return cr.getMetadata().getName() + Constants.KEYCLOAK_INGRESS_SUFFIX; + public static String getName(Keycloak keycloak) { + return keycloak.getMetadata().getName() + Constants.KEYCLOAK_INGRESS_SUFFIX; } } 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 eceb8aca43..9e1575254f 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 @@ -24,17 +24,18 @@ import io.fabric8.kubernetes.client.dsl.Resource; import io.quarkus.logging.Log; import io.quarkus.test.junit.QuarkusTest; import io.restassured.RestAssured; + import org.awaitility.Awaitility; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.keycloak.operator.Constants; +import org.keycloak.operator.controllers.KeycloakIngressDependentResource; import org.keycloak.operator.crds.v2alpha1.deployment.Keycloak; +import org.keycloak.operator.crds.v2alpha1.deployment.spec.HostnameSpecBuilder; import org.keycloak.operator.crds.v2alpha1.deployment.spec.IngressSpec; import org.keycloak.operator.crds.v2alpha1.deployment.spec.IngressSpecBuilder; -import org.keycloak.operator.crds.v2alpha1.deployment.spec.HostnameSpecBuilder; import org.keycloak.operator.crds.v2alpha1.deployment.spec.UnsupportedSpecBuilder; import org.keycloak.operator.testsuite.utils.K8sUtils; -import org.keycloak.operator.controllers.KeycloakIngress; import java.util.Map; @@ -162,7 +163,6 @@ public class KeycloakIngressTest extends BaseOperatorTest { K8sUtils.deployKeycloak(k8sclient, kc, true); - var ingress = new KeycloakIngress(k8sclient, kc); Awaitility.await() .ignoreExceptions() .untilAsserted(() -> { @@ -171,7 +171,7 @@ public class KeycloakIngressTest extends BaseOperatorTest { .v1() .ingresses() .inNamespace(namespace) - .withName(ingress.getName()) + .withName(KeycloakIngressDependentResource.getName(kc)) .get() .getSpec() .getRules() @@ -190,13 +190,12 @@ public class KeycloakIngressTest extends BaseOperatorTest { kc.getSpec().getIngressSpec().setAnnotations(Map.of("haproxy.router.openshift.io/disable_cookies", "true")); K8sUtils.deployKeycloak(k8sclient, kc, true); - var ingress = new KeycloakIngress(k8sclient, kc); var ingressSelector = k8sclient .network() .v1() .ingresses() .inNamespace(namespace) - .withName(ingress.getName()); + .withName(KeycloakIngressDependentResource.getName(kc)); Log.info("Trying to delete the ingress"); assertThat(ingressSelector.delete()).isNotNull(); @@ -210,7 +209,7 @@ public class KeycloakIngressTest extends BaseOperatorTest { var labels = Map.of("address", "EvergreenTerrace742"); ingressSelector.accept(currentIngress -> { currentIngress.getMetadata().setResourceVersion(null); - currentIngress.getSpec().getDefaultBackend().getService().setPort(new ServiceBackendPortBuilder().withName("foo").build()); + currentIngress.getSpec().getDefaultBackend().getService().setPort(new ServiceBackendPortBuilder().withNumber(6500).build()); currentIngress.getMetadata().getAnnotations().clear(); currentIngress.getMetadata().getLabels().putAll(labels); @@ -260,7 +259,7 @@ public class KeycloakIngressTest extends BaseOperatorTest { assertThat(k8sclient.network().v1().ingresses().inNamespace(namespace).list().getItems().size()).isEqualTo(1); }); - Log.info("Redeploying the Keycloak CR with default Ingress disabled"); + Log.info("Deploying the Keycloak CR with default Ingress disabled"); defaultKeycloakDeployment.getSpec().setIngressSpec(new IngressSpec()); defaultKeycloakDeployment.getSpec().getIngressSpec().setIngressEnabled(false); @@ -288,13 +287,12 @@ public class KeycloakIngressTest extends BaseOperatorTest { kc.getSpec().setIngressSpec(new IngressSpecBuilder().withIngressClassName("nginx").build()); K8sUtils.deployKeycloak(k8sclient, kc, true); - var ingress = new KeycloakIngress(k8sclient, kc); var ingressSelector = k8sclient .network() .v1() .ingresses() .inNamespace(namespace) - .withName(ingress.getName()); + .withName(KeycloakIngressDependentResource.getName(kc)); Awaitility.await() .ignoreExceptions() @@ -325,13 +323,12 @@ public class KeycloakIngressTest extends BaseOperatorTest { kc.getSpec().getIngressSpec().setAnnotations(Map.of("a", "b")); K8sUtils.deployKeycloak(k8sclient, kc, true); - var ingress = new KeycloakIngress(k8sclient, kc); var ingressSelector = k8sclient .network() .v1() .ingresses() .inNamespace(namespace) - .withName(ingress.getName()); + .withName(KeycloakIngressDependentResource.getName(kc)); Awaitility.await() .ignoreExceptions() diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/unit/IngressLogicTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/unit/IngressLogicTest.java index 046d66d61f..87798bddf0 100644 --- a/operator/src/test/java/org/keycloak/operator/testsuite/unit/IngressLogicTest.java +++ b/operator/src/test/java/org/keycloak/operator/testsuite/unit/IngressLogicTest.java @@ -17,22 +17,18 @@ package org.keycloak.operator.testsuite.unit; -import java.util.Collections; -import java.util.Map; -import java.util.Optional; +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.api.model.networking.v1.Ingress; -import io.fabric8.kubernetes.api.model.OwnerReference; -import io.fabric8.kubernetes.api.model.OwnerReferenceBuilder; import org.junit.jupiter.api.Test; -import org.keycloak.operator.controllers.KeycloakIngress; +import org.keycloak.operator.controllers.KeycloakIngressDependentResource; import org.keycloak.operator.crds.v2alpha1.deployment.Keycloak; import org.keycloak.operator.crds.v2alpha1.deployment.spec.IngressSpec; import org.keycloak.operator.crds.v2alpha1.deployment.spec.IngressSpecBuilder; import org.keycloak.operator.testsuite.utils.K8sUtils; -import io.fabric8.kubernetes.api.model.HasMetadata; -import io.fabric8.kubernetes.api.model.networking.v1.Ingress; -import io.fabric8.kubernetes.api.model.networking.v1.IngressBuilder; +import java.util.Map; +import java.util.Optional; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -43,7 +39,7 @@ public class IngressLogicTest { private static final String EXISTING_ANNOTATION_KEY = "annotation"; - static class MockKeycloakIngress extends KeycloakIngress { + static class MockKeycloakIngress { private static Keycloak getKeycloak(boolean tlsConfigured, IngressSpec ingressSpec) { var kc = K8sUtils.getDefaultKeycloakDeployment(); @@ -66,7 +62,6 @@ public class IngressLogicTest { } public static MockKeycloakIngress build(Boolean defaultIngressEnabled, boolean ingressExists, boolean ingressSpecDefined, boolean tlsConfigured, Map annotations) { - MockKeycloakIngress.ingressExists = ingressExists; IngressSpec ingressSpec = null; if (ingressSpecDefined) { ingressSpec = new IngressSpec(); @@ -77,18 +72,18 @@ public class IngressLogicTest { ingressSpec.setAnnotations(annotations); } } - return new MockKeycloakIngress(tlsConfigured, ingressSpec); + MockKeycloakIngress mock = new MockKeycloakIngress(tlsConfigured, ingressSpec); + mock.ingressExists = ingressExists; + return mock; } - public static boolean ingressExists = false; + private KeycloakIngressDependentResource keycloakIngressDependentResource = new KeycloakIngressDependentResource(); + private boolean ingressExists = false; private boolean deleted = false; - public MockKeycloakIngress(boolean tlsConfigured, IngressSpec ingressSpec) { - super(null, getKeycloak(tlsConfigured, ingressSpec)); - } + private Keycloak keycloak; - @Override - public Optional getReconciledResource() { - return super.getReconciledResource(); + public MockKeycloakIngress(boolean tlsConfigured, IngressSpec ingressSpec) { + this.keycloak = getKeycloak(tlsConfigured, ingressSpec); } public boolean reconciled() { @@ -99,35 +94,15 @@ public class IngressLogicTest { return deleted; } - @Override - protected Ingress fetchExistingIngress() { - if (ingressExists) { - - OwnerReference sameCROwnerRef = new OwnerReferenceBuilder() - .withApiVersion(cr.getApiVersion()) - .withKind(cr.getKind()) - .withName(cr.getMetadata().getName()) - .withUid(cr.getMetadata().getUid()) - .withBlockOwnerDeletion(true) - .withController(true) - .build(); - - return new IngressBuilder() - .withNewMetadata() - .withName(getName()) - .withNamespace(cr.getMetadata().getNamespace()) - .withOwnerReferences(Collections.singletonList(sameCROwnerRef)) - .withAnnotations(Map.of(EXISTING_ANNOTATION_KEY, "value")) - .endMetadata() - .build(); - } else { - return null; + public Optional getReconciledResource() { + if (!KeycloakIngressDependentResource.isIngressEnabled(keycloak)) { + if (ingressExists) { + deleted = true; + } + return Optional.empty(); } - } - - @Override - protected void deleteExistingIngress() { - deleted = true; + + return Optional.of(keycloakIngressDependentResource.desired(keycloak, null)); } } @@ -226,7 +201,7 @@ public class IngressLogicTest { assertEquals("passthrough", reconciled.get().getMetadata().getAnnotations().get("route.openshift.io/termination")); assertEquals("another-value", reconciled.get().getMetadata().getAnnotations().get(EXISTING_ANNOTATION_KEY)); } - + @Test public void testIngressSpecDefinedWithoutClassName() { var kc = new MockKeycloakIngress(true, new IngressSpec()); @@ -234,7 +209,7 @@ public class IngressLogicTest { Ingress ingress = reconciled.map(Ingress.class::cast).orElseThrow(); assertNull(ingress.getSpec().getIngressClassName()); } - + @Test public void testIngressSpecDefinedWithClassName() { var kc = new MockKeycloakIngress(true, new IngressSpecBuilder().withIngressClassName("my-class").build());