Allow changing AZ of masters #9017
Conversation
* Ensure every master runs etcd * Make it possible to remove masters * "Cross" Validate on IG creation
k8s-ci-robot
added
cncf-cla: yes
| } | ||
| } | ||
| if !hasEtcd { | ||
| allErrs = append(allErrs, field.NotFound(field.NewPath("spec", "etcdClusters").Key(etcd.Name), g.ObjectMeta.Name)) |
There was a problem hiding this comment.
| allErrs = append(allErrs, field.NotFound(field.NewPath("spec", "etcdClusters").Key(etcd.Name), g.ObjectMeta.Name)) | |
| allErrs = append(allErrs, field.NotFound(field.NewPath("spec", "etcdClusters").Key(k).Key("name"), etcd.Name, "has no member for master instancegroup " + g.ObjectMeta.Name)) |
where k is the index from the range over EtcdClusters
There was a problem hiding this comment.
Actually, the NotFound error is the other way around. It is the instancegroup's Name that isn't found in the etcdcluster's members. So it should be field.NewPath("objectMeta", "name")
There was a problem hiding this comment.
Thanks for spotting this.
NotFound unfortunately doesn't take a third description argument, so I was looking hard at how to make this more clear.
If I understand the NotFound description correctly, the error's field should point to the missing value, not the "cause" of the missing value (which may not even be a path). Since the validation happens as a part of a lookup in the etcdSpec, I found this direction to be correct.
Question is how to make this clear to users. The most programatically correct solution would be field.NotFound(field.NewPath("spec", "etcdClusters").Index(k).Key("instanceGroup") but not sure that is the most understandable for users as it won't contain the etcd cluster name.
There was a problem hiding this comment.
I changed the path to be more correct + added a deep validation test to see how this will look like to the end user.
There was a problem hiding this comment.
The value that's missing is the instancegroup name.
I think we need to use field.Forbidden here. I believe field.NotFound is for when there is an obvious list of possible choices for the field's value. field.NotFound on a path inside spec.etcdClusters would be appropriate for a validation that checks if an etcdCluster Member's InstanceGroup field value was actually a defined InstanceGroup (possibly restricting to instancegroups with role master).
There was a problem hiding this comment.
Would it not be reasonable for a five-master-IG cluster to have a Cilium etcd cluster with only three members? Perhaps not worth supporting that edge case, but maybe this restriction should only apply to the two etcd clusters used by apiserver?
There was a problem hiding this comment.
I think Forbidden is worth it just for the details string. I'll switch to that.
Having a different size for the cilium cluster could perhaps be interesting, but since we currently use the internal master DNS entry for this cluster, it won't work (or at least cilium will try to connect to members that don't exist). If someone really wants this, I can look into it later.
There was a problem hiding this comment.
@johngmyers Which field would you say should be the target here?
There is a similar case when creating an IG with a subnet that doesn't exist. Here we use NotFound, but the path is much simpler.
With Forbidden, it makes more sense to point to spec itself, but not sure that makes that much sense. What do you think?
There was a problem hiding this comment.
The field is field.NewPath("objectMeta", "name"), of the instancegroup. The detail would be something like instanceGroup "eu-central-1a" must have a member in etcdCluster "main".
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olemarkus, rifelpet 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 |
k8s-ci-robot
added
the
approved
|
/retest |
Fixes #8817
I have not done any testing on terraform though.
Care still needs to be taken when deleting master IGs. If you break quorum you need to restore etcd from backup. Preventing master IGs in an unsafe way is already possible though, so I didn't do anything to prevent this here.