Skip to content

Conversation

@evakhoni
Copy link

@evakhoni evakhoni commented Dec 14, 2025

Summary

This PR adds kubebuilder defaults to enable minimal Jumpstarter CR configuration.

Changes

Required Fields

  • spec is now required (validation at CRD level)
  • spec.baseDomain is now required (users must provide their cluster's domain)

Kubebuilder Defaults

  • Add +kubebuilder:default={} on Controller and Routers struct fields
  • Add +kubebuilder:default values for nested fields (image, replicas, imagePullPolicy, etc.)
  • Regenerated CRD manifests with cascading defaults

Dynamic Endpoint Generation

  • Auto-generate endpoints from baseDomain unless explicitly specified
  • Auto-select Route (OpenShift), Ingress (standard K8s) based on cluster capabilities, with fallback to ClusterIP.

Minimal Valid CR

apiVersion: operator.jumpstarter.dev/v1alpha1
kind: Jumpstarter
metadata:
  name: example
  namespace: jumpstarter
spec:
  baseDomain: jumpstarter.my-cluster-base-domain.com

Summary by CodeRabbit

  • New Features

    • Automatic default container images for controller and router
    • Runtime generation of default GRPC endpoints for controller and routers
    • Default replica counts set for router components
  • Documentation

    • Added an example Jumpstarter configuration to the operator manifest
  • Refactor

    • Centralized and improved defaults application in operator reconciliation flow

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 14, 2025

Walkthrough

Adds CRD defaults for controller and routers (including default images and empty-object defaults), updates bundle CSV examples/timestamp, and introduces runtime endpoint defaulting in the controller that sets/ensures endpoint service types based on Route/Ingress availability before reconciliation.

Changes

Cohort / File(s) Summary
API Type Updates
deploy/operator/api/v1alpha1/jumpstarter_types.go
Added kubebuilder default markers for spec.controller and spec.routers (default: {}), and default image markers for ControllerConfig and RoutersConfig (quay.io/jumpstarter-dev/jumpstarter-controller:latest).
CRD Base
deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
Added default: {} for spec.controller and spec.routers; set default images for controller and routers; adjusted replica defaults and ensured defaults present in OpenAPI schema.
Bundle / CRD Manifest
deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml, deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml
Bundle CRD aligned with base CRD defaults (controller/routers defaults and images); CSV timestamp updated.
Manifests (manifests/bases CSV)
deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
Replaced empty alm-examples with a concrete Jumpstarter CR example in CSV annotations.
Endpoint Defaults Implementation
deploy/operator/internal/controller/jumpstarter/endpoints/defaults.go
New ApplyEndpointDefaults(spec *JumpstarterSpec, routeAvailable, ingressAvailable bool) added; generates default controller and router endpoints when baseDomain is set and ensures a service type is enabled (prefers Route → Ingress → ClusterIP).
Endpoints Reconciler
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go
Added Reconciler.ApplyDefaults(spec *JumpstarterSpec) delegating to ApplyEndpointDefaults with discovered availability flags; removed previous static ClusterIP creation from controller/router endpoint reconcile paths.
Jumpstarter Controller Flow
deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go
Invokes r.EndpointReconciler.ApplyDefaults(&jumpstarter.Spec) early in reconcile (before RBAC reconciliation) to apply runtime endpoint defaults.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Reconciler
participant EndpointReconciler
participant ClusterDetector as Cluster (Route/Ingress)
participant Spec as JumpstarterSpec

Reconciler->>EndpointReconciler: ApplyDefaults(&Spec)
EndpointReconciler->>ClusterDetector: Check RouteAvailable / IngressAvailable
ClusterDetector-->>EndpointReconciler: routeAvailable, ingressAvailable
EndpointReconciler->>Spec: ApplyEndpointDefaults(spec, routeAvailable, ingressAvailable)
Spec-->>EndpointReconciler: updated endpoints (ensure service types)
EndpointReconciler-->>Reconciler: return
Reconciler->>Reconciler: continue reconcile (RBAC, other reconcilers)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Inspect ApplyEndpointDefaults for correct hostname generation, replica naming, and handling when baseDomain is unset.
  • Verify precedence logic for service-type enabling (Route → Ingress → ClusterIP) and interactions with existing endpoints.
  • Confirm CRD/base and bundle manifest defaults are consistent and sample CSV example is valid.

Possibly related PRs

Suggested reviewers

  • mangelajo
  • bkhizgiy

Poem

🐰
I nudged defaults into fields so neat,
Domains and routers now have a seat,
Routes first, then Ingress — ClusterIP when thin,
Endpoints hop in and the reconcile grins,
A tiny rabbit cheer for the new default spin! 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: adding sensible defaults to enable minimal Jumpstarter resource configuration, which is the core objective of the PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@evakhoni
Copy link
Author

@mangelajo PTAL if this is the right approach.
I have tested it and it seems to do the job well with just the defaults provided at the object creation, with only baseDomain requiring a change afterwards as I haven't found yet how to make it appear with the default initially.

Copy link
Member

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments, it's looking good

Copy link
Member

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks good, can you check the detail from my comment?

why did we make baseDomain mandatory? will that allow it to be empty?

I would leave baseDomain as it was originally, and we can handle that in another PR for baseDomain detection.

@evakhoni
Copy link
Author

it looks good, can you check the detail from my comment?

why did we make baseDomain mandatory? will that allow it to be empty?

I would leave baseDomain as it was originally, and we can handle that in another PR for baseDomain detection.

issue is, if we won't make it mandatory, and the user will forget (or won't know) to apply it, an instance of jumpstarter will be created with no errors, but it will not function. in this case the result will be the same as having example.com there, which is what we decided to avoid doing.

the addition of baseDomain auto detection in the next PR will just soften this requirement for openshift clusters, but iiuc there should be no use case where a user should be allowed to create a jumpstarter without endpoints at all (either default or explicit) which is what would happen if we lift this requirement. correct me if i'm wrong 🤔 :)

@evakhoni evakhoni marked this pull request as ready for review December 16, 2025 16:13
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (1)

5-21: Update alm-examples to use a valid Jumpstarter spec

Now that spec and spec.baseDomain are required, the alm-examples entry with "spec": null is no longer valid and will generate a non-applying example in OLM; please align this example with the new minimal spec (e.g. including a baseDomain like in the base CSV).

🧹 Nitpick comments (6)
deploy/operator/api/v1alpha1/jumpstarter_types.go (1)

162-166: Default controller/router images wired via kubebuilder annotations

Hard-wiring the default image to quay.io/jumpstarter-dev/jumpstarter-controller:latest for both controller and routers keeps CRD defaults, reconciler, and docs aligned; just remember this needs updating on future image/tag changes across all manifests.

Also applies to: 197-201

deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml (2)

1087-1091: CRD image defaults are consistent with type annotations

The CRD-level defaults for controller and router images align with the Go kubebuilder defaults and ensure a working deployment out of the box; just keep this in sync with future image/tag changes across Helm and operator manifests.

Also applies to: 1632-1637


1901-1903: Spec and baseDomain now enforced at schema level

Requiring spec.baseDomain under spec.required and spec under the root required list matches the Go type changes and prevents creation of non-functional Jumpstarter CRs; consider calling this out in release notes for users upgrading existing clusters.

Also applies to: 1910-1912

deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (1)

63-67: Reconciler.ApplyDefaults cleanly exposes endpoint defaulting

Exposing ApplyDefaults on the endpoints Reconciler and delegating to ApplyEndpointDefaults with discovered Route/Ingress availability gives the controller a simple, capability-aware hook for runtime defaults without duplicating logic.

deploy/operator/internal/controller/jumpstarter/endpoints/defaults.go (1)

48-85: Endpoint defaulting matches new baseDomain requirement and GRPC semantics

ApplyEndpointDefaults’s behavior—skip when BaseDomain is empty, otherwise generate grpc.<baseDomain> and router-$(replica).<baseDomain> gRPC endpoints and ensure all endpoints have at least one service type—fits the new required baseDomain constraint and provides sensible, cluster-aware defaults for both controller and routers.

deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml (1)

1086-1091: Consider recommending explicit version tags in documentation.

Using :latest is reasonable for quick-start scenarios, but can lead to unpredictable behavior in production when the underlying image changes. Consider adding documentation guidance that production deployments should override this with a pinned version tag.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a049e90 and a1f73a1.

⛔ Files ignored due to path filters (1)
  • deploy/operator/dist/install.yaml is excluded by !**/dist/**
📒 Files selected for processing (8)
  • deploy/operator/api/v1alpha1/jumpstarter_types.go (5 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (1 hunks)
  • deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml (5 hunks)
  • deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml (5 hunks)
  • deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml (1 hunks)
  • deploy/operator/internal/controller/jumpstarter/endpoints/defaults.go (1 hunks)
  • deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (1 hunks)
  • deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go (1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: hack/deploy_with_helm.sh:26-34
Timestamp: 2025-05-13T19:57:56.811Z
Learning: The jumpstarter project uses a custom kind cluster configuration with an expanded NodePort range (3000-32767) and explicit port mappings for ingress (5080/5443) and gRPC services (30010/30011 mapped to 8082/8083).
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: hack/deploy_with_helm.sh:26-34
Timestamp: 2025-05-13T19:57:56.811Z
Learning: The jumpstarter-dev repository uses a custom kind cluster configuration that allows NodePort services to use non-standard ports 5080 and 5443, outside the default Kubernetes NodePort range (30000-32767).
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 190
File: api/v1alpha1/exporter_helpers.go:16-24
Timestamp: 2025-11-14T15:47:36.325Z
Learning: In the jumpstarter-controller project, migration annotations (jumpstarter.dev/migrated-namespace and jumpstarter.dev/migrated-uid) that override namespace and UID values in authentication tokens are acceptable without additional validation webhooks because the security model assumes only administrators have write access to Exporter and Client resources via K8s RBAC.
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.

Applied to files:

  • deploy/operator/api/v1alpha1/jumpstarter_types.go
  • deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go
  • deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml
📚 Learning: 2025-11-14T15:47:36.325Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 190
File: api/v1alpha1/exporter_helpers.go:16-24
Timestamp: 2025-11-14T15:47:36.325Z
Learning: In the jumpstarter-controller project, migration annotations (jumpstarter.dev/migrated-namespace and jumpstarter.dev/migrated-uid) that override namespace and UID values in authentication tokens are acceptable without additional validation webhooks because the security model assumes only administrators have write access to Exporter and Client resources via K8s RBAC.

Applied to files:

  • deploy/operator/api/v1alpha1/jumpstarter_types.go
  • deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml
📚 Learning: 2025-05-13T19:56:27.924Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml:23-26
Timestamp: 2025-05-13T19:56:27.924Z
Learning: In the jumpstarter-controller project, the router service uses the same ConfigMap as the controller service (controller-cm.yaml) even though it has been moved to its own separate deployment.

Applied to files:

  • deploy/operator/api/v1alpha1/jumpstarter_types.go
  • deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
  • deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml
📚 Learning: 2025-05-13T19:57:56.811Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: hack/deploy_with_helm.sh:26-34
Timestamp: 2025-05-13T19:57:56.811Z
Learning: The jumpstarter project uses a custom kind cluster configuration with an expanded NodePort range (3000-32767) and explicit port mappings for ingress (5080/5443) and gRPC services (30010/30011 mapped to 8082/8083).

Applied to files:

  • deploy/operator/api/v1alpha1/jumpstarter_types.go
  • deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
  • deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml
📚 Learning: 2025-10-24T11:57:13.484Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/rbac.go:193-196
Timestamp: 2025-10-24T11:57:13.484Z
Learning: In the Jumpstarter operator codebase (deploy/operator/internal/controller/jumpstarter/rbac.go), the Role created by `createRole()` defines RBAC permissions for the managed Jumpstarter controller application, not for the operator itself. The managed controller needs delete permissions on secrets for its runtime operations.

Applied to files:

  • deploy/operator/api/v1alpha1/jumpstarter_types.go
  • deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go
  • deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml
📚 Learning: 2025-05-13T19:57:56.811Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: hack/deploy_with_helm.sh:26-34
Timestamp: 2025-05-13T19:57:56.811Z
Learning: The jumpstarter-dev repository uses a custom kind cluster configuration that allows NodePort services to use non-standard ports 5080 and 5443, outside the default Kubernetes NodePort range (30000-32767).

Applied to files:

  • deploy/operator/api/v1alpha1/jumpstarter_types.go
  • deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
  • deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml
📚 Learning: 2025-06-06T17:04:50.040Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 144
File: deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json:60-156
Timestamp: 2025-06-06T17:04:50.040Z
Learning: In the jumpstarter controller Helm chart, empty JWT authenticator configuration is valid by design. When the JWT config is empty, the system falls back to using the internal authenticator, which is why all properties in the JWTAuthenticator schema are optional.

Applied to files:

  • deploy/operator/api/v1alpha1/jumpstarter_types.go
🧬 Code graph analysis (2)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (2)
deploy/operator/api/v1alpha1/jumpstarter_types.go (1)
  • JumpstarterSpec (130-157)
deploy/operator/internal/controller/jumpstarter/endpoints/defaults.go (1)
  • ApplyEndpointDefaults (51-85)
deploy/operator/internal/controller/jumpstarter/endpoints/defaults.go (2)
deploy/operator/api/v1alpha1/jumpstarter_types.go (5)
  • Endpoint (385-419)
  • RouteConfig (422-435)
  • IngressConfig (438-456)
  • ClusterIPConfig (504-516)
  • JumpstarterSpec (130-157)
deploy/helm/jumpstarter/charts/jumpstarter-controller/model.py (2)
  • Route (86-91)
  • Ingress (75-83)
🔇 Additional comments (10)
deploy/operator/api/v1alpha1/jumpstarter_types.go (3)

131-136: BaseDomain required + non-omitempty looks appropriate

Making BaseDomain required with a strict DNS-style pattern and removing omitempty matches the endpoint-generation semantics and avoids silently broken instances; this is a solid trade-off given Jumpstarter can’t function without a base domain.


144-152: Struct-level defaults for Controller and Routers are consistent

Using +kubebuilder:default={} on Controller and Routers ensures those nested configs are always present even with minimal specs, which pairs well with the runtime defaulting logic and avoids nil-struct surprises.


533-535: Requiring Spec at the top level matches CRD tightening

Marking Spec as required and dropping omitempty ensures invalid spec: null CRs are rejected and matches the stricter OpenAPI schema in the CRD; this is a good consistency and UX improvement.

deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go (1)

131-134: Good placement of runtime defaulting before reconcile

Calling r.EndpointReconciler.ApplyDefaults(&jumpstarter.Spec) once after fetch/deletion checks and before all reconcile steps centralizes endpoint defaulting and keeps the rest of the reconciler working on a normalized spec without introducing new nil-pointer risks.

deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml (1)

5-18: Minimal alm-examples now matches CRD requirements

The updated alm-examples with a single Jumpstarter instance and spec.baseDomain is aligned with the new required fields and illustrates the minimal valid configuration well.

deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml (1)

433-434: default: {} for controller/routers matches Go defaults

Adding default: {} on spec.controller and spec.routers mirrors the kubebuilder default={} markers and guarantees these objects exist for minimal specs, which simplifies controller logic and defaulting.

Also applies to: 1373-1374

deploy/operator/internal/controller/jumpstarter/endpoints/defaults.go (1)

25-46: Centralized service-type selection logic looks solid

ensureEndpointServiceType correctly treats “no enabled type” as a cue to auto-pick Route, then Ingress, then ClusterIP, which matches the field docs and keeps this policy in one place instead of scattered across reconcilers.

deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml (3)

432-433: LGTM: Cascading defaults for controller configuration.

The empty object default enables nested field defaults (image, replicas, etc.) to be applied automatically when users omit the controller configuration entirely.


1372-1373: LGTM: Consistent defaulting pattern for routers.

Same cascading default approach as the controller configuration.


1901-1911: Verify: PR description mentions baseDomain default that isn't implemented.

The PR description states that spec.baseDomain defaults to jumpstarter.example.com, but the CRD schema (lines 425-431) does not define a default value for this field. Combined with the required constraint here, users must explicitly provide baseDomain.

Per the PR comments, this appears intentional to prevent non-functional deployments. If so, consider updating the PR description to clarify that baseDomain is required (not defaulted) to avoid confusion.

@evakhoni evakhoni requested a review from mangelajo December 16, 2025 16:25
@evakhoni
Copy link
Author

I have tested temporary removing the requirement for mandatory baseDomain, and indeed this resulted in a unwanted state where a user is allowed to apply

apiVersion: operator.jumpstarter.dev/v1alpha1
kind: Jumpstarter
metadata:
  name: example
  namespace: default

which results in the following resource

apiVersion: operator.jumpstarter.dev/v1alpha1
kind: Jumpstarter
metadata:
  creationTimestamp: "2025-12-16T19:29:33Z"
  generation: 1
  name: example
  namespace: default
  resourceVersion: "37990"
  uid: 5e9b7bc1-336a-4675-adfe-d0b50776e134
spec:
  controller:
    image: quay.io/jumpstarter-dev/jumpstarter-controller:latest
    imagePullPolicy: IfNotPresent
    replicas: 2
  routers:
    image: quay.io/jumpstarter-dev/jumpstarter-controller:latest
    imagePullPolicy: IfNotPresent
    replicas: 3
  useCertManager: true

and creates the pods with controller GRPC_ENDPOINT="" and router GRPC_ENDPOINT="router-0.:443", no routes, and no errors in neither the operator pod log, nor the controller or router ones, nothing to indicate to the user what is missing, quite an unwanted behavior imo 🤔

Copy link
Member

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this will not work today if the baseDomain is empty, but I expect that to be handled as I describe in the comment, in a follow up PR.

I am ok if you will remove this requirement in the next PR to allow empty base domain. deal? :D

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml (2)

1087-1087: Consider pinning to a specific version tag.

Using :latest makes deployments non-deterministic. While acceptable as a CRD default that users can override, pinning to a specific version (e.g., v0.8.0) would improve reproducibility.


1632-1632: Verify that routers intentionally use the controller image, and consider pinning to a specific version.

Both controller and routers default to the same image (jumpstarter-controller:latest). While the description mentions "Jumpstarter router," using the controller image might be intentional if they share the same binary with different configurations.

Additionally, using :latest creates non-deterministic deployments. Consider pinning to a specific version tag for better reproducibility.

Run the following to check if there's a separate router image or if they share the same image:

#!/bin/bash
# Search for router image references and deployment configurations
rg -n -C3 'router.*image|image.*router' --type=yaml

Based on learnings, routers use the same ConfigMap as controllers, which may indicate shared infrastructure.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a1f73a1 and f16765b.

⛔ Files ignored due to path filters (1)
  • deploy/operator/dist/install.yaml is excluded by !**/dist/**
📒 Files selected for processing (4)
  • deploy/operator/api/v1alpha1/jumpstarter_types.go (3 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (1 hunks)
  • deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml (4 hunks)
  • deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • deploy/operator/config/crd/bases/operator.jumpstarter.dev_jumpstarters.yaml
  • deploy/operator/api/v1alpha1/jumpstarter_types.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml:23-26
Timestamp: 2025-05-13T19:56:27.924Z
Learning: In the jumpstarter-controller project, the router service uses the same ConfigMap as the controller service (controller-cm.yaml) even though it has been moved to its own separate deployment.
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.

Applied to files:

  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml
📚 Learning: 2025-10-24T11:57:13.484Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/rbac.go:193-196
Timestamp: 2025-10-24T11:57:13.484Z
Learning: In the Jumpstarter operator codebase (deploy/operator/internal/controller/jumpstarter/rbac.go), the Role created by `createRole()` defines RBAC permissions for the managed Jumpstarter controller application, not for the operator itself. The managed controller needs delete permissions on secrets for its runtime operations.

Applied to files:

  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml
📚 Learning: 2025-05-13T19:56:27.924Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml:23-26
Timestamp: 2025-05-13T19:56:27.924Z
Learning: In the jumpstarter-controller project, the router service uses the same ConfigMap as the controller service (controller-cm.yaml) even though it has been moved to its own separate deployment.

Applied to files:

  • deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml
📚 Learning: 2025-10-13T09:05:03.088Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 169
File: deploy/operator/config/rbac/leader_election_role_binding.yaml:3-15
Timestamp: 2025-10-13T09:05:03.088Z
Learning: In Kubebuilder-generated Kubernetes operators, RBAC manifests (RoleBindings, Roles, etc.) under config/rbac/ typically do not include explicit `metadata.namespace` fields. The namespace is injected at build time by Kustomize via the `namespace` field in config/default/kustomization.yaml (or whichever kustomization file is used). This is intentional design to keep base manifests namespace-agnostic. Do not flag missing namespaces in such RBAC manifests as issues when they are part of a Kustomize-based operator deployment structure.

Applied to files:

  • deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml
🔇 Additional comments (3)
deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (1)

21-21: LGTM!

The timestamp update reflects bundle regeneration, which is expected when CRD defaults are added.

deploy/operator/bundle/manifests/operator.jumpstarter.dev_jumpstarters.yaml (2)

433-433: LGTM!

The default: {} marker enables minimal CR configuration by ensuring the controller field is populated with default values when not specified.


1373-1373: LGTM!

The default: {} marker ensures routers are configured with default values when not explicitly specified in the CR.

@evakhoni
Copy link
Author

following our agreement lifted the hard requirement in favor of future soft failure at the status level.

@evakhoni evakhoni requested a review from mangelajo December 17, 2025 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants