New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kubeadm: Split discovery from JoinConfiguration #67763

Merged
merged 1 commit into from Oct 17, 2018

Conversation

@rosti
Contributor

rosti commented Aug 23, 2018

What this PR does / why we need it:

This change splits out discovery fields from JoinConfiguration by performing
the following changes:

  • Introduce a BootstrapTokenDiscovery structure, that houses configuration
    options needed for bootstrap token based discovery.

  • Introduce a FileDiscovery structure, that houses configuration options
    (currently only a single option) needed for KubeConfig based discovery.

  • Introduce a Discovery structure, that houses common options (such as
    discovery timeout and TLS bootstrap token) as well as pointer to an instance
    of either BootstrapTokenDiscovery or FileDiscovery structures.

  • Replace the old discovery related JoinConfiguration members with a single
    Discovery member.

This change is required in order to cleanup the code of unnecessary logic and
make the serialized JoinConfiguration more structured (and therefore, more
intuitive).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
refs kubernetes/kubeadm#911, refs kubernetes/kubeadm#963

Special notes for your reviewer:

This is another one of those big and somewhat hard to review PRs, that are the stepping stones in the path to v1beta1.

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/kind api-change
/kind enhancement
/assign @luxas
/assign @timothysc
/assign @fabriziopandini

Release note:

kubeadm: JoinConfiguration now houses the discovery options in a nested Discovery structure, which in turn has a couple of other nested structures to house more specific options (BootstrapTokenDiscovery and FileDiscovery)
@dixudx

For current v1alpha3, I'd prefer not to update the json structure. Use inline instead.

We could update that in next coming v1beta1.

if len(fd.KubeConfigPath) != 0 {
cfg.Discovery.File = fd
cfg.Discovery.BootstrapToken = nil

This comment has been minimized.

@dixudx

dixudx Aug 23, 2018

Member

Default will be nil.
Remove it.

cfg.Discovery.BootstrapToken = nil
} else {
cfg.Discovery.BootstrapToken = btd
cfg.Discovery.File = nil

This comment has been minimized.

@dixudx

dixudx Aug 23, 2018

Member

Ditto.

nodeRegistration:
criSocket: relativepath
name: NODE-1
token: invalidtoken
name: NODE-1

This comment has been minimized.

@dixudx

dixudx Aug 23, 2018

Member

Missing newline?

- kube-apiserver:6443
DiscoveryTokenCACertHashes: null
DiscoveryTokenUnsafeSkipCAVerification: true
Discovery:

This comment has been minimized.

@dixudx

dixudx Aug 23, 2018

Member

This will break old configurations.

For coming v1beta1, we cound use a new section like this.

But for current v1alpha3, shall we use below instead in types.go,

Discovery Discovery `json:",inline"`

This comment has been minimized.

@rosti

rosti Aug 23, 2018

Contributor

Actually, it won't, since v1alpha3 is also unreleased and is massively different than v1alpha2 of 1.11.
I am also not a fan of inlining it, because that way the context of the options in Discovery is gone.

discoveryTokenAPIServers:
- kube-apiserver:6443
discoveryTokenUnsafeSkipCAVerification: true
discovery:

This comment has been minimized.

@dixudx

dixudx Aug 23, 2018

Member

Ditto for those following yaml files.

Better not change the struction of current released v1alpha3. We could do this in coming v1beta1 instead.

@rosti

This comment has been minimized.

Contributor

rosti commented Aug 23, 2018

For current v1alpha3, I'd prefer not to update the json structure. Use inline instead.

We could update that in next coming v1beta1.

This looks like a double work for me and I am willing to actually hold of this PR for v1beta1 if we are going to split it into user and back facing interface changes.
I am not a big fan of inlining the Discovery structure either - it gives us more context that way and makes the config file more structured.
The fact is that this change is literally tiny compared to the rest if the changes we have done in v1alpha3. I don't believe that a user facing interface change here is going to break anyone, since v1alpha3 is unreleased.

@timothysc

/hold

please coordinate this with @fabriziopandini there are overlapping issues right now.

@timothysc timothysc added this to the 1.13 milestone Sep 5, 2018

@liggitt liggitt modified the milestones: 1.13, v1.13 Sep 5, 2018

@rosti

This comment has been minimized.

Contributor

rosti commented Oct 8, 2018

Rebased the PR on v1beta1

@fabriziopandini @timothysc PTAL

@timothysc

This comment has been minimized.

Member

timothysc commented Oct 9, 2018

@rosti bot says it still needs a rebase.
I'd like to chat about config changes tomorrow during office hours too.

@timothysc

Given that you and @fabriziopandini have been working on this one I'll defer to him on lgtm

/approve
/assign @fabriziopandini

@fabriziopandini

@rosti thank you for this PR and +100 for keeping this going across two cycles!

Overall this PR looks fine, but the things that confused a little bit me is the move from tree token fields (discoveryToken, tlsBootstrapToken and token to override the former two) to two token fields. Some other minors inline

// TLSBootstrapToken is a token used for TLS bootstrapping.
// Defaults to Token.
// If .BootstrapToken is set, this field is defaulted to .BootstrapToken.Token, but can be overridden.
// If .File is set, this field **must be set** in case the KubeConfigFile does not contain any other authentication information

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 11, 2018

Contributor

Wandering if it makes more sense to have two TLS bootstrap tokens, one inside BootstrapToken and one inside File.
WDYT?

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

Ok according to original Lucas design

// FileDiscovery is used to specify a file or URL to a kubeconfig file from which to load cluster information
type FileDiscovery struct {
// KubeConfigPath is used to specify the actual file path or URL to the kubeconfig file from which to load cluster information
KubeConfigPath string

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 11, 2018

Contributor

I'm not fully convinced by calling this path, but I don't have better suggestion ATM...

This comment has been minimized.

@rosti

rosti Oct 12, 2018

Contributor

How about KubeConfigFile? Yet it sounds kind of the same to me.

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

Leave as it is ATM

}
// BootstrapTokenDiscovery is used to set the options for bootstrap token based discovery
type BootstrapTokenDiscovery struct {

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 11, 2018

Contributor

If I'm not wrong we are missing DiscoveryToken (unless it is intentional to conflate Token and DiscoveryToken)

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

Ok according to original Lucas design

return nil
}
func Convert_kubeadm_JoinConfiguration_To_v1alpha3_JoinConfiguration(in *kubeadm.JoinConfiguration, out *JoinConfiguration, s conversion.Scope) error {

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 11, 2018

Contributor

if I'm not wrong out.token is missing...

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

ok according to original Lucas design/the semantic of --token acting as a proxy for TLSBootstrapToken/DiscoveryToken

@@ -152,6 +136,34 @@ func SetDefaults_NodeRegistrationOptions(obj *NodeRegistrationOptions) {
}
}
// SetDefaults_Discovery assigns default values for the discovery process
func SetDefaults_Discovery(obj *Discovery) {

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 11, 2018

Contributor

DiscoveryToken is missing also here

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

ok according to original Lucas design

// APIServers is a set of IPs to API servers from which info
// will be fetched. Currently we only pay attention to one API server but
// hope to support >1 in the future.
APIServerEndpoints []string

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 11, 2018

Contributor

Wandering if we should make this string instead of accepting array of string and throwing an error in validation if more than one string is passed

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

Please make this string; we are not accepting more than one value ATM and there is no plan for supporting >1 API Endpoints as far as I know

This comment has been minimized.

@rosti

rosti Oct 15, 2018

Contributor

This will come in as a followup PR as the change is not trivial (currently there is a lot of code in kubeadm, that looks upon this as a []string and doing correct handling of multiple API Servers).

}
// ValidateFileDiscovery validates file discovery configuration
func ValidateFileDiscovery(f *kubeadm.FileDiscovery, fldPath *field.Path) field.ErrorList {

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 11, 2018

Contributor

might be ValidateDiscoveryFile?

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

Please rename to ValidateDiscoveryFile

}
allErrs = append(allErrs, ValidateToken(b.Token, fldPath.Child("token"))...)
allErrs = append(allErrs, ValidateJoinDiscoveryTokenAPIServer(b.APIServerEndpoints, fldPath.Child("apiServerEndpoints"))...)

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 11, 2018

Contributor

this is out of the scope of this PR, but it would be great to rename also ValidateJoinDiscoveryTokenAPIServer into ValidateDiscoveryTokenAPIServer

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

kindly rename ValidateJoinDiscoveryTokenAPIServer into ValidateDiscoveryTokenAPIServerfor consistency

@@ -159,6 +159,10 @@ func NewCmdJoin(out io.Writer) *cobra.Command {
cfg := &kubeadmapiv1beta1.JoinConfiguration{}
kubeadmscheme.Scheme.Default(cfg)
fd := &kubeadmapiv1beta1.FileDiscovery{}

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 11, 2018

Contributor

I think to understand why you are creating separated object instead of working on cfg, but I'm wondering if we can do the the other way around: use cfg and then clean up cfg.Discovery.File or cfg.Discovery.BootstrapToken if corresponding flags/args are not set

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

I'm not a fan of this solution but we can leave with it ATM

"For token-based discovery, allow joining without --discovery-token-ca-cert-hash pinning.")
flagSet.StringVar(
&cfg.Token, "token", "",
token, "token", "",

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 11, 2018

Contributor

This is a little bit confusing.
In the config we now there is BootstrapToken.Token and TLSBootstrapToken but not Token
here there is Token, TLSBootstrapToken but not discovery-token anymore

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

Please fix the flag documentation explaining the semantics of this flag

@rosti

This comment has been minimized.

Contributor

rosti commented Oct 12, 2018

@fabriziopandini the change is along the lines of the original proposal by Lucas, which I find the most sensible solution here.
The TLSBootstrapToken is moved to the common Discovery struct and is therefore shared by both discovery methods. The DiscoveryToken field is replaced by BootstrapToken.Token. The Token field, whose purpose in v1alpha3 was to supply a common value to use for TLSBootstrapToken and DiscoveryToken, when no value was supplied for some of them, was removed. You can still specify the --token switch on command line to supply a single value to use for both TLSBootstrapToken and DiscoveryToken. Also, v1alpha3 to internal config is making sure to do the correct conversion too.

P.S.: On second though, I got the semantics for --token a bit different than what it used to be, so I'll change fix them in a bit. They should not be overwritten by --token if they are not empty.

@rosti

This comment has been minimized.

Contributor

rosti commented Oct 12, 2018

Fixed the --token flag semantics (of kubeadm join).

@fabriziopandini

@rosti
/approve
Please fix minors still pending and then ready for lgtm

After your explanation I added a note on all my previous comments that does not apply anymore, and make it clear where IMO you should fix minors

// APIServers is a set of IPs to API servers from which info
// will be fetched. Currently we only pay attention to one API server but
// hope to support >1 in the future.
APIServerEndpoints []string

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

Please make this string; we are not accepting more than one value ATM and there is no plan for supporting >1 API Endpoints as far as I know

// ValidateArgSelection validates discovery related configuration and collects all encountered errors
func ValidateArgSelection(cfg *kubeadm.JoinConfiguration, fldPath *field.Path) field.ErrorList {
// ValidateBootstrapTokenDiscovery validates bootstrap token discovery configuration
func ValidateBootstrapTokenDiscovery(b *kubeadm.BootstrapTokenDiscovery, fldPath *field.Path) field.ErrorList {

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

Please rename to ValidateDiscoveryBootstrapToken

}
// TODO remove once we support multiple api servers
if len(cfg.DiscoveryTokenAPIServers) > 1 {
if len(b.APIServerEndpoints) > 1 {

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

This should go away when APIServerEndpoints changes from []string to string

}
allErrs = append(allErrs, ValidateToken(b.Token, fldPath.Child("token"))...)
allErrs = append(allErrs, ValidateJoinDiscoveryTokenAPIServer(b.APIServerEndpoints, fldPath.Child("apiServerEndpoints"))...)

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

kindly rename ValidateJoinDiscoveryTokenAPIServer into ValidateDiscoveryTokenAPIServerfor consistency

}
// ValidateFileDiscovery validates file discovery configuration
func ValidateFileDiscovery(f *kubeadm.FileDiscovery, fldPath *field.Path) field.ErrorList {

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

Please rename to ValidateDiscoveryFile

"For token-based discovery, allow joining without --discovery-token-ca-cert-hash pinning.")
flagSet.StringVar(
&cfg.Token, "token", "",
token, "token", "",

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 14, 2018

Contributor

Please fix the flag documentation explaining the semantics of this flag

@fabriziopandini

This comment has been minimized.

Contributor

fabriziopandini commented Oct 16, 2018

/approve
/lgtm

/test pull-kubernetes-e2e-gce

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 16, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, rosti, timothysc

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:

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

@rosti

This comment has been minimized.

Contributor

rosti commented Oct 16, 2018

A few minor details to address in a couple of hours and this will be ready to go.

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold and removed lgtm labels Oct 16, 2018

kubeadm: Split discovery from JoinConfiguration
This change splits out discovery fields from JoinConfiguration by performing
the following changes:

- Introduce a BootstrapTokenDiscovery structure, that houses configuration
  options needed for bootstrap token based discovery.

- Introduce a FileDiscovery structure, that houses configuration options
  (currently only a single option) needed for KubeConfig based discovery.

- Introduce a Discovery structure, that houses common options (such as
  discovery timeout and TLS bootstrap token) as well as pointer to an instance
  of either BootstrapTokenDiscovery or FileDiscovery structures.

- Replace the old discovery related JoinConfiguration members with a single
  Discovery member.

This change is required in order to cleanup the code of unnecessary logic and
make the serialized JoinConfiguration more structured (and therefore, more
intuitive).

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@rosti

This comment has been minimized.

Contributor

rosti commented Oct 16, 2018

@fabriziopandini should be ready now for re-lgtm.

/hold cancel

@fabriziopandini

This comment has been minimized.

Contributor

fabriziopandini commented Oct 16, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 16, 2018

@fabriziopandini

This comment has been minimized.

Contributor

fabriziopandini commented Oct 16, 2018

/test pull-kubernetes-kubemark-e2e-gce-big

@fejta-bot

This comment has been minimized.

fejta-bot commented Oct 16, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

fejta-bot commented Oct 17, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 1e4ad04 into kubernetes:master Oct 17, 2018

18 checks passed

cla/linuxfoundation rosti authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment