-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add support for loadbalancer interface #11
Add support for loadbalancer interface #11
Conversation
The current way that the relations are set up for CK, with kubeapi-load-balancer sitting in between the master and workers, is unnecessarily convoluted, and leads to mistakes due to the relations not being set up properly since they have to be done differently depending on whether or not kubeapi-load-balancer is being used. This change makes this charm support the `loadbalancer` interface and depends on the master to forward the LB address via the `kube-control` relation. This means that there's only one relation between the workers and master, the endpoint address is always handed off in the same way, and the relations are consistent regardless of whether the API LB charm is being used or if another provider (such as a cloud integrator charm) is providing the LB. Part of [lp:1897818][] [lp:1897818]: https://bugs.launchpad.net/charmed-kubernetes-testing/+bug/1897818
This adds support for the `loadbalancer` interface so that cloud-native LBs can be provided by the integrator charms. Additionally, it simplifies the confusing way the relations between the masters and workers change depending on whether kubeapi-load-balancer is being used or not by making that use the same `lb-provider` endpoint and always forwarding the API endpoint URLs via the `kube-control` relation. Part of [lp:1897818][] Depends on: * juju-solutions/loadbalancer-interface#13 * juju-solutions/interface-kube-control#33 * charmed-kubernetes/charm-kubernetes-worker#84 * charmed-kubernetes/charm-kubeapi-load-balancer#11 [lp:1897818]: https://bugs.launchpad.net/charmed-kubernetes-testing/+bug/1897818
Simplify the relations between the master, worker, and API LB charms in favor of the newer `loadbalancer` interface relation. Part of [lp:1897818][] Depends on: * juju-solutions/loadbalancer-interface#13 * juju-solutions/interface-kube-control#33 * charmed-kubernetes/charm-kubernetes-control-plane#153 * charmed-kubernetes/charm-kubernetes-worker#84 * charmed-kubernetes/charm-kubeapi-load-balancer#11 [lp:1897818]: https://bugs.launchpad.net/charmed-kubernetes-testing/+bug/1897818
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this is a good change. Previously, the network connection between kubeapi-load-balancer <-> kubernetes-worker was modeled with a direct relation. That made it possible to support cross-model relations and network bindings / network spaces.
With this change, we instead proxy the connection info through kubernetes-master. We can't properly support network spaces or cross-model relations when doing that, can we?
reactive/load_balancer.py
Outdated
'port': port, | ||
} | ||
for address, port in itertools.product( | ||
request.backends, request.port_mapping.values() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we're ignoring the port_mapping keys. Are consumers smart enough to handle this? If the consumer requests port-mapping {443: 6443}
but the kubeapi-load-balancer charm is configured with port: 12345
, what happens?
How is the real port communicated to the consumer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, yeah, this is a case where the philosophy diverges, since the loadbalancer
interface (and all other LB providers) expect the consumer to dictate what the ingress port should be. This should honor the port on the request and the config option on the kapi-lb charm should be deprecated.
reactive/load_balancer.py
Outdated
else: | ||
website.configure(port=hookenv.config('port')) | ||
address = hookenv.unit_get('public-address') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public-address is usually not what we want. We want internal services to talk to eachother via internal IPs, e.g. ingress-address on a relation. If we use a public address, then kubeapi-load-balancer must be juju expose
'd, or other units will not be able to reach it (on clouds with network security, like AWS). Bare metal deployments also tend to have stricter network requirements than we test with, and I expect this would cause problems with network routing in those environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loadbalancer
interface protocol can already support multiple requests with different ones marked as public or not. I was already thinking that it probably makes sense for the k8s-master to request two LBs, one public for the kube config files it writes for the user to download, and one private for internal use.
That still doesn't cover the case where the workers are CMR and on a different network than the master / kapi-lb, but that seems like a pretty strange setup anyway. Even in strict network environments, I would still at least expect the cluster to be in one network and then potentially need some form of "public" ingress for clients in another network.
reactive/load_balancer.py
Outdated
address = _get_lb_address() | ||
website.configure(port=hookenv.config('port'), | ||
private_address=address, | ||
hostname=address) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without hacluster or loadbalancer-ips configured, this will tell other units to connect to a public-address, which is not ideal.
The correct address to use here is the ingress address of the website relation (per relation id). This was previously handled by omitting the private_address and hostname arguments, in which case, interface-http uses the proper ingress address: https://github.com/juju-solutions/interface-http/blob/632131b1f122daf6fb601fd4c9f1e4dbb1a92e09/provides.py#L31
reactive/load_balancer.py
Outdated
services = [] | ||
if apiserver: | ||
services.extend(apiserver.services()) | ||
for request in lb_consumers.all_requests: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably verify the request protocol is http/https, since that's all we can really loadbalance to.
reactive/load_balancer.py
Outdated
private_address = lb_address | ||
public_address = lb_address | ||
else: | ||
rel_id = lb_consumers.relations[0].id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After relating kubernetes-master:loadbalancer-internal
and kubernetes-master:loadbalancer-external
to kubapi-load-balancer:lb-consumers
:
unit-kubeapi-load-balancer-0: 15:43:43 ERROR unit.kubeapi-load-balancer/0.juju-log lb-consumers:18: Hook error:
Traceback (most recent call last):
File "/var/lib/juju/agents/unit-kubeapi-load-balancer-0/.venv/lib/python3.8/site-packages/charms/reactive/__init__.py", line 74, in main
bus.dispatch(restricted=restricted_mode)
File "/var/lib/juju/agents/unit-kubeapi-load-balancer-0/.venv/lib/python3.8/site-packages/charms/reactive/bus.py", line 390, in dispatch
_invoke(other_handlers)
File "/var/lib/juju/agents/unit-kubeapi-load-balancer-0/.venv/lib/python3.8/site-packages/charms/reactive/bus.py", line 359, in _invoke
handler.invoke()
File "/var/lib/juju/agents/unit-kubeapi-load-balancer-0/.venv/lib/python3.8/site-packages/charms/reactive/bus.py", line 181, in invoke
self._action(*args)
File "/var/lib/juju/agents/unit-kubeapi-load-balancer-0/charm/reactive/load_balancer.py", line 265, in provide_lb_consumers
rel_id = lb_consumers.relations[0].id
IndexError: list index out of range
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm too braindead today to review this properly. I did hit a hook error though, check it out.
This adds support for the `loadbalancer` interface so that cloud-native LBs can be provided by the integrator charms. Additionally, it simplifies the confusing way the relations between the masters and workers change depending on whether kubeapi-load-balancer is being used or not by making that use the same `lb-provider` endpoint and always forwarding the API endpoint URLs via the `kube-control` relation. Part of [lp:1897818][] Depends on: * juju-solutions/loadbalancer-interface#13 * juju-solutions/interface-kube-control#33 * charmed-kubernetes/charm-kubernetes-worker#84 * charmed-kubernetes/charm-kubeapi-load-balancer#11 [lp:1897818]: https://bugs.launchpad.net/charmed-kubernetes-testing/+bug/1897818
This adds support for the `loadbalancer` interface so that cloud-native LBs can be provided by the integrator charms. Additionally, it simplifies the confusing way the relations between the masters and workers change depending on whether kubeapi-load-balancer is being used or not by making that use the same `lb-provider` endpoint and always forwarding the API endpoint URLs via the `kube-control` relation. Part of [lp:1897818][] Depends on: * juju-solutions/loadbalancer-interface#13 * juju-solutions/interface-kube-control#33 * charmed-kubernetes/charm-kubernetes-worker#84 * charmed-kubernetes/charm-kubeapi-load-balancer#11 [lp:1897818]: https://bugs.launchpad.net/charmed-kubernetes-testing/+bug/1897818
This adds support for the `loadbalancer` interface so that cloud-native LBs can be provided by the integrator charms. Additionally, it simplifies the confusing way the relations between the masters and workers change depending on whether kubeapi-load-balancer is being used or not by making that use the same `lb-provider` endpoint and always forwarding the API endpoint URLs via the `kube-control` relation. Part of [lp:1897818][] Depends on: * juju-solutions/loadbalancer-interface#13 * juju-solutions/interface-kube-control#33 * charmed-kubernetes/charm-kubernetes-worker#84 * charmed-kubernetes/charm-kubeapi-load-balancer#11 [lp:1897818]: https://bugs.launchpad.net/charmed-kubernetes-testing/+bug/1897818
Manual test run result using the modified k8s-master and k8s-worker charms. |
This adds support for the `loadbalancer` interface so that cloud-native LBs can be provided by the integrator charms. Additionally, it simplifies the confusing way the relations between the masters and workers change depending on whether kubeapi-load-balancer is being used or not by making that use the same `lb-provider` endpoint and always forwarding the API endpoint URLs via the `kube-control` relation. Part of [lp:1897818][] Depends on: * juju-solutions/loadbalancer-interface#13 * juju-solutions/interface-kube-control#33 * charmed-kubernetes/charm-kubernetes-worker#84 * charmed-kubernetes/charm-kubeapi-load-balancer#11 [lp:1897818]: https://bugs.launchpad.net/charmed-kubernetes-testing/+bug/1897818
This adds support for the `loadbalancer` interface so that cloud-native LBs can be provided by the integrator charms. Additionally, it simplifies the confusing way the relations between the masters and workers change depending on whether kubeapi-load-balancer is being used or not by making that use the same `lb-provider` endpoint and always forwarding the API endpoint URLs via the `kube-control` relation. Part of [lp:1897818][] Depends on: * juju-solutions/loadbalancer-interface#13 * juju-solutions/interface-kube-control#33 * charmed-kubernetes/charm-kubernetes-worker#84 * charmed-kubernetes/charm-kubeapi-load-balancer#11 [lp:1897818]: https://bugs.launchpad.net/charmed-kubernetes-testing/+bug/1897818
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the tests could use a bit of cleaning up, but no show stoppers here. Good work.
channel: edge | ||
num_units: 1 | ||
resources: | ||
easyrsa: 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove resource revs from the bundle. No point specifying those if the charm revisions themselves are unspecified.
expose: true | ||
num_units: 1 | ||
options: | ||
channel: 1.20/stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be at least 1.21/stable, possibly 1.22/edge to match what's in kubernetes-master.
constraints: cores=4 mem=4G root-disk=16G | ||
num_units: 1 | ||
options: | ||
channel: 1.20/stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be 1.21/stable or 1.22/edge?
kubeapi-load-balancer: | ||
charm: {{k8s_lb_charm}} | ||
num_units: 1 | ||
expose: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kubeapi-load-balancer is missing constraints
and to
fields.
await master.add_relation("loadbalancer", "kubeapi-load-balancer") | ||
await worker.add_relation("kube-api-endpoint", "kubeapi-load-balancer") | ||
await ops_test.model.wait_for_idle(wait_for_active=True, timeout=30 * 60) | ||
_check_status_messages(ops_test) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this test will leave the cluster with the old-style relations. Maybe it should revert it back to the new relations when it's done?
charm: cs:~containers/kubernetes-master | ||
channel: edge | ||
constraints: cores=2 mem=4G root-disk=16G | ||
expose: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kubernetes-master typically isn't exposed in deployments that have kubeapi-load-balancer
* Add support for loadbalancer interface This adds support for the `loadbalancer` interface so that cloud-native LBs can be provided by the integrator charms. Additionally, it simplifies the confusing way the relations between the masters and workers change depending on whether kubeapi-load-balancer is being used or not by making that use the same `lb-provider` endpoint and always forwarding the API endpoint URLs via the `kube-control` relation. Part of [lp:1897818][] Depends on: * juju-solutions/loadbalancer-interface#13 * juju-solutions/interface-kube-control#33 * charmed-kubernetes/charm-kubernetes-worker#84 * charmed-kubernetes/charm-kubeapi-load-balancer#11 [lp:1897818]: https://bugs.launchpad.net/charmed-kubernetes-testing/+bug/1897818 * Fix lint error * Add test for status reporting of incomplete LB relation * Fix lint errors in test * Fix hook error when built with old version of kube-control interface * Split lb-provider endpoint into separate, more explicit ones for internal / external * Fix handling of internal LB response and missing endpoints * Move test to new LB relation pattern and use edge charms * Improve status reporting around the auth-webhook and skip trying to create secrets before apiserver is available * Create a ~/.kube/config for the ubuntu user * Fix NoneType error for old-style relation * Fix unit test * Revert drive-by related to auth setup status reporting; splitting to separate bug
* Support loadbalancer interface between master and API LB Simplify the relations between the master, worker, and API LB charms in favor of the newer `loadbalancer` interface relation. Part of [lp:1897818][] Depends on: * juju-solutions/loadbalancer-interface#13 * juju-solutions/interface-kube-control#33 * charmed-kubernetes/charm-kubernetes-control-plane#153 * charmed-kubernetes/charm-kubernetes-worker#84 * charmed-kubernetes/charm-kubeapi-load-balancer#11 [lp:1897818]: https://bugs.launchpad.net/charmed-kubernetes-testing/+bug/1897818 * Split k8s-master:lb-provider relation into external / internal * Update core and converged bundle fragments
The current way that the relations are set up for CK, with kubeapi-load-balancer sitting in between the master and workers, is
unnecessarily convoluted, and leads to mistakes due to the relations not being set up properly since they have to be done differently depending on whether or not kubeapi-load-balancer is being used.
This change makes this charm support the
loadbalancer
interface and depends on the master to forward the LB address via thekube-control
relation. This means that there's only one relation between the workers and master, the endpoint address is always handed off in the same way, and the relations are consistent regardless of whether the API LB charm is being used or if another provider (such as a cloud integrator charm) is providing the LB.Part of lp:1921776