kubeadm: fix the bug that 'kubeadm init --dry-run --upload-certs' command failed with 'secret not found' error

[SataQiu]

What type of PR is this?

/kind bug

What this PR does / why we need it:

kubeadm: fix the bug that kubeadm init --dry-run --upload-certs command failed with secret not found error

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#2649

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SataQiu

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

The pull request process is described here

Needs approval from an approver in each of these files:

• cmd/kubeadm/OWNERS [SataQiu]

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

[SataQiu]

/test pull-kubernetes-e2e-kind-ipv6

[neolit123]

thanks for the PR @SataQiu

[neolit123]

we can fix this typo

Suggested change

	return nil, errors.Wrap(err, "error to get token reference")
	return nil, errors.Wrap(err, "error getting token reference")

[neolit123]

i think the better way to do this is modifying the "init getter".
this will result in no need to pass the isDryRun bool to functions in the stack.

here https://github.com/kubernetes/kubernetes/blob/f104ae885fb100278d9cbf2ec3a15b11f465872c/cmd/kubeadm/app/cmd/init.go#L533-L534
kubeadm constructs an init getter and dry run client.

inside the init getter there are some handlers:

kubernetes/cmd/kubeadm/app/util/apiclient/init_dryrun.go

Lines 61 to 64 in b0bc8ad

	idr.handleKubernetesService,
	idr.handleGetNode,
	idr.handleSystemNodesClusterRoleBinding,
	idr.handleGetBootstrapToken,

we can add a new handler for resource type "secret" called something like handleCopyCertsSecret
and in the handler we can make sure the secret name matches the prefix as defined here:

kubernetes/staging/src/k8s.io/cluster-bootstrap/token/util/helpers.go

Lines 109 to 111 in ea07644

	func BootstrapTokenSecretName(tokenID string) string {
	return fmt.Sprintf("%s%s", api.BootstrapTokenSecretPrefix, tokenID)
	}

and return a usable / blank secret object for dryrun.

i have not tested, but it should work..

[SataQiu]

Thank you for your patient explanation @neolit123

But according to the following code:

// handleGetBootstrapToken handles the case where kubeadm init creates the default token; and the token code GETs the
// bootstrap token secret first in order to check if it already exists
func (idr *InitDryRunGetter) handleGetBootstrapToken(action core.GetAction) (bool, runtime.Object, error) {
	if !strings.HasPrefix(action.GetName(), "bootstrap-token-") || action.GetNamespace() != metav1.NamespaceSystem || action.GetResource().Resource != "secrets" {
		// We can't handle this event
		return false, nil, nil
	}
	// We can safely return a NotFound error here as the code will just proceed normally and create the Bootstrap Token
	return true, nil, apierrors.NewNotFound(action.GetResource().GroupResource(), "secret not found")
}

It looks like the handler will always return a NotFound error whether or not the bootstrap token Secret has already been created. It's hard to decide when to return the NotFound error and when to return the fake Secret.

And we never actually store objects to the backend, because of :

kubernetes/cmd/kubeadm/app/util/apiclient/dryrunclient.go

Lines 162 to 166 in 0d46aee

	&core.SimpleReactor{
	Verb: "create",
	Resource: "*",
	Reaction: successfulModificationReactorFunc,
	},

kubernetes/cmd/kubeadm/app/util/apiclient/dryrunclient.go

Lines 199 to 206 in 0d46aee

	// successfulModificationReactorFunc is a no-op that just returns the POSTed/PUTed value if present; but does nothing to edit any backing data store.
	func successfulModificationReactorFunc(action core.Action) (bool, runtime.Object, error) {
	objAction, ok := action.(actionWithObject)
	if ok {
	return true, objAction.GetObject(), nil
	}
	return true, nil, nil
	}

Another approach might be to allow the bootstrap token Secret to be stored on the backend and then remove the handleGetBootstrapToken handler.

[SataQiu]

I tried the above approach. It also works. @neolit123

[SataQiu]

/test pull-kubernetes-e2e-kind

[neolit123]

/lgtm
i tested it locally and this does fix the problem. thanks.

it seems that it happens if users do kubeadm init --dry-run --upload-certs as well.
that is because --upload-certs executes into optional code.

i've logged this, because we need e2e tests for dry-run in kubeadm:
kubernetes/kubeadm#2653

in terms of the handlers in the dry-run client, i must admit i'm not very familiar with this area of the code, but i think they are a bit strange. maybe we can remove the handlers entirely...
for a dry-run client, i would expect it to always return some fake object...or allow an object to be pushed to the fake storage.

[k8s-triage-robot]

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

• The PR does have any do-not-merge/* labels
• The PR does not have the needs-ok-to-test label
• The PR is mergeable (does not have a needs-rebase label)
• The PR is approved (has cncf-cla: yes, lgtm, approved labels)
• The PR is failing tests required for merge

You can:

• Review the full test history for this PR
• Prevent this bot from retesting with /lgtm cancel or /hold
• Help make our tests less flaky by following our Flaky Tests Guide

/retest

[cpanato]

/triage accepted
/priority important-soon