diff --git a/operator/src/main/java/it/aboutbits/postgresql/core/BaseReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/core/BaseReconciler.java index 18452a2..2c85315 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/core/BaseReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/core/BaseReconciler.java @@ -46,6 +46,8 @@ public String getResourceNamespaceOrOwn( return resource.getMetadata().getNamespace(); } + /// Returns empty when the ClusterConnection does not exist, is not READY, or is READY for an older generation. + /// Callers treat this as "not yet", not "broken", and try again after 60 seconds. public Optional getReferencedClusterConnection( KubernetesClient kubernetesClient, CR resource, @@ -70,14 +72,42 @@ public Optional getReferencedClusterConnection( return Optional.empty(); } - var currentPhase = clusterConnection.getStatus().getPhase(); + var status = clusterConnection.getStatus(); + + // No status yet means the connection has not been probed yet. + //noinspection ConstantConditions + if (status == null) { + log.warn( + "The specified ClusterConnection has not been probed yet [resource={}/{}]", + connectionNamespace, + connectionName + ); + + return Optional.empty(); + } + + var currentPhase = status.getPhase(); var expectedPhase = CRPhase.READY; if (!Objects.equals(currentPhase, expectedPhase)) { log.warn( - "The specified ClusterConnection is not ready yet [resource={}/{}]", + "The specified ClusterConnection is not ready yet [resource={}/{}, status.phase={}]", connectionNamespace, - connectionName + connectionName, + currentPhase + ); + + return Optional.empty(); + } + + // READY of an older generation says nothing about the current spec. + if (status.getObservedGeneration() != clusterConnection.getMetadata().getGeneration()) { + log.warn( + "The specified ClusterConnection changed and has not been probed again yet [resource={}/{}, status.observedGeneration={}, metadata.generation={}]", + connectionNamespace, + connectionName, + status.getObservedGeneration(), + clusterConnection.getMetadata().getGeneration() ); return Optional.empty(); diff --git a/operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionSpec.java b/operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionSpec.java index 1961bff..70ba5b7 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionSpec.java +++ b/operator/src/main/java/it/aboutbits/postgresql/crd/clusterconnection/ClusterConnectionSpec.java @@ -19,11 +19,11 @@ @Getter @Setter @SchemaCustomizer(value = HostCustomizer.class, input = "host") -@NullMarked @ValidationRule( value = "has(self.adminSecretRef) != has(self.adminSecretFileRef)", message = "Exactly one of 'adminSecretRef' or 'adminSecretFileRef' must be provided" ) +@NullMarked public class ClusterConnectionSpec { @Required @ValidationRule( diff --git a/operator/src/main/java/it/aboutbits/postgresql/crd/role/RoleReconciler.java b/operator/src/main/java/it/aboutbits/postgresql/crd/role/RoleReconciler.java index 9d10e1b..029f978 100644 --- a/operator/src/main/java/it/aboutbits/postgresql/crd/role/RoleReconciler.java +++ b/operator/src/main/java/it/aboutbits/postgresql/crd/role/RoleReconciler.java @@ -107,10 +107,10 @@ public UpdateControl reconcile( // Read the Secret inside the try block, so that a missing or invalid Secret sets the phase to ERROR var password = passwordSecretRef != null ? kubernetesService.getSecretRefCredentials( - kubernetesClient, - passwordSecretRef, - namespace - ).password() + kubernetesClient, + passwordSecretRef, + namespace + ).password() : null; var passwordEncryption = spec.getPasswordEncryption(); diff --git a/operator/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/ClusterConnectionCreate.java b/operator/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/ClusterConnectionCreate.java index 72e41b4..319ade8 100644 --- a/operator/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/ClusterConnectionCreate.java +++ b/operator/src/test/java/it/aboutbits/postgresql/_support/testdata/persisted/creator/ClusterConnectionCreate.java @@ -109,12 +109,16 @@ protected ClusterConnection create(int index) { .resource(item) .serverSideApply(); + // Wait for the probe of the current generation: a re-apply with a new spec keeps the old status for a while, + // and the dependent resources do not use the status of an older generation. //noinspection ConstantConditions return kubernetesClient.resources(ClusterConnection.class) .inNamespace(namespace) .withName(name) .waitUntilCondition( - clusterConnection -> clusterConnection != null && clusterConnection.getStatus() != null, + clusterConnection -> clusterConnection != null + && clusterConnection.getStatus() != null + && clusterConnection.getStatus().getObservedGeneration() == clusterConnection.getMetadata().getGeneration(), 5, TimeUnit.SECONDS ); diff --git a/operator/src/test/java/it/aboutbits/postgresql/core/BaseReconcilerTest.java b/operator/src/test/java/it/aboutbits/postgresql/core/BaseReconcilerTest.java new file mode 100644 index 0000000..5cae721 --- /dev/null +++ b/operator/src/test/java/it/aboutbits/postgresql/core/BaseReconcilerTest.java @@ -0,0 +1,167 @@ +package it.aboutbits.postgresql.core; + +import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; +import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.server.mock.EnableKubernetesMockClient; +import it.aboutbits.postgresql.crd.clusterconnection.ClusterConnection; +import it.aboutbits.postgresql.crd.clusterconnection.ClusterConnectionSpec; +import it.aboutbits.postgresql.crd.database.Database; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import java.util.Optional; + +import static org.assertj.core.api.Assertions.assertThat; + +@EnableKubernetesMockClient(crud = true) +@NullMarked +class BaseReconcilerTest { + private static final String NAMESPACE = "test-namespace"; + private static final String CONNECTION_NAME = "test-connection"; + /// The generation that the mock server gives a new resource. + private static final long GENERATION = 1L; + + @SuppressWarnings("NullAway.Init") + KubernetesClient client; + + private final BaseReconciler reconciler = new BaseReconciler<>() { + @Override + protected CRStatus newStatus() { + return new CRStatus(); + } + }; + + @BeforeEach + void clearClusterConnections() { + client.resources(ClusterConnection.class).inAnyNamespace().delete(); + } + + @Nested + class GetReferencedClusterConnection { + @Test + @DisplayName("When the ClusterConnection does not exist, it should return empty") + void missingClusterConnection_returnsEmpty() { + // when + var result = getReferencedClusterConnection(); + + // then + assertThat(result).isEmpty(); + } + + @Test + @DisplayName("When the ClusterConnection has no status yet, it should return empty") + void clusterConnectionWithoutStatus_returnsEmpty() { + // given + createClusterConnection(null); + + // when + var result = getReferencedClusterConnection(); + + // then + assertThat(result).isEmpty(); + } + + @Test + @DisplayName("When the ClusterConnection is not READY, it should return empty") + void clusterConnectionNotReady_returnsEmpty() { + // given + createClusterConnection( + new CRStatus() + .updatePhase(CRPhase.ERROR) + .setObservedGeneration(GENERATION) + ); + + // when + var result = getReferencedClusterConnection(); + + // then + assertThat(result).isEmpty(); + } + + @Test + @DisplayName("When the ClusterConnection is READY for an older generation, it should return empty") + void clusterConnectionReadyForOlderGeneration_returnsEmpty() { + // given: the spec changed, and the ClusterConnection has not been probed again yet + createClusterConnection( + new CRStatus() + .updatePhase(CRPhase.READY) + .setObservedGeneration(GENERATION - 1) + ); + + // when + var result = getReferencedClusterConnection(); + + // then + assertThat(result).isEmpty(); + } + + @Test + @DisplayName("When the ClusterConnection is READY for its current generation, it should return it") + void clusterConnectionReady_returnsClusterConnection() { + // given + createClusterConnection( + new CRStatus() + .updatePhase(CRPhase.READY) + .setObservedGeneration(GENERATION) + ); + + // when + var result = getReferencedClusterConnection(); + + // then + assertThat(result) + .get() + .extracting(clusterConnection -> clusterConnection.getMetadata().getName()) + .isEqualTo(CONNECTION_NAME); + } + } + + private Optional getReferencedClusterConnection() { + var database = new Database(); + database.setMetadata(new ObjectMetaBuilder() + .withName("test-database") + .withNamespace(NAMESPACE) + .build() + ); + + var clusterRef = new ResourceRef(); + clusterRef.setName(CONNECTION_NAME); + + return reconciler.getReferencedClusterConnection( + client, + database, + clusterRef + ); + } + + private void createClusterConnection(@Nullable CRStatus status) { + var spec = new ClusterConnectionSpec(); + spec.setHost("localhost"); + spec.setPort(5432); + spec.setDatabase("postgres"); + + var clusterConnection = new ClusterConnection(); + clusterConnection.setMetadata(new ObjectMetaBuilder() + .withName(CONNECTION_NAME) + .withNamespace(NAMESPACE) + .build() + ); + clusterConnection.setSpec(spec); + clusterConnection.setStatus(status); + + client.resource(clusterConnection).create(); + + // The mock server sets the generation itself and must keep the status, or the tests above say nothing + var stored = client.resources(ClusterConnection.class) + .inNamespace(NAMESPACE) + .withName(CONNECTION_NAME) + .get(); + + assertThat(stored.getMetadata().getGeneration()).isEqualTo(GENERATION); + assertThat(stored.getStatus() == null).isEqualTo(status == null); + } +} diff --git a/operator/src/test/java/it/aboutbits/postgresql/core/KubernetesServiceTest.java b/operator/src/test/java/it/aboutbits/postgresql/core/KubernetesServiceTest.java index 5e6ab29..867ec4a 100644 --- a/operator/src/test/java/it/aboutbits/postgresql/core/KubernetesServiceTest.java +++ b/operator/src/test/java/it/aboutbits/postgresql/core/KubernetesServiceTest.java @@ -28,8 +28,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; -@NullMarked @EnableKubernetesMockClient(crud = true) +@NullMarked class KubernetesServiceTest { @SuppressWarnings("NullAway.Init") KubernetesClient client; diff --git a/operator/src/test/java/it/aboutbits/postgresql/core/PasswordFingerprintServiceTest.java b/operator/src/test/java/it/aboutbits/postgresql/core/PasswordFingerprintServiceTest.java index 6a11638..fe872bf 100644 --- a/operator/src/test/java/it/aboutbits/postgresql/core/PasswordFingerprintServiceTest.java +++ b/operator/src/test/java/it/aboutbits/postgresql/core/PasswordFingerprintServiceTest.java @@ -19,8 +19,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; -@NullMarked @EnableKubernetesMockClient(crud = true) +@NullMarked class PasswordFingerprintServiceTest { private static final String SECRET_NAME = "test-password-fingerprint-key";