Added support for APIServer using LB - #18459
Conversation
1621616 to
f7fee3c
Compare
Added support for APIServer without local Etcd Etcd is hosted on ControlPlane nodes. For GCP setup a LB and plumb it's address into /opt/kops/conf/kube_env.yaml Fix up /etc/hosts to point the etcd host to the LB address. Also added support for gs: storage buckets. Needed to use the hack/dev-build-gce.sh build. make gomod hack/update-expected.sh Fixed template for google storage copy & check Wired context up to make call stack more obvious Also provided better reuse of context object make gofmt make goimports Fixed "APIServer role forbidden on GCE with dns=None" test to reflect that use case should no longer generate an error Fixed unit test.
|
/test pull-kops-verify-terraform |
| }) | ||
| } | ||
|
|
||
| if b.HasAPIServer && !b.IsMaster { |
There was a problem hiding this comment.
Nit: we could probably just check whether b.BootConfig.EtcdLBAddress != "" - we're always trying to make nodeup "dumber" :-)
There was a problem hiding this comment.
Right now we're not writing different data to /opt/kops/conf/kube_env.yaml on different kinds of control plane/api server nodes. So I believe this field is currently going to be populated the same way on both Master and APIServer Nodes. I think writing etcd LB to /etc/hosts would work on both. However, I suspect its probably better to talk to the localhost etcd for master nodes rather than routing their traffic through the load balancer. (Though I guess there is a HA argument to be made for the latter)
| if cluster.GetCloudProvider() != kops.CloudProviderAWS && cluster.GetCloudProvider() != kops.CloudProviderGCE { | ||
| allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "role"), "APIServer role only supported on AWS and GCE")) | ||
| } | ||
| if cluster.UsesNoneDNS() { | ||
| // GCE now supports using an internal LB to resolve Etcd with topology.dns.type=None | ||
| if cluster.UsesNoneDNS() && cluster.GetCloudProvider() == kops.CloudProviderAWS { | ||
| allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "role"), "APIServer cannot be used with topology.dns.type=None")) | ||
| } |
There was a problem hiding this comment.
Nit: might be easier to refactor these into a switch statement (on GetCloudProvider), but not a blocker
There was a problem hiding this comment.
I think the logic is easier to read with your suggestion, so I 'switch'ed it.
| } | ||
| if !hasEtcd { | ||
| allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "metadata", "name"), fmt.Sprintf("InstanceGroup \"%s\" with role ControlPlane must have a member in etcd cluster \"%s\"", g.ObjectMeta.Name, etcd.Name))) | ||
| allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "metadata", "name"), fmt.Sprintf("InstanceGroup \"%s\" with role ControlPlane must have a member in etcd (IG: \"%s\") cluster \"%s\"", g.ObjectMeta.Name, last, etcd.Name))) |
There was a problem hiding this comment.
Do we need to log this? Obviously the two instance groups are confusing, but I'm not sure it's going to be easy to know what to do here. Did you hit this?
There was a problem hiding this comment.
I did hit this case. I was manually crafting the template and this helped my see what my misconfiguration was. I realize its a little annoying adding to the expected test output. However I think the extra data is helpful if you hit this case. Even if it just rules out getting the wrong IG.
| // NodeupConfigHash holds a secure hash of the nodeup.Config. | ||
| NodeupConfigHash string | ||
| // EtcdLBAddress holds the address of the load balancer to use if Etcd is not local. | ||
| EtcdLBAddress string `json:",omitempty"` |
There was a problem hiding this comment.
For api server, we use APIServerIPs (I think that's the closest equivalent). Should we name this EtcdIPs or similar? (I think I actually prefer EtcdAddresses or EtcdEndpoints, but I think what matters is that this probably should be a []string not a string)
There was a problem hiding this comment.
I agree with being consistent on IP vs Address or Endpoint. I also removed the 'LB' substring. Part of me suspects we my eventually want to track both actual Etcd IPs and the LB IP. However I can't come up with a case right now, so going with it. I do think for now we will to enforce EtcdIPs should be length 0 or 1.
| if ig.IsControlPlane() { | ||
| controlPlaneIGMs = append(controlPlaneIGMs, igm) | ||
| } else if ig.IsAPIServerOnly() { | ||
| requireEtcdLB = true |
There was a problem hiding this comment.
We could check for dns=none here, as I think this works with DNS?
But on the other hand, if DNS + APIServerOnly + GCE was previously blocked by validation, then we can just support our preferred approach (the load balancer)
But .... I don't think it was blocked, so we don't want to change the configuration of people that might be using this setup (unlikely as that might be). If this is behind a feature-flag we can change it, but ... there's no feature flag IIRC?
| } | ||
| c.AddTask(event_hc) | ||
| */ | ||
| bs := &gcetasks.BackendService{ |
There was a problem hiding this comment.
Wow, it took me way too long to understand what you were saying.
| } | ||
|
|
||
| if requireEtcdLB { | ||
| b.createEtcdInternalLB(c, controlPlaneIGMs) |
There was a problem hiding this comment.
We should check for error here
| if len(wellKnownAddresses[wellknownservices.EtcdMain]) > 0 { | ||
| bootConfig.EtcdLBAddress = strings.Join(wellKnownAddresses[wellknownservices.EtcdMain], ",") | ||
| } |
There was a problem hiding this comment.
Could we move this into BuildConfig? I think that's where we deal with other wellKnownAddresses (?)
| } | ||
|
|
||
| func (_ *LoadImageTask) RenderLocal(t *local.LocalTarget, a, e, changes *LoadImageTask) error { | ||
| // Not adding ctx to signature as RenderLocal seems to be part of a common interface |
There was a problem hiding this comment.
Exactly. I'll even go with soon but it seemed a bit large of a scope creep for this PR.
There was a problem hiding this comment.
I would like to try a parameterized interface when we do... replace some gnarly reflection.
|
Looks great, I think the only blocker is the naming of the EtcdLBAddress field in the BootConfig struct because that is actually a (private) API for nodeup, and is painful to change because of version skew |
One significant change was renaming EtcdLBAddresses as EtcdIPs. Cleanup k8s tf test file.
| case kops.CloudProviderGCE: | ||
| // Fully supported do nothing. | ||
| case kops.CloudProviderAWS: | ||
| // AWS only supports APIServer if DNS is set to None |
There was a problem hiding this comment.
Nit: not set to None, but no big deal
| if role == kops.InstanceGroupRoleAPIServer { | ||
| if len(wellKnownAddresses[wellknownservices.EtcdMain]) > 0 { | ||
| if len(wellKnownAddresses[wellknownservices.EtcdMain]) > 1 { | ||
| return nil, nil, fmt.Errorf("we currently do not support multiple Etcd IPs") |
There was a problem hiding this comment.
Not a blocker, but ... do we not?
There was a problem hiding this comment.
I don't have a CUJ and not sure if what we would try to do if there were multiple IPs is correct. I'd rather force a discussion so we can capture the CUJ and ensure we are behaving sanely. I'd rather not accidentally enshrine the current behavior for this case as correct.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
Added support for APIServer without local Etcd
Etcd is hosted on ControlPlane nodes.
For GCP setup a LB and plumb it's address into /opt/kops/conf/kube_env.yaml
Fix up /etc/hosts to point the etcd host to the LB address.