[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/hold for comment

Looks like the Terraform provider needs to consistently sort the listeners on an ELB in order for the integration tests to not flake.

/lgtm

/lgtm

We might need to go back to the drawing board on this one :( I finally had a chance to test it out:

error running task "LoadBalancer/api.foo.k8s.local" (9m54s remaining to succeed): error creating LoadBalancerListeners: InvalidConfigurationRequest: Listeners can't talk to InstancePort 443 with secure and insecure protocols at the same time

Might be worth an AWS ticket to let them know of the use case, but that probably won't help us.

What if also open port 8443 on host?

Opening a second port on the instances would require we forward traffic on that port to apiserver. I suppose the apiserver health check container is performing a similar function right now so we could extend it and give it a more generic name. That will vastly increase the scope of this change though, so it'd be nice to get agreement on that prior to implementation.

New changes are detected. LGTM label has been removed.

Ok I tried to use a second hostPort in the kube-apiserver pod spec and pointed the second ELB listener at the second port but that didn't work because hostNetwork: true requires matching containerPort and hostPort (of course)

Aug 20 21:14:54 ip-XX-XX-XX-XX kubelet[6631]: E0820 21:14:54.409160 6631 file.go:187] Can't process manifest file "/etc/kubernetes/manifests/kube-apiserver.manifest": invalid pod: [spec.containers[0].ports[1].containerPort: Invalid value: 443: must match hostPortwhenhostNetwork is true]

So it seems like if we want to keep pursuing the second listener option we'd need to have something listening on a second instance port (hijacking the healthcheck container is sounding better and better...)

Beyond that, I did notice when upgrading an existing cluster, we'll need to remind users not to use update cluster --admin before rolling-update cluster or else the generated kubeconfig file will be pointing to the new port which wont work until at least one master has been replaced, causing rolling-update to fail. Therefor we should recommend kops update cluster --yes; kops rolling-update cluster --yes; kops update cluster --yes --admin

I wonder if NLBs don't have the duplicate instancePort limitation that ELBs do. We have an open PR to add NLB support, maybe it's worth focusing on getting that merged for 1.19. That would be much more elegant than our other proposals.

I'll test an NLB tomorrow unless someone else gets a chance first.

One note on NLBs, in case you consider their use for from the control plane components as well: They only support hairpin connections from EC2 instances registered as targets by their IP address, and not by their instance ID. That precludes registering the instances automatically by virtue of their management by an ASG, as that registration method registers them by instance ID.

Instead, you have register and deregister the EC2 instances when they boot or shut down (such as via systemd units), but of course it's possible for the machines to die without the deregistration procedure running. A controller running elsewhere would be more resilient. though more complicated.

@rifelpet: The following tests failed, say /retest to rerun all failed tests:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Superseded by #10157
/close

@rifelpet: PR needs rebase.

@rifelpet: Closed this PR.