From 82a49f1258289efaca3299f193da0f8680ed88ee Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Wed, 19 Aug 2026 17:32:25 -0400 Subject: [PATCH] OCPBUGS-112272: node ServiceMonitor serverName uses guest namespace on HyperShift The node metrics ServiceMonitor's TLS serverName was built from ${NAMESPACE} (control-plane namespace) instead of the guest namespace. On HyperShift these namespaces differ, so serverName never matched the serving certificate's SANs, every node metrics scrape failed certificate verification, and TargetDown fired continuously. On standalone the two namespaces are identical, so the defect was invisible there. The shared patch common/metrics/service_monitor_add_port.yaml.patch hardcoded ${NAMESPACE}, which is only correct for the controller monitor. The node metrics Service runs guest-side and its serving certificate is issued for the guest namespace. Introduce a ${MONITOR_NAMESPACE} template variable in the patch and resolve it per service prefix in the generator: ${NODE_NAMESPACE} for the node monitor and ${NAMESPACE} for the controller monitor. Standalone output is functionally unchanged since both variables resolve to openshift-cluster-csi-drivers. --- .../service_monitor_add_port.yaml.patch | 2 +- .../hypershift/node_servicemonitor.yaml | 2 +- .../standalone/node_servicemonitor.yaml | 2 +- pkg/generator/asset_generator.go | 16 ++++ pkg/generator/asset_generator_test.go | 95 +++++++++++++++++++ 5 files changed, 114 insertions(+), 3 deletions(-) create mode 100644 pkg/generator/asset_generator_test.go diff --git a/assets/common/metrics/service_monitor_add_port.yaml.patch b/assets/common/metrics/service_monitor_add_port.yaml.patch index f2a3cd33a..e71c057c2 100644 --- a/assets/common/metrics/service_monitor_add_port.yaml.patch +++ b/assets/common/metrics/service_monitor_add_port.yaml.patch @@ -10,4 +10,4 @@ scheme: https tlsConfig: caFile: /etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt - serverName: ${ASSET_PREFIX}-${SERVICE_PREFIX}-metrics.${NAMESPACE}.svc + serverName: ${ASSET_PREFIX}-${SERVICE_PREFIX}-metrics.${MONITOR_NAMESPACE}.svc diff --git a/assets/overlays/azure-disk/generated/hypershift/node_servicemonitor.yaml b/assets/overlays/azure-disk/generated/hypershift/node_servicemonitor.yaml index 1268095b8..eed68aa62 100644 --- a/assets/overlays/azure-disk/generated/hypershift/node_servicemonitor.yaml +++ b/assets/overlays/azure-disk/generated/hypershift/node_servicemonitor.yaml @@ -19,7 +19,7 @@ spec: scheme: https tlsConfig: caFile: /etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt - serverName: azure-disk-csi-driver-node-metrics.${NAMESPACE}.svc + serverName: azure-disk-csi-driver-node-metrics.${NODE_NAMESPACE}.svc jobLabel: component selector: matchLabels: diff --git a/assets/overlays/azure-disk/generated/standalone/node_servicemonitor.yaml b/assets/overlays/azure-disk/generated/standalone/node_servicemonitor.yaml index 1268095b8..eed68aa62 100644 --- a/assets/overlays/azure-disk/generated/standalone/node_servicemonitor.yaml +++ b/assets/overlays/azure-disk/generated/standalone/node_servicemonitor.yaml @@ -19,7 +19,7 @@ spec: scheme: https tlsConfig: caFile: /etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt - serverName: azure-disk-csi-driver-node-metrics.${NAMESPACE}.svc + serverName: azure-disk-csi-driver-node-metrics.${NODE_NAMESPACE}.svc jobLabel: component selector: matchLabels: diff --git a/pkg/generator/asset_generator.go b/pkg/generator/asset_generator.go index a295e5ec1..85a619548 100644 --- a/pkg/generator/asset_generator.go +++ b/pkg/generator/asset_generator.go @@ -170,6 +170,20 @@ func (gen *AssetGenerator) generateDeployment() error { return nil } +// monitorNamespaceVariable returns the runtime namespace variable that the +// ServiceMonitor's TLS serverName must resolve to for the given servicePrefix. +// +// The node metrics Service runs guest-side, so its serving certificate is issued +// for the guest namespace (${NODE_NAMESPACE}). The controller and its metrics +// Service run control-plane side, so they use ${NAMESPACE}. On HyperShift these +// two namespaces differ; on standalone they are identical. See OCPBUGS-112272. +func monitorNamespaceVariable(servicePrefix string) string { + if servicePrefix == "node" { + return "${NODE_NAMESPACE}" + } + return "${NAMESPACE}" +} + // Add driver's metrics port to the metrics Service and ServiceMonitor. func (gen *AssetGenerator) generateDriverMetricsService(serviceYAML, serviceMonitorYAML *YAMLWithHistory, localMetricsPort, exposedMetricsPort uint16, servicePrefix string) error { if localMetricsPort == 0 { @@ -181,6 +195,7 @@ func (gen *AssetGenerator) generateDriverMetricsService(serviceYAML, serviceMoni "${EXPOSED_METRICS_PORT}", strconv.Itoa(int(exposedMetricsPort)), "${PORT_NAME}", "driver-m", "${SERVICE_PREFIX}", servicePrefix, + "${MONITOR_NAMESPACE}", monitorNamespaceVariable(servicePrefix), } var err error err = gen.applyAssetPatch(serviceYAML, "common/metrics/service_add_port.yaml", extraReplacements) @@ -208,6 +223,7 @@ func (gen *AssetGenerator) generateSidecarMetricsServices(serviceYAML, serviceMo "${EXPOSED_METRICS_PORT}", strconv.Itoa(exposedPortIndex), "${PORT_NAME}", sidecar.MetricPortName, "${SERVICE_PREFIX}", servicePrefix, + "${MONITOR_NAMESPACE}", monitorNamespaceVariable(servicePrefix), } localPortIndex++ exposedPortIndex++ diff --git a/pkg/generator/asset_generator_test.go b/pkg/generator/asset_generator_test.go new file mode 100644 index 000000000..12741dc85 --- /dev/null +++ b/pkg/generator/asset_generator_test.go @@ -0,0 +1,95 @@ +package generator + +import ( + "strings" + "testing" + + "github.com/openshift/csi-operator/assets" + generated_assets "github.com/openshift/csi-operator/pkg/generated-assets" +) + +// newMonitoringTestGenerator builds an AssetGenerator wired to the real embedded +// assets with a minimal config that only exercises the metrics ServiceMonitor +// generation path (driver metrics port set, no sidecars). +func newMonitoringTestGenerator(flavour ClusterFlavour) *AssetGenerator { + cfg := &CSIDriverGeneratorConfig{ + AssetPrefix: "test-csi-driver", + AssetShortPrefix: "test", + DriverName: "test.csi.example.com", + ControllerConfig: &ControlPlaneConfig{ + LocalMetricsPort: 8211, + ExposedMetricsPort: 9211, + }, + GuestConfig: &GuestConfig{ + LocalMetricsPort: 8206, + ExposedMetricsPort: 9206, + }, + } + gen := NewAssetGenerator(flavour, cfg, assets.ReadFile) + gen.controllerAssets = make(map[string]*YAMLWithHistory) + gen.guestAssets = make(map[string]*YAMLWithHistory) + return gen +} + +// renderedServerName extracts the tlsConfig.serverName from a rendered +// ServiceMonitor asset. +func renderedServerName(t *testing.T, rendered []byte) string { + t.Helper() + const key = "serverName:" + for _, line := range strings.Split(string(rendered), "\n") { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, key) { + return strings.TrimSpace(strings.TrimPrefix(trimmed, key)) + } + } + t.Fatalf("no serverName found in rendered ServiceMonitor:\n%s", rendered) + return "" +} + +// TestNodeServiceMonitorServerNameUsesNodeNamespace verifies that the node +// metrics ServiceMonitor's TLS serverName resolves to the guest namespace +// (${NODE_NAMESPACE}) so that it matches the serving certificate's SANs on +// HyperShift, where the guest and control-plane namespaces differ. +// +// Regression test for OCPBUGS-112272. +func TestNodeServiceMonitorServerNameUsesNodeNamespace(t *testing.T) { + gen := newMonitoringTestGenerator(FlavourHyperShift) + + if err := gen.generateGuestMonitoringService(); err != nil { + t.Fatalf("generateGuestMonitoringService failed: %v", err) + } + + sm, ok := gen.guestAssets[generated_assets.NodeMetricServiceMonitorAssetName] + if !ok { + t.Fatalf("node ServiceMonitor asset %q was not generated", generated_assets.NodeMetricServiceMonitorAssetName) + } + + serverName := renderedServerName(t, sm.Render()) + want := "test-csi-driver-node-metrics.${NODE_NAMESPACE}.svc" + if serverName != want { + t.Errorf("node ServiceMonitor serverName = %q, want %q", serverName, want) + } +} + +// TestControllerServiceMonitorServerNameUsesControlPlaneNamespace verifies that +// the controller metrics ServiceMonitor keeps resolving its serverName to the +// control-plane namespace (${NAMESPACE}), where the controller and its metrics +// Service run. +func TestControllerServiceMonitorServerNameUsesControlPlaneNamespace(t *testing.T) { + gen := newMonitoringTestGenerator(FlavourStandalone) + + if err := gen.generateControllerMonitoringService(); err != nil { + t.Fatalf("generateControllerMonitoringService failed: %v", err) + } + + sm, ok := gen.controllerAssets[generated_assets.ControllerMetricServiceMonitorAssetName] + if !ok { + t.Fatalf("controller ServiceMonitor asset %q was not generated", generated_assets.ControllerMetricServiceMonitorAssetName) + } + + serverName := renderedServerName(t, sm.Render()) + want := "test-csi-driver-controller-metrics.${NAMESPACE}.svc" + if serverName != want { + t.Errorf("controller ServiceMonitor serverName = %q, want %q", serverName, want) + } +}