Add external-load-balancer relation #234

HomayoonAlimohammadi · 2025-01-06T09:02:55Z

Overview

This PR adds external-load-balancer relation. This relation enables k8s-operator to utilize external LB sources like Openstack Octavia or Amazon ELB.

The external-load-balancer-port config is also added. This config should be set according to the external LB provider config. Example for Openstack integrator: https://github.com/charmed-kubernetes/charm-openstack-integrator/blob/main/reactive/openstack.py#L245-L254

Please merge me as well #235 (Happy new year!)

bschimke95

great work @HomayoonAlimohammadi
Did a first pass with a general question.

charms/worker/k8s/src/charm.py

charms/worker/k8s/charmcraft.yaml

charms/worker/k8s/src/consts.py

charms/worker/k8s/src/charm.py

addyess

Good work down this road... there's still a bit to clean up here

addyess · 2025-01-18T04:22:20Z

charms/worker/k8s/requirements.txt

@@ -17,3 +17,5 @@ websocket-client==1.8.0
 poetry-core==1.9.1
 lightkube==0.16.0
 httpx==0.27.2
+loadbalancer_interface == 1.2.0
+cryptography==44.0.0


OH dude, i have bad news. You can't use this library b/c that library requires our charm not be able to support multiple series.

cryptography compiles C code specific for the ubuntu base's python. Building on focal, yields code that doesn't run on jammy or noble. The same is true if you build on jammy -- it doesn't run on focal/noble.

my best advice. subprocess out to openssl on the device

addyess

Dude, hue this is looking so good. Feel free to be angry with me.
Thanks for your defensive coding practices

addyess · 2025-01-18T04:25:42Z

charms/worker/k8s/src/charm.py

+        binding = self.model.get_binding(CLUSTER_RELATION)
+        try:
+            addresses = binding and binding.network.ingress_addresses
+            if addresses:
+                for addr in addresses:
+                    extra_sans.add(str(addr))
+        except ops.RelationNotFoundError as e:
+            log.error(f"Failed to get ingress addresses for extra SANs: {e}")


so, the CLUSTER_RELATION should ALWAYS be available b/c it's a peer relation. it's required to be there

Would this work?

Suggested change

binding = self.model.get_binding(CLUSTER_RELATION)

try:

addresses = binding and binding.network.ingress_addresses

if addresses:

for addr in addresses:

extra_sans.add(str(addr))

except ops.RelationNotFoundError as e:

log.error(f"Failed to get ingress addresses for extra SANs: {e}")

binding = self.model.get_binding(CLUSTER_RELATION)

extra_sans |= {str(addr) for addr in binding and binding.network.ingress_addresses or [])

This was because of some unit tests that were expecting this relation but it was not there.

I support Adam's suggestion. You can resolve the unit testing issue by using the Harness and creating the required relations, right?

addyess · 2025-01-18T04:28:40Z

charms/worker/k8s/src/charm.py

+            log.error(f"Failed to get ingress addresses for extra SANs: {e}")
+
+        # Add the external load balancer address
+        if self.is_control_plane and self.external_load_balancer.is_available:


it doesn't make sense for the worker to need this list -- basically ever. If you wanna protect against it occuring, just do a this near the beginning of the func

if not self.is_control_plane: raise ReconcilerError("Intended only for the Control Plane Charm")

Tried removing this but we're calling _assemble_bootstrap_config from a unit test with both a control plane and a worker charm.

charms/worker/k8s/src/pki.py

mateoflorido · 2025-01-20T16:50:58Z

charms/worker/k8s/lib/charms/k8s/v0/k8sd_api_manager.py

We should update the version of this library whenever we introduce new changes. In this case, could you please bump the patch version of the library?

mateoflorido · 2025-01-20T18:25:23Z

charms/worker/k8s/src/charm.py

+        binding = self.model.get_binding(CLUSTER_RELATION)
+        try:
+            addresses = binding and binding.network.ingress_addresses
+            if addresses:
+                for addr in addresses:
+                    extra_sans.add(str(addr))
+        except ops.RelationNotFoundError as e:
+            log.error(f"Failed to get ingress addresses for extra SANs: {e}")


I support Adam's suggestion. You can resolve the unit testing issue by using the Harness and creating the required relations, right?

charms/worker/k8s/src/charm.py

mateoflorido · 2025-01-20T20:39:50Z

charms/worker/k8s/src/charm.py

+        for san in extra_sans:
+            if san not in all_cert_sans:
+                log.info(f"{san} not in cert SANs, refreshing certs with new SANs: {extra_sans}")
+                status.add(ops.MaintenanceStatus("Refreshing Certificates"))
+                self.api_manager.refresh_certs(extra_sans)
+                log.info("Certificates have been refreshed")


This calls refresh_certs in a loop, once for each missing SAN. Therefore, the certificates will be refreshed multiple times unnecessarily:

Suggested change

for san in extra_sans:

if san not in all_cert_sans:

log.info(f"{san} not in cert SANs, refreshing certs with new SANs: {extra_sans}")

status.add(ops.MaintenanceStatus("Refreshing Certificates"))

self.api_manager.refresh_certs(extra_sans)

log.info("Certificates have been refreshed")

missing_sans = [san for san in extra_sans if san not in all_cert_sans]

if missing_sans:

log.info("Missing SANs found, refreshing certificates.")

status.add(ops.MaintenanceStatus("Refreshing Certificates"))

self.api_manager.refresh_certs(extra_sans)

log.info("Certificates have been refreshed")

That's a really good catch. Thanks!

mateoflorido · 2025-01-20T20:52:24Z

pyproject.toml

@@ -48,7 +48,8 @@ plugins = "pydantic.mypy"
 [tool.pylint]
 # Ignore too-few-public-methods due to pydantic models
 # Ignore no-self-argument due to pydantic validators
-disable = "wrong-import-order,redefined-outer-name,too-many-instance-attributes,too-few-public-methods,no-self-argument,fixme,protected-access"
+# Ignore logging-fstring-interpolation which instructs to use %s instead of f-strings in logs which is objectively worse.


F-strings are evaluated even if the log message is not displayed (e.g., log level): https://docs.python.org/3/howto/logging.html#optimization

Nice, TIL! Changing to %s, but TBH I'm not fully convinced that this optimization is actually worth the readability hit.

addyess

Looking really really good. Thanks for the switch to openssl.

I'm going to build this charm today and try it out

charms/worker/k8s/src/charm.py

charms/worker/k8s/lib/charms/k8s/v0/k8sd_api_manager.py

addyess

Finally, it all looks so good. We know it's not 100% tested yet but the impl looks right. Nice job @HomayoonAlimohammadi

HomayoonAlimohammadi · 2025-01-24T21:05:13Z

Thanks @addyess! I need to add multiple unit and integration tests. Didn't test the Openstack LB integration since you mentioned the issue with the openstack integrator charm (incompatible ops versions), but manually tried changing the extra SANs through charm config and seems like the certs refresh alright.

github-actions · 2025-01-29T07:57:21Z

Test coverage for 8a4ebbe

coverage-report: install_deps /home/runner/work/k8s-operator/k8s-operator/charms/worker/k8s> /home/runner/.local/share/uv/tools/tox/bin/uv pip install 'coverage[toml]'
coverage-report: commands[0] /home/runner/work/k8s-operator/k8s-operator/charms/worker/k8s> coverage report
Name                                    Stmts   Miss  Cover
-----------------------------------------------------------
lib/charms/k8s/v0/k8sd_api_manager.py     311     29    91%
src/charm.py                              580    307    47%
src/cloud_integration.py                   80      3    96%
src/config/extra_args.py                   31      2    94%
src/containerd.py                         140     22    84%
src/cos_integration.py                     33     12    64%
src/endpoints.py                           40      1    98%
src/events/update_status.py                68     24    65%
src/inspector.py                           41      4    90%
src/kube_control.py                        44     32    27%
src/literals.py                            34      0   100%
src/pki.py                                 33      7    79%
src/protocols.py                           28      5    82%
src/reschedule.py                          77      4    95%
src/snap.py                               193     29    85%
src/token_distributor.py                  181    109    40%
src/upgrade.py                            108     48    56%
-----------------------------------------------------------
TOTAL                                    2022    638    68%
coverage-report: OK (0.31=setup[0.03]+cmd[0.29] seconds)
congratulations :) (0.36 seconds)

Static code analysis report

Run started:2025-01-29 07:56:52.825314

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 4213
  Total lines skipped (#nosec): 3
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

HomayoonAlimohammadi requested a review from a team as a code owner January 6, 2025 09:02

This comment was marked as resolved.

Sign in to view

This comment was marked as duplicate.

Sign in to view

This comment was marked as resolved.

Sign in to view

HomayoonAlimohammadi force-pushed the KU-2292/add-loadbalancer-relation branch 2 times, most recently from 33093a9 to d8a90d4 Compare January 6, 2025 15:19

bschimke95 reviewed Jan 7, 2025

View reviewed changes

charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved

charms/worker/k8s/charmcraft.yaml Outdated Show resolved Hide resolved

mateoflorido reviewed Jan 7, 2025

View reviewed changes

charms/worker/k8s/src/consts.py Outdated Show resolved Hide resolved

HomayoonAlimohammadi force-pushed the KU-2292/add-loadbalancer-relation branch from c733800 to f278456 Compare January 7, 2025 16:32

This comment was marked as resolved.

Sign in to view

HomayoonAlimohammadi force-pushed the KU-2292/add-loadbalancer-relation branch from f278456 to 3aa9688 Compare January 7, 2025 16:37

This comment was marked as resolved.

Sign in to view

HomayoonAlimohammadi force-pushed the KU-2292/add-loadbalancer-relation branch from a809297 to 74860c9 Compare January 8, 2025 08:25

HomayoonAlimohammadi mentioned this pull request Jan 8, 2025

lb-consumers: Ignore config lb-port if port mapping was provided charmed-kubernetes/charm-openstack-integrator#10

Merged

addyess reviewed Jan 8, 2025

View reviewed changes

charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved

addyess reviewed Jan 8, 2025

View reviewed changes

charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved

addyess reviewed Jan 8, 2025

View reviewed changes

charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved

addyess reviewed Jan 8, 2025

View reviewed changes

charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved

addyess requested changes Jan 8, 2025

View reviewed changes

HomayoonAlimohammadi force-pushed the KU-2292/add-loadbalancer-relation branch 2 times, most recently from 78fef20 to 4efd89f Compare January 8, 2025 16:22

HomayoonAlimohammadi requested a review from addyess January 8, 2025 16:22

HomayoonAlimohammadi force-pushed the KU-2292/add-loadbalancer-relation branch from 430f2a7 to 461e9e8 Compare January 14, 2025 15:22

HomayoonAlimohammadi requested a review from addyess January 17, 2025 13:36

addyess reviewed Jan 18, 2025

View reviewed changes

addyess requested changes Jan 18, 2025

View reviewed changes

HomayoonAlimohammadi requested a review from addyess January 20, 2025 10:39

HomayoonAlimohammadi force-pushed the KU-2292/add-loadbalancer-relation branch from 4ebc2eb to 84f7501 Compare January 20, 2025 16:13

mateoflorido requested changes Jan 20, 2025

View reviewed changes

HomayoonAlimohammadi force-pushed the KU-2292/add-loadbalancer-relation branch from 84f7501 to 5a360e7 Compare January 21, 2025 12:26

HomayoonAlimohammadi requested a review from mateoflorido January 21, 2025 12:26

addyess requested changes Jan 23, 2025

View reviewed changes

charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved

charms/worker/k8s/src/charm.py Outdated Show resolved Hide resolved

addyess reviewed Jan 24, 2025

View reviewed changes

charms/worker/k8s/lib/charms/k8s/v0/k8sd_api_manager.py Show resolved Hide resolved

addyess reviewed Jan 24, 2025

View reviewed changes

charms/worker/k8s/lib/charms/k8s/v0/k8sd_api_manager.py Show resolved Hide resolved

HomayoonAlimohammadi force-pushed the KU-2292/add-loadbalancer-relation branch 2 times, most recently from 78f2aa2 to 1f6f431 Compare January 24, 2025 18:17

addyess approved these changes Jan 24, 2025

View reviewed changes

HomayoonAlimohammadi and others added 10 commits January 29, 2025 11:55

Add external-load-balancer relation

c5ae2d6

Remove external-load-balancer-port config

589b4d5

Remove retry on lb response

ae0b636

Implement build_url, Fix lint

67713ef

Ensure SANs from apiserver cert and refresh certs if needed

c9dd243

Address comments

2b594c6

Review changes worked over with Hue

1aedff2

Address linting issues

97415fd

Add comment for seed

a2f8fec

Refactor fetching external lb address

8a4ebbe

HomayoonAlimohammadi force-pushed the KU-2292/add-loadbalancer-relation branch from 7117a19 to 8a4ebbe Compare January 29, 2025 07:55

mateoflorido approved these changes Jan 29, 2025

View reviewed changes

HomayoonAlimohammadi merged commit d019173 into main Jan 29, 2025
70 checks passed

HomayoonAlimohammadi deleted the KU-2292/add-loadbalancer-relation branch January 29, 2025 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add external-load-balancer relation #234

Add external-load-balancer relation #234

HomayoonAlimohammadi commented Jan 6, 2025 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as duplicate.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

bschimke95 left a comment

This comment was marked as resolved.

This comment was marked as resolved.

addyess left a comment

addyess Jan 18, 2025

addyess left a comment

addyess Jan 18, 2025

HomayoonAlimohammadi Jan 20, 2025

mateoflorido Jan 20, 2025

addyess Jan 18, 2025

HomayoonAlimohammadi Jan 20, 2025

mateoflorido Jan 20, 2025

mateoflorido Jan 20, 2025

mateoflorido Jan 20, 2025

HomayoonAlimohammadi Jan 21, 2025

mateoflorido Jan 20, 2025

HomayoonAlimohammadi Jan 21, 2025

addyess left a comment

addyess left a comment

HomayoonAlimohammadi commented Jan 24, 2025

github-actions bot commented Jan 29, 2025

Add external-load-balancer relation #234

Add external-load-balancer relation #234

Conversation

HomayoonAlimohammadi commented Jan 6, 2025 • edited Loading

Overview

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as duplicate.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

bschimke95 left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

addyess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addyess 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

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

addyess left a comment

Choose a reason for hiding this comment

addyess left a comment

Choose a reason for hiding this comment

HomayoonAlimohammadi commented Jan 24, 2025

github-actions bot commented Jan 29, 2025

HomayoonAlimohammadi commented Jan 6, 2025 •

edited

Loading