From 3bae1f68759034dc33af40fc7b0118d5d660ef00 Mon Sep 17 00:00:00 2001 From: Phillipp Glanz Date: Mon, 17 Aug 2026 09:44:05 +0200 Subject: [PATCH] fix: the api jar did not start, and nothing in the build noticed Two failures, one release. Both were found by the cluster rather than by the build, which is the part worth fixing. ## The api would not start TenantGroupIndexLoader declared @Scheduled(fixedDelay = "60s"). Micronaut's converter rejects that spelling, and because the scheduled-method processor runs during context start, it did not fail one bean -- it failed the application. The single api replica went into CrashLoopBackOff and took the whole API down for about eight minutes, until the platform chart was rolled back to 0.8.0. It now schedules through TaskScheduler with a typed Duration. A constant cannot be spelled wrong, and if it could, the compiler would say so instead of the kubelet. ## Nothing in the build could have caught it This took two attempts to establish honestly. A test that boots a real Micronaut context with an embedded server passed with the bug faithfully reintroduced -- so that "guard" was worthless. The difference is not the environment, it is the shadowed jar: on the plain test classpath "60s" converts fine. So :api:startupSmokeTest boots the shadowed jar and fails if the context does not start, and it is wired into check. Verified in both directions -- green on the fix, red on the reintroduced bug, with the real error in the failure message. ApplicationStartupTest stays for what it does catch, with its Javadoc corrected to say plainly that this is not it. ## The operator could not read Rook's own resource Separately, and not new: CephObjectStoreUserStatus modelled only `phase`, while Rook had added `status.info` and `status.observedGeneration`. TenantReconciler reads the existing user before touching it, so every reconciliation of every tenant threw UnrecognizedPropertyException and exhausted its retries. Nothing looked broken -- a tenant only reconciles when something changes -- until this release needed one, and then the tenant silently never got its application instance. All four Rook model types now ignore unmodelled fields. The fix is deliberately not "add info": that is a race against another project's roadmap. A model of somebody else's resource must not fail on a field it never reads. The test uses the verbatim JSON from the live cluster. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019Bff5mpWkUnZA77jys8DiR --- api/build.gradle.kts | 75 +++++++++++++++++++ .../api/directory/TenantGroupIndexLoader.java | 58 +++++++++++--- .../apus/api/ApplicationStartupTest.java | 66 ++++++++++++++++ .../rook/CephObjectStoreUserSpec.java | 11 ++- .../rook/CephObjectStoreUserStatus.java | 19 ++++- .../operator/rook/ObjectBucketClaimSpec.java | 10 ++- .../rook/ObjectBucketClaimStatus.java | 11 ++- .../rook/RookResourceSerialisationTest.java | 48 ++++++++++++ 8 files changed, 283 insertions(+), 15 deletions(-) create mode 100644 api/src/test/java/net/onelitefeather/apus/api/ApplicationStartupTest.java diff --git a/api/build.gradle.kts b/api/build.gradle.kts index 63eb571..a31e52f 100644 --- a/api/build.gradle.kts +++ b/api/build.gradle.kts @@ -160,6 +160,81 @@ tasks { } } +// Boots the shadowed jar and fails if the application context does not start. +// +// This exists because of an outage that every other check in this repository was blind to. A +// @Scheduled(fixedDelay = "60s") annotation -- a spelling Micronaut's converter rejects -- made the +// context fail during start(). It was published, rolled out, and the single api replica went into +// CrashLoopBackOff, taking the whole API down. The entire test suite passed throughout. +// +// It passed for a reason worth knowing: the failure only happens in the *shadowed* jar. On the +// plain test classpath the very same annotation converts fine, so no @MicronautTest -- not even one +// that starts an embedded server -- can see it. Only running the artifact that ships can. +// +// Deliberately not a Test task: there is no test to write. The assertion is "the jar we are about +// to publish starts", and the only way to make it is to start it. +val startupSmokeTest by tasks.registering { + group = "verification" + description = "Boots the shadowed api jar and fails if the Micronaut context does not start." + dependsOn(tasks.shadowJar) + + val jar = tasks.shadowJar.flatMap { it.archiveFile } + val javaLauncher = javaToolchains.launcherFor(java.toolchain) + inputs.file(jar) + outputs.upToDateWhen { false } + + doLast { + val process = ProcessBuilder( + javaLauncher.get().executablePath.asFile.absolutePath, + "-jar", + jar.get().asFile.absolutePath, + ).apply { + redirectErrorStream(true) + // Everything it would reach is pointed somewhere unreachable on purpose. The question + // is only whether startup gets far enough to announce itself. + environment()["APUS_JWT_ISSUER"] = "https://issuer.invalid/v2.0" + environment()["APUS_JWT_JWKS_URI"] = "http://127.0.0.1:1/unused-jwks-endpoint" + environment()["KUBECONFIG"] = "/dev/null" + environment()["MICRONAUT_SERVER_PORT"] = "0" + }.start() + + val output = StringBuilder() + var started = false + try { + process.inputStream.bufferedReader().use { reader -> + val deadline = System.currentTimeMillis() + 90_000 + while (System.currentTimeMillis() < deadline) { + val line = reader.readLine() ?: break + output.appendLine(line) + // Micronaut prints this only once the context is fully up. + if (line.contains("Startup completed") || line.contains("Server Running")) { + started = true + break + } + // Fail fast and loudly rather than waiting out the timeout. + if (line.contains("Error starting Micronaut server")) { + break + } + } + } + } finally { + process.destroyForcibly() + process.waitFor() + } + + if (!started) { + throw GradleException( + "the shadowed api jar did not start -- this is what reaches the cluster:\n\n$output", + ) + } + logger.lifecycle("startupSmokeTest: the shadowed api jar starts") + } +} + +tasks.named("check") { + dependsOn(startupSmokeTest) +} + // TenantIsolationIntegrationTest starts a k3s container (via Testcontainers), applies the // `:operator` module's generated CRDs to it, and proves cross-tenant isolation over a real, // JWT-authenticated HTTP call against a real API server -- minutes of work and Docker, exactly diff --git a/api/src/main/java/net/onelitefeather/apus/api/directory/TenantGroupIndexLoader.java b/api/src/main/java/net/onelitefeather/apus/api/directory/TenantGroupIndexLoader.java index 9d59705..6f00a60 100644 --- a/api/src/main/java/net/onelitefeather/apus/api/directory/TenantGroupIndexLoader.java +++ b/api/src/main/java/net/onelitefeather/apus/api/directory/TenantGroupIndexLoader.java @@ -17,8 +17,13 @@ */ package net.onelitefeather.apus.api.directory; -import io.micronaut.scheduling.annotation.Scheduled; +import io.micronaut.context.event.ApplicationEventListener; +import io.micronaut.runtime.event.ApplicationStartupEvent; +import io.micronaut.scheduling.TaskExecutors; +import io.micronaut.scheduling.TaskScheduler; +import jakarta.inject.Named; import jakarta.inject.Singleton; +import java.time.Duration; import net.onelitefeather.apus.api.rest.tenant.TenantRepository; import net.onelitefeather.apus.api.support.PrincipalResolver; import org.slf4j.Logger; @@ -30,14 +35,23 @@ * {@link DirectoryGuard}, which decides which groups Apus may act on. * *

Those two must be the same set. If they ever diverged, one of them would be wider than the - * other -- either recognising members of a group Apus refuses to manage, or managing a group - * whose members it does not recognise. Handing both from one place is what makes divergence - * impossible rather than merely unlikely. + * other -- either recognising members of a group Apus refuses to manage, or managing a group whose + * members it does not recognise. Handing both from one place is what makes divergence impossible + * rather than merely unlikely. * *

Polled rather than watched. A tenant's group id changes about as often as a tenant is * created, the list is small, and a poll cannot get stuck half-subscribed the way a watch can -- - * the failure mode of a stalled watch here would be members silently failing to resolve, with - * nothing obviously broken to look at. + * a stalled watch here would show up as members silently failing to resolve, with nothing + * obviously broken to look at. + * + *

Scheduled through {@link TaskScheduler} with a typed {@link Duration}, not {@code + * @Scheduled}. The annotation takes its interval as a string, and this class originally used + * {@code @Scheduled(fixedDelay = "60s")} -- a spelling Micronaut's converter rejects. Because the + * scheduled-method processor runs during context startup, that did not fail the one bean; it + * failed the whole application, and the api pod went into CrashLoopBackOff in the cluster with + * {@code SchedulerConfigurationException: Invalid fixed delay definition: 60s}. A {@code Duration} + * constant cannot be spelled wrong, and if it could, the compiler would say so instead of the + * kubelet. * *

A failed refresh leaves the previous index in place. Not an empty one: the Kubernetes * API being briefly unreachable must not log everybody out of their tenant. The very first load @@ -45,29 +59,51 @@ * honestly be. */ @Singleton -public class TenantGroupIndexLoader { +public class TenantGroupIndexLoader implements ApplicationEventListener { private static final Logger LOGGER = LoggerFactory.getLogger(TenantGroupIndexLoader.class); + /** How often the index is rebuilt. See the class Javadoc for why this is not a string. */ + static final Duration REFRESH_INTERVAL = Duration.ofSeconds(60); + private final TenantRepository tenants; private final PrincipalResolver principals; private final DirectoryGuard guard; + private final TaskScheduler scheduler; - public TenantGroupIndexLoader(TenantRepository tenants, PrincipalResolver principals, DirectoryGuard guard) { + public TenantGroupIndexLoader( + TenantRepository tenants, + PrincipalResolver principals, + DirectoryGuard guard, + @Named(TaskExecutors.SCHEDULED) TaskScheduler scheduler) { this.tenants = tenants; this.principals = principals; this.guard = guard; + this.scheduler = scheduler; + } + + /** + * Loads the index once the context is up, then keeps it fresh. + * + *

Deliberately not in the constructor: a constructor that talks to the Kubernetes API makes + * bean creation depend on a network call, and a failure there is far harder to attribute than + * one after startup has been announced. + */ + @Override + public void onApplicationEvent(ApplicationStartupEvent event) { refresh(); + scheduler.scheduleAtFixedRate(REFRESH_INTERVAL, REFRESH_INTERVAL, this::refresh); } /** Rebuilds the index from the current tenant list and publishes it to both consumers. */ - @Scheduled(fixedDelay = "60s") - public final void refresh() { + void refresh() { try { TenantGroupIndex index = TenantGroupIndex.of(tenants.list()); principals.setGroupIndex(index); guard.setManagedGroups(index.managedGroups()); - LOGGER.debug("tenant group index refreshed: {} managed group(s)", index.managedGroups().size()); + LOGGER.debug( + "tenant group index refreshed: {} managed group(s)", + index.managedGroups().size()); } catch (RuntimeException e) { // Keep serving with what we had. Losing the index would sign everybody out of their // tenant over a transient API-server hiccup. diff --git a/api/src/test/java/net/onelitefeather/apus/api/ApplicationStartupTest.java b/api/src/test/java/net/onelitefeather/apus/api/ApplicationStartupTest.java new file mode 100644 index 0000000..7d76163 --- /dev/null +++ b/api/src/test/java/net/onelitefeather/apus/api/ApplicationStartupTest.java @@ -0,0 +1,66 @@ +/** + * Apus - render and host BlueMap maps on Kubernetes. + * Copyright (C) 2026 OneLiteFeather and contributors + *

+ * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + *

+ * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + *

+ * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package net.onelitefeather.apus.api; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.micronaut.context.ApplicationContext; +import io.micronaut.runtime.server.EmbeddedServer; +import net.onelitefeather.apus.api.directory.TenantGroupIndexLoader; +import net.onelitefeather.apus.api.security.ImpersonationPolicy; +import org.junit.jupiter.api.Test; + +/** + * Boots a real context with an embedded server and asserts it comes up, then resolves the beans + * this module's newest wiring added. Deliberately shallow: it asserts nothing about behaviour. Its + * job is to fail when the application would not start at all -- an unsatisfiable bean, a factory + * that throws, a configuration property that will not bind. + * + *

What it cannot do, stated because it was written believing otherwise. This test was + * added in response to an outage: {@code TenantGroupIndexLoader} declared + * {@code @Scheduled(fixedDelay = "60s")}, a spelling Micronaut's converter rejects, the context + * failed during {@code start()}, and the api pod went into CrashLoopBackOff and took the whole API + * down. This test was then checked against that exact bug, reintroduced faithfully -- and it + * passed. On the plain test classpath the annotation converts fine; it only fails inside + * the shadowed jar. No {@code @MicronautTest}, not even one that starts a server, can see that + * class of failure. + * + *

What does see it is {@code :api:startupSmokeTest}, which boots the shadowed jar itself and is + * wired into {@code check}. That task was verified in both directions -- green on the fix, red on + * the reintroduced bug. If you are looking for the guard against "the artifact does not start", + * it is there, not here. + */ +class ApplicationStartupTest { + + @Test + void theApplicationStarts() { + try (ApplicationContext context = ApplicationContext.run(EmbeddedServer.class, "apitest") + .getApplicationContext()) { + assertTrue(context.isRunning(), "the application context must be running"); + + // Resolved explicitly rather than trusting that startup implies they exist: a bean + // nothing asks for can be broken in ways a context start never notices. + assertNotNull( + context.getBean(TenantGroupIndexLoader.class), + "the tenant group index loader must be resolvable -- it is what maps a token's" + + " groups claim onto a tenant"); + assertNotNull(context.getBean(ImpersonationPolicy.class)); + } + } +} diff --git a/operator/src/main/java/net/onelitefeather/apus/operator/rook/CephObjectStoreUserSpec.java b/operator/src/main/java/net/onelitefeather/apus/operator/rook/CephObjectStoreUserSpec.java index e126afd..35aac2c 100644 --- a/operator/src/main/java/net/onelitefeather/apus/operator/rook/CephObjectStoreUserSpec.java +++ b/operator/src/main/java/net/onelitefeather/apus/operator/rook/CephObjectStoreUserSpec.java @@ -17,7 +17,16 @@ */ package net.onelitefeather.apus.operator.rook; -/** Desired state of a Rook CephObjectStoreUser. Plain data, no Kubernetes access. */ +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; + +/** + * Desired state of a Rook CephObjectStoreUser. Plain data, no Kubernetes access. + * + *

Tolerates unmodelled fields like the status types do. A spec is read back as well as written: + * this operator reads an existing user before touching it, and a field somebody set through + * {@code kubectl} -- or one a Rook upgrade defaults in -- must not make that read throw. + */ +@JsonIgnoreProperties(ignoreUnknown = true) public class CephObjectStoreUserSpec { private String store; diff --git a/operator/src/main/java/net/onelitefeather/apus/operator/rook/CephObjectStoreUserStatus.java b/operator/src/main/java/net/onelitefeather/apus/operator/rook/CephObjectStoreUserStatus.java index f5b541e..b8594e9 100644 --- a/operator/src/main/java/net/onelitefeather/apus/operator/rook/CephObjectStoreUserStatus.java +++ b/operator/src/main/java/net/onelitefeather/apus/operator/rook/CephObjectStoreUserStatus.java @@ -17,7 +17,24 @@ */ package net.onelitefeather.apus.operator.rook; -/** Observed state of a Rook CephObjectStoreUser. */ +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; + +/** + * Observed state of a Rook CephObjectStoreUser. + * + *

Ignores everything it does not model, and that is load-bearing. Rook owns this CRD and + * extends it whenever it likes. It added {@code status.info} and {@code status.observedGeneration}; + * this class knew only {@code phase}, so reading a real user threw + * {@code UnrecognizedPropertyException: Unrecognized field "info"} -- inside {@code + * TenantReconciler}, which reads the existing user before touching it. Every reconciliation of + * every tenant failed from that moment on, and because a tenant only reconciles when something + * changes, nothing looked broken until a release needed one: the operator exhausted its retries and + * the tenant silently never got the resources it was owed. + * + *

The answer is not to add the missing fields as they appear -- that is a race against another + * project's roadmap. A model of somebody else's resource must not fail on a field it does not read. + */ +@JsonIgnoreProperties(ignoreUnknown = true) public class CephObjectStoreUserStatus { private String phase; diff --git a/operator/src/main/java/net/onelitefeather/apus/operator/rook/ObjectBucketClaimSpec.java b/operator/src/main/java/net/onelitefeather/apus/operator/rook/ObjectBucketClaimSpec.java index e7c4cc2..b236311 100644 --- a/operator/src/main/java/net/onelitefeather/apus/operator/rook/ObjectBucketClaimSpec.java +++ b/operator/src/main/java/net/onelitefeather/apus/operator/rook/ObjectBucketClaimSpec.java @@ -17,10 +17,18 @@ */ package net.onelitefeather.apus.operator.rook; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import java.util.LinkedHashMap; import java.util.Map; -/** Desired state of a Rook ObjectBucketClaim. Plain data, no Kubernetes access. */ +/** + * Desired state of a Rook ObjectBucketClaim. Plain data, no Kubernetes access. + * + *

Tolerates unmodelled fields for the same reason {@link CephObjectStoreUserSpec} does: Rook + * fills in defaults of its own (e.g. {@code objectBucketName}), and reading a claim back must not + * fail over one. + */ +@JsonIgnoreProperties(ignoreUnknown = true) public class ObjectBucketClaimSpec { private String bucketName; diff --git a/operator/src/main/java/net/onelitefeather/apus/operator/rook/ObjectBucketClaimStatus.java b/operator/src/main/java/net/onelitefeather/apus/operator/rook/ObjectBucketClaimStatus.java index d36ff56..328fa6e 100644 --- a/operator/src/main/java/net/onelitefeather/apus/operator/rook/ObjectBucketClaimStatus.java +++ b/operator/src/main/java/net/onelitefeather/apus/operator/rook/ObjectBucketClaimStatus.java @@ -17,7 +17,16 @@ */ package net.onelitefeather.apus.operator.rook; -/** Observed state of a Rook ObjectBucketClaim. */ +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; + +/** + * Observed state of a Rook ObjectBucketClaim. + * + *

Ignores unmodelled fields for the same reason {@link CephObjectStoreUserStatus} does, and + * before it costs anything to learn: Rook owns this CRD, and reading one of its objects must not + * fail over a field this operator never looks at. + */ +@JsonIgnoreProperties(ignoreUnknown = true) public class ObjectBucketClaimStatus { /** Rook sets this to "Bound" once the bucket exists and credentials are written. */ diff --git a/operator/src/test/java/net/onelitefeather/apus/operator/rook/RookResourceSerialisationTest.java b/operator/src/test/java/net/onelitefeather/apus/operator/rook/RookResourceSerialisationTest.java index 79d5e1d..6562548 100644 --- a/operator/src/test/java/net/onelitefeather/apus/operator/rook/RookResourceSerialisationTest.java +++ b/operator/src/test/java/net/onelitefeather/apus/operator/rook/RookResourceSerialisationTest.java @@ -85,4 +85,52 @@ void deserialisesAClaimStatusFromTheCluster() { assertEquals("Bound", claim.getStatus().getPhase()); assertEquals("apus-friends-survival", claim.getSpec().getBucketName()); } + + /** + * Rook owns these CRDs and may add a field to them whenever it likes -- and it did. + * + *

The JSON below is copied verbatim from a live {@code CephObjectStoreUser} in + * {@code rook-ceph-fr01}. Only {@code phase} was modelled here, so reading that object threw + * {@code UnrecognizedPropertyException: Unrecognized field "info"} -- inside {@code + * TenantReconciler}, which reads the existing user before touching it. Every reconciliation of + * every tenant failed from the moment Rook populated {@code status.info}, and because a tenant + * only reconciles when something changes, nothing appeared broken until a release needed one. + * The operator then exhausted its retries and the tenant silently never got its application + * instance. + * + *

The fix is not to add {@code info}: it is to stop caring what else Rook sends. A model of + * somebody else's resource has no business failing on a field it does not read. + */ + @Test + void aRookStatusWithFieldsWeDoNotModelIsReadAnyway() { + String json = + """ + {"apiVersion":"ceph.rook.io/v1","kind":"CephObjectStoreUser",\ + "metadata":{"name":"apus-onelitefeather-dev","namespace":"rook-ceph-fr01"},\ + "spec":{"store":"feather-s3","displayName":"apus-onelitefeather-dev"},\ + "status":{"info":{"secretName":"rook-ceph-object-user-feather-s3-apus-onelitefeather-dev"},\ + "observedGeneration":1,"phase":"Ready"}}"""; + + CephObjectStoreUser user = Serialization.unmarshal(json, CephObjectStoreUser.class); + + assertEquals("Ready", user.getStatus().getPhase()); + assertEquals("feather-s3", user.getSpec().getStore()); + } + + /** The same tolerance on the other Rook resource, before it costs anything to learn. */ + @Test + void anObjectBucketClaimWithFieldsWeDoNotModelIsReadAnyway() { + String json = + """ + {"apiVersion":"objectbucket.io/v1alpha1","kind":"ObjectBucketClaim",\ + "metadata":{"name":"apus-friends-survival","namespace":"bluemap-friends"},\ + "spec":{"bucketName":"apus-friends-survival","storageClassName":"ceph-bucket-fr01",\ + "objectBucketName":"obc-bluemap-friends-apus-friends-survival"},\ + "status":{"phase":"Bound","conditions":[],"somethingRookAddsLater":true}}"""; + + ObjectBucketClaim claim = Serialization.unmarshal(json, ObjectBucketClaim.class); + + assertEquals("Bound", claim.getStatus().getPhase()); + assertEquals("apus-friends-survival", claim.getSpec().getBucketName()); + } }