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) + } +}