refactor: Build RBAC with recommended labels by adwk67 · Pull Request #806 · stackabletech/hdfs-operator

adwk67 · 2026-07-20T10:51:56Z

Description

Part of: stackabletech/issues#869

Add missing RBAC labels (service account, role binding)
use infallible rbac functions (requires factor: add infallible rbac functions operator-rs#1251)
some label cleanup
consistent role_name function

Definition of Done Checklist

Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
Please make sure all these things are done and tick the boxes

Author

Changes are OpenShift compatible
CRD changes approved
CRD documentation for all fields, following the style guide.
Helm chart can be installed and deployed operator works
Integration tests passed (for non trivial changes)
Changes need to be "offline" compatible
Links to generated (nightly) docs added
Release note snippet added

Reviewer

Code contains useful comments
(Integration-)Test cases added
Changelog updated
Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

Feature Tracker has been updated
Proper release label has been added
Links to generated (nightly) docs added
Release note snippet added
Add type/deprecation label & add to the deprecation schedule
Add type/experimental label & add to the experimental features tracker

adwk67 · 2026-07-24T10:19:05Z

--- PASS: kuttl (713.35s)
    --- PASS: kuttl/harness (0.00s)
        --- PASS: kuttl/harness/smoke_hadoop-3.5.0_zookeeper-3.9.4_zookeeper-latest-3.9.5_number-of-datanodes-2_datanode-pvcs-2hdd-1ssd_listener-class-cluster-internal_openshift-false (200.51s)
        --- PASS: kuttl/harness/smoke_hadoop-3.5.0_zookeeper-3.9.4_zookeeper-latest-3.9.5_number-of-datanodes-2_datanode-pvcs-2hdd-1ssd_listener-class-external-unstable_openshift-false (169.36s)
        --- PASS: kuttl/harness/smoke_hadoop-3.5.0_zookeeper-3.9.5_zookeeper-latest-3.9.5_number-of-datanodes-2_datanode-pvcs-2hdd-1ssd_listener-class-external-unstable_openshift-false (182.27s)
        --- PASS: kuttl/harness/smoke_hadoop-3.5.0_zookeeper-3.9.5_zookeeper-latest-3.9.5_number-of-datanodes-2_datanode-pvcs-2hdd-1ssd_listener-class-cluster-internal_openshift-false (713.34s)
PASS

siegfriedweber · 2026-07-24T13:38:56Z

+    /// Every metrics Service must carry the Prometheus scrape label and the
+    /// `prometheus.io/path|port|scheme|scrape` annotations, or Prometheus stops discovering the
+    /// endpoints (caught by the HDFS smoke test 2026-07-23 after the labels migration dropped
+    /// them).
+    #[test]
+    fn metrics_services_carry_prometheus_label_and_annotations() {
+        let cluster = validated_cluster();
+        let resources = build(&cluster, &cluster_info()).expect("build succeeds");
+
+        let metrics_services: Vec<_> = resources
+            .services
+            .iter()
+            .filter(|service| {
+                service
+                    .metadata
+                    .name
+                    .as_deref()
+                    .is_some_and(|name| name.ends_with("-metrics"))
+            })
+            .collect();
+        assert!(!metrics_services.is_empty(), "no metrics Services built");
+
+        for service in metrics_services {
+            let name = service.metadata.name.as_deref().unwrap_or_default();
+            let labels = service.metadata.labels.as_ref().expect("labels are set");
+            assert_eq!(
+                labels.get("prometheus.io/scrape").map(String::as_str),
+                Some("true"),
+                "{name} lacks the scrape label"
+            );
+
+            // The native metrics port of the role, as asserted by the smoke test.
+            let expected_port = match name {
+                n if n.contains("-namenode-") => "9870",
+                n if n.contains("-datanode-") => "9864",
+                n if n.contains("-journalnode-") => "8480",
+                other => panic!("unexpected metrics Service {other}"),
+            };
+            let expected_annotations = BTreeMap::from(
+                [
+                    ("prometheus.io/path", "/prom"),
+                    ("prometheus.io/port", expected_port),
+                    ("prometheus.io/scheme", "http"),
+                    ("prometheus.io/scrape", "true"),
+                ]
+                .map(|(key, value)| (key.to_string(), value.to_string())),
+            );
+            assert_eq!(
+                service.metadata.annotations.as_ref(),
+                Some(&expected_annotations),
+                "{name} annotations mismatch"
+            );
+        }
+    }


In my opinion, no specific error cases should be tested in the unit tests. One problem is that you lose track of the test coverage. Which parts are not tested, and which are tested twice?
Instead, I would test each function with a sensible branch or path coverage. The given test case targets the function controller::build::resource::service::rolegroup_metrics_service. You
could define the following test case there:

#[cfg(test)] mod tests { use serde_json::json; use stackable_operator::v2::types::operator::RoleGroupName; use super::*; use crate::{ controller::build::properties::test_support::validated_cluster, crd::HdfsNodeRole, }; #[test] fn test_rolegroup_metrics_service() { let cluster = validated_cluster(); let role = &HdfsNodeRole::Name; let role_group_name = &RoleGroupName::from_str_unsafe("default"); let service = rolegroup_metrics_service(&cluster, role, role_group_name).expect("should not fail"); assert_eq!( json!({ "apiVersion": "v1", "kind": "Service", "metadata": { // Every metrics Service must carry the Prometheus scrape label and the // `prometheus.io/path|port|scheme|scrape` annotations, or Prometheus stops // discovering the endpoints. "annotations": { "prometheus.io/path": "/prom", "prometheus.io/port": "9870", "prometheus.io/scheme": "http", "prometheus.io/scrape": "true" }, "labels": { "app.kubernetes.io/component": "namenode", "app.kubernetes.io/instance": "hdfs", "app.kubernetes.io/managed-by": "hdfs.stackable.tech_hdfs-operator-hdfs-controller", "app.kubernetes.io/name": "hdfs", "app.kubernetes.io/role-group": "default", "app.kubernetes.io/version": "3.4.0-stackable0.0.0-dev", "prometheus.io/scrape": "true", "stackable.tech/vendor": "Stackable" }, "name": "hdfs-namenode-default-metrics", "namespace": "default", "ownerReferences": [ { "apiVersion": "hdfs.stackable.tech/v1alpha1", "controller": true, "kind": "HdfsCluster", "name": "hdfs", "uid": "c2c8c5c0-0b5a-4b1e-9f3e-1a2b3c4d5e6f" } ] }, "spec": { "clusterIP": "None", "ports": [ { "name": "metrics", "port": 9870, "protocol": "TCP" }, { "name": "jmx-metrics", "port": 8183, "protocol": "TCP" } ], "publishNotReadyAddresses": true, "selector": { "app.kubernetes.io/component": "namenode", "app.kubernetes.io/instance": "hdfs", "app.kubernetes.io/name": "hdfs", "app.kubernetes.io/role-group": "default", "group": "default", "role": "namenode" }, "type": "ClusterIP" } }), serde_json::to_value(service).expect("must be serializable") ); } }

The code requires stackable-operator = { workspace = true, features = ["test-support"] } in the dev-dependencies of the rust/operator-binary/Cargo.toml.

Yes, that's a fair point. That test was added when I discovered I had missed/dropped some elements on the refactor and broke the smoke test, more as a regression test than anything more thorough.

siegfriedweber · 2026-07-24T14:04:10Z

    /// one StatefulSet and one ConfigMap per role group, one headless plus one metrics Service per
    /// role group, and one default PDB per role.
    #[test]
    fn build_produces_expected_resource_names() {


This test case is good. It checks that the build function returns all expected resources. It just checks the names and not the contents because it relies on the fact that the builder functions have been tested in their own unit tests.

Service accounts and role bindings were added to the returned resources. They must also be added here and the first half of the test case build_produces_rbac can be removed.

siegfriedweber · 2026-07-24T14:09:35Z

+    /// swapped `name`/`instance` label values cannot pass unnoticed (the shared fixture is named
+    /// `hdfs`, which would mask exactly that swap).
+    #[test]
+    fn build_produces_rbac() {


The produced RBAC resources should actually be tested in the rbac module.

siegfriedweber · 2026-07-24T14:20:14Z

+        let product_version = ProductVersion::from_str(&image.app_version_label_value)
+            .expect("the app version label value is a valid product version");


What's the purpose of image.product_version if only image.app_version_label_value is used?

image.product_version is used when upgrading e.g. https://github.com/stackabletech/hdfs-operator/blob/feat/smooth-operator/build-rbac/rust/operator-binary/src/hdfs_controller.rs#L292.

Having the whole label there would cause problems.

siegfriedweber · 2026-07-24T14:25:36Z

+
+    /// Returns an [`ObjectMetaBuilder`] pre-filled with the namespace, the resource `name`, an owner
+    /// reference back to this cluster, and the given recommended `labels`.
+    pub(crate) fn object_meta(&self, name: impl Into<String>, labels: Labels) -> ObjectMetaBuilder {


Functions which build Kubernetes resources or parts of them, should be located in the build step. To put it somewhat pointedly: If building parts of Kubernetes resources in ValidatedCluster is a good idea, then impl From<ValidatedCluster> for StatefulSet would be even better.

Would you say the same applies to governing_service_name? (directly under object_meta)?

Moved object_meta: 43207ec

adwk67 mentioned this pull request Jul 20, 2026

tracking: Smooth Operator Iteration 4: Build RBAC in build() with recommended labels stackabletech/issues#869

Open

12 tasks

Base automatically changed from feat/smooth-operator/introduce-builder to main July 21, 2026 15:15

adwk67 self-assigned this Jul 21, 2026

factor: add infallible rbac functions

1c5b9cd

adwk67 force-pushed the feat/smooth-operator/build-rbac branch from 3327a68 to 1c5b9cd Compare July 22, 2026 08:44

adwk67 added 7 commits July 22, 2026 10:52

regenerate nix

827580e

changelog

26f2390

switch op-rs to main branch

0efc549

rename resource names functions, add role parse test

c97d2da

merge main

593c69c

merge main and refer to op-rs 0.114.0

5198088

replace role_name with From impls, add missing labels

f5b2e16

siegfriedweber self-requested a review July 24, 2026 10:09

siegfriedweber requested changes Jul 24, 2026

View reviewed changes

adwk67 added 3 commits July 24, 2026 16:58

added missed role/From instance

06c9f7e

improve tests: placement and thoroughness

b45ed81

move object_meta to build step

43207ec

siegfriedweber mentioned this pull request Jul 24, 2026

refactor: Build RBAC with recommended labels stackabletech/kafka-operator#990

Open

18 tasks

		let product_version = ProductVersion::from_str(&image.app_version_label_value)
		.expect("the app version label value is a valid product version");

Uh oh!

Uh oh!

Conversation

adwk67 commented Jul 20, 2026 • edited by siegfriedweber Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Definition of Done Checklist

Author

Reviewer

Acceptance

Uh oh!

adwk67 commented Jul 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adwk67 commented Jul 20, 2026 •

edited by siegfriedweber

Loading