RHOAIENG-38496 | feat: Add TLS support for Perses-Tempo datasource #2888
Conversation
WalkthroughAdds storage-backend constants and extends TracesStorage with backend/size/secret/retention; centralizes TLS/protocol decision (TLS disabled by default unless explicitly enabled); injects a tempo-service-ca ConfigMap template and constant; updates template-data generation, templates, unit and e2e TLS tests (some duplicates present). Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(248,249,250)
participant Controller as Controller
participant Support as SupportLogic
participant Renderer as TemplateRenderer
participant K8s as Kubernetes/Tempo
end
Controller->>Support: request traces template data (Traces spec)
Support->>Support: determineTLSEnabled(traces) %% defaults false unless explicit
alt TLS enabled
Support->>Renderer: TempoTLSEnabled=true, protocol=https, set certName/caName, TempoCertificateSecret
else TLS disabled
Support->>Renderer: TempoTLSEnabled=false, protocol=http, clear cert/CA keys
end
Note over Support,Renderer: select endpoints using backend constants (pv / s3 / gcs) and include Retention/Size
Support->>Renderer: provide TempoQueryEndpoint, OtlpEndpoint, Backend, Retention, Size
Renderer->>K8s: render & apply manifests (includes tempo-service-ca ConfigMap when templated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/19561569399 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/services/v1alpha1/monitoring_types.go (1)
98-144: Fix TracesStorage XValidation placement and alignsecretrule with message
+kubebuilder:validation:XValidationfor thesecretfield currently sits above theconstblock, so it will be associated with the consts (or ignored) rather than withTracesStorage. That means the intended CEL validation for non‑PV backends likely does not apply. Also, the rule only checkshas(self.secret)while the message claims “specified and non-empty”.To ensure the validation is actually enforced and matches the message, move and tighten the rule so it’s directly on the
TracesStoragetype, e.g.:-// TracesStorage defines the storage configuration for tracing. -// +kubebuilder:validation:XValidation:rule="self.backend != 'pv' ? has(self.secret) : true", message="When backend is s3 or gcs, the 'secret' field must be specified and non-empty" -// Storage backend type constants -const ( +// Storage backend type constants +const ( // StorageBackendPV represents persistent volume storage backend StorageBackendPV = "pv" @@ -) - -// +kubebuilder:validation:XValidation:rule="self.backend != 'pv' ? !has(self.size) : true", message="Size is supported when backend is pv only" -type TracesStorage struct { +) + +// TracesStorage defines the storage configuration for tracing. +// +kubebuilder:validation:XValidation:rule="self.backend != 'pv' ? (has(self.secret) && self.secret != '') : true", message="When backend is s3 or gcs, the 'secret' field must be specified and non-empty" +// +kubebuilder:validation:XValidation:rule="self.backend != 'pv' ? !has(self.size) : true", message="Size is supported when backend is pv only" +type TracesStorage struct {This keeps the new backend constants, enforces that S3/GCS always specify a non‑empty secret, and ensures both CEL rules are actually attached to the CRD schema for
TracesStorage.[scratchpad_end] -->
🧹 Nitpick comments (4)
internal/controller/services/monitoring/monitoring_controller_actions.go (1)
25-49: CA ConfigMap template is correctly wired; consider optional cleanup on traces removalAdding
TempoServiceCAConfigMapTemplateand appending it indeployPersesTempoIntegrationensures the CA ConfigMap is always created next to the Tempo datasource, which is required for the TLS client config.One minor behavioral note: when
Spec.Tracesbecomes nil, the code deletes the Perses datasource and dashboard but does not remove thetempo-service-caConfigMap. That’s harmless but leaves a small orphan. If you want fully symmetric cleanup, you could optionally delete that ConfigMap in the same branch.Also applies to: 543-552
tests/e2e/monitoring_test.go (1)
1665-1835: End-to-end TLS tests for S3/GCS backends are well-structured; consider a small helper to reduce duplicationBoth
ValidatePersesDatasourceTLSWithS3BackendandValidatePersesDatasourceTLSWithGCSBackend:
- Ensure PersesDatasource CRD existence and start from a clean monitoring state.
- Create a backend-specific secret in the monitoring namespace.
- Enable traces with the appropriate backend and
.spec.monitoring.traces.tls.enabled = true.- Wait for
MonitoringandTempoStackto be ready with the right storage secret.- Assert that the Perses Tempo datasource:
- Uses an HTTPS gateway URL on port 8080.
- Has
client.tls.enable == trueand CA config pointing at thetempo-service-caConfigMap andservice-ca.crt.- Clean up traces config, TempoStack, and secrets.
The logic is solid and matches the intended TLS behavior. The two functions are nearly identical apart from backend/secret names; if you find yourself adding more backends later, you could factor them through a common helper (similar to
validateTempoStackCreationWithBackend) to cut duplication.internal/controller/services/monitoring/monitoring_controller_support_test.go (1)
929-996: TLS unit tests match the new behavior; a couple of tiny polish opportunities
TestDetermineTLSEnabledandTestAddTracesTemplateData_TLSaccurately pin down the new TLS semantics:
- TLS is true only when
traces.TLSis non‑nil andEnabledis true; otherwise false regardless of backend.addTracesTemplateDatasetsTempoTLSEnabled, chooseshttpvshttpsaccordingly, and builds the correctTempoQueryEndpointfor PV (:3200) vs S3 (:8080gateway), while also ensuringOtlpEndpoint,SampleRatio,Backend, andTracesRetentionare present.Optional nits:
- In
TestDetermineTLSEnabled, you could useserviceApi.StorageBackendPV/S3/GCSinstead of raw"pv"/"s3"/"gcs"strings to keep tests aligned with the exported constants.- In
TestAddTracesTemplateData_TLS, theexpectedHTTPProtocolfield isn’t used; either drop it or assert on the scheme via the endpoint to avoid dead fields.Functionally, the tests look good.
Also applies to: 998-1107
internal/controller/services/monitoring/monitoring_controller_support.go (1)
208-259: TLS/template data behavior is coherent; consider deduplicating shared trace fieldsThe refactor around traces template data behaves consistently:
determineTLSEnablednow returns true only iftraces.TLSis non‑nil andEnabledis true; otherwise TLS is off, which matches the new “TLS disabled by default” behavior exercised by the unit and e2e tests.addTracesTemplateDatauses that flag to:
- Set
TempoTLSEnabled.- Pick
httpvshttpsand buildTempoQueryEndpointcorrectly for:
- PV →
tempo-data-science-tempomonolithic.<ns>.svc.cluster.local:3200- S3/GCS →
tempo-data-science-tempostack-gateway.<ns>.svc.cluster.local:8080setTempoEndpointAndStorageDatanow switches onserviceApi.StorageBackendPV/S3/GCS, which avoids hard‑coded backend strings.addTracesDatahandles TLS secrets (TempoCertificateSecret,TempoCAConfigMap) and always initializes them to""when TLS is disabled, preventing template key errors.One minor cleanup opportunity:
OtlpEndpoint,SampleRatio,Backend,TempoTLSEnabled, andTracesRetentionare now set in bothaddTracesDataandaddTracesTemplateDatawith the same values. It’s harmless (last writer wins with identical content), but you could centralize those assignments in one helper to reduce duplication and make future changes less error‑prone.Overall, the TLS enablement and endpoint selection logic look correct and align with the new tests.
Also applies to: 536-575, 578-587
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/services/v1alpha1/monitoring_types.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(3 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/controller/services/monitoring/monitoring_controller_support_test.go (1)
api/services/v1alpha1/monitoring_types.go (6)
Traces(82-96)TracesTLS(99-109)TracesStorage(124-144)StorageBackendPV(116-116)StorageBackendS3(118-118)StorageBackendGCS(120-120)
internal/controller/services/monitoring/monitoring_controller_support.go (1)
api/services/v1alpha1/monitoring_types.go (4)
StorageBackendPV(116-116)StorageBackendS3(118-118)StorageBackendGCS(120-120)Traces(82-96)
tests/e2e/monitoring_test.go (7)
tests/e2e/resource_options_test.go (4)
WithCustomErrorMsg(287-291)WithMinimalObject(155-170)WithCondition(262-266)WithEventuallyTimeout(294-298)pkg/cluster/resources.go (1)
HasCRD(366-368)pkg/cluster/gvk/gvk.go (3)
PersesDatasource(562-566)TempoStack(514-518)Namespace(28-32)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)internal/controller/status/status.go (1)
ConditionTypeReady(74-74)tests/e2e/controller_test.go (1)
TestTimeouts(100-108)
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build/push catalog image
- GitHub Check: golangci-lint
- GitHub Check: Run tests and collect coverage on internal and pkg
🔇 Additional comments (4)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
19-27: TLS client configuration for Perses Tempo datasource looks sound (yamllint warning is template-related)The conditional
client.tlsblock keyed off.TempoTLSEnabledis consistent with the template data (TempoQueryEndpoint+ TLS flag) and the e2e assertions onspec.config.plugin.spec.proxy.spec.client.tls.*. The use of{{- if ...}}/{{- end }}will render correct YAML; the YAMLlint “expected ':'” error is due to linting the raw Go template, not the rendered manifest.No changes needed here.
internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml (1)
1-8: Service CA ConfigMap template is correctly wired for TLS useThe
tempo-service-caConfigMap withservice.beta.openshift.io/inject-cabundle: "true"and emptydatais exactly what the Perses datasource TLS block expects (caCert.type=configmap,name=tempo-service-ca,certPath=service-ca.crt). This should integrate cleanly with OpenShift service CA injection.internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (1)
15-19: Multitenancy and TLS server wiring look consistent; just verify field names against Tempo CRDThe new multitenancy block (enabled=true, mode=openshift, auth derived from
Namespace) and the TLS sections usingcertName/caNameplus optionalserver.http_tls_configgated byTempoCertificateSecretalign with the template data and comments. This should give you OpenShift-style multitenancy and optional explicit HTTP TLS config while still allowing the Tempo operator to use serving certs when no explicit secret is provided.Please double-check that
certName,caName, andserver.http_tls_configmatch the currentTempoMonolithicCRD schema to avoid silent no-ops.Also applies to: 32-37, 42-46, 52-60
tests/e2e/monitoring_test.go (1)
122-123: Perses datasource expectations updated correctly for “TLS disabled by default”Registering the two new TLS test cases in the monitoring test suite and updating
ValidatePersesDatasourceConfigurationto expect an HTTP Tempo endpoint when TLS is not configured matches the newdetermineTLSEnabledbehavior (TLS off unless explicitly enabled). The custom error message now clearly documents this default, which is helpful for future debugging.Also applies to: 1445-1465
dayakar349
force-pushed
the
RHOAIENG-38496
branch
from
b609ecb to
cabb1db
Compare
openshift-ci
bot
requested review from
CFSNM,
StevenTobin,
den-rgb and
kahowell
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/controller/services/monitoring/monitoring_controller_actions.go (1)
25-53: Tempo service CA ConfigMap template is wired correctly into Perses datasource deploymentThe
TempoServiceCAConfigMapTemplateconstant and its inclusion indeployPersesTempoIntegrationcleanly ensure the CA bundle ConfigMap is applied whenever the PersesTempo datasource is managed (and the CRDs exist). If you want to avoid creating this resource when tracing/TLS is entirely unused, you could optionally gate adding this template onmonitoring.Spec.Traces(or the TLS flag), but functionally this is fine as-is.Also applies to: 545-552
internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (1)
15-60: Confirm TempoMonolithic multitenancy and TLS field names against Tempo operator APIThe new multitenancy block (
enabled: true,mode: openshift, namespace‑derived tenant mapping) and the TLS updates (certName/caNameplusextraConfig.tempo.server.http_tls_configgated onTempoCertificateSecret) look aligned with the intended behavior and the broader TLS changes in this PR.Because these fields are owned by the external
tempo.grafana.com/v1alpha1API and current e2e coverage forTempoMonolithiconly checks storage/retention, please:
- Double‑check that
multitenancy.*,tls.certName, andtls.caNameare the correct field names for the Tempo operator versions you support.- Consider extending
ValidateTempoMonolithicCRCreation(or a new test) to assert at least one of the new multitenancy/TLS fields so regressions in this area are caught.tests/e2e/monitoring_test.go (2)
1446-1465: Clarify wording around default TLS behavior in Perses datasource config testThe test correctly validates the HTTP (non‑TLS) endpoint when
traces.tlsisn’t enabled. To avoid confusion with the API’sTracesTLS.enableddefault, consider rephrasing the comment and error message to something like “when TLS is not enabled” rather than “TLS is disabled by default”.
122-123: New S3/GCS TLS e2e tests for PersesDatasource are thorough and well‑isolatedThe added
ValidatePersesDatasourceTLSWithS3Backend/...WithGCSBackendtests:
- Properly gate on
PersesDatasourceCRD presence.- Start from a clean monitoring state, create backend‑specific secrets, enable
traces.tls.enabled, and wait forTempoStackReady.- Assert the expected HTTPS gateway URL and full
client.tlsconfig (includingtempo-service-caConfigMap andservice-ca.crtpath), then clean up traces, TempoStack, and secrets.This gives good end‑to‑end coverage of the new TLS behavior. If you want to tighten things further, you could:
- Also assert that
ConditionPersesTempoDataSourceAvailableisTrueinMonitoring.status.conditionsin these tests.- Extract a small helper (similar to
validateTempoStackCreationWithBackend) to reduce duplication between the S3 and GCS flows.Also applies to: 1665-1835
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(2 hunks)docs/api-overview.md(2 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(3 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controller/services/monitoring/monitoring_controller_support.go
- api/services/v1alpha1/monitoring_types.go
- internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml
- internal/controller/services/monitoring/monitoring_controller_support_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/monitoring_test.go (7)
tests/e2e/resource_options_test.go (4)
WithCustomErrorMsg(287-291)WithMinimalObject(155-170)WithCondition(262-266)WithEventuallyTimeout(294-298)pkg/cluster/resources.go (1)
HasCRD(366-368)pkg/cluster/gvk/gvk.go (4)
PersesDatasource(562-566)Monitoring(310-314)TempoStack(514-518)Namespace(28-32)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)internal/controller/status/status.go (1)
ConditionTypeReady(74-74)tests/e2e/controller_test.go (1)
TestTimeouts(100-108)
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build/push catalog image
- GitHub Check: golangci-lint
- GitHub Check: Run tests and collect coverage on internal and pkg
🔇 Additional comments (2)
docs/api-overview.md (1)
2827-2843: TracesTLS documentation matches the new TLS behaviorThe updated TracesTLS description and
enabledfield text correctly reflect TLS covering both OTLP ingestion (gRPC/HTTP) and query APIs (HTTP). No issues from an API-doc perspective.internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
18-27: TLS client block in Perses Tempo datasource template is consistent and well‑formedThe conditional
client.tlsblock underproxy.specis correctly indented and matches the S3/GCS TLS e2e expectations (HTTPS endpoint plustempo-service-caConfigMap andservice-ca.crtpath). The YAMLlint “expected ':'” error is expected on raw Go templates and doesn’t indicate a real syntax issue in the rendered YAML.
|
/retest |
dayakar349
force-pushed
the
RHOAIENG-38496
branch
2 times, most recently
from
47b96b0 to
7c84df2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/monitoring_test.go (1)
1665-1750: Well-structured helper function, consider validating the CA ConfigMap.The helper function is well-designed with:
- Proper parameterization for S3/GCS backends
- Logical test flow (setup → configure → wait → validate → cleanup)
- Comprehensive TLS validation (HTTPS endpoint, TLS enabled, CA cert config)
- Extensive logging for debugging
- Proper cleanup for test isolation
Consider adding validation that the
tempo-service-caConfigMap exists and has the expected structure (injection annotation, CA bundle). This would provide more comprehensive coverage of the TLS setup.Add validation for the
tempo-service-caConfigMap:+ // Validate tempo-service-ca ConfigMap exists with correct structure + tc.EnsureResourceExists( + WithMinimalObject(gvk.ConfigMap, types.NamespacedName{Name: "tempo-service-ca", Namespace: tc.MonitoringNamespace}), + WithCondition( + And( + jq.Match(`.metadata.annotations."service.beta.openshift.io/inject-cabundle" == "true"`), + jq.Match(`.data."service-ca.crt" != null`), + ), + ), + WithCustomErrorMsg("tempo-service-ca ConfigMap should exist with CA injection annotation and certificate"), + ) + // This is the critical assertion - proves cloud backend now has TLS
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(2 hunks)docs/api-overview.md(3 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controller/services/monitoring/monitoring_controller_actions.go
- internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml
- docs/api-overview.md
- internal/controller/services/monitoring/monitoring_controller_support_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/controller/services/monitoring/monitoring_controller_support.go (1)
api/services/v1alpha1/monitoring_types.go (4)
StorageBackendPV(114-114)StorageBackendS3(116-116)StorageBackendGCS(118-118)Traces(82-96)
tests/e2e/monitoring_test.go (6)
tests/e2e/resource_options_test.go (4)
WithCustomErrorMsg(287-291)WithMinimalObject(155-170)WithCondition(262-266)WithEventuallyTimeout(294-298)pkg/cluster/resources.go (1)
HasCRD(366-368)pkg/cluster/gvk/gvk.go (3)
PersesDatasource(562-566)TempoStack(514-518)Namespace(28-32)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)internal/controller/status/status.go (1)
ConditionTypeReady(74-74)tests/e2e/controller_test.go (1)
TestTimeouts(100-108)
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: kube-linter
- GitHub Check: golangci-lint
🔇 Additional comments (13)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
19-27: LGTM! TLS configuration is correctly structured.The conditional TLS block properly configures client-side TLS for the Perses datasource when
TempoTLSEnabledis true. The CA certificate reference totempo-service-caConfigMap aligns with the new ConfigMap template introduced in this PR.Note: The YAMLlint syntax error on line 20 is a false positive—YAMLlint cannot parse Go template directives like
{{- if .TempoTLSEnabled }}. This is expected for template files.internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (2)
48-60: LGTM! Conditional HTTP TLS configuration is well-structured.The
extraConfigblock correctly:
- Configures explicit HTTP TLS only when
TempoCertificateSecretis provided- Falls back to Tempo operator's OpenShift serving certificate service otherwise
- Uses standard Kubernetes secret mount paths for certificates
The comment clearly explains the conditional logic.
32-46: TLS field names are correct per Tempo operator documentation.The field names
certNameandcaNamematch the official Tempo operator specification for TempoMonolithic CRD TLS configuration. However, verify the upgrade path for any existing TempoMonolithic resources to ensure they transition smoothly to the new field names or are compatible with both old and new naming conventions.api/services/v1alpha1/monitoring_types.go (3)
98-109: LGTM! TLS documentation improvements clarify scope and defaults.The updated comments clearly specify:
- TLS applies to both ingestion and query APIs
- TLS is disabled by default for backward compatibility
- The specific protocols covered (OTLP gRPC/HTTP for ingestion, HTTP for query)
111-119: LGTM! Storage backend constants improve code maintainability.These public constants eliminate magic strings and are properly reused in the controller code (as seen in
monitoring_controller_support.go).
121-144: TracesStorage API redesign is sound; no critical migration issues identified.Verification confirms:
- CEL validation messages are clear and user-friendly (e.g., "When backend is s3 or gcs, the 'secret' field must be specified and non-empty")
- Backend field default "pv" ensures backwards compatibility for new resources
- Retention changed from string to metav1.Duration is type-safe and has sensible default (2160h)
- No existing Monitoring CRs with traces configurations found in repo to validate migration paths
Since this is a v1alpha1 API with no prior version in the codebase and no existing deployed instances to upgrade, migration concerns are not applicable. The API design itself is solid.
internal/controller/services/monitoring/monitoring_controller_support.go (4)
216-235: LGTM! TLS configuration logic is well-structured.The centralized TLS handling correctly:
- Determines TLS enablement state via
determineTLSEnabled- Sets explicit certificate/CA values when TLS enabled and certs provided
- Uses empty strings for fallback to OpenShift serving certificates
- Prevents template rendering errors with consistent empty values when TLS disabled
The three-path logic (enabled with certs, enabled without certs, disabled) is clear and well-commented.
237-253: LGTM! Protocol selection and endpoint construction are correct.The protocol selection logic:
- Defaults to
httpfor backward compatibility- Switches to
httpswhen TLS is enabled- Applies consistently to both PV (TempoMonolithic) and cloud (TempoStack) query endpoints
244-255: LGTM! Backend-specific logic correctly uses new constants.The backend handling properly:
- Uses constants (
serviceApi.StorageBackendPV,StorageBackendS3,StorageBackendGCS) instead of hardcoded strings- Configures appropriate endpoints for each backend (TempoMonolithic for PV, TempoStack gateway for cloud)
- Includes backend-specific fields (Size for PV, Secret for cloud backends)
- Applies protocol selection based on TLS enablement
552-561: LGTM! determineTLSEnabled correctly implements explicit opt-in.The function properly:
- Defaults to
falsefor backward compatibility- Requires explicit
traces.TLS.Enabled = trueto enable TLS- Includes clear comments explaining the rationale (avoiding Tempo operator certificate provisioning issues)
tests/e2e/monitoring_test.go (3)
122-123: LGTM! New TLS test cases properly registered.The test cases are well-named and logically ordered after the existing Perses datasource tests.
1446-1464: LGTM! Comments accurately reflect TLS disabled by default.The updated comments and error messages correctly document the default TLS disabled behavior, improving test clarity and maintainability.
1752-1762: LGTM! Test functions properly delegate to parameterized helper.Both test functions are clean wrappers that correctly:
- Use
t.Helper()for proper test reporting- Delegate to the shared helper with the appropriate backend parameter
| multitenancy: | ||
| enabled: true # Required for OpenShift | ||
| enabled: true | ||
| mode: openshift # Required for OpenShift | ||
| authentication: | ||
| - tenantId: {{.Namespace}} | ||
| tenantName: {{.Namespace}} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify multitenancy enablement impact on existing deployments.
Multitenancy is now unconditionally enabled with mode: openshift, which represents a behavior change. Ensure:
- This doesn't break existing TempoMonolithic deployments without multitenancy
- The OpenShift-specific mode is appropriate for all target environments (ODH vs RHOAI)
- Existing traces remain accessible after this change
🏁 Script executed:
#!/bin/bash
# Verify if multitenancy was previously configurable or disabled by default
rg -n "multitenancy" --type yaml -g "tempo*.yaml" -g "tempo*.tmpl.yaml" -C3Length of output: 743
Let me also search the tempo template for any conditional logic or parameters:
🏁 Script executed:
#!/bin/bash
# Find all tempo-related config files to understand the broader context
echo "=== All tempo config files ==="
fd "tempo" --type f -e yaml -e tmpl.yaml
echo -e "\n=== Search for multitenancy-related parameters or conditionals ==="
rg -n "multitenancy|tenantId|tenantName" --type yaml -C2
echo -e "\n=== Search for configuration parameters or conditionals ==="
rg -n "{{\..*Multitenancy|{{\..*Tenant|if.*multitenancy|if.*tenant" --type yaml -C2Length of output: 1758
🏁 Script executed:
#!/bin/bash
# Check if there's documentation about multitenancy requirements
fd "README|CHANGELOG|CONTRIBUTING" --type f | head -20Length of output: 203
🏁 Script executed:
#!/bin/bash
# Examine the full tempo template to understand parameters and conditionals
cat internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml | head -100Length of output: 1646
🏁 Script executed:
#!/bin/bash
# Search for parameter definitions and how this template is used
echo "=== Search for parameter definitions or configuration structs ==="
rg -n "TempoCPURequest|TempoMemoryRequest|Namespace" --type go | grep -E "(type|struct|field)" | head -20
echo -e "\n=== Look for where tempo-monolithic.tmpl.yaml is referenced/instantiated ==="
rg -n "tempo-monolithic" --type go -C2 | head -40
echo -e "\n=== Check for multitenancy parameter or flag definitions ==="
rg -n "multitenancy|Multitenancy" --type go -C3 | head -40Length of output: 3442
🏁 Script executed:
#!/bin/bash
# Check git history to see if multitenancy is new or changed
echo "=== Recent changes to tempo-monolithic.tmpl.yaml ==="
git log --oneline -n 20 -- internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml 2>/dev/null || echo "Git history not available"
echo -e "\n=== Show the full tempo-monolithic template to see all conditionals ==="
cat internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yamlLength of output: 2235
🏁 Script executed:
#!/bin/bash
# Find the monitoring controller or parameter struct
echo "=== Look for TempoMonolithic or monitoring parameters struct ==="
rg -n "struct.*Tempo|type.*Tempo" --type go -C4 | head -60
echo -e "\n=== Search for where TempoMonolithicTemplate values are populated ==="
rg -n "TempoMonolithicTemplate|tempo-monolithic" --type go -A5 -B5 | head -60Length of output: 4979
🏁 Script executed:
#!/bin/bash
# Check the most recent commit to see what changed
echo "=== Show the most recent commit that touched tempo-monolithic ==="
git show --stat 7c84df2 -- internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml 2>/dev/null | head -50
echo -e "\n=== Show the commit before that ==="
git show 6d581a0 -- internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml 2>/dev/null | head -100Length of output: 1630
🏁 Script executed:
#!/bin/bash
# Search for any mention of multitenancy in the entire repo
echo "=== All references to multitenancy in the repo ==="
rg -n "multitenancy" --type yaml --type go --type md 2>/dev/null
echo -e "\n=== Search for tempo configuration or values files ==="
find . -name "*tempo*values*" -o -name "*tempo*config*" | grep -v node_modulesLength of output: 291
Add parameter control for multitenancy configuration to respect environment differences.
Multitenancy is unconditionally enabled with mode: openshift hardcoded in the template. This is a breaking change (added Nov 19) and contradicts the principle of respecting user changes. Unlike other toggles (TLS, storage size), there's no way to disable multitenancy or adjust the mode for non-OpenShift environments (e.g., plain Kubernetes deployments of ODH).
Required actions:
- Add template parameters:
MultitenancyEnabledandTempoMultitenancyMode - Update template to conditionally render multitenancy block:
{{- if .MultitenancyEnabled }} - Update template to use parameterized mode:
mode: {{.TempoMultitenancyMode}} - Ensure defaults maintain current behavior while allowing customization
- Update documentation on when/why multitenancy should be enabled per deployment type
🤖 Prompt for AI Agents
In internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml
around lines 14-19, the multitenancy stanza is hardcoded and always enabled; add
two template parameters MultitenancyEnabled (bool, default true) and
TempoMultitenancyMode (string, default "openshift"), wrap the multitenancy block
in a conditional using {{- if .MultitenancyEnabled }} ... {{- end }}, replace
the hardcoded mode with mode: {{.TempoMultitenancyMode}}, and ensure the
parameter defaults preserve current behavior while documenting the new
parameters and guidance on when to enable/disable multitenancy for different
deployment types.
7c84df2 to
52954ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
19-27: TLS client block wiring for Perses Tempo datasource looks correct; linter warning is likely a template false positiveThe conditional
client.tlsblock is correctly guarded byTempoTLSEnabledand matches the CA ConfigMap you introduce (tempo-service-ca/service-ca.crt). The rendered YAML when TLS is enabled/disabled will be valid.If yamllint is part of CI and actually failing on Line 20, you may want to align with whatever pattern other templates in this repo use (e.g., placing
{{- if ... }}/{{- end }}at column 0 or wrapping them in comments) so the raw file parses for the linter, but functionally this template is fine.internal/controller/services/monitoring/monitoring_controller_support_test.go (2)
929-986: Table coverage fordetermineTLSEnabledis good; consider using backend constantsThe test cases exercise explicit true/false and nil TLS across all backends, which matches the intended “opt‑in TLS” behavior.
For consistency with the production code (which now uses
serviceApi.StorageBackendPV/S3/GCS), consider using those constants instead of string literals in the testTracesStorage.Backendfields to avoid backend name drift if the enum ever evolves.
998-1107: TLS template data tests are effective; tidy up unused expectations and broaden coverage if desiredThe assertions around
TempoTLSEnabled,TempoQueryEndpoint, and presence of other fields correctly validate the new TLS/path logic for PV and S3.Two minor test cleanups you might consider:
- The
expectedHTTPProtocolfield in the table is never used; either drop it or add an assertion on the scheme ofTempoQueryEndpointto make the intent explicit.- Optionally add a GCS backend case with TLS enabled here as well (even though S3/GCS share the same branch), mirroring the e2e coverage for cloud backends.
None of this affects correctness, just test clarity.
tests/e2e/monitoring_test.go (1)
52-56: Cloud-backend TLS e2e coverage is strong; consider small consistency tweaksThe new
validatePersesDatasourceTLSWithCloudBackendhelper plus S3/GCS wrappers nicely verify the full path:
- Monitoring traces configured with a cloud backend and TLS enabled.
- TempoStack Ready with the expected secret wiring.
- PersesDatasource proxy URL using
https://tempo-data-science-tempostack-gateway...:8080and theclient.tls.caCertblock pointing attempo-service-ca/service-ca.crt.Two minor consistency nits you might consider:
- You already define
TracesStorageBackendS3/TracesStorageBackendGCSconstants at the top of the file; using those instead of string literals"s3"/"gcs"when calling the helper and in any future assertions will reduce the chance of typos.- If you expect more backends in the future, you could add a small check (or even a
switch) insidevalidatePersesDatasourceTLSWithCloudBackendto fail fast on unsupported backend values, but that’s not strictly necessary today.Functionally these tests look very solid.
Also applies to: 122-123, 1665-1762
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(2 hunks)docs/api-overview.md(3 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api-overview.md
- internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml
🧰 Additional context used
🧬 Code graph analysis (3)
internal/controller/services/monitoring/monitoring_controller_support.go (1)
api/services/v1alpha1/monitoring_types.go (4)
StorageBackendPV(114-114)StorageBackendS3(116-116)StorageBackendGCS(118-118)Traces(82-96)
internal/controller/services/monitoring/monitoring_controller_support_test.go (1)
api/services/v1alpha1/monitoring_types.go (6)
Traces(82-96)TracesTLS(99-109)TracesStorage(124-144)StorageBackendPV(114-114)StorageBackendS3(116-116)StorageBackendGCS(118-118)
tests/e2e/monitoring_test.go (6)
tests/e2e/resource_options_test.go (4)
WithCustomErrorMsg(287-291)WithMinimalObject(155-170)WithCondition(262-266)WithEventuallyTimeout(294-298)pkg/cluster/resources.go (1)
HasCRD(366-368)pkg/cluster/gvk/gvk.go (3)
PersesDatasource(562-566)TempoStack(514-518)Namespace(28-32)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)internal/controller/status/status.go (1)
ConditionTypeReady(74-74)tests/e2e/controller_test.go (1)
TestTimeouts(100-108)
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: golangci-lint
🔇 Additional comments (4)
internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml (1)
1-8: Service CA ConfigMap template is consistent and minimalConfigMap metadata (name/namespace) and
inject-cabundleannotation look correct for OpenShift service CA injection, and the emptydata: {}is standard for this pattern. No changes needed here.api/services/v1alpha1/monitoring_types.go (1)
98-144: Traces TLS semantics and backend-driven storage config look consistent with the controller logicThe updated
TracesTLSdocumentation (explicit default-off, covering ingestion + query) and the newStorageBackend*constants align with how the controller now derives Tempo endpoints and TLS behavior.
TracesStorage’s enum/default plus XValidation rules (“secret required when backend != pv”, “size only allowed for pv”) match the usage inaddTracesTemplateDataand the e2e tests. This is a good tightening of the API surface without obvious backward-compat issues.internal/controller/services/monitoring/monitoring_controller_actions.go (1)
48-52: Wiring the tempo service CA ConfigMap into Perses–Tempo deployment is soundIncluding
TempoServiceCAConfigMapTemplatealongsidePersesTempoDatasourceTemplateindeployPersesTempoIntegrationensures the CA bundle resource needed by the datasource TLS block is always present in the monitoring namespace, without adding extra conditionals here.This integration looks correct given the new template and avoids sprinkling the ConfigMap logic across multiple places.
Also applies to: 545-552
internal/controller/services/monitoring/monitoring_controller_support.go (1)
216-255: Centralized TLS and protocol handling for traces looks correct and matches the new API/testsThe
addTracesTemplateDatachanges cleanly encapsulate:
- An explicit
TempoTLSEnabledflag derived fromdetermineTLSEnabled(respectingtraces.TLS.Enabledand defaulting to false).- Consistent population of
TempoCertificateSecretandTempoCAConfigMap, with empty-string defaults when TLS is off or certs aren’t specified, which keeps the templates simple.- Backend-aware endpoints using
serviceApi.StorageBackendPV/S3/GCS, and anhttpvshttpsscheme only onTempoQueryEndpoint, which is exactly what the Perses datasource template consumes.
determineTLSEnabled’s default-false behavior is also aligned with the updated comments and tests, and should avoid unexpected certificate provisioning behavior by requiring an explicit opt-in.Also applies to: 553-561
dayakar349
force-pushed
the
RHOAIENG-38496
branch
from
52954ae to
9b3f6a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
23-26: Consider parameterizing the CA ConfigMap name for Perses TLS.Right now the CA source is hardcoded to the
tempo-service-caConfigMap. That matches the new service-CA template, but it means a user-specifiedtraces.tls.caConfigMap(custom CA) cannot be reused for Perses. If supporting custom CAs for Tempo is in scope, consider wiring this to the same value (or exposing a separate parameter) so Perses can trust non-service-CA certificates as well.internal/controller/services/monitoring/monitoring_controller_actions.go (1)
543-553: Including the tempo-service-ca ConfigMap in the Perses Tempo integration is reasonable.Appending
TempoServiceCAConfigMapTemplateto the templates fordeployPersesTempoIntegrationensures the CA ConfigMap exists whenever the Tempo datasource is managed. Creating it even when TLS is disabled is harmless and simplifies the controller logic.If you later want to tighten this, you could gate the ConfigMap template on
traces.tls.enabledin the template data rather than always deploying it.api/services/v1alpha1/monitoring_types.go (1)
111-119: Backend constants are a good addition; ensure they’re used consistently.
StorageBackendPV,StorageBackendS3, andStorageBackendGCScentralize the backend strings. To avoid drift, it’s worth using these constants in controller logic (e.g., comparisons indeployTracingStackand related helpers) instead of inline"pv","s3","gcs"literals.internal/controller/services/monitoring/monitoring_controller_support.go (1)
225-240: TLS certificate config has an effectively unreachable branchGiven
determineTLSEnabledonly returnstruewhentraces.TLS != nil && traces.TLS.Enabled(Lines 560-563), theelsebranch underif tlsEnabled { ... }for thetraces.TLS == nilcase (Lines 231-235) cannot currently be hit. You could simplify this to reduce confusion, e.g. by dropping the innerelseand relying on the!tlsEnabledblock to normalize empty values, or by adjustingdetermineTLSEnabledif you do expect a “TLS on with implicit TLS struct” case in future.Also applies to: 557-566
tests/e2e/monitoring_test.go (1)
1665-1750: Minor test cleanups: reuse backend constants and consider tightening TLS assertionsThe shared helper is well-structured and readable; a couple of small improvements you might consider:
- Instead of passing raw
"s3"/"gcs"strings, reuse theTracesStorageBackendS3/TracesStorageBackendGCSconstants already defined at the top of the file, to keep tests aligned with other usages and avoid typos if backend identifiers ever change.- (Optional) If you want even stronger validation of the TLS path, you could add an assertion that the
tempo-service-caConfigMapactually exists and has the expectedservice-ca.crtkey, to ensure the CA material is present in addition to the PersesDatasource spec wiring.These are niceties only; the current implementation is correct as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(2 hunks)docs/api-overview.md(3 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/services/monitoring/monitoring_controller_support_test.go
- docs/api-overview.md
🧰 Additional context used
🧬 Code graph analysis (2)
internal/controller/services/monitoring/monitoring_controller_support.go (1)
api/services/v1alpha1/monitoring_types.go (4)
StorageBackendPV(114-114)StorageBackendS3(116-116)StorageBackendGCS(118-118)Traces(82-96)
tests/e2e/monitoring_test.go (7)
tests/e2e/resource_options_test.go (4)
WithCustomErrorMsg(287-291)WithMinimalObject(155-170)WithCondition(262-266)WithEventuallyTimeout(294-298)pkg/cluster/resources.go (1)
HasCRD(366-368)pkg/cluster/gvk/gvk.go (4)
PersesDatasource(562-566)Monitoring(310-314)TempoStack(514-518)Namespace(28-32)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)internal/controller/status/status.go (1)
ConditionTypeReady(74-74)tests/e2e/controller_test.go (1)
TestTimeouts(100-108)
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml
[error] 15-15: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build/push catalog image
- GitHub Check: golangci-lint
- GitHub Check: kube-linter
- GitHub Check: Run tests and collect coverage on internal and pkg
🔇 Additional comments (10)
internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml (1)
1-8: Service CA ConfigMap template looks correct and aligned with Perses TLS usage.Name, namespace templating, and the inject-cabundle annotation all look consistent with the Perses datasource TLS block and the expected
service-ca.crtkey.internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
19-27: YAML templating: structure is valid post-render; static analysis error is likely a false positive.The
client.tlsblock is correctly nested underproxy.spec, and the{{- if .TempoTLSEnabled }}guard should render valid YAML when processed. The YAMLlint “expected ':'” error is almost certainly due to the Go template directive itself, not the final manifest. It would be good to validate the rendered PersesDatasource (e.g., from a running cluster or unit test golden) to confirm.If your CI treats the template as raw YAML, you may need to adjust linting config or add an ignore rule for template directives rather than changing this structure.
internal/controller/services/monitoring/monitoring_controller_actions.go (1)
25-48: New CA ConfigMap template constant is wired correctly.
TempoServiceCAConfigMapTemplatematches the new template path and keeps all resource templates centralized alongside the other monitoring-related templates.internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (3)
14-21: Multitenancy block is now correctly parameterized and conditionally rendered.Wrapping
multitenancyin{{- if .MultitenancyEnabled }}with a configurableTempoMultitenancyModeaddresses the prior concern about hardcoded multitenancy. The YAMLlint syntax error around this region is expected on raw templates; the rendered YAML structure (withmultitenancy:aligned underspec) is valid.If not already covered, consider adding/keeping a unit test that asserts the multitenancy block is omitted when
.MultitenancyEnabledis false.
28-48: TLS field renames under ingestion.otlp look consistent but should be checked against the Tempo CRD.Switching from
certificateSecret/caConfigMaptocertName/caNameunder both gRPC and HTTP TLS blocks is fine as long as these match the TempoMonolithic API fields. The conditional guards with.TempoCertificateSecretand.TempoCAConfigMapare preserved, so behavior should be unchanged apart from the field names.Please double‑check these names against the Tempo operator CRD (or related tests) to avoid subtle schema mismatches.
54-62: extraConfig.http_tls_config conditional aligns with explicit-certificate behavior.Tying
server.http_tls_configto.TempoCertificateSecretensures HTTP TLS is only explicitly configured when a dedicated cert is provided, leaving the operator to manage OpenShift serving certs otherwise. The path and filenames (/var/run/tls/http/server/tls.crt/.key) are consistent within the template.If not already covered, a unit or e2e test that toggles
traces.tls.certificateSecretwould help ensure the http_tls_config block appears/disappears as expected.api/services/v1alpha1/monitoring_types.go (2)
98-109: TracesTLS documentation now clearly reflects the TLS scope.The updated comment explicitly covering both OTLP ingestion and query APIs improves clarity without changing behavior.
121-144: TracesStorage schema and XValidation rules capture key invariants for PV/S3/GCS.The
backendenum + default, secret requirement for non‑PV backends, and “size only for PV” rule together encode the expected constraints well. This should prevent misconfigured cloud-storage setups at admission time.It would be good to have table‑driven unit tests (or CRD validation tests) covering:
- backend unset (default pv),
- backend = s3/gcs with/without secret,
- backend = pv with/without size,
to guard against regressions in the XValidation expressions.internal/controller/services/monitoring/monitoring_controller_support.go (1)
216-260: Tempo multitenancy + TLS/query endpoint wiring looks consistentUnconditionally setting
MultitenancyEnabled/TempoMultitenancyMode, drivingTempoTLSEnabledviadetermineTLSEnabled, and using theprotocolvariable forTempoQueryEndpointacross PV and S3/GCS backends lines up with the intended behaviour (HTTP by default, HTTPS when TLS is enabled) and avoids template key-missing issues by always populating the TLS-related fields. I don’t see correctness issues in this block.tests/e2e/monitoring_test.go (1)
122-123: Good coverage for default (non‑TLS) and TLS Perses datasource pathsAdding the two TLS-focused test cases to the suite, plus the explicit assertion that the Tempo datasource uses HTTP when TLS is disabled, and the S3/GCS-specific wrappers, gives nice end-to-end coverage of both the default and TLS-enabled behaviours without overcomplicating the tests. The CRD existence guards and cleanup sections also look solid.
Also applies to: 1446-1465, 1752-1762
|
/retest |
dayakar349
force-pushed
the
RHOAIENG-38496
branch
from
9b3f6a9 to
895ac1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
19-27: TLS client block for Perses Tempo datasource looks consistent; YAMLlint error is a template false positiveThe conditional
client.tlssection matches the newTempoTLSEnabled/TempoQueryEndpointdata and what the e2e tests assert for S3/GCS TLS. Indentation is correct once the{{- if ... }}/{{- end }}directives are rendered, so the YAMLlint “expected ':'” error on the template line can be safely ignored or suppressed in tooling.If YAMLlint runs on raw templates in CI, consider configuring it to ignore
{{...}}lines to avoid noisy false positives.internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (1)
14-21: Multitenancy + TLS wiring for TempoMonolithic looks correct; toggle is prepared but still hardcodedThe new multitenancy and TLS sections line up with the template data:
MultitenancyEnabled/TempoMultitenancyModecorrectly gate themultitenancyblock and parameterizemode.- OTLP gRPC/HTTP
tls.enabledpluscertName/caNameare wired to the new template vars and match the TempoMonolithic TLS schema.- The
extraConfig.tempo.server.http_tls_configblock is only emitted whenTempoCertificateSecretis set, which avoids interfering with default OpenShift service-serving certs.Note that
MultitenancyEnabledis currently always settruein code, so this is not yet user-configurable, but the template is ready once an API knob is added. The YAMLlint syntax error is a false positive from the{{- if ... }}line and does not reflect a real YAML problem after rendering.Also applies to: 28-49, 54-62
internal/controller/services/monitoring/monitoring_controller_support_test.go (1)
929-996: TLS unit tests are solid;expectedHTTPProtocolis currently unusedThe new tests give good coverage of
determineTLSEnabledandaddTracesTemplateDatafor PV vs S3 backends with TLS on/off, and they validate theTempoQueryEndpointURLs and required template keys.One small clean-up: the
expectedHTTPProtocolfield inTestAddTracesTemplateData_TLSis never asserted, so it’s effectively dead test data. Either drop the field or add an explicit assertion on the protocol if you want to keep it.Also applies to: 998-1107
tests/e2e/monitoring_test.go (1)
122-123: E2E coverage for Perses TLS with S3/GCS backends looks thorough; consider optional ConfigMap assertionThe new suite entries and
validatePersesDatasourceTLSWithCloudBackendhelper exercise the full path for TLS-enabled cloud backends:
- Traces with S3/GCS + TLS enabled
- TempoStack Ready with the correct backend secret
- PersesDatasource proxy URL switched to HTTPS via the Tempo gateway
- HTTP client TLS enabled with the expected
caCertconfigmap and key pathThis gives good confidence that the new TLS wiring behaves as intended for both S3 and GCS.
If you want to go one step further in the future, you could also assert that the
tempo-service-caConfigMap exists in the monitoring namespace with the expected injection annotation and populatedservice-ca.crtkey, but that’s an enhancement rather than a blocker for this PR.Also applies to: 1446-1447, 1665-1762
api/services/v1alpha1/monitoring_types.go (1)
98-109: Traces TLS/storage API shape looks good; XValidation could be made a bit more defensiveThe TracesTLS comment, backend constants, and expanded TracesStorage (Backend/Size/Secret/Retention) align well with how the controller and tests now distinguish PV vs S3/GCS and wire TLS.
For the CEL validations, the current rules:
- Enforce a non-empty
secretwhenbackend != "pv".- Forbid
sizewhenbackend != "pv".are logically correct, assuming
backendis reliably set (defaulting to"pv"). To be slightly more future‑proof, you might consider guarding for a missingbackend, for example:// Secret required for non-PV backends // +kubebuilder:validation:XValidation:rule="!has(self.backend) || self.backend == 'pv' || (has(self.secret) && self.secret != '')",message="When backend is s3 or gcs, the 'secret' field must be specified and non-empty" // Size only allowed for PV // +kubebuilder:validation:XValidation:rule="!has(self.backend) || self.backend == 'pv' || !has(self.size)",message="Size is supported when backend is pv only"Not strictly required for this PR, but it would make validation more robust to defaulting/omission edge cases.
Also applies to: 111-144
internal/controller/services/monitoring/monitoring_controller_support.go (1)
208-260: Centralized TLS/protocol handling for traces looks correct; multitenancy toggle and CA usage are acceptable for nowThe refactor in
addTracesTemplateDataplusdetermineTLSEnableddoes a nice job of:
- Making TLS strictly opt‑in via
traces.TLS.Enabled, with a safe default offalse.- Centralizing HTTP vs HTTPS protocol selection for both TempoMonolithic (port 3200) and TempoStack gateway (port 8080) query endpoints.
- Populating
TempoTLSEnabled,TempoCertificateSecret, andTempoCAConfigMapconsistently for use across templates.- Using the new backend constants to drive
TempoEndpoint,TempoQueryEndpoint,Size, andSecret.Two small notes:
MultitenancyEnabled/TempoMultitenancyModeare now clearly surfaced to the template but still hardcoded (true/"openshift"). That’s fine for current OpenShift‑only targets and future‑proofs the templates once you decide to expose a real knob.- On the client side, the Perses datasource TLS config always trusts the service CA via the fixed
tempo-service-caConfigMap; custom CA scenarios currently only flow into Tempo itself viaTempoCAConfigMap. That’s an acceptable limitation for this PR’s goal (TLS with the OpenShift service CA), but worth keeping in mind if you later add support for user‑supplied CAs end‑to‑end.Overall, the new logic is consistent with the updated templates and tests.
Also applies to: 557-566
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(2 hunks)docs/api-overview.md(3 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/api-overview.md
- internal/controller/services/monitoring/monitoring_controller_actions.go
- internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml
🧰 Additional context used
🧬 Code graph analysis (3)
internal/controller/services/monitoring/monitoring_controller_support_test.go (1)
api/services/v1alpha1/monitoring_types.go (6)
Traces(82-96)TracesTLS(99-109)TracesStorage(124-144)StorageBackendPV(114-114)StorageBackendS3(116-116)StorageBackendGCS(118-118)
internal/controller/services/monitoring/monitoring_controller_support.go (1)
api/services/v1alpha1/monitoring_types.go (4)
StorageBackendPV(114-114)StorageBackendS3(116-116)StorageBackendGCS(118-118)Traces(82-96)
tests/e2e/monitoring_test.go (7)
tests/e2e/resource_options_test.go (4)
WithCustomErrorMsg(287-291)WithMinimalObject(155-170)WithCondition(262-266)WithEventuallyTimeout(294-298)pkg/cluster/resources.go (1)
HasCRD(366-368)pkg/cluster/gvk/gvk.go (4)
PersesDatasource(562-566)Monitoring(310-314)TempoStack(514-518)Namespace(28-32)pkg/utils/test/testf/testf_support.go (1)
Transform(78-106)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)internal/controller/status/status.go (1)
ConditionTypeReady(74-74)tests/e2e/controller_test.go (1)
TestTimeouts(100-108)
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml
[error] 15-15: syntax error: could not find expected ':'
(syntax)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build/push catalog image
- GitHub Check: golangci-lint
- GitHub Check: kube-linter
- GitHub Check: Run tests and collect coverage on internal and pkg
dayakar349
force-pushed
the
RHOAIENG-38496
branch
from
895ac1a to
0c773eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
internal/controller/services/monitoring/monitoring_controller_support_test.go (2)
929-996: TLS enablement tests look correct; consider using backend constants and adding a nil-traces caseThe table cases exercise the key TLS-on/off and TLS-nil scenarios and match the “TLS disabled by default” contract. For better future-proofing, consider:
- Using
serviceApi.StorageBackendPV/S3/GCSinstead of raw"pv"/"s3"/"gcs"literals to avoid drift if the constants ever change.- Optionally adding a
traces == nilcase ifdetermineTLSEnabledis expected to be nil-safe, to lock that behavior in tests.
998-1107: Template-data TLS test is solid; clean up unused field and consider GCS coverageThe test correctly checks:
TempoTLSEnabledTempoQueryEndpoint(including http vs https via the URL)- Presence of the other key template fields.
Two minor nits:
expectedHTTPProtocolis never used; either assert a corresponding template key (if one exists) or drop the field to avoid confusion.- You only cover S3 for the cloud-backend path; adding a GCS case would make this unit test align better with the new e2e TLS tests for S3/GCS.
api/services/v1alpha1/monitoring_types.go (1)
98-144: Backend and storage validation look consistent; tighten comments and keep strings in syncThe new backend constants and
TracesStorageXValidation rules look coherent:
- Default
backend="pv"with enum validation avoids breaking existing CRs.self.backend != 'pv' ? (has(self.secret) && self.secret != '') : truecorrectly forces a non-emptysecretfor S3/GCS while leaving PV unchanged.self.backend != 'pv' ? !has(self.size) : trueenforces PV-onlysize, which matches the intent.A couple of small polish items:
- The field comment on
Traces.TLSstill says “TLS configuration for Tempo gRPC connections”, whileTracesTLSnow documents ingestion (gRPC/HTTP) and query APIs. Consider updating the field-level comment to avoid mixed messaging.- The XValidation rules inline
'pv'while the code exposesStorageBackendPV/S3/GCSconstants; if you ever change the constant values, remember to update these CEL expressions to match.internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
19-27: TLS client block and CA wiring look correct; YAMLlint warning is a template false positiveThe conditional
client.tlsblock wired totempo-service-camatches the new TLS behavior and is correctly gated on.TempoTLSEnabled. Indentation is consistent with the surrounding HTTPProxyspecand will render valid YAML both when TLS is enabled and when it’s disabled (block omitted).The YAMLlint “expected ':'” error is coming from the raw
{{- if .TempoTLSEnabled }}line, not from the rendered manifest. If lint noise is a problem, you could switch to{{ if .TempoTLSEnabled }}(no left trim) or add a lint ignore for templates, but functionally this template is fine.internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (1)
14-62: Multitenancy gating looks good; double‑check TLS field names against TempoMonolithic APIThe changes here look directionally right:
- Wrapping
multitenancyin{{- if .MultitenancyEnabled }}and parameterizingmode: {{.TempoMultitenancyMode}}fixes the previous issue where multitenancy was unconditionally enabled and hardcoded toopenshift. This should be much friendlier across environments while allowing defaults to preserve current behavior.- The new
extraConfig.tempo.server.http_tls_configblock gated on.TempoCertificateSecretis a sensible way to only force HTTP TLS when an explicit cert is provided and otherwise defer to the operator’s serving cert wiring.Because these stanzas talk directly to the TempoMonolithic CRD, please re-verify the exact field names and shapes:
ingestion.otlp.grpc.tls.certName/caNameingestion.otlp.http.tls.certName/caNameextraConfig.tempo.server.http_tls_config.{cert_file,key_file}to ensure they match the current Grafana Tempo operator schema; otherwise these keys may be ignored or rejected at admission time. Existing tests in this file focus on retention and PV backend behavior and don’t appear to assert monolithic TLS, so an extra TLS-on test for the PV path (or manual validation) would be valuable.
The YAMLlint error around the
{{- if .MultitenancyEnabled }}line is again a template-vs-lint artifact, not a problem in the rendered YAML.If you’d like, I can sketch an additional e2e or unit test that exercises TempoMonolithic with TLS enabled on the PV backend to lock this behavior in.
tests/e2e/monitoring_test.go (1)
1665-1762: Cloud-backend TLS helper is well-structured; consider reusing backend constants and optionally asserting the CA ConfigMapThe new
validatePersesDatasourceTLSWithCloudBackendhelper does a good job of:
- Ensuring CRD presence before running.
- Starting from a clean slate (
ensureMonitoringCleanSlate+ secret cleanup).- Creating a backend-specific secret, enabling traces with the chosen backend and
tls.enabled = true.- Waiting for
TempoStackto be Ready with the correct backend/secret wiring.- Asserting that the PersesDatasource:
- uses an HTTPS gateway URL for TempoStack,
- turns on
client.tls.enable,- and references
tempo-service-cawithservice-ca.crt.A few small improvements you might consider:
- Use the existing constants (
TracesStorageBackendS3/TracesStorageBackendGCS) when calling this helper and when constructing/validating backends, instead of hardcoding"s3"/"gcs", to keep tests in sync with the API types.- For completeness around the new CA flow, you could also assert that the
tempo-service-caConfigMap exists and has the injection annotation, similar to what you manually verified on cluster, though that’s more of an enhancement than a requirement for this PR.Overall, the control flow and cleanup (
cleanupTracesConfiguration+cleanupTempoStackAndSecret) look solid and should keep these tests from leaking state into others.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(2 hunks)docs/api-overview.md(3 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml
- docs/api-overview.md
- internal/controller/services/monitoring/monitoring_controller_actions.go
- internal/controller/services/monitoring/monitoring_controller_support.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/controller/services/monitoring/monitoring_controller_support_test.go (1)
api/services/v1alpha1/monitoring_types.go (6)
Traces(82-96)TracesTLS(99-109)TracesStorage(124-144)StorageBackendPV(114-114)StorageBackendS3(116-116)StorageBackendGCS(118-118)
tests/e2e/monitoring_test.go (4)
tests/e2e/resource_options_test.go (2)
WithCustomErrorMsg(287-291)WithEventuallyTimeout(294-298)pkg/cluster/resources.go (1)
HasCRD(366-368)pkg/cluster/gvk/gvk.go (3)
PersesDatasource(574-578)Monitoring(316-320)TempoStack(526-530)pkg/utils/test/testf/testf_witht.go (1)
WithEventuallyTimeout(46-50)
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml
[error] 15-15: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build/push catalog image
- GitHub Check: kube-linter
- GitHub Check: golangci-lint
- GitHub Check: Run tests and collect coverage on internal and pkg
🔇 Additional comments (2)
tests/e2e/monitoring_test.go (2)
122-123: Nice addition of explicit TLS cloud-backend tests into the suiteRegistering
ValidatePersesDatasourceTLSWithS3BackendandValidatePersesDatasourceTLSWithGCSBackendin the main monitoring test suite gives good end‑to‑end coverage for the new TLS behavior without changing the structure of the harness.
1446-1465: Perses datasource default HTTP expectation matches “TLS disabled by default” semanticsThe updated comment,
expectedTempoEndpoint(HTTP), and the custom error message now clearly reflect that traces TLS is off by default and that the baseline Perses Tempo datasource should use an HTTP endpoint unless TLS is explicitly enabled. This keeps the e2e expectation aligned withdetermineTLSEnabledand the new TLS defaults.
dayakar349
force-pushed
the
RHOAIENG-38496
branch
from
0c773eb to
e3da982
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/monitoring_test.go (2)
52-56: Backend constants improve test readability; consider reusing them consistently.Defining
TracesStorageBackendPV/S3/GCSandTracesStorageSize1Giin the test file is a good move away from magic strings. There are still a few call sites that pass"pv"directly (e.g. somewithMonitoringTracesusages); if you want to tighten things up, you could switch those to these constants as well for consistency, but it’s not strictly necessary.
1665-1762: TLS helper for PersesDatasource with cloud backends is sound and reuses existing utilities.The
validatePersesDatasourceTLSWithCloudBackendflow is well-structured:
- Ensures a clean slate and deletes any leftover TempoStack/secret for the given backend.
- Creates a backend‑specific dummy secret (reusing
createDummySecret).- Configures traces with the chosen backend and
traces.tls.enabled = true.- Waits for:
- Monitoring CR to show the expected backend
- TempoStack to be Ready with matching
spec.storage.secret.{type,name}- PersesDatasource to expose the HTTPS gateway endpoint and the expected
client.tlsConfigMap CA wiring.Cleanup via
cleanupTracesConfigurationandcleanupTempoStackAndSecretmirrors other tests and should prevent cross‑test contamination. The S3/GCS wrapper methods are thin and clear. If you’d like to tighten coupling with your backend constants, you could passTracesStorageBackendS3/GCSinstead of raw"s3"/"gcs", but the current implementation is correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(2 hunks)docs/api-overview.md(3 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/controller/services/monitoring/monitoring_controller_support.go
- internal/controller/services/monitoring/monitoring_controller_actions.go
- docs/api-overview.md
- internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml
- internal/controller/services/monitoring/monitoring_controller_support_test.go
🧰 Additional context used
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml
[error] 15-15: syntax error: could not find expected ':'
(syntax)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: golangci-lint
🔇 Additional comments (6)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
19-27: TLS block wiring and indentation look correct; YAMLlint error is expected.The conditional
client.tlsblock is correctly indented as a sibling ofurl/headersunderproxy.spec, and it cleanly injects thetempo-service-cabundle when.TempoTLSEnabledis true. The YAMLlint “could not find expected ':'” at Line 20 is just the linter not understanding the{{- if ... }}template control line; the rendered YAML will be valid, so no change is needed here.api/services/v1alpha1/monitoring_types.go (1)
98-109: Traces TLS semantics and storage XValidations are consistent and enforce the right invariants.The updated
TracesTLScomment now matches the actual behavior: TLS is opt‑in viaenabledand applies to both ingestion and query paths, with backward compatibility preserved by the zero value. The newTracesStorageschema plus CEL rules correctly enforce:
secretmust be present and non‑empty for non‑PV backendssizeis only allowed for the PV backendThis matches the e2e expectations for PV vs S3/GCS paths and should prevent misconfigured CRs from slipping through.
Also applies to: 111-144
internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (2)
14-21: Multitenancy is now properly parameterized and gated.Wrapping the
multitenancystanza in{{- if .MultitenancyEnabled }}and drivingmodefrom.TempoMultitenancyModerestores configurability and avoids hard‑coding OpenShift multitenancy for all environments. This addresses the earlier concern about unconditionally enabling multitenancy while preserving current defaults via the parameter values.
28-49: TLS field renames and conditional HTTP TLS config align with the new traces TLS model.The switch to
certName/caNameunderingestion.otlp.grpc/http.tlsmatches the updated controller/template fields and keeps TLS wiring consistent. TheextraConfig.tempo.compactor.compaction.block_retention: {{.TracesRetention}}hook plus the guardedserver.http_tls_configblock (only when.TempoCertificateSecretis set) cleanly separate the explicit‑cert path from the default OpenShift‑serving‑cert behavior. YAMLlint’s complaint on the templateifis again just linter noise and doesn’t affect rendered YAML.Also applies to: 54-62
tests/e2e/monitoring_test.go (2)
122-123: Adding TLS test cases for S3 and GCS completes the e2e coverage matrix.Registering
ValidatePersesDatasourceTLSWithS3Backendand...WithGCSBackendin the suite ensures TLS behavior is exercised for both cloud storage backends alongside the existing PV/non‑TLS scenarios.
1446-1465: Perses datasource expectations correctly reflect “TLS disabled by default”.The updated comment and error message, plus the
expectedTempoEndpointusinghttp://tempo-data-science-tempomonolithic...:3200, align with the new API contract where traces TLS is opt‑in. This keeps the baseline configuration test authoritative for the non‑TLS path.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/monitoring_test.go (1)
51-56: New backend constants and Perses datasource configuration tests align with TLS-off default behaviorDefining
TracesStorageBackendPV/S3/GCSand using them in both traces/TempoStack tests and the TLS tests makes the scenarios easier to read, and the updatedValidatePersesDatasourceConfigurationexplicitly asserting an HTTP endpoint with a “TLS disabled by default” message matches the controller’sdetermineTLSEnabledbehavior andTempoQueryEndpointconstruction. The two new test cases wired intomonitoringTestSuitegive good coverage over the S3/GCS TLS permutations. If you want to avoid string drift long-term, you could optionally import and use theStorageBackend*constants from the API package in this file instead of redefining them.Also applies to: 123-124, 1517-1536
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(2 hunks)docs/api-overview.md(3 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/api-overview.md
- internal/controller/services/monitoring/monitoring_controller_actions.go
- internal/controller/services/monitoring/monitoring_controller_support_test.go
- internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/monitoring_test.go (6)
tests/e2e/resource_options_test.go (3)
WithMinimalObject(155-170)WithCondition(262-266)WithEventuallyTimeout(294-298)pkg/cluster/resources.go (1)
HasCRD(366-368)api/services/v1alpha1/monitoring_types.go (1)
Monitoring(159-165)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)internal/controller/status/status.go (1)
ConditionTypeReady(74-74)pkg/utils/test/testf/testf_witht.go (1)
WithEventuallyTimeout(46-50)
internal/controller/services/monitoring/monitoring_controller_support.go (1)
api/services/v1alpha1/monitoring_types.go (4)
StorageBackendPV(114-114)StorageBackendS3(116-116)StorageBackendGCS(118-118)Traces(82-96)
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: golangci-lint
🔇 Additional comments (5)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
19-27: TLS client configuration for Perses Tempo datasource looks consistent with controller data and testsThe conditional TLS block correctly switches on
.TempoTLSEnabledand wiresclient.tlsto thetempo-service-caConfigMap andservice-ca.crtpath, matching the new controller template data and e2e expectations. The remaining YAMLlint error is almost certainly due to the raw Go template directives rather than the rendered YAML; I’d keep this structure and, if the linter still complains, adjust your linter configuration or exclusions rather than the template.internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (1)
15-19: TLS propagation into Tempo ingestion and extraConfig.server HTTP TLS looks coherentThe template wiring matches the controller’s template data:
certName/caNameare only emitted whenTempoCertificateSecret/TempoCAConfigMapare non-empty, so TLS can rely on OpenShift serving certs when no explicit secret/CA is configured.TempoTLSEnableddrivesenabled: truefor both gRPC and HTTP OTLP ingestion and also the HTTPS protocol used on query endpoints.block_retentionbound to.TracesRetentionlines up with the newmetav1.Duration-backed retention and the e2e assertions.This all looks consistent with the new TLS semantics and retention handling.
Also applies to: 33-37, 52-60
tests/e2e/monitoring_test.go (1)
1903-2000: Cloud backend TLS helper for PersesDatasource provides solid coverage and cleanup
validatePersesDatasourceTLSWithCloudBackendis structured well: it skips cleanly when the PersesDatasource CRD is absent, ensures a clean slate, creates a backend-specific secret, enables traces with TLS, waits for a Ready TempoStack using that secret, and then verifies that thetempo-datasourcePersesDatasource uses an HTTPS gateway URL plus a TLS client pointing at thetempo-service-caConfigMap andservice-ca.crt. The per-backend wrappers for S3 and GCS reuse this path and the final cleanup viacleanupTracesConfigurationandcleanupTempoStackAndSecretshould keep state isolated between tests.internal/controller/services/monitoring/monitoring_controller_support.go (1)
210-231: TLS enablement and Tempo query endpoint protocol handling are correctly centralized
determineTLSEnabledprovides a clear, explicit rule (“TLS only whentraces.TLS != nil && traces.TLS.Enabled”), andaddTracesTemplateDatauses it to:
- Set
TempoTLSEnabledfor templates,- Populate
TempoCertificateSecret/TempoCAConfigMaponly when TLS is truly enabled, falling back to empty strings otherwise to avoid missing-key issues, and- Compute an
httpvshttpsprotocol that’s consistently applied toTempoQueryEndpointfor both PV (tempo-monolithic:3200) and S3/GCS (tempostack-gateway:8080) backends.The switch on
StorageBackendPV/S3/GCSmatches the new API constants, and this logic lines up with the new templates and e2e tests.Also applies to: 233-251, 561-570
api/services/v1alpha1/monitoring_types.go (1)
98-144: Traces TLS and storage backend schema changes match controller and template usageThe clarified
TracesTLSdocs, newStorageBackendPV/S3/GCSconstants, and theTracesStorageschema/XValidation rules line up well with how the controller and templates now behave: PV can carry an optionalsizeand no secret, whereas S3/GCS must provide a non-emptysecretand omitsize, and the backend is constrained to the three supported values with a default of"pv". Usingmetav1.DurationforRetentiondovetails with the controller’s use ofRetention.Duration.String()and the e2e assertions onblock_retention.
| // Enabled enables TLS for Tempo gRPC connections | ||
| // +kubebuilder:default=true | ||
| // Enabled enables TLS for Tempo OTLP ingestion (gRPC/HTTP) and query APIs (HTTP) | ||
| // TLS is disabled by default to maintain backward compatibility |
There was a problem hiding this comment.
At the moment this flag sets both ingestion and querying to false, is this the expected result or would it be better to keep tls for ingestion and perses seperate?
There's currently no e2e test confirming that non-TLS trace ingestion actually works. The PR should add one to validate the tls.insecure: true path functions correctly
There was a problem hiding this comment.
Thanks for the review @den-rgb, I have added the two tests in ValidateOpenTelemetryCollectorConfigurations that validate both non-TLS (default) and TLS-enabled trace ingestion. Regarding the unified TLS flag, I kept ingestion and querying together for simplicity and consistency.
dayakar349
force-pushed
the
RHOAIENG-38496
branch
from
e662d87 to
30d23a4
Compare
|
/test opendatahub-operator-e2e-hypershift |
dayakar349
force-pushed
the
RHOAIENG-38496
branch
from
30d23a4 to
bccad19
Compare
| SampleRatio string `json:"sampleRatio,omitempty"` | ||
| // TLS configuration for Tempo gRPC connections | ||
| // +optional | ||
| // +nullable |
There was a problem hiding this comment.
np: Don't think the nullable adds any functional value here
There was a problem hiding this comment.
You're right, it was redundant. I've removed // +nullable from monitoring_types.go.
There was a problem hiding this comment.
I updated the ValidateMonitoringServiceDisabled test to use .spec.ingestion.otlp.grpc.tls.enabled != true. This correctly handles cases where the entire ingestion block is omitted when TLS is disabled.
|
test failing on When TempoTLSEnabled is false, the entire ingestion block is omitted, not set to enabled. But the test expects |
dayakar349
force-pushed
the
RHOAIENG-38496
branch
from
bccad19 to
944ddf3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (1)
15-19: Multitenancy hardcoded to OpenShift; confirm this matches support matrix
multitenancy.enabled: trueandmode: openshiftare now unconditional, with tenant IDs derived from the namespace. That’s fine if this template is only ever used on OpenShift clusters, but it will be rigid for any non‑OpenShift use.If ODH needs to support other environments, you may eventually want these fields parameterized off API spec or defaults rather than hardcoded.
🧹 Nitpick comments (3)
api/services/v1alpha1/monitoring_types.go (1)
89-104: Nit: align TLS field comment with expanded TracesTLS semanticsThe
Traces.TLSfield comment still says “TLS configuration for Tempo gRPC connections”, butTracesTLSis now documented (and used) for both OTLP ingestion and query HTTP APIs. Consider updating this field comment for consistency with the type’s doc.tests/e2e/monitoring_test.go (2)
993-1012: Reasonable TLS-disable workaround before monitoring teardownThe pre‑teardown step that forces
.spec.monitoring.traces.tls.enabled = falseand then waits until.spec.ingestion.otlp.grpc.tls.enabled != trueonTempoMonolithicis a pragmatic way to avoid the tempo‑operator cert finalizer race. TheWithIgnoreNotFound(true)guard keeps this safe when TempoMonolithic isn’t present.
123-124: Cloud-backend Perses TLS helper and tests comprehensively validate the new flow
validatePersesDatasourceTLSWithCloudBackenddoes a full end‑to‑end check for both S3 and GCS:
- ensures a clean slate (including TempoStack + secret cleanup),
- creates backend‑appropriate dummy secrets,
- configures traces with TLS enabled and waits for
TempoStackReady with the right secret, and- asserts that PersesDatasource uses an HTTPS gateway URL plus a TLS client config pointing at the
tempo-service-caConfigMap andservice-ca.crt.The S3/GCS wrapper tests plug this into the main suite cleanly. If you want, you could reuse the
TracesStorageBackendS3/GCSconstants instead of string literals in the wrappers for consistency, but that’s purely cosmetic.Also applies to: 1944-2041
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(3 hunks)docs/api-overview.md(3 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/controller/services/monitoring/monitoring_controller_actions.go
- docs/api-overview.md
- internal/controller/services/monitoring/monitoring_controller_support_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/monitoring_test.go (6)
pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)tests/e2e/resource_options_test.go (2)
WithMinimalObject(155-170)WithEventuallyTimeout(294-298)pkg/cluster/gvk/gvk.go (4)
TempoMonolithic(533-537)Namespace(29-33)PersesDatasource(587-591)TempoStack(539-543)pkg/cluster/resources.go (1)
HasCRD(366-368)internal/controller/status/status.go (1)
ConditionTypeReady(74-74)pkg/utils/test/testf/testf_witht.go (1)
WithEventuallyTimeout(46-50)
internal/controller/services/monitoring/monitoring_controller_support.go (1)
api/services/v1alpha1/monitoring_types.go (4)
StorageBackendPV(115-115)StorageBackendS3(117-117)StorageBackendGCS(119-119)Traces(82-97)
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build/push catalog image
- GitHub Check: golangci-lint
- GitHub Check: Run tests and collect coverage on internal and pkg
🔇 Additional comments (7)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
19-27: TLS client configuration for Perses datasource looks correctThe conditional TLS block cleanly toggles client TLS based on
TempoTLSEnabledand correctly references thetempo-service-caConfigMap andservice-ca.crtkey. This matches the new CA ConfigMap template and should render valid YAML for both TLS and non‑TLS cases.internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml (1)
1-8: ConfigMap template correctly wired for service CA injectionThe
tempo-service-caConfigMap matches the Perses TLS reference and uses the correctinject-cabundleannotation; leavingdata: {}is fine since service-ca will populateservice-ca.crt.api/services/v1alpha1/monitoring_types.go (1)
99-110: TracesTLS and TracesStorage API/validation look consistent and safeUsing a pointer
TLSfield with anEnabledflag defaulting to false gives clear, backward‑compatible TLS semantics, anddetermineTLSEnabledcan rely on that without nil‑pointer risk. The newTracesStoragebackend enum and XValidation rules correctly enforce that:
- non‑
pvbackends require a non‑emptysecret, andsizeis only allowed for thepvbackend.The
metav1.Durationretention with a default keeps Tempo templates simple via.Retention.Duration.String(). All of this lines up with the controller/template changes.Also applies to: 112-145
internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (1)
26-47: TLS ingestion and extraConfig wiring for TempoMonolithic looks correctThe
{{- if .TempoTLSEnabled }}gate cleanly omits the ingestion block when TLS is disabled (matching the E2E expectation), and when enabled:
- both gRPC and HTTP OTLP ingestion set
tls.enabled: true,certName/caNameare only rendered when provided, allowing the Tempo operator to fall back to OpenShift serving certs otherwise, andextraConfig.tempo.server.http_tls_configis only added when an explicit certificate secret is used.
block_retention: {{.TracesRetention}}also lines up with the newTracesStorage.Retentionduration handling.Also applies to: 52-60
internal/controller/services/monitoring/monitoring_controller_support.go (1)
218-251: TLS templateData wiring and defaulting logic look solid
determineTLSEnabled’s “explicit enable only” behavior (nil TLS → false) pairs well withaddTracesTemplateData: you only readtraces.TLS.*when TLS is enabled, otherwise you set empty strings and keepTempoTLSEnabledfalse. The protocol toggle forTempoQueryEndpointand the backend‑aware endpoints for TempoMonolithic vs TempoStack all look consistent with the templates and new Perses TLS behavior.Also applies to: 561-570
tests/e2e/monitoring_test.go (2)
52-56: OTel collector TLS/non‑TLS tests nicely pin down expected behaviorThe added backend constants plus the two new
ValidateOpenTelemetryCollectorConfigurationscases clearly assert:
- default non‑TLS ingestion uses
otlp/tempowithtls.insecure == true, and- enabling
.spec.monitoring.traces.tls.enabledflips to CA‑based TLS and bearer‑token auth.This gives good coverage for the new TLS behavior without changing existing cases.
Also applies to: 349-385
1558-1577: Perses datasource non‑TLS expectations align with new default TLS semanticsExpecting the Tempo query URL to be HTTP by default and updating the custom error message to mention “TLS disabled by default” matches the controller logic (TLS opt‑in via
.spec.monitoring.traces.tls.enabled). The rest of the assertions keep the existing contract intact.
dayakar349
force-pushed
the
RHOAIENG-38496
branch
from
944ddf3 to
2e5c70b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/controller/services/monitoring/monitoring_controller_support_test.go (1)
967-1045: TLS determination and template-data tests are solid; minor test cleanups possibleThe new tests correctly pin down that TLS is purely driven by
traces.TLS.Enabled(nil → false) and thataddTracesTemplateDatachooses HTTP vs HTTPS and the appropriate query endpoint per backend. Two small polish suggestions you can take or leave:
- Consider using
serviceApi.StorageBackendPV/S3/GCSinstead of raw"pv","s3","gcs"in the test traces to keep tests aligned with the API constants.- Either assert on the protocol embedded in
OtlpEndpoint/TempoQueryEndpointor drop the unusedexpectedHTTPProtocolfield to avoid dead test data.Also applies to: 1036-1145
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(3 hunks)docs/api-overview.md(3 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml
- docs/api-overview.md
- internal/controller/services/monitoring/monitoring_controller_actions.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/controller/services/monitoring/monitoring_controller_support_test.go (1)
api/services/v1alpha1/monitoring_types.go (6)
Traces(82-97)TracesTLS(100-110)TracesStorage(125-145)StorageBackendPV(115-115)StorageBackendS3(117-117)StorageBackendGCS(119-119)
internal/controller/services/monitoring/monitoring_controller_support.go (1)
api/services/v1alpha1/monitoring_types.go (4)
StorageBackendPV(115-115)StorageBackendS3(117-117)StorageBackendGCS(119-119)Traces(82-97)
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: golangci-lint
🔇 Additional comments (7)
internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (1)
15-19: TLS and multitenancy template wiring looks consistentMultitenancy, ingestion TLS, and extraConfig HTTP TLS are wired to the new template data (
TempoTLSEnabled,TempoCertificateSecret,TempoCAConfigMap,TracesRetention) and match the controller logic and e2e expectations; no changes needed here.Also applies to: 33-37, 42-46, 52-59
api/services/v1alpha1/monitoring_types.go (1)
90-110: Traces TLS and storage API shape/validation look correctTLS being explicitly opt‑in and the TracesStorage enum/XValidation (pv vs s3/gcs with required non‑empty secret and pv‑only size) align well with controller/template usage; this should give good server‑side guarantees without surprising defaults.
Also applies to: 112-145
tests/e2e/monitoring_test.go (3)
123-124: New OTel/TLS tests correctly capture default and enabled behaviorsThe added test cases for non‑TLS (tls.insecure == true) and TLS‑enabled (ca_file + bearertokenauth) exporters, plus wiring them into the main suite, accurately exercise the new TLS semantics without over-coupling to implementation details.
Also applies to: 365-384
1537-1555: Perses datasource defaulting to HTTP when TLS is disabled is clearly assertedUpdating the Perses datasource configuration test to expect an HTTP Tempo endpoint (and adjusting the error message) correctly reflects the “TLS disabled by default” behavior and keeps the contract explicit.
1923-2007: Cloud-backend Perses TLS helper is well-structured and aligned with templatesThe
validatePersesDatasourceTLSWithCloudBackendhelper plus S3/GCS wrappers nicely enforce:
- clean slate + backend secret creation,
- TLS enablement on traces and TempoStack readiness, and
- PersesDatasource assertions for HTTPS gateway URL and client.tls config pointing at
tempo-service-ca/service-ca.crt.The cleanup of traces, TempoStack, and secrets at the end should keep tests isolated.
Also applies to: 2010-2020
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
19-27: Perses TLS client configuration matches CA ConfigMap designThe conditional
client.tlsblock keyed byTempoTLSEnabledcorrectly sources CA data from thetempo-service-caConfigMap (service-ca.crt) and matches the expectations asserted in the new e2e tests; indentation/control-flow also look good.internal/controller/services/monitoring/monitoring_controller_support.go (1)
218-251: TLS gating and endpoint generation logic are consistent with the new API and tests
determineTLSEnablednow being purelytraces.TLS.Enabled(defaulting to false) plus the updatedaddTracesTemplateData:
- always populates
TempoTLSEnabled,TempoCertificateSecret, andTempoCAConfigMap(empty when TLS is off), and- cleanly switches query endpoints between HTTP/HTTPS for PV vs S3/GCS backends using the shared protocol variable,
lines up well with the CRD comments and the added unit/e2e coverage. No changes needed here.
Also applies to: 561-570
|
/lgtm |
|
/test opendatahub-operator-e2e-hypershift |
|
/hold Revision 2e5c70b was retested 3 times: holding |
dayakar349
force-pushed
the
RHOAIENG-38496
branch
from
2e5c70b to
b1af5d8
Compare
|
New changes are detected. LGTM label has been removed. |
dayakar349
force-pushed
the
RHOAIENG-38496
branch
from
b1af5d8 to
8141b11
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/controller/services/monitoring/monitoring_controller_actions.go (1)
562-565: Consider conditionally deploying the CA ConfigMap only when TLS is enabled.The
TempoServiceCAConfigMapTemplateis always deployed alongside the datasource template, even when TLS is disabled. While this is harmless (the ConfigMap will be created but unused), it adds unnecessary resources to the cluster.Consider wrapping the template addition in a TLS check if feasible, or document that this is intentional for simplicity.
// Deploy datasource template (always deploy if CRD exists) templates := []odhtypes.TemplateInfo{ { FS: resourcesFS, Path: PersesTempoDatasourceTemplate, }, - { - FS: resourcesFS, - Path: TempoServiceCAConfigMapTemplate, - }, } + + // Deploy CA ConfigMap template when TLS is enabled + if monitoring.Spec.Traces != nil && monitoring.Spec.Traces.TLS != nil && monitoring.Spec.Traces.TLS.Enabled { + templates = append(templates, odhtypes.TemplateInfo{ + FS: resourcesFS, + Path: TempoServiceCAConfigMapTemplate, + }) + }internal/controller/services/monitoring/monitoring_controller_support_test.go (1)
1036-1145: Consider adding a GCS backend test case for completeness.The test covers PV and S3 backends with TLS enabled/disabled scenarios, but there's no explicit test case for GCS backend with TLS enabled. While line 1134 handles both S3 and GCS in the assertion, having an explicit GCS test case would improve coverage clarity.
Additionally, the Retention duration uses raw nanoseconds (
90 * 24 * 60 * 60 * 1000000000). Consider usingtime.Hourortime.Durationconstants for readability:- Retention: metav1.Duration{Duration: 90 * 24 * 60 * 60 * 1000000000}, // 90 days in nanoseconds + Retention: metav1.Duration{Duration: 90 * 24 * time.Hour}, // 90 daystests/e2e/monitoring_test.go (2)
993-1009: TempoMonolithic TLS-disable workaround before monitoring removal is reasonable; keep an eye on potential flakesThe workaround that first forces
managementState=Managed, sets.spec.monitoring.traces.tls.enabled = false(when traces exist), and then waits forTempoMonolithicto satisfy.spec.ingestion.otlp.grpc.tls.enabled != truedirectly targets the deletion issue you were seeing. UsingWithIgnoreNotFound(true)also makes this safe when aTempoMonolithicisn’t present.There is a brief window where monitoring is toggled back to Managed even if it was previously Removed, which could start (re)creating resources just before you flip it to Removed again. If you ever see intermittent flakes around this test, it might be worth tightening the precondition (e.g., only doing this dance when traces/TLS are actually enabled), but as written it’s a pragmatic fix for the current problem.
123-124: Cloud-backend Perses TLS tests look good; consider minor reuse and extra assertionThe new S3/GCS test entries and
validatePersesDatasourceTLSWithCloudBackendhelper give strong end‑to‑end coverage for the feature this PR is adding:
- Clean slate + backend secret creation per backend.
- Enabling traces with that backend and TLS.
- Waiting for
TempoStackto be Ready with the correctspec.storage.secret.- Asserting that
tempo-datasourcenow:
- Uses an HTTPS gateway URL on port 8080, and
- Has a TLS client config pointing at the
tempo-service-caConfigMap with the expected key path.Cleanup via
cleanupTracesConfigurationandcleanupTempoStackAndSecretkeeps these tests well isolated.Two small, optional follow‑ups you might consider:
- Reuse the
TracesStorageBackendS3/TracesStorageBackendGCSconstants instead of raw"s3"/"gcs"strings in the wrappers, so test inputs stay in sync with the values you use elsewhere.- Since a key part of the feature is ensuring the
tempo-service-caConfigMap has the injection annotation, adding an assertion on that ConfigMap (existence + annotation) in this flow would make the e2e coverage fully reflect the PR’s behavior, not just the PersesDatasource reference.Also applies to: 1941-2038
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(3 hunks)docs/api-overview.md(3 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml
- docs/api-overview.md
🧰 Additional context used
🧬 Code graph analysis (2)
internal/controller/services/monitoring/monitoring_controller_support_test.go (1)
api/services/v1alpha1/monitoring_types.go (6)
Traces(82-97)TracesTLS(100-110)TracesStorage(125-145)StorageBackendPV(115-115)StorageBackendS3(117-117)StorageBackendGCS(119-119)
internal/controller/services/monitoring/monitoring_controller_support.go (1)
api/services/v1alpha1/monitoring_types.go (4)
StorageBackendPV(115-115)StorageBackendS3(117-117)StorageBackendGCS(119-119)Traces(82-97)
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: kube-linter
- GitHub Check: golangci-lint
🔇 Additional comments (13)
api/services/v1alpha1/monitoring_types.go (4)
90-91: LGTM!The
+optionalannotation correctly marks the TLS field as optional, aligning with the backward-compatible default of TLS being disabled.
99-103: LGTM!The updated TracesTLS documentation clearly explains that TLS covers both ingestion and query APIs, and explicitly states TLS is disabled by default for backward compatibility.
112-120: LGTM!Well-documented storage backend constants that centralize the backend type values, improving maintainability over hardcoded strings.
122-145: LGTM!The TracesStorage struct with XValidation rules properly enforces:
- Secret is required when backend is not "pv"
- Size is only valid for "pv" backend
The validation logic and field documentation are clear.
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
19-27: LGTM!The TLS conditional block is correctly structured:
- Template directives at column 0 to avoid YAMLlint issues in the rendered output
- TLS configuration references
tempo-service-caConfigMap with the standard OpenShiftservice-ca.crtpath- Conditional rendering ensures TLS is only configured when explicitly enabled
The YAMLlint syntax error is a false positive since raw Go templates are not valid YAML, but the rendered output will be correct.
internal/controller/services/monitoring/monitoring_controller_actions.go (1)
49-49: LGTM!The new constant follows the existing naming convention and correctly references the tempo-service-ca-configmap template.
internal/controller/services/monitoring/monitoring_controller_support_test.go (1)
967-1034: LGTM!Comprehensive test coverage for
determineTLSEnabledcovering:
- TLS explicitly enabled/disabled
- TLS nil with all three storage backends (pv, s3, gcs)
The tests correctly validate that TLS defaults to
falsefor all backends when not explicitly set.internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (3)
14-19: LGTM!The multitenancy configuration is now properly structured with OpenShift mode and namespace-based tenant authentication. This addresses previous review feedback about parameterization.
52-59: LGTM!The extraConfig section correctly:
- Uses
TracesRetentiontemplate variable for block_retention- Only configures HTTP TLS server settings when an explicit certificate is provided
- Includes helpful comments explaining the OpenShift serving certs fallback behavior
33-46: The field namescertNameandcaNameare correct per the Tempo Operator CRD specification.These match the official TempoMonolithic CRD API for configuring OTLP gRPC and HTTP TLS.
certNamereferences a Kubernetes Secret containing tls.crt and tls.key, whilecaNameoptionally references a ConfigMap for mutual TLS (mTLS). No changes needed.internal/controller/services/monitoring/monitoring_controller_support.go (1)
218-251: TLS template data and protocol handling look consistent; just confirm PV+TLS query semanticsThe new flow where
determineTLSEnableddrivesTempoTLSEnabled, certificate/CA template keys, and thehttp/httpsprotocol forTempoQueryEndpointis clean and predictable. The invariant thattlsEnabledimpliestraces.TLS != nilis enforced bydetermineTLSEnabled, and the explicit else‑branch with empty strings guards against missing-key template issues. The “TLS disabled by default, explicitly opt‑in viatraces.TLS.Enabled” behavior also aligns with the new unit/e2e coverage.One thing to double‑check (non‑blocking): when TLS is enabled for a PV/TempoMonolithic backend, we’ll now emit an
https://...:3200query URL as well. If TempoMonolithic’s query port is not actually TLS‑terminated under that configuration, Perses would fail to connect; if it is, then this is exactly what we want. Worth validating against the TempoMonolithic TLS behavior to be sure we’re not over‑promising HTTPS there.Also applies to: 561-569
tests/e2e/monitoring_test.go (2)
365-384: New OTEL collector TLS/non‑TLS tests give solid coverage ofotlp/tempoexporter behaviorThe added “Non‑TLS Trace Ingestion (default)” and “TLS Trace Ingestion (enabled)” cases nicely pin down the expected config for the
otlp/tempoexporter:tls.insecure == trueby default when no TLS is configured, and explicittls.ca_file/auth.authenticatorwhen.spec.monitoring.traces.tls.enabled = true. This should make regressions in the TLS decision logic very visible while reusing the samewithMonitoringTracessetup as the basic traces test.No changes requested here.
1555-1574: Perses datasource config test correctly reflects “TLS disabled by default” semanticsThe comment and expectations here now match the controller semantics: with a PV backend and no TLS stanza, the Perses Tempo datasource should use an HTTP URL pointing at the TempoMonolithic query endpoint and have no TLS client config requirements. The custom error message was also updated to call that out explicitly, which will be helpful when this test fails.
No issues from my side.
|
/retest |
dayakar349
force-pushed
the
RHOAIENG-38496
branch
from
8141b11 to
d64884d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/monitoring_test.go (1)
1927-2024: Consider splitting the TLS validation helper into smaller functions.The
validatePersesDatasourceTLSWithCloudBackendhelper (97 lines) orchestrates a complete E2E TLS validation flow. While functional and well-commented, consider extracting sub-steps into smaller helpers for improved maintainability:
setupTLSBackend(backend, secret)- lines 1944-1956validateTempoStackTLSReady(backend, secret)- lines 1972-1984validatePersesDatasourceTLSConfig(backend, endpoint)- lines 1987-2005This would make the test flow easier to follow and the individual validations more reusable.
Current implementation is acceptable - the function is cohesive and correctly validates TLS with cloud backends, including the critical assertions for HTTPS endpoints and CA configuration (lines 1988-2005).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(3 hunks)docs/api-overview.md(3 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support_test.go(1 hunks)internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml(1 hunks)internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml(2 hunks)internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml(1 hunks)tests/e2e/monitoring_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controller/services/monitoring/monitoring_controller_actions.go
- docs/api-overview.md
- internal/controller/services/monitoring/monitoring_controller_support_test.go
- internal/controller/services/monitoring/resources/tempo-service-ca-configmap.tmpl.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
internal/controller/services/monitoring/monitoring_controller_support.go (1)
api/services/v1alpha1/monitoring_types.go (4)
StorageBackendPV(115-115)StorageBackendS3(117-117)StorageBackendGCS(119-119)Traces(82-97)
🪛 YAMLlint (1.37.1)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml
[error] 20-20: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: kube-linter
- GitHub Check: golangci-lint
🔇 Additional comments (9)
internal/controller/services/monitoring/resources/perses-tempo-datasource.tmpl.yaml (1)
19-27: TLS client block wiring for Perses Tempo datasource looks correctConditionally adding
client.tlson.TempoTLSEnabledwith thetempo-service-caConfigMap andservice-ca.crtpath matches the controller’s TLS flag and CA injection design, and the template directive placement avoids YAMLlint issues.internal/controller/services/monitoring/resources/tempo-monolithic.tmpl.yaml (2)
14-19: LGTM! Multitenancy configuration is correctly parameterized.The multitenancy configuration correctly uses template variables (
.Namespace) and aligns with OpenShift tenant isolation requirements. Past review concerns about hardcoding have been addressed.
26-60: LGTM! TLS configuration correctly implements conditional TLS with proper field mappings.The TLS configuration changes are well-structured:
- Conditional TLS blocks ensure ingestion TLS only enabled when
TempoTLSEnabledis true- Field renames (
certName/caName) align with Tempo operator API requirements- Explicit server TLS config (lines 53-59) only emitted when
TempoCertificateSecretis provided, with clear fallback documentation for OpenShift serving certsapi/services/v1alpha1/monitoring_types.go (2)
89-110: LGTM! TLS field documentation accurately reflects default-disabled behavior.The documentation clearly states that TLS covers both ingestion and query APIs and is disabled by default, aligning with the controller logic in
determineTLSEnabled. The+optionalmarker correctly indicates the field is not required.
112-145: LGTM! Storage backend constants and validation rules are well-designed.Excellent improvements:
- Public constants (
StorageBackendPV,StorageBackendS3,StorageBackendGCS) centralize backend type strings and prevent typos- CEL validation correctly enforces that cloud backends (S3/GCS) require the
secretfield- The
Backendfield has proper enum validation and sensible default ("pv")This provides a clean, type-safe API for storage configuration.
internal/controller/services/monitoring/monitoring_controller_support.go (2)
210-273: LGTM! TLS configuration logic is correct and well-structured.The TLS handling in
addTracesTemplateDatais clean and correct:
determineTLSEnabledcentralizes TLS state determination- Template data correctly populated with TLS certificates when enabled, empty strings otherwise
- Protocol selection (lines 234-237) properly defaults to
httpand switches tohttpsonly when TLS is enabled- Endpoint generation (lines 241-250) uses the new storage backend constants, eliminating hardcoded strings
- Past review concerns about dead code have been resolved
The comment on line 224 correctly documents the invariant that
traces.TLSis non-nil whentlsEnabledis true.
561-570: LGTM! TLS determination logic correctly implements explicit opt-in behavior.The
determineTLSEnabledfunction correctly implements the design requirement that TLS must be explicitly enabled:
- Returns
falsewhentraces.TLSisnil(default state)- Returns
traces.TLS.Enabledwhen TLS configuration is provided- Clear documentation states TLS is disabled by default
This aligns with the PR objective to avoid Tempo operator certificate provisioning issues by requiring explicit TLS enablement.
tests/e2e/monitoring_test.go (2)
123-124: LGTM! Test coverage correctly validates both TLS-disabled and TLS-enabled scenarios.The test cases provide comprehensive validation:
- Lines 123-124 register new test wrappers for S3/GCS TLS validation
- Lines 366-372: "Non-TLS Trace Ingestion (default)" validates the default behavior where
tls.insecure == truewhen TLS is not explicitly enabled- Lines 374-384: "TLS Trace Ingestion (enabled)" validates proper TLS configuration when explicitly enabled
This coverage ensures both the default (TLS-disabled) and opt-in (TLS-enabled) behaviors work correctly.
Also applies to: 365-384
1541-1559: LGTM! Test correctly validates TLS-disabled default behavior.The test validation correctly expects an HTTP endpoint (line 1542) when TLS is not explicitly enabled, aligning with the controller's
determineTLSEnabledfunction that returnsfalseby default. The comment on line 1541 clearly documents this expectation.
|
@dayakar349: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
Description
Adds TLS support for the Perses-Tempo datasource connection
https://issues.redhat.com/browse/RHOAIENG-38496
How Has This Been Tested?
created with HTTPS endpoint
(https://tempo-data-science-tempostack-gateway.opendatahub.svc.cluster.local:8080)
to Service CA ConfigMap (tempo-service-ca)
(service.beta.openshift.io/inject-cabundle=true) and certificate is populated by OpenShift
Screenshot or short clip
Merge criteria
E2E test suite update requirement
When bringing new changes to the operator code, such changes are by default required to be accompanied by extending and/or updating the E2E test suite accordingly.
To opt-out of this requirement:
E2E update requirement opt-out justificationsection belowE2E update requirement opt-out justification
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.