Repository navigation
fix(admin): back k8s client/scaler beans off their own types and guard the missing client (#7912) - #7917
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Review: #7917 fix(admin): back k8s client/scaler beans off their own types and guard the missing client (#7912)
Verdict: changes requested — one concrete, reproduced startup regression in the exact configuration the linked issue reports, plus a test that hides it.
First, the parts I checked and agree with:
- Deleting the
kubernetesScaler@Beanis right. Its@ConditionalOnMissingBean({AppsV1Api.class, DeploymentProperties.class})could not fire onceapiClientwas gone, and the live scaler is the component-scanned@Component.KubernetesScalerdoes keepthis.appsV1Api = appsV1Api.orElse(null);, so the newObjects.isNull(appsV1Api)guards are effective, andScaleService.applyTargetalready treats afalsereturn as "not applied", so the no-op path does not corruptappliedTarget. - The guards are needed, not defensive noise:
ScaleTaskInitializeris an unconditionalCommandLineRunnerthat callsscaleService.executeScaling(), andshenyu.k8s.scale.enabledonly gates the client, not the scaler, so an active policy row NPE'd the startup path before this change. TestPropertyValuesdoes acceptkey:value—TestPropertyValues.Pair.getSeparatorIndextakesMath.min(indexOf(':'), indexOf('='))— so theca-cert-path:entry is not a bug (see nit 5 below), andpr_buildreally did executeKubernetesConfigurationTest(4/4) andKubernetesScalerTest(4/4) on6ef826af.
Blocking: a pre-registered DeploymentProperties now fails startup, and the new test passes by accident
@EnableConfigurationProperties(DeploymentProperties.class) stays on KubernetesConfiguration, so it always registers its own properties bean (shenyu.k8s.deployment-…DeploymentProperties). Once apiClient is actually created, it needs a DeploymentProperties argument — and with a user-supplied properties bean there are two candidates.
I reproduced this against the real KubernetesConfiguration/DeploymentProperties sources on Spring Boot 3.3.1 / Spring 6.1.10 (same as the root pom), only stubbing Config/ApiClient/AppsV1Api:
T1 new | enabled + standard binding -> containsBean(apiClient)=true; AppsV1Api beans=[apiClient]
T2 new | enabled + props pre-registered (as written) -> containsBean(apiClient)=true; AppsV1Api beans=[apiClient]
T6 new | enabled + props pre-registered (explicit name) -> STARTUP FAILED: NoUniqueBeanDefinitionException :
expected single matching bean but found 2: preRegisteredK8sProperties,
shenyu.k8s.deployment-org.apache.shenyu.admin.config.properties.DeploymentProperties
O1 old | enabled + standard binding -> containsBean(apiClient)=true; AppsV1Api beans=[apiClient]
O2 old | enabled + props pre-registered -> containsBean(apiClient)=false; AppsV1Api beans=[] (context starts)
Two things follow.
shouldCreateClientEvenWhenPropertiesBeanIsPreRegisteredonly passes because of a bean-name coincidence.withBean(DeploymentProperties.class, this::deploymentProperties)registers the extra bean under the namedeploymentProperties, which is exactly theapiClientfactory parameter name (<arg>-parameters</arg>is enabled in the root pom), sodetermineAutowireCandidateresolves the ambiguity by name. Give the same bean any other name — which is what a real user would write, e.g.@Bean DeploymentProperties k8sProperties()— and the context fails withNoUniqueBeanDefinitionException. The test therefore does not pin the behaviour it claims; rename it explicitly (.withBean("preRegisteredProperties", DeploymentProperties.class, …)) and it goes red.- That failure is a behaviour change for the reported configuration.
O2(old code, pre-registered properties) starts and merely leavesappsV1Apiempty;T6(this PR) refuses to start. Turning "scaling silently does nothing" into "Admin does not boot" in the scenario the issue is about is worse than the bug being fixed, and the failure is not limited toapiClient— any other injection point ofDeploymentPropertiesbecomes ambiguous too.
Suggested repair: stop having two sources for the same properties bean. Either
-
register the properties from
KubernetesConfigurationitself, so a user bean can cleanly win, e.g. replace@EnableConfigurationProperties(DeploymentProperties.class)with@Bean @ConditionalOnMissingBean @ConfigurationProperties(prefix = "shenyu.k8s.deployment") public DeploymentProperties deploymentProperties() { return new DeploymentProperties(); }
which keeps the issue's requested "custom properties override" working and makes
@ConditionalOnMissingBean(AppsV1Api.class)onapiClientmeaningful; or -
keep
@EnableConfigurationPropertiesand qualify the parameter so the@EnableConfigurationPropertiesbean is always the one injected, documenting that overriding the properties bean is unsupported.
Either way, the test should use an explicitly named pre-registered bean so it exercises the real case.
Non-blocking
-
@ConditionalOnMissingBeanon a scanned@Configurationis ordering-dependent.shouldBackOffWhenACustomClientIsProvidedpasses becausewithBeanregisters the custom client beforerefresh(). In a real deployment a user@Bean AppsV1Apiin their own scanned@Configurationthat is processed afterKubernetesConfigurationproduces twoAppsV1Apibeans, andKubernetesScaler(Optional<AppsV1Api> …)then fails withNoUniqueBeanDefinitionException(Optionalis not one ofindicatesMultipleBeans, so this is a hard failure, not an empty Optional). I reproduced it:ours imported first -> Client beans=[apiClient, customClient] Optional<Client> injection -> NoUniqueBeanDefinitionException (startup would FAIL) custom client imported first -> Client beans=[customClient], Optional<Client> -> one clientThe old code had the same latent problem, so this is not a regression, but Spring Boot documents this condition as auto-configuration-only. Moving this configuration to
@AutoConfiguration(or injectingObjectProvider<AppsV1Api>and picking the first candidate) would make the back-off deterministic, and it deserves a line in the PR description rather than a test that only proves harness ordering. -
containsBean("apiClient")cannot distinguish "client created" from "@Beanreturnednull".containsBeanis satisfied by the bean definition, which exists whenever@ConditionalOnPropertymatched —apiClientstillreturn nullonIOException, which Spring registers as aNullBean. The twocontainsBeanassertions therefore only prove the condition outcome, not client creation, whileshouldNotCreateClientWhenScalingIsDisabledusesgetBeansOfType(AppsV1Api.class). Use the samegetBeansOfType/hasSingleBeanassertion in all four tests. -
The PR description overstates the bug. It says "the condition can never pass and the
AppsV1Apibean is never created, no matter what the user configures". MyO1/Cruns show the old code does create the client on the plain binding path, because the@EnableConfigurationPropertiesregistrar of the same configuration class runs after its@Beanmethods, soDeploymentPropertiesis not yet visible toOnBeanCondition. The verified regression is exactly the pre-registered-properties case — which is the case in issue #7912 and which that issue itself hedges. Worth correcting, since the description becomes the changelog entry. -
scaleByActiondrops the newfalse.scaleByActioncallsscaleByNum(newReplicas)and ignores the result, so "client unavailable" and "ApiException" are both invisible to callers; the only signal is the WARN. Consider returningboolean(or logging once at the caller) so policy reconciliation can react. -
Config.fromToken(apiServer, token, false)disables TLS verification and then sets a CA cert on the client, so the cert is never used for verification. That was harmless while the bean was never created; now this path runs for the first time, so please confirm the intent —trueis almost certainly what "trust this CA" means. Also notesetSslCaCertreceives a try-with-resources stream that is closed immediately after the call, i.e. the client keeps a stream it cannot re-read. -
Mixed property separators in
enabledProps()."shenyu.k8s.deployment.ca-cert-path:" + certFile()works, butgetSeparatorIndextakes the first:or=of the whole string, so a value that itself contains a colon before the=would silently truncate the key. Use=for readability and safety, as the other entries do. -
testScaleByNumWithoutClientIsSafeNoOp/testScaleByActionWithoutClientIsSafeNoOpbuild the scaler withOptional.empty()— good, that pins the exact NPE from the issue. No notes there.
CI
build (shenyu-integrated-test-http) is red on this head, but it is the pre-existing master regression from #7903 (CryptorResponsePluginTest.testWhenDoesntExistFiled still asserts the old 10001 contract while CryptorResponsePlugin.fieldErrorParse now throws ResponsiveException(-116)). Not caused by this PR — it needs a separate fix on master.
|
Fixed, and thanks for the reproduction — you were right that the test only passed on the bean-name coincidence. I went with the first repair route:
On the non-blocking points:
I also corrected the description below — you are right that the old code created the client on the plain binding path; the regression was the pre-registered-properties case. The ordering caveat for |
There was a problem hiding this comment.
Confirmation: I independently reviewed this change and agree with @Aias00's blocking conclusion — the DeploymentProperties registered by @EnableConfigurationProperties and a user-supplied bean with an explicit name create two candidates, so injecting apiClient fails with NoUniqueBeanDefinitionException; the existing withBean(DeploymentProperties.class, ...) only resolves the ambiguity by the parameter name deploymentProperties by coincidence. I suggest removing the duplicate source as described above and changing the test to use an explicitly named bean.
Also, the CI failure in build (shenyu-integrated-test-http) is the pre-existing master issue from #7903, not introduced by this PR.
|
Friendly nudge — the branch is still at The one blocking item is small and self-contained: the PR makes a pre-registered Today's The fix I suggested still holds: declare the properties bean yourself instead of relying on @Bean
@ConditionalOnMissingBean
@ConfigurationProperties(prefix = shenyu.k8s.deployment)
public DeploymentProperties deploymentProperties() {
return new DeploymentProperties();
}and swap the The non-blocking notes from the same review (PR-body wording about the condition never passing, |
|
Apologies for the confusion — my previous reply went out before the branch did. The fixes are now pushed (head
The description is also updated. Local run: |
Aias00
left a comment
There was a problem hiding this comment.
Re-review: #7917 fix(admin): back k8s client/scaler beans off their own types and guard the missing client (#7912)
Verdict: approved. The blocking regression I reported on 6ef826af is fixed, and the test now pins the real scenario instead of passing by bean-name coincidence.
The blocking issue is resolved
I had reproduced NoUniqueBeanDefinitionException with a DeploymentProperties bean registered under a name other than the factory parameter (T6 in my previous review), while the old test only passed because withBean(DeploymentProperties.class, …) registers the bean as deploymentProperties, matching the apiClient parameter name.
Both halves of that are now addressed:
@EnableConfigurationPropertiesis gone; the properties are registered by a@Bean @ConditionalOnMissingBean @ConfigurationProperties(prefix = "shenyu.k8s.deployment")method, so there is no second candidate to compete withapiClient's argument.shouldCreateClientEvenWhenPropertiesBeanIsPreRegisterednow registers the bean explicitly aspreRegisteredProperties, and the test asserts a singleAppsV1Apibean rather thancontainsBean-style presence. That is exactly the case that used to fail.shouldUseThePreRegisteredPropertiesgoes further and assertsassertSame(custom, context.getBean(DeploymentProperties.class)), i.e. the user bean is not merely tolerated but is the one the client is built from.
The rest checks out
@ConditionalOnMissingBean(AppsV1Api.class)onapiClientis the right key now that the client is actually created. A@Beanreturningnullis registered as aNullBean, which does not matchAppsV1Apiby type, so the condition still fires andOptional<AppsV1Api>still resolves to empty —shouldNotCreateClientWhenScalingIsDisabledand the twoOptional.empty()scaler tests cover both ends of that.KubernetesScaleris still the component-scanned@Component, so removing the deadkubernetesScaler@Beanleaves the injection point intact, andScaleObservernow logs a warning whenscaleByActionreportsfalseinstead of silently doing nothing.- The two real latent bugs folded in here are worth having:
Config.fromToken(..., true)actually verifies the API server certificate, andnew ByteArrayInputStream(caCertStream.readAllBytes())replaces aFileInputStreamthat the try-with-resources closed before the client ever read it. Both are called out in the PR description, which is what I asked for. catch (IOException | RuntimeException e)around client construction is broad, but it is the correct trade-off for a component whose failure mode should be "logged no-op", not "Admin does not boot".
Non-blocking
- The ordering caveat is still there, and now it applies to the properties bean too.
@ConditionalOnMissingBeanon a component-scanned@Configurationis evaluated in registration order, so a user@Bean DeploymentPropertiesin a configuration parsed afterKubernetesConfigurationproduces two candidates andapiClientfails withNoUniqueBeanDefinitionExceptionagain — the same class of failure, only narrower. You have already documented this as a known follow-up and named@AutoConfiguration/ObjectProvideras the deterministic fix, so I am not blocking on it; please keep the follow-up open rather than letting the "Known follow-up" section age out of the description. testScaleByActionWithoutClientIsSafeNoOpassertsassertDoesNotThrowbut not thefalsereturn, which is the contractScaleObservernow relies on.assertFalsewould pin it more tightly.
Nit: script/shenyu_checkstyle.xml has CustomImportOrder commented out, so the assertEquals / assertFalse / assertNotNull / assertDoesNotThrow / assertTrue ordering in KubernetesScalerTest is not a build problem — just unusual to read.
|
Thanks for this PR — I reviewed it against #7912 and previously approved it, but the current head ( What breaks
This is a hard I reproduced this in isolation against Spring Boot 3.3.1 (the version this repo pins) and got the identical diagnostic, then confirmed the fix below. Suggested fixDrop the @Bean
@ConditionalOnMissingBean
public DeploymentProperties deploymentProperties() {
return new DeploymentProperties();
}I verified locally that this still compiles and that the prefix is bound correctly ( The other red checks are cascade
Happy to re-review as soon as a fix is pushed. |
Aias00
left a comment
There was a problem hiding this comment.
Request changes: the current head does not compile.
pr_build fails with KubernetesConfiguration.java:[54,33] Duplicate @ConfigurationProperties definition for prefix 'shenyu.k8s.deployment' — DeploymentProperties is already annotated with that prefix at the class level, and the new @Bean deploymentProperties() method repeats it, which spring-boot-configuration-processor rejects as a hard javac error.
Please drop the method-level @ConfigurationProperties annotation (keep the class-level one); that compiles and still binds shenyu.k8s.deployment.*, so the #7912 override behaviour is preserved. Details in the comment above. I will re-review once fixed.
|
Fixed — the method-level I also added One honest caveat: a local 2 files, |
Aias00
left a comment
There was a problem hiding this comment.
Good hardening pass on the Kubernetes scaler wiring. I checked the bean graph specifically because the kubernetesScaler @Bean is removed:
KubernetesScaleris already annotated@Component, so it is still discovered; itsOptional<AppsV1Api>+DeploymentPropertiesconstructor arguments resolve (the new@ConditionalOnMissingBean DeploymentPropertiesbean supplies the latter, andOptionaltolerates a nullAppsV1Api). No missing-bean startup failure.
The individual fixes are all correct:
client.setSslCaCert(new ByteArrayInputStream(caCertStream.readAllBytes()))fixes a real bug:FileInputStreamwas closed by try-with-resources whileApiClientreads the CA lazily when it builds the HTTP client, so the cert could be unreadable at that point. A re-readable in-memory copy is the right answer.Config.fromToken(..., true)turns on certificate verification (previouslyfalse). That is the correct security posture; combined with the catch ofRuntimeException, an unreachable cluster or unusable CA now degrades to a logged no-op instead of failing Admin startup.@ConditionalOnMissingBean(AppsV1Api.class)is the right condition for the client bean, and@ConditionalOnMissingBeanondeploymentProperties()lets a user-supplied bean win.scaleByActionreturningbooleanwith null-client guards, plusScaleObserverlogging a warning when an action was not applied, makes silent no-ops observable.
One note: with TLS verification now on, clusters using a CA that does not match the API server certificate will stop scaling (by design) but only log an error. Worth mentioning in the deployment docs so the behaviour change is discoverable.
LGTM.
|
Root cause found for the The job fails on: I traced this across three unrelated PRs (#7917, #7895, #7888) and all three fail on exactly the same two assertions, so it is not specific to your change. It is already fixed on master. The same job passes on newer heads:
Your branch is behind master (this PR: 58 commits), which is why it still carries the broken cache code. What to do: rebase onto current master and let CI re-run. That should clear this job without touching your diff. If it still fails after the rebase, the regression is genuinely yours and I will dig in again. Additional note for this PR: the |
…d the missing client (apache#7912)
…TLS against the CA (apache#7912)
091ae16 to
ec3f685
Compare
Aias00
left a comment
There was a problem hiding this comment.
Additional validation of the already-approved head ec3f685: KubernetesConfigurationTest and KubernetesScalerTest passed all 10 tests, zero failures/errors/skips. Rechecked the earlier named-properties-bean ambiguity and duplicate ConfigurationProperties prefix concerns: the current conditional properties bean and class-only prefix address them. No new blocking finding; my existing approval still applies. No live Kubernetes API server was exercised. Validation baseline: PR head plus a clean, uncommitted merge of master 52339b2 in the isolated worktree. No source was manually patched or pushed.
Used isolated worktrees and the Maven wrapper with JDK 21/project compiler settings. Checkstyle passed for the targeted Maven runs; RAT and JaCoCo were skipped. No full reactor/e2e pass is claimed.
Fixes #7912.
Summary
A deployment that registers its own
DeploymentPropertiesbean silently loses the Kubernetes scaling client:apiClientwas guarded by@ConditionalOnMissingBean(DeploymentProperties.class), so the presence of that bean — which the issue's configuration has — suppresses client creation, leaving the component-scannedKubernetesScalerwith a null client whose first scale call NPEs (scaleByNumonly catchesApiException). On the plain binding path the old code did create the client; the regression is specific to a pre-registered properties bean. ThekubernetesScaler@Beanmethod was dead code (its condition could not fire onceapiClientwas gone) and is removed.Modifications
@Beanmethod (@ConditionalOnMissingBean) instead of@EnableConfigurationProperties, so a user-suppliedDeploymentPropertiescleanly wins instead of competing with a second candidate — the previous arrangement madeapiClientambiguous and failed startup withNoUniqueBeanDefinitionExceptiononce the client was actually created. The prefix stays on theDeploymentPropertiesclass: repeating@ConfigurationProperties(prefix = "shenyu.k8s.deployment")on the factory method is a duplicate definition thatspring-boot-configuration-processorrejects at compile time.apiClientbacks off on@ConditionalOnMissingBean(AppsV1Api.class); theshenyu.k8s.scale.enabledgate is unchanged.scaleByNum/scaleByActionlog a warning and no-op when the client is absent, andscaleByActionnow returns the outcome soScaleObservercan report a scale that was not applied. Client construction failures (unreachable cluster, unusable CA) are caught and degrade to the same logged no-op rather than failing startup.Config.fromToken(..., true)) and the CA is handed to the client as a re-readable copy instead of a stream that was already closed.Verifying this change
KubernetesConfigurationTest: standard binding creates exactly oneAppsV1Api; a pre-registeredDeploymentProperties(under a name unrelated to the factory parameter) no longer suppresses it — fails with the reportedNoUniqueBeanDefinitionExceptionon the previous configuration; a user client wins the back-off; the user properties bean is the one used; disabled creates no client.shouldBindDeploymentPropertiesFromTheConfiguredPrefixpins that the prefix is still bound with the annotation on the class alone (name/apiServer/tokenread fromshenyu.k8s.deployment.*) — removing the class-level annotation fails it withexpected: <shenyu-bootstrap> but was: <null>and drops the client bean count to zero.KubernetesScalerTest:Optional.empty()makes both scale entry points safe no-ops.All related tests and checkstyle pass:
Known follow-up
@ConditionalOnMissingBeanon a component-scanned@Configurationis ordering-dependent: a user@Bean AppsV1Apiin a configuration processed later would still produce two candidates. Moving this configuration to@AutoConfiguration(or injectingObjectProvider<AppsV1Api>) is the deterministic fix and is left for a follow-up rather than widened into this PR. Pre-existing behavior, unchanged here.