🐛validation for not having both local and external etcd in control plane #2598

nader-ziada · 2020-03-09T17:34:43Z

What this PR does / why we need it:

add validation in webhook to check that only one of local and external etcd is set

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2580

vincepri

/approve
/assign @chuckha

vincepri · 2020-03-09T18:11:27Z

/milestone v0.3.0

k8s-ci-robot · 2020-03-09T18:11:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nader-ziada, vincepri

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:

~~OWNERS~~ [vincepri]

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

fabriziopandini · 2020-03-09T18:18:03Z

controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go

+		)
+	}
+
+	if in.Spec.KubeadmConfigSpec.InitConfiguration.Etcd.External != nil && in.Spec.KubeadmConfigSpec.InitConfiguration.Etcd.Local != nil {


not blocking for this PR, but the "real" kubeadm configuration/v1alpha3 does not have embedded ClusterConfiguration; wondering why we are doing this in the bootstrap provider...

This case can be removed as well as the first two cases will cover all the checks we need.

The only reason this check would ever get triggered is if someone first deletes the local configuration and then tries to update both local and external at the same time. I don't think we need to handle this edge case right now.

CecileRobertMichon

lgtm

chuckha

One minor change in semantics, but this is 90% of the way there

@fabriziopandini it's likely just an oversight. The details of kubeadm sometimes get lost when thinking too much about cluster api. If you have suggestions, we are all ears!

chuckha · 2020-03-09T18:52:53Z

controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook_test.go

@@ -293,6 +291,23 @@ func TestKubeadmControlPlaneValidateUpdate(t *testing.T) {
 	scaleToEvenExternalEtcdCluster := beforeExternalEtcdCluster.DeepCopy()
 	scaleToEvenExternalEtcdCluster.Spec.Replicas = pointer.Int32Ptr(2)

+	beforeInvalidEtcdCluster := before.DeepCopy()
+	beforeInvalidEtcdCluster.Spec.KubeadmConfigSpec.ClusterConfiguration = &kubeadmv1beta1.ClusterConfiguration{


This is testing a slightly different use case than what the issue describes. This test cases is for an object that is invalid to start with won't accept any updates.

Could you modify this test so that the before configuration has local and then the after is trying to add an external configuration?

chuckha · 2020-03-09T18:54:42Z

controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go

@@ -229,6 +231,30 @@ func (in *KubeadmControlPlane) validateCommon() (allErrs field.ErrorList) {
 	return allErrs
 }

+func (in *KubeadmControlPlane) validateEtcd() (allErrs field.ErrorList) {


This will need to accept prev as an argument to do a check between previous and new

actually, what exists should be sufficient, just the test would need updating

ok sorry for the back and forth haha, i'm thinking about various use cases.

But I've got an example that this should cover:

If an existing config defines local
then the user manually removes the local config and supplies an external config in the same transaction

the check that only looks at in's configuration is not sufficient and will need to see if previous had either local or external and ensure that the incoming structure does not define the opposite while removing the old one.

Does that make sense? That felt hard to explain so I'd be happy to hop on a call to explain further if necessary

@chuckha please take a look

nader-ziada · 2020-03-09T19:43:24Z

/test pull-cluster-api-capd-e2e

chuckha · 2020-03-09T19:44:18Z

controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go

+		)
+	}
+
+	if in.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.External != nil && in.Spec.KubeadmConfigSpec.ClusterConfiguration.Etcd.Local != nil {


This shouldn't reference the ClusterConfiguration, please see: https://github.com/kubernetes-sigs/cluster-api/pull/2605/files

This can be removed

ok, didn't see that other PR :D

chuckha

few minor updates

chuckha · 2020-03-09T19:49:29Z

controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go

+		)
+	}
+
+	if in.Spec.KubeadmConfigSpec.InitConfiguration.Etcd.External != nil && in.Spec.KubeadmConfigSpec.InitConfiguration.Etcd.Local != nil {


This case can be removed as well as the first two cases will cover all the checks we need.

The only reason this check would ever get triggered is if someone first deletes the local configuration and then tries to update both local and external at the same time. I don't think we need to handle this edge case right now.

chuckha · 2020-03-09T19:49:50Z

controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go

+		allErrs = append(
+			allErrs,
+			field.Forbidden(
+				field.NewPath("spec", "kubeadmConfigSpec", "clusterConfiguration", "etcd", "external"),


this should be local instead of external

chuckha · 2020-03-09T19:51:55Z

controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook_test.go

+			External: &kubeadmv1beta1.ExternalEtcd{
+				Endpoints: []string{"127.0.0.1"},
+			},
+			Local: &kubeadmv1beta1.LocalEtcd{


this local should be undefined in this struct. The incoming would only have the external defined and our validations should catch that there is already local defined and we're trying to also define an external configuration.

chuckha · 2020-03-09T20:03:25Z

controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_webhook.go

+		allErrs = append(
+			allErrs,
+			field.Forbidden(
+				field.NewPath("spec", "kubeadmConfigSpec", "initConfiguration", "etcd", "local"),


Ah, this one should be "external" instead of "local".

The reason is that this field is the thing that gets bubbled up to the user and the field on the in type is the one that the user tried to change. So if they change external but then we tell them local is forbidden that's a pretty confusing error message

makes sense

chuckha · 2020-03-09T20:28:17Z

That failure is a known flake, please see #2593 for more information.

This looks good, thanks

/lgtm

nader-ziada · 2020-03-09T20:29:19Z

/test pull-cluster-api-capd-e2e

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 9, 2020

k8s-ci-robot requested review from CecileRobertMichon and ncdc March 9, 2020 17:35

k8s-ci-robot assigned chuckha Mar 9, 2020

vincepri approved these changes Mar 9, 2020

View reviewed changes

k8s-ci-robot added this to the v0.3.0 milestone Mar 9, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2020

fabriziopandini reviewed Mar 9, 2020

View reviewed changes

CecileRobertMichon approved these changes Mar 9, 2020

View reviewed changes

chuckha reviewed Mar 9, 2020

View reviewed changes

nader-ziada force-pushed the etcd branch from 70bef15 to e2705aa Compare March 9, 2020 19:33

chuckha reviewed Mar 9, 2020

View reviewed changes

nader-ziada force-pushed the etcd branch from e2705aa to f1b2729 Compare March 9, 2020 19:59

chuckha reviewed Mar 9, 2020

View reviewed changes

validation for not having both local and external etcd in kcp

d765b94

nader-ziada force-pushed the etcd branch from f1b2729 to d765b94 Compare March 9, 2020 20:07

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2020

k8s-ci-robot merged commit 96ccd37 into kubernetes-sigs:master Mar 9, 2020

nader-ziada deleted the etcd branch April 3, 2020 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛validation for not having both local and external etcd in control plane #2598

🐛validation for not having both local and external etcd in control plane #2598

nader-ziada commented Mar 9, 2020

vincepri left a comment

vincepri commented Mar 9, 2020

k8s-ci-robot commented Mar 9, 2020

fabriziopandini Mar 9, 2020 •

edited

Loading

vincepri Mar 9, 2020

chuckha Mar 9, 2020

CecileRobertMichon left a comment

chuckha left a comment

chuckha Mar 9, 2020

chuckha Mar 9, 2020

chuckha Mar 9, 2020

chuckha Mar 9, 2020 •

edited

Loading

nader-ziada Mar 9, 2020

nader-ziada commented Mar 9, 2020

chuckha Mar 9, 2020 •

edited

Loading

nader-ziada Mar 9, 2020

chuckha left a comment

chuckha Mar 9, 2020

chuckha Mar 9, 2020

chuckha Mar 9, 2020

chuckha Mar 9, 2020

nader-ziada Mar 9, 2020

chuckha commented Mar 9, 2020

nader-ziada commented Mar 9, 2020

🐛validation for not having both local and external etcd in control plane #2598

🐛validation for not having both local and external etcd in control plane #2598

Conversation

nader-ziada commented Mar 9, 2020

vincepri left a comment

Choose a reason for hiding this comment

vincepri commented Mar 9, 2020

k8s-ci-robot commented Mar 9, 2020

fabriziopandini Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

chuckha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckha Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada commented Mar 9, 2020

chuckha Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckha commented Mar 9, 2020

nader-ziada commented Mar 9, 2020

fabriziopandini Mar 9, 2020 •

edited

Loading

chuckha Mar 9, 2020 •

edited

Loading

chuckha Mar 9, 2020 •

edited

Loading