Authentication for metrics and version endpoint #1607
Conversation
naveenpaul1
force-pushed
the
metrics-auth-token
branch
from
0788a0f to
1286643
Compare
naveenpaul1
requested review from
dannyzaken,
guymguym,
jackyalbo,
romayalon and
tangledbytes
naveenpaul1
force-pushed
the
metrics-auth-token
branch
from
1286643 to
3523050
Compare
naveenpaul1
force-pushed
the
metrics-auth-token
branch
from
3523050 to
7c92b87
Compare
naveenpaul1
force-pushed
the
metrics-auth-token
branch
from
7c92b87 to
d286e70
Compare
naveenpaul1
force-pushed
the
metrics-auth-token
branch
from
d286e70 to
d49700f
Compare
WalkthroughThis change introduces metrics and version endpoint authorization to the system. It adds two new environment variables to the core StatefulSet and endpoint Deployment, manages a new Kubernetes secret for metrics authorization, and updates the reconciler logic to create and inject a metrics token into Prometheus ServiceMonitors for secure endpoint access. Changes
Sequence Diagram(s)sequenceDiagram
participant Operator
participant Reconciler
participant Kubernetes API
participant NBClient
participant Prometheus
Operator->>Reconciler: Start reconciliation
Reconciler->>Kubernetes API: Ensure SecretMetricsAuth exists
alt Secret does not exist
Reconciler->>NBClient: CreateAuthAPI(metrics role)
NBClient-->>Reconciler: Return metrics token
Reconciler->>Kubernetes API: Create Secret with metrics token
end
Reconciler->>Kubernetes API: Patch StatefulSet and Deployment with METRICS_AUTH_ENABLED and VERSION_AUTH_ENABLED
Reconciler->>Kubernetes API: Patch ServiceMonitors with Bearer token ref to SecretMetricsAuth
Prometheus->>Kubernetes API: Scrape endpoints using Bearer token
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/system/phase4_configuring.go (1)
1672-1684: Nil-slice guard & namespace explicitness
endpointsmay be nil if a user prunes endpoints from the CRD; add a quick nil-check to avoid panic.- You rely on defaulting to the ServiceMonitor’s namespace. Being explicit increases readability and avoids surprises if cross-namespace ServiceMonitor support lands.
func (r *Reconciler) setServiceMonitorAuthorization(endpoints []monitoringv1.Endpoint) { - for i := range endpoints { + if endpoints == nil { + return + } + for i := range endpoints { endpoints[i].Authorization = &monitoringv1.SafeAuthorization{ Type: "Bearer", Credentials: &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ - Name: r.SecretMetricsAuth.Name, + Name: r.SecretMetricsAuth.Name, + Namespace: r.SecretMetricsAuth.Namespace, // be explicit }, Key: metricsAuthKey, }, } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
deploy/internal/statefulset-core.yaml(1 hunks)pkg/bundle/deploy.go(2 hunks)pkg/system/phase2_creating.go(1 hunks)pkg/system/phase4_configuring.go(3 hunks)pkg/system/reconciler.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/system/reconciler.go (3)
pkg/util/util.go (1)
KubeObject(276-286)pkg/bundle/deploy.go (1)
File_deploy_internal_secret_empty_yaml(4858-4865)pkg/options/options.go (1)
Namespace(60-60)
pkg/system/phase4_configuring.go (4)
pkg/system/reconciler.go (1)
Reconciler(59-133)pkg/util/util.go (1)
SecretResetStringDataFromData(958-968)pkg/nb/types.go (1)
CreateAuthParams(320-325)pkg/options/options.go (1)
OperatorAccountEmail(47-47)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-dev-test
- GitHub Check: golangci-lint
- GitHub Check: run-cli-tests
- GitHub Check: run-admission-test
- GitHub Check: run-hac-test
- GitHub Check: run-operator-tests
🔇 Additional comments (4)
pkg/system/phase2_creating.go (1)
1420-1422: Set default auth flags for metrics and version endpoints
EnsuresMETRICS_AUTH_ENABLEDandVERSION_AUTH_ENABLEDdefault to"true"in the core ConfigMap, matching the new StatefulSet environment variables.deploy/internal/statefulset-core.yaml (1)
104-113: Enable runtime auth flags for metrics and version endpoints
AddedMETRICS_AUTH_ENABLEDandVERSION_AUTH_ENABLEDenv vars sourced from thenoobaa-configConfigMap. This aligns with the reconciler defaults and allows toggling endpoint auth without redeploying the pod.pkg/bundle/deploy.go (1)
5047-5047:✅ Verification successful
Verify updated SHA256 checksum
The checksum constant was bumped after adding environment variables to the core StatefulSet manifest. Please confirm that this value exactly matches the SHA256 ofdeploy/internal/statefulset-core.yamlto avoid bundle mismatches.
🏁 Script executed:
#!/bin/bash # Verify the Sha256_deploy_internal_statefulset_core_yaml matches the file content. expected="94af9a850a7bf063f236f7ff3c8a23630a882329137bd854e33b040cec52de44" actual=$(sha256sum deploy/internal/statefulset-core.yaml | awk '{print $1}') if [ "$expected" != "$actual" ]; then echo "❌ SHA mismatch: expected $expected, got $actual" exit 1 fi echo "✅ SHA matches"Length of output: 549
🏁 Script executed:
#!/bin/bash # Re-verify the SHA256 checksum using shasum since sha256sum is unavailable. expected="94af9a850a7bf063f236f7ff3c8a23630a882329137bd854e33b040cec52de44" actual=$(shasum -a 256 deploy/internal/statefulset-core.yaml | awk '{print $1}') if [ "$expected" != "$actual" ]; then echo "❌ SHA mismatch: expected $expected, got $actual" exit 1 fi echo "✅ SHA matches"Length of output: 550
🏁 Script executed:
#!/usr/bin/env bash # Use Python to compute SHA256 since sha256sum and shasum are unavailable. expected="94af9a850a7bf063f236f7ff3c8a23630a882329137bd854e33b040cec52de44" actual=$(python3 - << 'EOF' import hashlib, sys try: data = open('deploy/internal/statefulset-core.yaml', 'rb').read() print(hashlib.sha256(data).hexdigest()) except Exception as e: sys.exit(1) EOF ) if [ -z "$actual" ]; then echo "❌ Failed to compute SHA256. Please verify manually. " exit 1 fi if [ "$expected" != "$actual" ]; then echo "❌ SHA mismatch: expected $expected, got $actual" exit 1 fi echo "✅ SHA matches"Length of output: 422
Checksum Verified for StatefulSet Manifest
The SHA256 constant inpkg/bundle/deploy.go(line 5047) matches the computed SHA256 ofdeploy/internal/statefulset-core.yaml(94af9a850a7bf063f236f7ff3c8a23630a882329137bd854e33b040cec52de44). No further action is needed.pkg/system/phase4_configuring.go (1)
49-50: 👍 Dedicated key constantUsing a single, file-local
metricsAuthKeyconstant prevents typos in multiple call-sites.
pkg/bundle/deploy.go
Outdated
| - name: METRICS_AUTH_ENABLED | ||
| valueFrom: | ||
| configMapKeyRef: | ||
| name: noobaa-config | ||
| key: METRICS_AUTH_ENABLED | ||
| - name: VERSION_AUTH_ENABLED | ||
| valueFrom: | ||
| configMapKeyRef: | ||
| name: noobaa-config | ||
| key: VERSION_AUTH_ENABLED |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Validate ConfigMap defaults and indentation for new flags
- Ensure that
noobaa-configdefines default entries forMETRICS_AUTH_ENABLEDandVERSION_AUTH_ENABLED(e.g.,"false"). Missing keys will result in empty env values in the pod. - Double-check that the
- name:andvalueFrom:lines align with existing env entries (same two-space YAML indentation per level) to prevent parsing errors.
🏁 Script executed:
#!/bin/bash
# Check that defaults exist in the internal ConfigMap manifest
grep -E "METRICS_AUTH_ENABLED|VERSION_AUTH_ENABLED" deploy/internal/configmap*.yamlLength of output: 174
Action Required: Add default entries for METRICS_AUTH_ENABLED and VERSION_AUTH_ENABLED in noobaa-config
- No occurrences of these keys were found in the internal ConfigMap manifests (e.g.,
deploy/internal/configmap-*.yaml), so pods will receive empty values at runtime. - Please update each
deploy/internal/configmap-*.yamlunder thedata:section with defaults, for example:data: METRICS_AUTH_ENABLED: "false" VERSION_AUTH_ENABLED: "false" - Verify that in
pkg/bundle/deploy.go(around lines 5152–5161) the new env entries use the same two-space indentation per YAML level as existing entries to avoid parsing errors.
🤖 Prompt for AI Agents
In pkg/bundle/deploy.go around lines 5152 to 5161, ensure the new environment
variables METRICS_AUTH_ENABLED and VERSION_AUTH_ENABLED have the same two-space
YAML indentation as existing env entries to avoid parsing errors. Additionally,
update all deploy/internal/configmap-*.yaml files by adding default values for
these keys under the data: section, for example, METRICS_AUTH_ENABLED: "false"
and VERSION_AUTH_ENABLED: "false", so pods receive proper default values at
runtime.
| SecretMetricsAuth *corev1.Secret | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Hard-coded secret name can collide when multiple NooBaa CRs share a namespace
SecretMetricsAuth is always named metrics-auth-secret.
If two NooBaa CRs are deployed in the same namespace (common in test/CI scenarios) the second reconcile will fail with “already exists” error.
- r.SecretMetricsAuth.Name = "metrics-auth-secret"
+// keep name stable for ServiceMonitors but unique per-system
+ r.SecretMetricsAuth.Name = r.Request.Name + "-metrics-auth"Remember to update all references (setServiceMonitorAuthorization, RBAC, docs).
Also applies to: 199-200, 248-249, 294-295
🏁 Script executed:
#!/bin/bash
# Locate reconciler.go
fd --max-depth 4 --name reconciler.go
# Show relevant snippet around lines 128–130
sed -n '120,150p' pkg/system/reconciler.go
# Find all occurrences of the hard-coded secret name
rg -n '"metrics-auth-secret"' .
# Find all assignments to SecretMetricsAuth.Name
rg -n 'SecretMetricsAuth\.Name' -n pkg/system/reconciler.goLength of output: 1725
🏁 Script executed:
#!/bin/bash
# Show the reconciler logic around SecretMetricsAuth and ServiceMonitor setup
sed -n '240,360p' pkg/system/reconciler.go
# Locate where setServiceMonitorAuthorization is invoked
rg -n 'setServiceMonitorAuthorization' -n pkg/system/reconciler.goLength of output: 5312
Use a per-CR unique name for the metrics-auth secret
The reconciler currently hard-codes
r.SecretMetricsAuth.Name = "metrics-auth-secret"which will collide if you deploy more than one NooBaa CR in the same namespace. Please change it to derive the name from the CR’s name, for example:
- r.SecretMetricsAuth.Name = "metrics-auth-secret"
+ // keep name stable for ServiceMonitors but unique per-CR
+ r.SecretMetricsAuth.Name = r.Request.Name + "-metrics-auth"• pkg/system/reconciler.go: replace the hard-coded assignment at line 294.
• Verify and update any ServiceMonitor specs, RBAC Role/RoleBinding rules, helper functions or documentation that refer to "metrics-auth-secret".
🤖 Prompt for AI Agents
In pkg/system/reconciler.go around lines 128-129 and also at lines 199-200,
248-249, and 294-295, the SecretMetricsAuth.Name is hard-coded as
"metrics-auth-secret", which causes conflicts when multiple NooBaa CRs are
deployed in the same namespace. To fix this, change the secret name assignment
to derive it uniquely from the CR's name, for example by appending or prefixing
the CR name to "metrics-auth-secret". Also update all references to this secret
name in setServiceMonitorAuthorization, RBAC rules, and documentation to use the
new dynamic naming scheme to avoid collisions.
naveenpaul1
force-pushed
the
metrics-auth-token
branch
from
d49700f to
9c2144c
Compare
Signed-off-by: naveenpaul1 <[email protected]>
naveenpaul1
force-pushed
the
metrics-auth-token
branch
from
9c2144c to
e4c340c
Compare
Explain the changes
metricsto get the JWT token in the configuration phase, And the response token will be saved as secret(metrics-auth-secret)Issues: Fixed #xxx / Gap #xxx
DFBUGS-1802
Testing Instructions:
containerized deployment
metrics-auth-secret, secret can be used for accessing noobaa management and endpoint metrics/version.Design doc : https://ibm.ent.box.com/notes/1853310270159
Summary by CodeRabbit
Summary by CodeRabbit