-
Notifications
You must be signed in to change notification settings - Fork 33
feat(operator): deploy and manage jumpstarter-telemetry in the operator (JEP-0013) #997
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,6 +114,13 @@ func (r *JumpstarterReconciler) reconcileCertificates(ctx context.Context, js *o | |
| } | ||
| } | ||
|
|
||
| // Create telemetry certificate if telemetry is enabled | ||
| if js.Spec.Telemetry != nil && js.Spec.Telemetry.Enabled { | ||
| if err := r.reconcileTelemetryCertificate(ctx, js, issuerRef); err != nil { | ||
| return fmt.Errorf("failed to reconcile telemetry certificate: %w", err) | ||
| } | ||
| } | ||
|
|
||
| // Reconcile CA ConfigMap AFTER certificates are created | ||
| // This ensures cert-manager has had a chance to create the CA secret | ||
| // which we need to populate the ConfigMap for the login service | ||
|
|
@@ -369,6 +376,28 @@ func (r *JumpstarterReconciler) reconcileRouterCertificate(ctx context.Context, | |
| return r.reconcileServerCertificate(ctx, js, issuerRef, certName, "router", dnsNames, extraLabels) | ||
| } | ||
|
|
||
| // reconcileTelemetryCertificate creates the TLS certificate for the telemetry service. | ||
| func (r *JumpstarterReconciler) reconcileTelemetryCertificate(ctx context.Context, js *operatorv1alpha1.Jumpstarter, issuerRef cmmeta.ObjectReference) error { | ||
| certName := getTelemetryCertSecretName(js) | ||
| includeInternalNames := !isExternalIssuer(js) | ||
| dnsNames := r.collectTelemetryDNSNames(js, includeInternalNames) | ||
| return r.reconcileServerCertificate(ctx, js, issuerRef, certName, "telemetry", dnsNames, nil) | ||
| } | ||
|
|
||
| // collectTelemetryDNSNames collects all DNS names for the telemetry certificate. | ||
|
Comment on lines
+379
to
+387
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add an integration test inside |
||
| 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 | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| // collectControllerDNSNames collects all DNS names for the controller certificate. | ||
| // When includeInternalNames is false, internal Kubernetes service DNS names are | ||
| // omitted so that external issuers (e.g. ACME/Let's Encrypt) don't attempt to | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -210,12 +210,24 @@ func (r *JumpstarterReconciler) Reconcile(ctx context.Context, req ctrl.Request) | |
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| // Reconcile Services | ||
| // Reconcile Telemetry Deployment (Service is reconciled below in the networking stage) | ||
| if err := r.reconcileTelemetryDeploymentStage(ctx, &jumpstarter); err != nil { | ||
| log.Error(err, "Failed to reconcile Telemetry deployment") | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| // Reconcile Services (controller, router, login endpoints, and telemetry ClusterIP) | ||
| if err := r.reconcileServices(ctx, &jumpstarter); err != nil { | ||
| log.Error(err, "Failed to reconcile Services") | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| // Reconcile Telemetry ClusterIP Service (part of the networking stage) | ||
| if err := r.reconcileTelemetryServiceStage(ctx, &jumpstarter); err != nil { | ||
| log.Error(err, "Failed to reconcile Telemetry service") | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| // Reconcile ConfigMaps (after deployments and services, before secrets) | ||
| if err := r.reconcileConfigMaps(ctx, &jumpstarter, desiredConfigMap); err != nil { | ||
| log.Error(err, "Failed to reconcile ConfigMaps") | ||
|
|
@@ -851,6 +863,14 @@ func (r *JumpstarterReconciler) createControllerDeployment(jumpstarter *operator | |
| }, | ||
| } | ||
|
|
||
| // Add telemetry endpoint env var when telemetry is enabled | ||
| if jumpstarter.Spec.Telemetry != nil && jumpstarter.Spec.Telemetry.Enabled { | ||
| envVars = append(envVars, corev1.EnvVar{ | ||
| Name: "GRPC_TELEMETRY_ENDPOINT", | ||
| Value: telemetryEndpointFor(jumpstarter.Namespace), | ||
| }) | ||
| } | ||
|
Comment on lines
+866
to
+872
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This env var is set on the controller deployment but nothing in the controller binary reads Is this intended for a future consumer, or can it be removed? If kept, it should at least have a comment explaining why it exists alongside the ConfigMap path. |
||
|
|
||
| var volumeMounts []corev1.VolumeMount | ||
| var volumes []corev1.Volume | ||
|
|
||
|
|
@@ -1292,6 +1312,20 @@ func (r *JumpstarterReconciler) buildConfig(ctx context.Context, jumpstarter *op | |
| Keys: jumpstarter.Spec.DeprecatedLabels.Keys, | ||
| } | ||
|
|
||
| // Telemetry configuration. | ||
| // Certificate is intentionally omitted until the telemetry binary supports TLS serving. | ||
| if jumpstarter.Spec.Telemetry != nil && jumpstarter.Spec.Telemetry.Enabled { | ||
| t := jumpstarter.Spec.Telemetry | ||
| telemetryCfg := &config.Telemetry{ | ||
| Enabled: true, | ||
| Endpoint: telemetryEndpointFor(jumpstarter.Namespace), | ||
| } | ||
| if t.Logging.Filter.MinSeverity != "" { | ||
| telemetryCfg.Logging.Filter.MinSeverity = t.Logging.Filter.MinSeverity | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor naming inconsistency: the CRD field here uses Would recommend updating the config struct's JSON tag to AI Generated, but reviewed by me. |
||
| } | ||
| cfg.Telemetry = telemetryCfg | ||
| } | ||
|
|
||
| // gRPC keepalive configuration | ||
| if jumpstarter.Spec.Controller.GRPC.Keepalive != nil { | ||
| ka := &cfg.Grpc.Keepalive | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.