Skip to content

🐛 Return errors from HasKubeadmConfig instead of swallowing them - #14067

Open
mrueg wants to merge 1 commit into
kubernetes-sigs:mainfrom
mrueg:fix/kcp-haskubeadmconfig-error-handling
Open

🐛 Return errors from HasKubeadmConfig instead of swallowing them#14067
mrueg wants to merge 1 commit into
kubernetes-sigs:mainfrom
mrueg:fix/kcp-haskubeadmconfig-error-handling

Conversation

@mrueg

@mrueg mrueg commented Aug 11, 2026

Copy link
Copy Markdown
Member

What this PR does / why we need it:

HasKubeadmConfig discarded every error returned by the API server and reported it as "the cluster does not have a kubeadm-config ConfigMap":

err := w.Client.Get(ctx, key, &corev1.ConfigMap{})
// TODO: Consider if this should only return false if the error is IsNotFound.
// TODO: Consider adding a third state of 'unknown' when there is an error retrieving the config map.
return err == nil, nil

A request timeout, an RBAC denial or a transient workload cluster apiserver outage was therefore indistinguishable from the ConfigMap genuinely being absent.

Both callers, in controlplane/kubeadm/reconcilers/kubeadmcontrolplane/status.go (in updateV1Beta1Status and updateStatus), do check the returned error:

hasKubeadmConfig, err := workloadCluster.HasKubeadmConfig(ctx)
if err != nil {
    return err
}

Since the error was always nil, those checks could never fire.

The practical effect is that a transient failure to reach the workload cluster made KCP conclude the control plane was not initialized, so it left status.initialization.controlPlaneInitialized and the Available condition unset and waited for the next resync instead of surfacing the failure and retrying. The error was also invisible to operators, since it was neither returned nor logged.

This addresses the first of the two pre-existing TODOs on that function. The second one, about introducing a third "unknown" state, is intentionally left alone as it would be an API change for WorkloadCluster.

Changes:

  • HasKubeadmConfig now maps only NotFound to false; any other error is returned to the caller, which makes the existing error handling in both callers effective.
  • Adds a test case covering a non-NotFound error, injected with interceptor.NewClient. The expectErr field already present in the test table was previously unused, so no case exercised the error path.

Which issue(s) this PR fixes:

Additional context:

Verified that the new test case fails against the current implementation on main and passes with the change, so it genuinely covers the fixed behaviour. make lint and the full ./controlplane/kubeadm/... suite pass.

/area control-plane

@kubernetes-prow kubernetes-prow Bot added the area/control-plane Issues or PRs related to control-plane lifecycle management label Aug 11, 2026
@kubernetes-prow
kubernetes-prow Bot requested review from g-gaston and sivchari August 11, 2026 20:51
@kubernetes-prow kubernetes-prow Bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 11, 2026
@linux-foundation-easycla

linux-foundation-easycla Bot commented Aug 11, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: mrueg / name: Manuel Rüger (6e53179)

@kubernetes-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubernetes-prow kubernetes-prow Bot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 11, 2026
HasKubeadmConfig discarded every error returned by the API server and
reported it as "the cluster does not have a kubeadm-config ConfigMap":

    err := w.Client.Get(ctx, key, &corev1.ConfigMap{})
    return err == nil, nil

A request timeout, an RBAC denial or a transient workload cluster
apiserver outage was therefore indistinguishable from the ConfigMap
genuinely being absent. Both callers in the KubeadmControlPlane status
reconciler check the returned error, so those checks could never fire.

As a result a transient failure to reach the workload cluster made KCP
conclude the control plane was not initialized, so it left
status.initialization.controlPlaneInitialized and the Available condition
unset and waited for the next resync rather than surfacing the failure
and retrying.

Only NotFound now maps to false; any other error is returned to the
caller, which makes the existing error handling in the two callers
effective.

Also adds a test case covering a non-NotFound error, using
interceptor.NewClient to inject it. The existing expectErr field in the
test table was previously unused.

Signed-off-by: Manuel Rüger <[email protected]>
@mrueg
mrueg force-pushed the fix/kcp-haskubeadmconfig-error-handling branch from f08ec86 to 6e53179 Compare August 11, 2026 20:57
@kubernetes-prow kubernetes-prow Bot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 11, 2026
@sbueringer

sbueringer commented Aug 12, 2026

Copy link
Copy Markdown
Member

Same comment as here: #13960

What is the issue with this code that we are suddenly getting PRs to change it?
I think it has been working exactly as is without issues for years

Please also consider https://www.kubernetes.dev/docs/guide/pull-requests/#ai-guidance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/control-plane Issues or PRs related to control-plane lifecycle management cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants