feat(operator): deploy and manage jumpstarter-telemetry in the operator (JEP-0013) - #997
feat(operator): deploy and manage jumpstarter-telemetry in the operator (JEP-0013)#997bkhizgiy wants to merge 2 commits into
Conversation
…e operator (JEP-0013) Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com> Assisted-by: claude-opus-4.6
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe operator adds optional telemetry configuration to the Jumpstarter API and CRD. It reconciles telemetry Deployments, Services, certificates, controller settings, and readiness status. Tests cover resource lifecycle, configuration, TLS, status, defaults, and helper behavior. ChangesTelemetry management
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
controller/deploy/operator/internal/controller/jumpstarter/certificates.go (1)
379-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
GetTelemetryCertSecretNamefor the certificate name.Line 381 duplicates the
js.Name + telemetryCertSuffixconcatenation thatGetTelemetryCertSecretNameintelemetry.goalready performs. The Deployment mounts the Secret by that helper. A future change to the helper then silently breaks the mount.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/deploy/operator/internal/controller/jumpstarter/certificates.go` around lines 379 - 385, Update reconcileTelemetryCertificate to obtain certName through the existing GetTelemetryCertSecretName helper instead of concatenating js.Name with telemetryCertSuffix, keeping the certificate reconciliation flow unchanged.controller/deploy/operator/internal/controller/jumpstarter/telemetry.go (2)
88-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
fmt.Printfwith the structured logger.These
fmt.Printfcalls write to stdout and bypass the controller-runtime logger. They lose log level, timestamps, and reconcile context. Uselog.V(1).Infowith the diff as a field.♻️ Proposed change
diff, diffErr := generateDiff(existingDeployment, desiredDeployment) if diffErr != nil { log.V(1).Info("Failed to generate deployment diff", "error", diffErr) } else if diff != "" { - fmt.Printf("\n=== Telemetry deployment differences detected ===\n") - fmt.Printf("Name: %s\n", existingDeployment.Name) - fmt.Printf("Namespace: %s\n", existingDeployment.Namespace) - fmt.Printf("\n%s\n", diff) - fmt.Printf("==================================================\n\n") + log.V(1).Info("Telemetry deployment differences detected", + "name", existingDeployment.Name, + "namespace", existingDeployment.Namespace, + "diff", diff) }If the surrounding code uses the same
fmt.Printfpattern for the controller and router deployments, treat this as a consistency question instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/deploy/operator/internal/controller/jumpstarter/telemetry.go` around lines 88 - 97, Replace the fmt.Printf calls in the generateDiff success branch with a single structured log.V(1).Info call, including the deployment diff as a named field and preserving the existing telemetry-difference context. Apply the same change to any matching controller or router deployment diff logging nearby for consistency.
376-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn an explicit signal when an external issuer has no
caBundle.Line 384 returns
("", nil). The caller cannot distinguish "no CA needed" from "user forgot to setcaBundle". Exporters then get an empty CA and fail TLS verification at runtime with no operator-side signal.Log a warning at this branch, or set a status condition so the misconfiguration is visible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/deploy/operator/internal/controller/jumpstarter/telemetry.go` around lines 376 - 397, Update resolveTelemetryCA so the external-issuer branch with an empty IssuerRef.CABundle emits an operator-visible warning or sets an appropriate status condition before returning. Preserve the existing return behavior while clearly signaling that the external issuer is missing caBundle.controller/deploy/operator/internal/controller/jumpstarter/telemetry_test.go (1)
354-392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert on parsed config instead of raw substrings.
ContainSubstring("warning")andContainSubstring("enabled: true")match any part of the config document. The first can pass because of an unrelated log-level field, and the second can pass because of another feature block. The negative assertion at line 391 also fails if the wordtelemetryappears anywhere for another reason.Unmarshal
configDatainto the config struct and assert the telemetry fields directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/deploy/operator/internal/controller/jumpstarter/telemetry_test.go` around lines 354 - 392, The telemetry propagation tests should validate structured configuration rather than raw text matches. Update the test cases around getConfigData to unmarshal the ConfigMap data into the relevant config struct, then assert the telemetry enabled, service/image, and logging MinSeverity fields directly; for the disabled case, assert the parsed telemetry configuration is absent or disabled.
🔇 Additional comments (12)
controller/deploy/operator/internal/controller/jumpstarter/telemetry.go (2)
197-213: 🩺 Stability & Availability | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the
GRPC_TELEMETRY_ENDPOINTandCONTROLLER_KEYvalues on the telemetry pod.Two concerns in this env block:
GRPC_TELEMETRY_ENDPOINTresolves to the telemetry service itself. The telemetry pod does not need to dial itself. The controller Deployment is the consumer of this variable, andtelemetry_test.goline 297 asserts it there.- The secret name
"jumpstarter-controller-secret"is hardcoded, while other names in this file are CR-scoped (%s-telemetry,%s-controller-manager). If the operator creates the controller secret with a CR-scoped name, the pod stays inCreateContainerConfigError.
44-61: LGTM!Also applies to: 341-374, 399-429
controller/deploy/operator/internal/controller/jumpstarter/telemetry_test.go (1)
40-105: LGTM!Also applies to: 106-353, 394-575, 577-754
controller/deploy/operator/internal/controller/jumpstarter/certificates.go (1)
117-123: LGTM!controller/deploy/operator/api/v1alpha1/jumpstarter_types.go (3)
49-51: LGTM!
207-213: 📐 Maintainability & Code QualityRun the required operator checks.
Before merge, run
make lint-fix,make pkg-ty-operator,make pkg-test-operator, andmake testfrom the repository root. Runmake manifests generatefromcontroller/deploy/operatorafter the CRD type change. Confirm that generation leaves no unexpected diff.As per coding guidelines: run package tests, type checks, linting, the complete test suite, and regenerate manifests after CRD type changes.
Source: Coding guidelines
279-307: LGTM!controller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.go (1)
527-531: LGTM!Also applies to: 893-945
controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml (1)
2093-2119: LGTM!Also applies to: 2142-2211
controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go (2)
860-866: LGTM!
1322-1328: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that a pending telemetry CA triggers a prompt configuration refresh.
If
resolveTelemetryCAfails, this branch applies telemetry configuration withoutCertificate. It does not request an immediate retry. Confirm that a watch on the exact CA Secret or ConfigMap requeues theJumpstarterwhen the CA becomes available. Otherwise, return a requeueable error when the CA is required.controller/deploy/operator/internal/controller/jumpstarter/status.go (1)
481-503: LGTM!
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/deploy/operator/api/v1alpha1/jumpstarter_types.go`:
- Around line 309-325: Add +kubebuilder:default={} to the Logging field in
TelemetryConfig and the Filter field in TelemetryLoggingConfig so nested
defaults are applied when either object is absent. Regenerate the CRD using make
manifests generate from controller/deploy/operator; update
controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
accordingly at lines 2120-2141.
In `@controller/deploy/operator/internal/controller/jumpstarter/certificates.go`:
- Around line 387-399: The telemetry TLS Secret mount and Certificate creation
use inconsistent conditions, causing pods to wait for a Secret that is never
created. In
controller/deploy/operator/internal/controller/jumpstarter/certificates.go lines
387-399, update collectTelemetryDNSNames to provide DNS names for external
issuers, or skip telemetry Certificate creation and document that the Secret
must be user-supplied; in
controller/deploy/operator/internal/controller/jumpstarter/telemetry.go lines
218-244, gate the tls-certs volume using the same condition that creates the
telemetry Certificate.
In
`@controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go`:
- Around line 213-217: Update the reconciliation flow around reconcileTelemetry
and reconcileServices so the telemetry Deployment is reconciled in the
Deployment stage, while the telemetry ClusterIP Service is reconciled only
within the Services/networking stage after reconcileServices begins. Preserve
existing error handling and ensure the loop follows the required ordering before
ConfigMaps, Secrets, and status updates.
In `@controller/deploy/operator/internal/controller/jumpstarter/status.go`:
- Around line 114-125: Update the telemetry readiness handling in the status
reconciliation flow to explicitly remove ConditionTypeTelemetryDeploymentReady
when js.Spec.Telemetry is nil or disabled. Preserve the existing
checkTelemetryDeploymentReady and setCondition behavior for enabled telemetry.
In `@controller/deploy/operator/internal/controller/jumpstarter/telemetry.go`:
- Around line 37-42: The telemetry Service name must be unique per Jumpstarter
CR rather than using the fixed telemetryServiceName constant. Update the Service
creation and cleanup paths, including the logic around cleanupTelemetry, to
derive and consistently reuse a name based on jumpstarter.Name, matching the
telemetry Deployment naming and selector so multiple CRs can reconcile
independently.
---
Nitpick comments:
In `@controller/deploy/operator/internal/controller/jumpstarter/certificates.go`:
- Around line 379-385: Update reconcileTelemetryCertificate to obtain certName
through the existing GetTelemetryCertSecretName helper instead of concatenating
js.Name with telemetryCertSuffix, keeping the certificate reconciliation flow
unchanged.
In
`@controller/deploy/operator/internal/controller/jumpstarter/telemetry_test.go`:
- Around line 354-392: The telemetry propagation tests should validate
structured configuration rather than raw text matches. Update the test cases
around getConfigData to unmarshal the ConfigMap data into the relevant config
struct, then assert the telemetry enabled, service/image, and logging
MinSeverity fields directly; for the disabled case, assert the parsed telemetry
configuration is absent or disabled.
In `@controller/deploy/operator/internal/controller/jumpstarter/telemetry.go`:
- Around line 88-97: Replace the fmt.Printf calls in the generateDiff success
branch with a single structured log.V(1).Info call, including the deployment
diff as a named field and preserving the existing telemetry-difference context.
Apply the same change to any matching controller or router deployment diff
logging nearby for consistency.
- Around line 376-397: Update resolveTelemetryCA so the external-issuer branch
with an empty IssuerRef.CABundle emits an operator-visible warning or sets an
appropriate status condition before returning. Preserve the existing return
behavior while clearly signaling that the external issuer is missing caBundle.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b421c4af-37cc-4765-8be0-1517a5471d88
📒 Files selected for processing (8)
controller/deploy/operator/api/v1alpha1/jumpstarter_types.gocontroller/deploy/operator/api/v1alpha1/zz_generated.deepcopy.gocontroller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yamlcontroller/deploy/operator/internal/controller/jumpstarter/certificates.gocontroller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.gocontroller/deploy/operator/internal/controller/jumpstarter/status.gocontroller/deploy/operator/internal/controller/jumpstarter/telemetry.gocontroller/deploy/operator/internal/controller/jumpstarter/telemetry_test.go
| // Logging configuration for the telemetry log ingestion path. | ||
| Logging TelemetryLoggingConfig `json:"logging,omitempty"` | ||
| } | ||
|
|
||
| // TelemetryLoggingConfig configures the log push path to the telemetry service. | ||
| type TelemetryLoggingConfig struct { | ||
| // Filter controls which log entries are forwarded to the telemetry service. | ||
| Filter TelemetryLoggingFilterConfig `json:"filter,omitempty"` | ||
| } | ||
|
|
||
| // TelemetryLoggingFilterConfig controls which log entries are forwarded to the telemetry service. | ||
| type TelemetryLoggingFilterConfig struct { | ||
| // Minimum log severity to forward. | ||
| // Accepted values: debug, info, warning, error, critical. Defaults to "info". | ||
| // +kubebuilder:default="info" | ||
| // +kubebuilder:validation:Enum=debug;info;warning;error;critical | ||
| MinSeverity string `json:"minSeverity,omitempty"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- source definitions ---'
sed -n '280,340p' controller/deploy/operator/api/v1alpha1/jumpstarter_types.go
printf '%s\n' '--- generated schema ---'
sed -n '2095,2155p' controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
printf '%s\n' '--- nearby object-default markers ---'
rg -n -C 3 '\+kubebuilder:default=\{\}' controller/deploy/operator/api/v1alpha1 controller/deploy/operator
printf '%s\n' '--- manifest generation targets ---'
rg -n -C 3 'manifests|generate' controller/deploy/operator/Makefile Makefile controller/deploy/operator 2>/dev/null | head -120Repository: jumpstarter-dev/jumpstarter
Length of output: 1943
🏁 Script executed:
#!/bin/bash
set -e
sed -n '280,340p' controller/deploy/operator/api/v1alpha1/jumpstarter_types.go
sed -n '2095,2155p' controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
rg -n -C 3 '\+kubebuilder:default=\{\}' controller/deploy/operator/api/v1alpha1 controller/deploy/operator
rg -n -C 3 'manifests|generate' controller/deploy/operator/Makefile Makefile controller/deploy/operator 2>/dev/null | head -120Repository: jumpstarter-dev/jumpstarter
Length of output: 1943
🌐 Web query:
Kubernetes CRD defaulting nested fields parent object absent default object schema defaulting
💡 Result:
In Kubernetes Custom Resource Definitions (CRDs), defaulting occurs top-down [1][2]. When a parent object is absent from a submitted manifest, the Kubernetes API server does not "invent" the parent object, which means it cannot reach or apply defaults to any nested fields within that missing parent [3]. Consequently, if the parent field is missing, the nested defaults are skipped entirely [3]. To ensure nested fields are defaulted when their parent object is absent, you must ensure the parent object itself exists in the resource instance [4][3]. There are two primary strategies to achieve this: 1. Default the parent object to an empty object: By setting a default value of {} (an empty object) for the parent field in your OpenAPI schema, the API server will materialize the parent object when it is missing [4][5]. Once the parent object exists, the API server can proceed to apply the defaults for its nested fields [6][3]. 2. Default the entire nested structure: Alternatively, you can provide a complete default value for the entire nested structure at the parent level [5][3]. This ensures that when the parent is missing, the entire sub-tree is populated with the desired default values at once [3]. Key Considerations: - Validation: All default values must comply with the defined OpenAPI structural schema [1][2]. If a nested field is marked as required, providing a default of {} for the parent will cause the API server to reject the CRD because the default object itself does not satisfy the required field constraint [6][5]. In such cases, you must provide a full, valid default object that includes all required nested fields [5]. - Process: The API server applies defaults during deserialization [2]. It recursively traverses the object; if a field is undefined, it applies the specified default [1][2]. Because this is a static, schema-based process, it cannot handle dynamic defaults (e.g., those dependent on other fields or cluster state) [3]. If you require dynamic defaulting, you must use a mutating admission webhook instead [3]. These behaviors are consistent with how native Kubernetes resources handle defaulting [7][2]. Always ensure your CRD schema is "structural" to support consistent defaulting, validation, and pruning [1][2].
Citations:
- 1: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/575-crd-defaulting
- 2: https://www.kubernetes.dev/resources/keps/575/
- 3: https://codemia.io/knowledge-hub/path/missing_default_value_in_nested_field_of_kubernetes_custom_resource
- 4: https://stackoverflow.com/questions/70524978/missing-default-value-in-nested-field-of-kubernetes-custom-resource
- 5: https://ahmet.im/blog/crd-generation-pitfalls/
- 6: +kubebuilder:default={} does not use nested defaults kubernetes-sigs/controller-tools#622
- 7: apiextensions: implement defaulting kubernetes/kubernetes#77558
🌐 Web query:
controller-tools kubebuilder default={} struct field CRD OpenAPI schema
💡 Result:
In Kubebuilder and controller-tools, using the marker // +kubebuilder:default={} is the standard way to set an empty object or map as the default value for a struct field in a Custom Resource Definition (CRD) [1]. Key points regarding this functionality: 1. Support for Empty Defaults: Support for +kubebuilder:default={} was explicitly added to ensure that empty maps or structs are correctly serialized into the CRD's OpenAPI schema as default: {} rather than being omitted or incorrectly set to null [1]. This ensures the field is present in the resource even if it is otherwise empty [1]. 2. Mechanism: When this marker is applied to a field, controller-gen includes a default: {} entry within that field's definition in the generated OpenAPI V3 schema [2][3]. At runtime, the Kubernetes API server utilizes this default value during the object creation process [3]. 3. Recursive Defaulting: Setting a parent field to default: {} is the recommended practice for activating nested defaults [1][3]. The Kubernetes API server performs defaulting in a top-down manner: it applies the parent's default (e.g., {}) and then recursively applies defaults defined for individual nested fields [3]. 4. Troubleshooting Required Fields: If a nested field within the struct is marked as required in the OpenAPI schema, an empty default ({}) may cause validation errors because the default value itself must satisfy the schema's requirements [3]. In such cases, you must provide a default value that contains the required fields [3]. 5. Alternative Markers: You can also use the // +default={} marker (without the kubebuilder: prefix), which was introduced to align with core Kubernetes type conventions [4][5][6]. If both +kubebuilder:default and +default are specified on the same field, the kubebuilder: prefixed marker takes precedence [5]. When defining these defaults, ensure they are provided in the correct format (JSON) for the target type, and note that controller-gen performs best-effort validation during generation, with full validation occurring upon submission to the Kubernetes API server [7][6].
Citations:
- 1: 🐛 Add support for empty maps or lists kubernetes-sigs/controller-tools#863
- 2: Cannot use an empty map with kubebuilder:default marker kubernetes-sigs/controller-tools#550
- 3: +kubebuilder:default={} does not use nested defaults kubernetes-sigs/controller-tools#622
- 4: Kubebuilder doesn't respect
+defaultkubernetes-sigs/controller-tools#939 - 5: ✨ Add support for +default markers kubernetes-sigs/controller-tools#938
- 6: https://book.kubebuilder.io/reference/markers/crd-validation.html
- 7: https://github.com/kubernetes-sigs/controller-tools/blob/master/pkg/crd/markers/validation.go
🌐 Web query:
site:github.com/jumpstarter-dev/jumpstarter jumpstarter_types.go TelemetryLoggingFilterConfig minSeverity
💡 Result:
Based on the search results for the jumpstarter-dev/jumpstarter repository, there is no evidence of a field or type named TelemetryLoggingFilterConfig or minSeverity defined within the file controller/deploy/operator/api/v1alpha1/jumpstarter_types.go [1][2]. The jumpstarter_types.go file defines the API for the Jumpstarter Kubernetes operator, primarily containing structs such as JumpstarterSpec, JumpstarterStatus, ControllerConfig, and RoutersConfig [1][2]. While the project includes configuration and logging mechanisms, current source code and recent pull requests do not indicate the presence of a TelemetryLoggingFilterConfig struct [3][4][5]. It is possible that this field was part of a different project, a deprecated experimental feature, or a misnamed reference. If you are working with a specific fork or an unmerged branch, you may want to verify the exact path or repository where this configuration is defined.
Citations:
- 1: https://github.com/jumpstarter-dev/jumpstarter/blob/5bfa067b/controller/deploy/operator/api/v1alpha1/jumpstarter_types.go
- 2: 84085b1
- 3: feat: add tags field to lease, to allow user set metadata #622
- 4: feat: operator auto-detects version for controller/router image defaults #844
- 5: https://github.com/jumpstarter-dev/jumpstarter/blob/5bfa067b/controller/internal/config/types.go
Default the intermediate logging objects.
When spec.telemetry.logging or spec.telemetry.logging.filter is absent, Kubernetes cannot apply the nested minSeverity default. Add +kubebuilder:default={} to both fields, then regenerate the CRD with make manifests generate from controller/deploy/operator.
📍 Affects 2 files
controller/deploy/operator/api/v1alpha1/jumpstarter_types.go#L309-L325(this comment)controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml#L2120-L2141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/deploy/operator/api/v1alpha1/jumpstarter_types.go` around lines
309 - 325, Add +kubebuilder:default={} to the Logging field in TelemetryConfig
and the Filter field in TelemetryLoggingConfig so nested defaults are applied
when either object is absent. Regenerate the CRD using make manifests generate
from controller/deploy/operator; update
controller/deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
accordingly at lines 2120-2141.
Source: Coding guidelines
| // collectTelemetryDNSNames collects all DNS names for the telemetry certificate. | ||
| func (r *JumpstarterReconciler) collectTelemetryDNSNames(js *operatorv1alpha1.Jumpstarter, includeInternalNames bool) []string { | ||
| var dnsNames []string | ||
| if includeInternalNames { | ||
| dnsNames = append(dnsNames, | ||
| telemetryServiceName, | ||
| fmt.Sprintf("%s.%s", telemetryServiceName, js.Namespace), | ||
| fmt.Sprintf("%s.%s.svc", telemetryServiceName, js.Namespace), | ||
| fmt.Sprintf("%s.%s.svc.cluster.local", telemetryServiceName, js.Namespace), | ||
| ) | ||
| } | ||
| return dnsNames | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The telemetry TLS Secret can be mounted although no Certificate creates it. The Deployment gates the TLS mount on Spec.CertManager.Enabled, while the Certificate is created only when an issuer resolves and at least one DNS name exists. For an external issuer the DNS name list is empty, and for an unresolved issuer reconcileCertificates returns early. The pods then wait forever for a Secret that no controller creates.
controller/deploy/operator/internal/controller/jumpstarter/certificates.go#L387-L399: return DNS names for external issuers, or skip telemetry certificate creation and document that the user supplies the Secret.controller/deploy/operator/internal/controller/jumpstarter/telemetry.go#L218-L244: gate thetls-certsvolume on the same condition that creates the telemetry Certificate.
📍 Affects 2 files
controller/deploy/operator/internal/controller/jumpstarter/certificates.go#L387-L399(this comment)controller/deploy/operator/internal/controller/jumpstarter/telemetry.go#L218-L244
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/deploy/operator/internal/controller/jumpstarter/certificates.go`
around lines 387 - 399, The telemetry TLS Secret mount and Certificate creation
use inconsistent conditions, causing pods to wait for a Secret that is never
created. In
controller/deploy/operator/internal/controller/jumpstarter/certificates.go lines
387-399, update collectTelemetryDNSNames to provide DNS names for external
issuers, or skip telemetry Certificate creation and document that the Secret
must be user-supplied; in
controller/deploy/operator/internal/controller/jumpstarter/telemetry.go lines
218-244, gate the tls-certs volume using the same condition that creates the
telemetry Certificate.
| // Reconcile Telemetry (Deployment + ClusterIP Service) | ||
| if err := r.reconcileTelemetry(ctx, &jumpstarter); err != nil { | ||
| log.Error(err, "Failed to reconcile Telemetry") | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reconcile the telemetry Service in the Services/networking stage.
reconcileTelemetry manages a ClusterIP Service, but Line 214 runs before reconcileServices at Line 220. Split Deployment and Service reconciliation, or move the Service portion into the Services/networking stage.
As per coding guidelines, “The reconcile loop must follow this order: fetch CR, apply runtime defaults, reconcile RBAC, reconcile Controller Deployment, reconcile Router Deployments, reconcile Services/networking, reconcile ConfigMaps, reconcile Secrets, update status”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@controller/deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go`
around lines 213 - 217, Update the reconciliation flow around reconcileTelemetry
and reconcileServices so the telemetry Deployment is reconciled in the
Deployment stage, while the telemetry ClusterIP Service is reconciled only
within the Services/networking stage after reconcileServices begins. Preserve
existing error handling and ensure the loop follows the required ordering before
ConfigMaps, Secrets, and status updates.
Source: Coding guidelines
| // Check telemetry deployment readiness (only if enabled) | ||
| if js.Spec.Telemetry != nil && js.Spec.Telemetry.Enabled { | ||
| telReady, telMsg := r.checkTelemetryDeploymentReady(ctx, js) | ||
| setCondition(js, operatorv1alpha1.ConditionTypeTelemetryDeploymentReady, | ||
| telReady, | ||
| conditionReason(telReady, "DeploymentAvailable", "DeploymentNotAvailable"), | ||
| telMsg) | ||
| if !telReady { | ||
| allReady = false | ||
| messages = append(messages, telMsg) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clear telemetry readiness when telemetry is disabled.
If telemetry was enabled and is then disabled, this branch does not update or remove ConditionTypeTelemetryDeploymentReady. The status can continue to report a ready telemetry Deployment after reconciliation deletes it. Remove the condition in the disabled branch.
Proposed fix
if js.Spec.Telemetry != nil && js.Spec.Telemetry.Enabled {
// ...
+ } else {
+ meta.RemoveStatusCondition(&js.Status.Conditions, operatorv1alpha1.ConditionTypeTelemetryDeploymentReady)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check telemetry deployment readiness (only if enabled) | |
| if js.Spec.Telemetry != nil && js.Spec.Telemetry.Enabled { | |
| telReady, telMsg := r.checkTelemetryDeploymentReady(ctx, js) | |
| setCondition(js, operatorv1alpha1.ConditionTypeTelemetryDeploymentReady, | |
| telReady, | |
| conditionReason(telReady, "DeploymentAvailable", "DeploymentNotAvailable"), | |
| telMsg) | |
| if !telReady { | |
| allReady = false | |
| messages = append(messages, telMsg) | |
| } | |
| } | |
| // Check telemetry deployment readiness (only if enabled) | |
| if js.Spec.Telemetry != nil && js.Spec.Telemetry.Enabled { | |
| telReady, telMsg := r.checkTelemetryDeploymentReady(ctx, js) | |
| setCondition(js, operatorv1alpha1.ConditionTypeTelemetryDeploymentReady, | |
| telReady, | |
| conditionReason(telReady, "DeploymentAvailable", "DeploymentNotAvailable"), | |
| telMsg) | |
| if !telReady { | |
| allReady = false | |
| messages = append(messages, telMsg) | |
| } | |
| } else { | |
| meta.RemoveStatusCondition(&js.Status.Conditions, operatorv1alpha1.ConditionTypeTelemetryDeploymentReady) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/deploy/operator/internal/controller/jumpstarter/status.go` around
lines 114 - 125, Update the telemetry readiness handling in the status
reconciliation flow to explicitly remove ConditionTypeTelemetryDeploymentReady
when js.Spec.Telemetry is nil or disabled. Preserve the existing
checkTelemetryDeploymentReady and setCondition behavior for enabled telemetry.
| const ( | ||
| telemetryPort = 9093 | ||
| telemetryCertSuffix = "-telemetry-tls" | ||
| telemetryServiceName = "jumpstarter-telemetry" | ||
| telemetryComponentApp = "jumpstarter-telemetry" | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Scope the telemetry Service name to the CR name.
telemetryServiceName is a fixed string, but the telemetry Deployment name is CR-scoped (fmt.Sprintf("%s-telemetry", jumpstarter.Name)). Two Jumpstarter CRs in one namespace then target the same Service object. The second reconcile calls controllerutil.SetControllerReference on a Service already owned by the first CR, which returns AlreadyOwnedError and fails reconciliation. cleanupTelemetry also deletes that shared Service for all CRs when one CR disables telemetry.
The selector adds "controller": jumpstarter.Name, so the Service also cannot route to more than one CR's pods.
Consider deriving the Service name from the CR name, or documenting and validating the single-CR-per-namespace assumption.
Also applies to: 131-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/deploy/operator/internal/controller/jumpstarter/telemetry.go`
around lines 37 - 42, The telemetry Service name must be unique per Jumpstarter
CR rather than using the fixed telemetryServiceName constant. Update the Service
creation and cleanup paths, including the logic around cleanupTelemetry, to
derive and consistently reuse a name based on jumpstarter.Name, matching the
telemetry Deployment naming and selector so multiple CRs can reconcile
independently.
b5cd056 to
4dad268
Compare
Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com> Assisted-by: claude-opus-4.6
4dad268 to
9492a6e
Compare
1 participant
Summary
Integrates the
jumpstarter-telemetryservice introduced in #930 into the Jumpstarter operator.Telemetry can now be configured through the
JumpstarterCR and is automatically deployed and managed by the operator, following the same patterns as the controller and router.What changed
Added telemetry Deployment and ClusterIP Service reconciliation, including cleanup when telemetry is disabled.
Added a new optional
spec.telemetryconfiguration with:enabledimage/imagePullPolicyreplicaslogging.filter.minSeverityresourcesAdded telemetry reconciliation to the main controller loop.
Configured the controller to advertise the telemetry endpoint to exporters through
GetServiceEndpoints.Added telemetry endpoint, certificate, and log filter configuration to the controller ConfigMap.
Added TLS certificate reconciliation through cert-manager, supporting both self-signed and external issuers.
Added a
TelemetryDeploymentReadystatus condition.Added integration and unit tests covering the telemetry lifecycle, replicas, TLS, configuration, probes, and status handling.
Usage
Telemetry can be enabled through the
JumpstarterCR:When enabled, the operator creates the telemetry Deployment and Service, configures TLS when cert-manager is enabled, and configures the controller to advertise the telemetry endpoint to exporters.
To disable telemetry and clean up its resources: