Fix k8s label selectors being overridden by the user

src/stack/constants.py

@@ -51,6 +51,10 @@
 kube_config_filename = "kubeconfig.yml"
 kube_config_key = "kube-config"
 labels_key = "labels"
+# The pod labels stack generates for itself.  Both the Deployment and the Service
+# select their pods on these, so a spec's `labels` may not redefine them -- see
+# _check_labels in deployment_create.
+reserved_label_keys = ("app", "service")
 network_key = "network"
 node_affinities_key = "node-affinities"
 node_tolerations_key = "node-tolerations"

src/stack/deploy/deployment_create.py

@@ -540,6 +540,26 @@ def _check_volume_definitions(spec):
             raise Exception(f"Affinity for volume {volume_name} must specify label and value")
 
 
+def _check_labels(spec):
+    # A spec's labels go onto the pod template, where two keys are already spoken
+    # for: the Deployment selects its own pods on app and service, and so does the
+    # Service.  Redefining either leaves a selector that no longer matches the pods
+    # it names, which k8s rejects outright -- with an error about label selectors
+    # rather than about the spec that caused it, at the point of creating the
+    # object rather than here.  Every other key is the author's to use.  Checked on
+    # every target and not just k8s: the names are reserved either way, and labels
+    # that do nothing on compose today would break the deployment on retargeting.
+    labels = spec.get_labels()
+    if not isinstance(labels, dict):
+        raise Exception(f"{constants.labels_key} must be a mapping of label name to value")
+    reserved = sorted(key for key in labels if key in constants.reserved_label_keys)
+    if reserved:
+        raise Exception(
+            f"{constants.labels_key} cannot redefine {', '.join(reserved)}: "
+            f"stack sets {' and '.join(constants.reserved_label_keys)} on every pod and selects on them"
+        )
+
+
 def _check_runtime_class(spec):
     # A RuntimeClass is a k8s object, so naming one on a compose deployment cannot
     # mean anything.  Rejected rather than ignored, on the same reasoning as the
@@ -617,6 +637,7 @@ def create_operation(deployment_command_context, parsed_spec: Spec | MergedSpec,
     log_debug(f"parsed spec: {parsed_spec}")
     _check_volume_definitions(parsed_spec)
     _check_runtime_class(parsed_spec)
+    _check_labels(parsed_spec)
     # Validated here as well as at init, since a spec file is edited by hand.
     stack_secrets.validate_spec_secrets(parsed_spec)
 

src/stack/deploy/k8s/cluster_info.py

@@ -617,7 +617,14 @@ def get_deployments(self, image_pull_policy: str = None):
                 spec = client.V1DeploymentSpec(
                     replicas=self.spec.get_replicas(),
                     template=template,
-                    selector={"matchLabels": {"app": self.app_name}},
+                    # The selector has to name the service as well as the app: with
+                    # "app" alone every Deployment in the deployment nominally selects
+                    # every pod in it, and only the pod-template-hash the controller
+                    # adds to each ReplicaSet keeps them from fighting.  What that costs
+                    # in the meantime is legibility -- "kubectl logs deploy/deploy-foo"
+                    # picks an arbitrary pod out of all of them.  Immutable once the
+                    # Deployment exists, so this applies to newly created ones.
+                    selector={"matchLabels": {"app": self.app_name, "service": service_name}},
                 )
 
                 deployment = client.V1Deployment(

tests/unit/test_k8s_objects.py

@@ -27,6 +27,7 @@
 import pytest
 
 from conftest import TEST_CLUSTER_ID, k8s_dict, make_cluster_info
+from stack import constants
 
 
 MINIMAL_POD = """\
@@ -961,7 +962,7 @@ def test_deployment_shape_and_defaults(tmp_path):
     assert deployment["kind"] == "Deployment"
     assert deployment["metadata"]["name"] == "deploy-web"
     assert deployment["spec"]["replicas"] == 1
-    assert deployment["spec"]["selector"] == {"matchLabels": {"app": TEST_CLUSTER_ID}}
+    assert deployment["spec"]["selector"] == {"matchLabels": {"app": TEST_CLUSTER_ID, "service": "web"}}
     assert deployment["spec"]["template"]["metadata"]["labels"] == {
         "app": TEST_CLUSTER_ID,
         "service": "web",
@@ -996,6 +997,12 @@ def test_one_deployment_per_service(tmp_path):
 
     deployments = cluster_info.get_deployments()
     assert [k8s_dict(d)["metadata"]["name"] for d in deployments] == ["deploy-web", "deploy-worker"]
+    # Each Deployment selects its own pods and nobody else's: with "app" alone in the
+    # selector every one of them nominally owns every pod in the deployment.
+    assert [k8s_dict(d)["spec"]["selector"]["matchLabels"] for d in deployments] == [
+        {"app": TEST_CLUSTER_ID, "service": "web"},
+        {"app": TEST_CLUSTER_ID, "service": "worker"},
+    ]
 
 
 # ---------------------------------------------------------------------------
@@ -1050,6 +1057,47 @@ def test_labels_substitute_container_name_and_keep_defaults(tmp_path):
     }
 
 
+def _check_labels(spec_obj):
+    from stack.deploy.deployment_create import _check_labels as check
+    from stack.deploy.spec import Spec
+
+    check(Spec(obj=spec_obj))
+
+
+def test_reserved_label_keys_are_the_ones_generated(tmp_path):
+    # The check below knows which keys are stack's own; this is what keeps that
+    # list honest if the generated set ever changes.
+    cluster_info = make_cluster_info(tmp_path, MINIMAL_POD, k8s_spec())
+
+    labels = k8s_dict(cluster_info.get_deployments()[0])["spec"]["template"]["metadata"]["labels"]
+    assert set(labels) == set(constants.reserved_label_keys)
+
+
+@pytest.mark.parametrize("key", ["app", "service"])
+def test_reserved_label_key_is_rejected(key):
+    # Redefining either leaves the Deployment's selector naming pods that no longer
+    # carry the label, which k8s rejects when the object is created -- too late, and
+    # complaining about label selectors rather than about the spec.
+    with pytest.raises(Exception, match="cannot redefine"):
+        _check_labels(k8s_spec(labels={key: "whatever"}))
+
+
+def test_reserved_label_key_is_rejected_on_compose_too(tmp_path):
+    # Nothing reads labels on compose today, so this one costs the author nothing
+    # now and saves them the rejection on retargeting to k8s.
+    with pytest.raises(Exception, match="cannot redefine"):
+        _check_labels(k8s_spec(labels={"app": "whatever"}, **{"deploy-to": "compose"}))
+
+
+def test_other_label_keys_are_accepted(tmp_path):
+    _check_labels(k8s_spec(labels={"{name}-tier": "web", "owner": "team"}))
+
+
+def test_scalar_labels_are_rejected(tmp_path):
+    with pytest.raises(Exception, match="must be a mapping"):
+        _check_labels(k8s_spec(labels="web"))
+
+
 def test_node_affinity_from_spec(tmp_path):
     spec = k8s_spec(**{"node-affinities": [{"label": "disk", "value": "ssd"}]})
     cluster_info = make_cluster_info(tmp_path, MINIMAL_POD, spec)