Skip to content
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: Remove ClusterConfiguration from InitConfiguration in v1beta2 #77739

Merged
merged 1 commit into from May 29, 2019

Conversation

@rosti
Copy link
Member

commented May 10, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Ever since v1alpha3, InitConfiguration is containing ClusterConfiguration embedded in it. This was done to mimic the internal InitConfiguration, which in turn is used throughout the kubeadm code base as if it is the old MasterConfiguration of v1alpha2.

This, however, is confusing to users who vendor in kubeadm as the embedded ClusterConfiguration inside InitConfiguration is not marshaled to YAML.
For this to happen, special care must be taken for the ClusterConfiguration field to marshaled separately.

Thus, to make things smooth for users and to reduce third party exposure to technical debt, this change removes ClusterConfiguration embedding from InitConfiguration.

Doing this would also mean, that the internal config types refactoring can be now decoupled from the v1beta2 config release schedule.

Which issue(s) this PR fixes:

Refs kubernetes/enhancements#970 kubernetes/kubeadm#1439

Special notes for your reviewer:

Most of this deals with extracting ClusterConfiguration from InitConfiguration in kubeadm argument passing and test code.

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/priority important-longterm
/assign @fabriziopandini
/assign @timothysc
/assign @neolit123

Does this PR introduce a user-facing change?:

kubeadm: v1beta2 InitConfiguration no longer embeds ClusterConfiguration it it.
@neolit123
Copy link
Member

left a comment

thanks @rosti
should we also clean this as part of this PR:
kubernetes/kubeadm#1431
(possibly delete these tests?)

^ cc @fabriziopandini
xref: https://github.com/kubernetes/kubernetes/pull/74797/files

})
}
clusterCfg := &kubeadmapiv1beta2.ClusterConfiguration{
KubernetesVersion: fmt.Sprintf("v1.%d.0", constants.MinimumControlPlaneVersion.Minor()+1),

This comment has been minimized.

clusterCfg := &kubeadmapiv1beta2.ClusterConfiguration{
// KubernetesVersion is not used, but we set this explicitly to avoid
// the lookup of the version from the internet when executing LoadOrDefaultInitConfiguration
KubernetesVersion: constants.MinimumControlPlaneVersion.String(),

This comment has been minimized.

@rosti rosti force-pushed the rosti:initclustersplit-v1beta2 branch from 5bac619 to 8e60963 May 10, 2019

@rosti

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

should we also clean this as part of this PR:
kubernetes/kubeadm#1431
(possibly delete these tests?)

This is outside of the scope of this PR. But I do envision a test grooming PR for the config tests after all the v1beta2 changes are merged. That can be part of it.

@rosti rosti force-pushed the rosti:initclustersplit-v1beta2 branch from 8e60963 to e3263f4 May 13, 2019

@fabriziopandini
Copy link
Contributor

left a comment

@rosti thanks for this PR!
Just for confirmation, are you planning to change the internal API as well in as second stage?
I ask this because the disalignement introduced by this PR leads to some weird semantics that could create confusion in the long term e.g.

 internalcfg, err := configutil.DefaultedInitConfiguration(initCfg, clusterCfg)
cmd.Flags().StringVar(&cfg.CertificatesDir, options.CertificatesDir, cfg.CertificatesDir, "The path where certificates are stored")
cmd.Flags().StringVar(&cfg.LocalAPIEndpoint.AdvertiseAddress, options.APIServerAdvertiseAddress, cfg.LocalAPIEndpoint.AdvertiseAddress, "The IP address the API server is accessible on")
cmd.Flags().Int32Var(&cfg.LocalAPIEndpoint.BindPort, options.APIServerBindPort, cfg.LocalAPIEndpoint.BindPort, "The port the API server is accessible on")
cmd.Flags().StringVar(&clusterCfg.CertificatesDir, options.CertificatesDir, clusterCfg.CertificatesDir, "The path where certificates are stored")

This comment has been minimized.

Copy link
@fabriziopandini

fabriziopandini May 13, 2019

Contributor

nit
It would be great if we can split flags in groups (init configuration flags, cluster flags, command configuration specific flags)

This comment has been minimized.

Copy link
@timothysc

timothysc May 13, 2019

Member

I'd hope we get that for free in the ComponentConfig Machinery in the long haul.
/cc @luxas

This comment has been minimized.

Copy link
@rosti

rosti May 14, 2019

Author Member

@fabriziopandini fixed
@timothysc that's one of the main goals of the component standard work ATM. There is even a proof of concept design on how to do that in #73494

@timothysc
Copy link
Member

left a comment

Nothing stands out on initial inspection, are there any cases we can think about where we would want initconfig to force a change in clusterconfig? I'd need to think about it.

cmd.Flags().StringVar(&cfg.CertificatesDir, options.CertificatesDir, cfg.CertificatesDir, "The path where certificates are stored")
cmd.Flags().StringVar(&cfg.LocalAPIEndpoint.AdvertiseAddress, options.APIServerAdvertiseAddress, cfg.LocalAPIEndpoint.AdvertiseAddress, "The IP address the API server is accessible on")
cmd.Flags().Int32Var(&cfg.LocalAPIEndpoint.BindPort, options.APIServerBindPort, cfg.LocalAPIEndpoint.BindPort, "The port the API server is accessible on")
cmd.Flags().StringVar(&clusterCfg.CertificatesDir, options.CertificatesDir, clusterCfg.CertificatesDir, "The path where certificates are stored")

This comment has been minimized.

Copy link
@timothysc

timothysc May 13, 2019

Member

I'd hope we get that for free in the ComponentConfig Machinery in the long haul.
/cc @luxas

@k8s-ci-robot k8s-ci-robot requested a review from luxas May 13, 2019

@rosti rosti force-pushed the rosti:initclustersplit-v1beta2 branch from e3263f4 to a88ddf0 May 14, 2019

@rosti

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

Just for confirmation, are you planning to change the internal API as well in as second stage?

@fabriziopandini indeed that's the case. The purpose for this PR is to allow v1beta2 release with 1.15 and that to be free from technical debt as much as possible. This will allow us to tackle the internal config in the next release cycle(s) via a series of consecutive PRs. Changes to the API will be made as part of this followup work and it will look way better.

@rosti

This comment has been minimized.

Copy link
Member Author

commented May 14, 2019

Nothing stands out on initial inspection, are there any cases we can think about where we would want initconfig to force a change in clusterconfig? I'd need to think about it.

@timothysc There is only one thing, that I can think of. ClusterConfiguration.ControlPlaneEndpoint can depend on InitConfiguration.LocalAPIEndpoint ATM. See here.

@rosti rosti force-pushed the rosti:initclustersplit-v1beta2 branch from a88ddf0 to 45dd8f3 May 14, 2019

@timothysc

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@timothysc There is only one thing, that I can think of. ClusterConfiguration.ControlPlaneEndpoint can depend on InitConfiguration.LocalAPIEndpoint ATM. See here.

Blarg, can we truly separate then?

@fabriziopandini

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@rosti

ClusterConfiguration.ControlPlaneEndpoint can depend on InitConfiguration.LocalAPIEndpoint ATM. See here.

We can re-think this as a simplification of the single control-plane plane scenario: controlPlaneEndpoint, if empty, get initialised with local API endpoint.

Shifting from the current behaviour to a simple "defaulting" rule will remove the dependency and probably also cleanup some code
wdyt?

More generally speaking, controlPlaneEndpoint (always one) and local API endpoints (potentially many) were splitted intentionally

@rosti

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

@timothysc @fabriziopandini yes, we can simplify things there. We need to dynamically default ControlPlaneEndpoint to the LocalAPIEndpoint even for the single control plane case.
We also have to ensure that we do the same on upgrade too.

However, this is outside of the scope of this PR. I can do a followup today or tomorrow.

@rosti

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

@timothysc @fabriziopandini after looking at this I reached the following conclusions:

  1. We should consider only GetControlPlaneEndpoint as it's the only place where ControlPlaneEndpoint and LocalAPIEndpoint depend on each other.
  2. GetControlPlaneEndpoint is (almost) always used in a context where we are joining a control plane or have a control plane. This implies, that LocalAPIEndpoint is always present, either from Init or Join configs or reconstructed from the config map.
  3. Only kubeadm alpha kubeconfig user uses GetControlPlaneEndpoint when not having a control plane, but there the usage is sort of a hack to get a control plane URL from the LocalAPIEndpoint (supplied at the command line).

At this point I think, that it's safe to leave things as they are. If we need we can address this later on.

Either way, the steps to get always something inside ControlPlaneEndpoint are the following:

  • Make sure that this field is always filled in during kubeadm init - even for the single control plane case. The existing logic in GetControlPlaneEndpoint can be used for that. This needs to be done in the early stages of init, before any phases are executed and, preferably, just after the config is loaded.
  • Make sure that the init upload-config phase does the same thing (in case it's called standalone).
  • Make sure that upon upgrade this field is filled in. If it's empty, then the usual case is a single control plane cluster so filling it in won't be a big deal.
  • Make sure that kubeadm config upload functionality will require complicating a few things to ensure a proper ControlPlaneEndpoint is uploaded.
  • Replace GetControlPlaneEndpoint with GetControlPlaneURL that uses ClusterConfiguration.ControlPlaneEndpoint, performs a bunch of checks on it and returns an HTTPS URL.
  • Add a check to config upload APIs.
  • Some tests and lots of testing.

One strange thing, that I noticed in GetControlPlaneEndpoint, is that if ControlPlaneEndpoint has only a host name, but not a port in it, then the port is taken from the LocalAPIEndpoint. This means, that one can override the port number of the API server gateway on a per-control-plane basis. This case is not covered by the above steps as they require fixed port number in GetControlPlaneEndpoint or for the port number to be defaulted. I cannot think of a valid case where such port redirection would be useful for someone.

Anyway, it's late in the cycle, so I think, that we are safe with doing nothing. I have some partial code implementing the above schema, but let's keep it shelved for now and only proceed with this if there is no other option.

@rosti rosti force-pushed the rosti:initclustersplit-v1beta2 branch from 45dd8f3 to 7ee74b4 May 16, 2019

@rosti

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

/test pull-kubernetes-e2e-gce-device-plugin-gpu

@fabriziopandini

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@rosti thanks for the detailed info
+1 for me on going on without changes and logging an issue for executing on "getting always something in controlPlaneAddres" next cycle

I noticed in GetControlPlaneEndpoint, is that if ControlPlaneEndpoint has only a hostname, but not a port in it

this is something that probably should be fixed as well

@neolit123
Copy link
Member

left a comment

/lgtm
/hold

@fabriziopandini
Copy link
Contributor

left a comment

@rosti, Great job!
/approve
/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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.

Copy link
Member Author

commented May 28, 2019

/hold cancel

@fejta-bot

This comment has been minimized.

Copy link

commented May 28, 2019

/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 or /hold comment for consistent failures.

@neolit123

This comment has been minimized.

@fejta-bot

This comment has been minimized.

Copy link

commented May 29, 2019

/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 or /hold comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

commented May 29, 2019

/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 or /hold comment for consistent failures.

kubeadm: Remove ClusterConfiguration from InitConfiguration in v1beta2
Ever since v1alpha3, InitConfiguration is containing ClusterConfiguration
embedded in it. This was done to mimic the internal InitConfiguration, which in
turn is used throughout the kubeadm code base as if it is the old
MasterConfiguration of v1alpha2.

This, however, is confusing to users who vendor in kubeadm as the embedded
ClusterConfiguration inside InitConfiguration is not marshalled to YAML.
For this to happen, special care must be taken for the ClusterConfiguration
field to marshalled separately.

Thus, to make things smooth for users and to reduce third party exposure to
technical debt, this change removes ClusterConfiguration embedding from
InitConfiguration.

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

@rosti rosti force-pushed the rosti:initclustersplit-v1beta2 branch from 7ee74b4 to 5671ea9 May 29, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 29, 2019

@fabriziopandini
Copy link
Contributor

left a comment

Thanks @rosti
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 29, 2019

@k8s-ci-robot k8s-ci-robot merged commit 6a0db7b into kubernetes:master May 29, 2019

20 of 21 checks passed

pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation rosti authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@rosti rosti referenced this pull request Jun 10, 2019

Closed

v1beta2 config #1439

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.