feat(config): Prevent changing bootstrap charm config #319

HomayoonAlimohammadi · 2025-02-10T07:24:58Z

Overview

This PR adds a way to prevent the bootstrap- charm configs from getting changed after the cluster is bootstrapped.

Rationale

Some of the charm config options start with bootstrap- which means that these options should only be specified upon deploying the charm (before the underlying Kubernetes cluster is bootstrapped) and changing them afterwards might result in unexpected behavior. We've discussed that we like to have a mechanism to prevent the user from changing these options.

HomayoonAlimohammadi · 2025-02-12T12:49:08Z

Converting this draft as we need to implement the k8sd call for bootstrap config first.

HomayoonAlimohammadi · 2025-02-21T14:56:48Z

charms/worker/k8s/templates/snap_installation.yaml

@@ -5,9 +5,9 @@ amd64:
  - name: k8s
    install-type: store
    classic: true
-    channel: latest/edge
+    channel: latest/edge/hue-cluster-config


This is for testing purposes, needs to be removed before merge.

addyess

Well, i have a few questions.
I'd also like @mateoflorido to weigh in here too

addyess · 2025-02-21T20:39:31Z

charms/worker/k8s/charmcraft.yaml

-      default: dqlite
+      default: k8s-dqlite
      type: string
      description: |
        The datastore to use in Canonical Kubernetes. This cannot be changed
-        after deployment. Allowed values are "dqlite" and "etcd". If "etcd" is
+        after deployment. Allowed values are "k8s-dqlite" and "etcd". If "etcd" is


I don't think we can just change this. charms deployed from 1.32 will deploy with dqlite as their default. Upgrading to this revision will cause an immediate failure.

The default config from 1.32 was dqlite, so that's what we are "stuck with"

I agree with Adam. Also, I think this reflect better that we are using dqlite as the datastore. The translation mechanism is something that we can hide internally.

You're right. I also thought that this might be a breaking change, but here's what I think:
We use the dqlite string in the following places:

https://github.com/canonical/k8s-operator/blob/main/charms/worker/k8s/src/charm.py#L729

https://github.com/canonical/k8s-operator/blob/main/charms/worker/k8s/src/upgrade.py#L211

https://github.com/canonical/k8s-operator/blob/main/charms/worker/k8s/src/literals.py#L26
And we're not communicating that string with any other source and it's completely contained in the charm. So we're technically not breaking anything but the user experience. At least this is what I assume.
Or maybe the charm config is preserved during an upgrade. If so, that's breaking.
I'll change this back to dqlite anyways.

addyess · 2025-02-21T20:42:13Z

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

@@ -87,15 +87,31 @@ class InvalidResponseError(K8sdAPIManagerError):
        code (int): HTTP Status code
    """

-    def __init__(self, code: int, msg: str) -> None:


there's a LIBPATCH version at the head of this file that if it gets changed, we must also bump

As I always forget this. Lemme create a card for adding a CI check for this.

addyess · 2025-02-21T20:47:11Z

charms/worker/k8s/src/charm.py

+        # NOTE(Hue): The way we're saving certificates has nothing to do with the snap
+        # so we can't rely on the snap to tell us if the certificates have changed.
+        self._stored.set_default(certificates=self.config.get(CONFIG_BOOTSTRAP_CERTIFICATES))


so we're back to using stored-state?

We can infer this from the certificates in the unit:

def check_ca_key(): return os.path.isFile("/etc/kubernetes/pki/ca.key")

This way we can avoid using stored state

addyess · 2025-02-21T22:31:56Z

charms/worker/k8s/src/charm.py

+        """Prevent bootstrap config changes after bootstrap."""
+        log.info("Preventing bootstrap config changes after bootstrap")
+
+        try:
+            cluster_config = bootstrap_config_options_from_cluster_config(
+                self.api_manager.get_cluster_config().metadata
+            )
+        except InvalidResponseError as e:
+            if e.code == HTTP_503_SERVICE_UNAVAILABLE:
+                log.info("k8sd is not ready, skipping bootstrap config check")
+                return
+            raise
+
+        # NOTE(Hue): The way we're saving certificates has nothing to do with the snap
+        # so we can't rely on the snap to tell us if the certificates have changed.
+        self._stored.set_default(certificates=self.config.get(CONFIG_BOOTSTRAP_CERTIFICATES))
+
+        if self.is_control_plane:
+            self._prevent_control_plane_bootstrap_config_change(cluster_config)
+        else:
+            self._prevent_worker_bootstrap_config_change(cluster_config)
+
+    def _prevent_control_plane_bootstrap_config_change(self, ref: BootstrapConfigOptions):
+        """Prevent control-plane bootstrap config changes after bootstrap."""
+        changed: List[ChangedConfig] = []
+
+        if self.config.get(CONFIG_BOOTSTRAP_DATASTORE, "") != ref.datastore:
+            changed.append(
+                ChangedConfig(
+                    name=CONFIG_BOOTSTRAP_DATASTORE,
+                    cluster_config=ref.datastore,
+                    charm_config=self.config.get(CONFIG_BOOTSTRAP_DATASTORE, ""),
+                )
+            )
+
+        # NOTE(Hue): bootstrap-certificates is not represented in the cluster config and should be
+        # checked against the stored value.
+        if self.config.get(CONFIG_BOOTSTRAP_CERTIFICATES, "") != self._stored.certificates:
+            changed.append(
+                ChangedConfig(
+                    name=CONFIG_BOOTSTRAP_CERTIFICATES,
+                    cluster_config=self._stored.certificates,
+                    charm_config=self.config.get(CONFIG_BOOTSTRAP_CERTIFICATES, ""),
+                )
+            )
+
+        if self.config.get(CONFIG_BOOTSTRAP_NODE_TAINTS, "") != ref.node_taints:
+            changed.append(
+                ChangedConfig(
+                    name=CONFIG_BOOTSTRAP_NODE_TAINTS,
+                    cluster_config=ref.node_taints,
+                    charm_config=self.config.get(CONFIG_BOOTSTRAP_NODE_TAINTS, ""),
+                )
+            )
+
+        if self.config.get(CONFIG_BOOTSTRAP_POD_CIDR, "") != ref.pod_cidr:
+            changed.append(
+                ChangedConfig(
+                    name=CONFIG_BOOTSTRAP_POD_CIDR,
+                    cluster_config=ref.pod_cidr,
+                    charm_config=self.config.get(CONFIG_BOOTSTRAP_POD_CIDR, ""),
+                )
+            )
+
+        if self.config.get(CONFIG_BOOTSTRAP_SERVICE_CIDR, "") != ref.service_cidr:
+            changed.append(
+                ChangedConfig(
+                    name=CONFIG_BOOTSTRAP_SERVICE_CIDR,
+                    cluster_config=ref.service_cidr,
+                    charm_config=self.config.get(CONFIG_BOOTSTRAP_SERVICE_CIDR, ""),
+                )
+            )
+
+        if changed:
+            for c in changed:
+                log.error(
+                    "Bootstrap config '%s' should NOT be changed after bootstrap. %s", c.name, c
+                )
+            raise BootstrapConfigChangeError(changed)
+
+    def _prevent_worker_bootstrap_config_change(self, ref: BootstrapConfigOptions):
+        """Prevent worker bootstrap config changes after bootstrap."""
+        changed: List[ChangedConfig] = []
+
+        if self.config.get(CONFIG_BOOTSTRAP_NODE_TAINTS, "") != ref.node_taints:
+            changed.append(
+                ChangedConfig(
+                    name=CONFIG_BOOTSTRAP_NODE_TAINTS,
+                    cluster_config=ref.node_taints,
+                    charm_config=self.config.get(CONFIG_BOOTSTRAP_NODE_TAINTS, ""),
+                )
+            )
+
+        if changed:
+            for c in changed:
+                log.error(
+                    "Bootstrap config '%s' should NOT be changed after bootstrap. %s", c.name, c
+                )
+            raise BootstrapConfigChangeError(changed)
+


Could i talk you into moving all of this out of charm.py. Maybe bootstrap.py?

Agree, charm.py is becoming a behemoth

Of course you can :D

mateoflorido · 2025-02-21T22:09:32Z

charms/worker/k8s/charmcraft.yaml

-      default: dqlite
+      default: k8s-dqlite
      type: string
      description: |
        The datastore to use in Canonical Kubernetes. This cannot be changed
-        after deployment. Allowed values are "dqlite" and "etcd". If "etcd" is
+        after deployment. Allowed values are "k8s-dqlite" and "etcd". If "etcd" is


I agree with Adam. Also, I think this reflect better that we are using dqlite as the datastore. The translation mechanism is something that we can hide internally.

mateoflorido · 2025-02-21T22:14:22Z

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

+
+    status: UserFacingClusterConfig
+    datastore: Optional[UserFacingDatastoreConfig] = Field(default=None)
+    node_taints: Optional[List[str]] = Field(default=None, alias="nodeTaints")


This alias looks odd... Are we going to change the naming schema?

I changed this in k8s-snap-api, k8s-snap and also here.

mateoflorido · 2025-02-21T22:18:04Z

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

@@ -942,6 +990,8 @@ def create_connection(self):
 class K8sdAPIManager:
    """Manager for K8sd API interactions."""

+    CLUSTER_CONFIG_ENDPOINT: str = "/1.0/k8sd/cluster/config"


Well, we can keep it inline or create another PR to refactor all the other endpoints. It would be a good opportunity to improve the library, but I think it would make the PR harder to review.

I'll remove this and create a card for the refactor.

mateoflorido · 2025-02-21T22:33:35Z

charms/worker/k8s/src/charm.py

+        # NOTE(Hue): The way we're saving certificates has nothing to do with the snap
+        # so we can't rely on the snap to tell us if the certificates have changed.
+        self._stored.set_default(certificates=self.config.get(CONFIG_BOOTSTRAP_CERTIFICATES))


We can infer this from the certificates in the unit:

def check_ca_key(): return os.path.isFile("/etc/kubernetes/pki/ca.key")

This way we can avoid using stored state

…to config/bootstrap.py

HomayoonAlimohammadi requested a review from a team as a code owner February 10, 2025 07:24

HomayoonAlimohammadi marked this pull request as draft February 10, 2025 07:42

HomayoonAlimohammadi force-pushed the KU-2306/prevent-changing-bootstrap-config branch from 89cf36e to fd1c0c4 Compare February 10, 2025 12:35

HomayoonAlimohammadi changed the base branch from main to fix-formatting February 10, 2025 12:35

This comment was marked as resolved.

Sign in to view

HomayoonAlimohammadi marked this pull request as ready for review February 10, 2025 12:49

Base automatically changed from fix-formatting to main February 10, 2025 14:31

HomayoonAlimohammadi force-pushed the KU-2306/prevent-changing-bootstrap-config branch from b623f9f to 3e07cd0 Compare February 10, 2025 14:35

HomayoonAlimohammadi marked this pull request as draft February 11, 2025 11:52

HomayoonAlimohammadi force-pushed the KU-2306/prevent-changing-bootstrap-config branch 3 times, most recently from 8d0af0e to 4483690 Compare February 11, 2025 13:44

HomayoonAlimohammadi marked this pull request as ready for review February 11, 2025 13:57

HomayoonAlimohammadi marked this pull request as draft February 12, 2025 12:48

HomayoonAlimohammadi force-pushed the KU-2306/prevent-changing-bootstrap-config branch from 4483690 to ab1e09a Compare February 21, 2025 13:53

HomayoonAlimohammadi marked this pull request as ready for review February 21, 2025 14:25

HomayoonAlimohammadi changed the title ~~charm: Add bootstrap config change preventer~~ feat(config): Prevent changing bootstrap charm config Feb 21, 2025

HomayoonAlimohammadi commented Feb 21, 2025

View reviewed changes

addyess reviewed Feb 21, 2025

View reviewed changes

mateoflorido reviewed Feb 21, 2025

View reviewed changes

HomayoonAlimohammadi added 7 commits February 24, 2025 12:02

config: Prevent changing bootstrap configs

5a628bd

fix: Revert k8s-dqlite back to dqlite

a0cbd2b

fix: Update nodeTaints field name to node-taints

b4d3360

fix: Update k8sd_api_manager LIBPATCH

0dde53e

fix: Update k8s-dqlite to dqlite

5be36e0

fix: Remove CLUSTER_CONFIG_ENDPOINT class attr

d6a9728

fix: Extract bootstrap config change prevention logic from charms.py …

77bfcb3

…to config/bootstrap.py

HomayoonAlimohammadi force-pushed the KU-2306/prevent-changing-bootstrap-config branch from ab1e09a to 77bfcb3 Compare February 24, 2025 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(config): Prevent changing bootstrap charm config #319

feat(config): Prevent changing bootstrap charm config #319

HomayoonAlimohammadi commented Feb 10, 2025 •

edited

Loading

This comment was marked as resolved.

HomayoonAlimohammadi commented Feb 12, 2025

HomayoonAlimohammadi Feb 21, 2025

addyess left a comment

addyess Feb 21, 2025

mateoflorido Feb 21, 2025

HomayoonAlimohammadi Feb 24, 2025

addyess Feb 21, 2025

HomayoonAlimohammadi Feb 24, 2025

addyess Feb 21, 2025

mateoflorido Feb 21, 2025

addyess Feb 21, 2025

mateoflorido Feb 21, 2025

HomayoonAlimohammadi Feb 24, 2025

mateoflorido Feb 21, 2025

mateoflorido Feb 21, 2025

HomayoonAlimohammadi Feb 24, 2025

mateoflorido Feb 21, 2025

HomayoonAlimohammadi Feb 24, 2025 •

edited

Loading

mateoflorido Feb 21, 2025

feat(config): Prevent changing bootstrap charm config #319

Are you sure you want to change the base?

feat(config): Prevent changing bootstrap charm config #319

Conversation

HomayoonAlimohammadi commented Feb 10, 2025 • edited Loading

Overview

Rationale

This comment was marked as resolved.

HomayoonAlimohammadi commented Feb 12, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HomayoonAlimohammadi Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HomayoonAlimohammadi commented Feb 10, 2025 •

edited

Loading

HomayoonAlimohammadi Feb 24, 2025 •

edited

Loading