Skip to content

Conversation

@mangelajo
Copy link
Member

@mangelajo mangelajo commented Nov 20, 2025

TODO:

  • create bundle
  • add CI to build and push the bundle
  • test it

Summary by CodeRabbit

  • New Features

    • Added an operator bundle (bundle image, manifests, and bundle tests) and CRDs for Clients, Exporters, ExporterAccessPolicies, and Leases.
    • Added RBAC roles and a metrics Service for operator observability; OpenShift Route support registered.
  • Chores

    • Increased operator manager memory request (64Mi → 256Mi) and limit (128Mi → 512Mi).
    • Added CI workflows to validate/build the bundle and installer; updated bundle metadata and scorecard configuration.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 20, 2025

Walkthrough

Adds an OLM operator bundle (Dockerfile, annotations, bundle manifests, scorecard tests), new CRDs and ClusterRoles, CI for bundle checks and bundle image build, manager resource and OpenShift route registration, a fixed ConfigMap name, and small comment/CRD description updates and an e2e timestamp formatting tweak.

Changes

Cohort / File(s) Summary
Bundle packaging & CI
deploy/operator/bundle.Dockerfile, deploy/operator/bundle/metadata/annotations.yaml, deploy/operator/bundle/tests/scorecard/config.yaml, .github/workflows/check-bundle.yaml, .github/workflows/build.yaml, deploy/operator/Makefile
Add scratch-based bundle image Dockerfile, bundle annotations and scorecard config (6 parallel tests), CI "Check Bundle" workflow to validate bundle/installer on PRs, extend build matrix to build/push bundle image, and bump default bundle VERSION to 0.8.0.
Bundle manifests & CSV
deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml, deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
Add bundle CSV and base CSV with operator metadata, owned CRDs, install strategy, deployment spec and RBAC/permissions.
RBAC, Service & controller changes
deploy/operator/bundle/manifests/jumpstarter-operator-controller-manager-metrics-service_v1_service.yaml, deploy/operator/bundle/manifests/*-role_rbac.authorization.k8s.io_v1_clusterrole.yaml, deploy/operator/bundle/manifests/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml, deploy/operator/config/rbac/role.yaml, deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go, deploy/operator/config/rbac/metrics_reader_role.yaml
Add metrics Service (port 8443); add ClusterRoles (admin/editor/viewer/metrics-reader) and label metrics-reader; add routes/custom-host RBAC rule; add controller RBAC marker for routes/custom-host; change ConfigMap name to fixed jumpstarter-controller.
CRDs added/updated
deploy/operator/bundle/manifests/jumpstarter.dev_clients.yaml, .../jumpstarter.dev_exporters.yaml, .../jumpstarter.dev_exporteraccesspolicies.yaml, .../jumpstarter.dev_leases.yaml, deploy/helm/jumpstarter/crds/*, deploy/operator/config/crd/bases/*
Add four v1alpha1 CRDs (Client, Exporter, ExporterAccessPolicy, Lease) with OpenAPIv3 schemas and status subresources; update descriptive strings in several helm/config CRD files.
Manager & runtime changes
deploy/operator/config/manager/manager.yaml, deploy/operator/cmd/main.go
Increase manager memory requests/limits (memory request 64Mi→256Mi, limit 128Mi→512Mi) and register OpenShift Route API (routev1.Install(scheme)) in runtime scheme.
Tests & API comments
deploy/operator/test/e2e/e2e_test.go, api/v1alpha1/*_types.go, deploy/helm/jumpstarter/crds/jumpstarter.dev_clients.yaml, deploy/operator/config/crd/bases/*
Use LastTimestamp.Format(...) in e2e event printing; replace scaffold placeholder comments/descriptions in API types and CRD schemas (no API signature changes).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant PR as Pull Request
  participant GH as GitHub Actions
  participant Make as make (deploy/operator)
  participant Gen as bundle generator
  participant Bundle as Bundle image
  participant Score as Scorecard tests

  PR->>GH: PR opened (main or release-*)
  GH->>Make: run `make bundle` (IMG=...:TAG)
  Make->>Gen: generate manifests, metadata, tests
  Gen->>Bundle: assemble bundle image (bundle.Dockerfile)
  GH->>GH: git-diff check (fail on non-timestamp changes)
  GH->>Make: run `make build-installer`
  Make->>GH: produce installer artifacts
  GH->>Score: execute scorecard tests (bundle/tests/scorecard/config.yaml)
  Score-->>GH: report results
  GH-->>PR: report workflow status
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Pay special attention to:
    • CRD schemas (Lease and Exporter validations, additionalPrinterColumns).
    • Bundle CSV RBAC vs. owned CRDs consistency.
    • CI workflow git-diff handling (timestamp-only versus substantive changes).
    • Controller ConfigMap name change (impacts uniqueness/multi-instance setups).
    • OpenShift route registration and added RBAC for routes/custom-host.

Possibly related PRs

Suggested reviewers

  • bennyz
  • kirkbrauer

"🐰 I hopped through manifests, crates, and charts,
Packed CRDs, RBAC, and a bundle of parts.
CI ran tests, the CSV took flight,
A steady controller name through day and night,
Hop, build, release — a dev-rabbit's delight!"

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'operator: create basic bundle' directly describes the main objective of this changeset, which introduces operator bundle manifests, Dockerfile, CI workflow, and related configuration.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch operator-bundle

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 203c786 and e4c4bd4.

⛔ Files ignored due to path filters (1)
  • deploy/operator/dist/install.yaml is excluded by !**/dist/**
📒 Files selected for processing (6)
  • .github/workflows/build.yaml (1 hunks)
  • .github/workflows/check-bundle.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (1 hunks)
  • deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml (1 hunks)
  • deploy/operator/config/rbac/metrics_reader_role.yaml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • deploy/operator/config/rbac/metrics_reader_role.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
  • deploy/operator/bundle/manifests/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml
  • .github/workflows/check-bundle.yaml
🧰 Additional context used
🧠 Learnings (5)
📚 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/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml
  • .github/workflows/build.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/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.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/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.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/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.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/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml
⏰ 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). (7)
  • GitHub Check: tests
  • GitHub Check: deploy-kind
  • GitHub Check: e2e-test-operator
  • GitHub Check: e2e-tests (ubuntu-24.04-arm)
  • GitHub Check: e2e-tests (ubuntu-24.04)
  • GitHub Check: lint-go
  • GitHub Check: e2e-tests-release-0-7
🔇 Additional comments (8)
deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml (1)

1-59: Base CSV template is well-structured.

The template defines appropriate metadata, keywords, and install modes for cluster-wide operator deployment. Placeholder values (namespace, version 0.0.0, empty icon data, minimal CRD/install sections) are expected for a base template that will be expanded during bundle generation.

deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (6)

1-25: Bundle CSV metadata is complete and auto-generated correctly.

The manifest includes proper operator-sdk metadata, release version (0.8.0), and timestamp. The alm-examples annotation provides a useful sample Jumpstarter CR for users.


59-270: ClusterPermissions RBAC is comprehensive and appropriate for the operator scope.

The permissions cover all necessary resource categories: core Kubernetes resources, custom jumpstarter.dev CRDs, OpenShift routes (new), monitoring integration, networking, and authentication checks. The inclusion of routes and routes/custom-host indicates intentional OpenShift support, which aligns with the managed controller infrastructure design mentioned in the learnings.

The permission scope—including create/delete/patch/update for most resources—is appropriate for an operator managing workload infrastructure. Broad permissions on rolebindings and roles are necessary to manage RBAC for the controlled Jumpstarter application instances, consistent with the learnings about managed controller permissions.


314-320: Container resource requests and limits are reasonable.

The operator is allocated 10m–500m CPU and 256Mi–512Mi memory. These limits are appropriate for a control-plane operator managing custom resources, with adequate headroom for leader-election and metrics serving.


332-365: Namespace-scoped permissions (leader election & events) are correctly configured.

The permissions section provides necessary ConfigMap and Lease access for leader election and event recording, standard for Kubernetes operators.


384-386: Operator link has been updated to correct documentation URL.

The link at line 386 now points to the actual Jumpstarter documentation (https://jumpstarter.dev/main/getting-started/installation/service/index.html) instead of a placeholder, resolving the earlier flagged concern.


28-46: CRD ownership declarations verified and are comprehensive.

All five CRDs declared in the ClusterServiceVersion are present in the bundle manifests directory: clients.jumpstarter.dev, exporteraccesspolicies.jumpstarter.dev, exporters.jumpstarter.dev, jumpstarters.operator.jumpstarter.dev, and leases.jumpstarter.dev.

.github/workflows/build.yaml (1)

29-31: All verification checks passed.

The bundle.Dockerfile exists at deploy/operator/bundle.Dockerfile with valid Docker syntax, and the supporting bundle artifacts (manifests, metadata, configuration) are properly in place. The workflow matrix entry is correctly configured and will resolve paths appropriately.


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.

@mangelajo mangelajo marked this pull request as draft November 20, 2025 16:23
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: 4

🧹 Nitpick comments (2)
deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)

10-21: Consider enumerating verbs instead of using wildcard.

The admin ClusterRole grants wildcard verbs (*) on the jumpstarters resource. Even for admin roles, explicitly listing verbs (create, delete, get, list, patch, update, watch) is more maintainable and aligns with security best practices. This also makes audit logs clearer about what permissions are actually needed.

- apiGroups:
  - operator.jumpstarter.dev
  resources:
  - jumpstarters
  verbs:
-  - '*'
+  - create
+  - delete
+  - get
+  - list
+  - patch
+  - update
+  - watch
deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (1)

305-311: Memory limit may be too restrictive for operator workload.

The memory limit of 128Mi (line 308) is quite conservative for a Go-based Kubernetes operator managing multiple resource types. This could lead to OOMKilled pods under normal operational load, especially if managing many Client, Exporter, or Lease resources simultaneously. Consider increasing to 256Mi or 512Mi, or conduct load testing to validate the current limits are sufficient. Memory request of 64Mi may also need adjustment.

Would you like me to help determine appropriate resource limits based on typical Jumpstarter operator usage patterns?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8193ed2 and 9d4687c.

⛔ Files ignored due to path filters (1)
  • deploy/operator/dist/install.yaml is excluded by !**/dist/**
📒 Files selected for processing (15)
  • deploy/operator/bundle.Dockerfile (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator-controller-manager-metrics-service_v1_service.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter.dev_clients.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter.dev_exporteraccesspolicies.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter.dev_exporters.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter.dev_leases.yaml (1 hunks)
  • deploy/operator/bundle/metadata/annotations.yaml (1 hunks)
  • deploy/operator/bundle/tests/scorecard/config.yaml (1 hunks)
  • deploy/operator/config/manager/manager.yaml (1 hunks)
  • deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml (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: 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.
📚 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-jumpstarter-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
  • deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml
📚 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-jumpstarter-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
  • deploy/operator/bundle/manifests/jumpstarter.dev_leases.yaml
  • deploy/operator/bundle/manifests/jumpstarter.dev_clients.yaml
  • deploy/operator/bundle/manifests/jumpstarter.dev_exporters.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
  • deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml
  • deploy/operator/bundle/manifests/jumpstarter.dev_exporteraccesspolicies.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/jumpstarter-operator-jumpstarter-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.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/bundle/manifests/jumpstarter-operator-controller-manager-metrics-service_v1_service.yaml
  • deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.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/bundle/manifests/jumpstarter-operator-controller-manager-metrics-service_v1_service.yaml
  • deploy/operator/config/manifests/bases/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.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/jumpstarter-operator-controller-manager-metrics-service_v1_service.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/bundle/manifests/jumpstarter.dev_clients.yaml
  • deploy/operator/bundle/manifests/jumpstarter.dev_exporters.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml
  • deploy/operator/bundle/manifests/jumpstarter.dev_exporteraccesspolicies.yaml
🪛 Checkov (3.2.334)
deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-admin-role_rbac.authorization.k8s.io_v1_clusterrole.yaml

[medium] 1-21: Minimize wildcard use in Roles and ClusterRoles

(CKV_K8S_49)

⏰ 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). (7)
  • GitHub Check: tests
  • GitHub Check: e2e-tests (ubuntu-24.04)
  • GitHub Check: e2e-tests (ubuntu-24.04-arm)
  • GitHub Check: e2e-tests-release-0-7
  • GitHub Check: deploy-kind
  • GitHub Check: lint-go
  • GitHub Check: e2e-test-operator
🔇 Additional comments (15)
deploy/operator/config/manager/manager.yaml (1)

92-92: Verify memory resource increase justification.

The memory limits and requests have been increased 4x (limit: 128Mi→512Mi, request: 64Mi→256Mi). While this aligns with the addition of metrics exposure, verify that this increase is necessary for the operator's runtime needs and is not overly conservative. Document the rationale if significant.

Also applies to: 95-95

deploy/operator/bundle/manifests/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)

1-10: LGTM!

The metrics-reader ClusterRole correctly defines GET access to the /metrics endpoint. This follows standard OLM conventions for operator metrics exposure.

deploy/operator/bundle/metadata/annotations.yaml (1)

1-14: LGTM!

Bundle metadata annotations correctly define the OLM structure with proper references to manifests, metadata, and scorecard test configuration. This follows operator-sdk and OLM conventions.

deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-viewer-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)

1-23: LGTM!

The viewer ClusterRole correctly defines read-only access (get, list, watch) to Jumpstarter resources and read access to status subresources. This appropriately implements the viewer permission tier.

deploy/operator/bundle/manifests/jumpstarter-operator-jumpstarter-editor-role_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)

1-27: LGTM!

The editor ClusterRole correctly defines write access (create, delete, patch, update) and read access to Jumpstarter resources, while restricting status to read-only. This appropriately implements the editor permission tier.

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

1-59: Verify ClusterServiceVersion is intended as a skeleton.

The CSV has several empty/placeholder fields (lines 8, 30-31, 5-6, 51):

  • namespace: placeholder
  • install.spec.deployments: null, strategy: ""
  • Empty alm-examples and icon base64data
  • Links URL points to jumpstarter-operator.domain

Verify that this is an intentional skeleton meant to be populated by the build/kustomize process, or if these fields require manual completion before bundle publication.

deploy/operator/bundle/manifests/jumpstarter-operator-controller-manager-metrics-service_v1_service.yaml (1)

1-20: LGTM!

The metrics service correctly exposes the controller-manager metrics endpoint on port 8443 with appropriate selector and labeling. This follows standard Kubernetes service patterns for operator metrics.

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

1-77: LGTM!

The Client CRD is well-structured with proper OpenAPI schema, status subresources enabled, and appropriate scope. The credential field uses x-kubernetes-map-type: atomic for semantic correctness. Per learnings, the security model assumes admin-controlled write access to Client resources via RBAC, which is reflected in the bundle's admin/editor roles.

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

1-172: CRD structure looks good.

The ExporterAccessPolicy CRD provides a well-structured policy framework with proper label selectors, policy rules, and validation. No concerns identified.

deploy/operator/bundle.Dockerfile (1)

1-20: Bundle Dockerfile structure is correct.

The Dockerfile follows OLM bundle image conventions with appropriate labels and manifest copies. Using scratch as base is correct for bundle images.

deploy/operator/bundle/tests/scorecard/config.yaml (1)

15-17: Verify storage specification in scorecard tests.

The storage configurations use mountPath: {} (empty object) across all tests and at the top level (lines 69-70). This appears unusual—typically, mountPath would be an empty string ("") or omitted. Please verify this YAML is valid and functions as intended with the operator-framework tooling.

Also applies to: 25-27, 35-37, 45-47, 55-57, 65-67, 69-70

deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (4)

24-25: Verify placeholder namespace is correct.

The ClusterServiceVersion has namespace: placeholder specified at line 25. In OLM bundle installations, ensure this is properly substituted or that it's the intended default. Consider whether this should be empty or removed if not required.


28-46: CRD ownership declaration looks good.

The five owned CRDs are properly listed and match the manifests provided in the bundle. The separation of API groups (operator.jumpstarter.dev for Jumpstarter, jumpstarter.dev for others) is clear.


59-260: RBAC permissions comprehensively cover operator requirements.

The ClusterPermissions section (lines 59-260) appropriately grants all necessary permissions for the operator to manage Jumpstarter-related resources, leader election, deployment orchestration, and monitoring. Verbs are explicitly enumerated rather than using wildcards, which is best practice.


358-366: Install modes properly restrict to cluster-wide deployment.

The CSV correctly supports only AllNamespaces install mode, which is appropriate for an operator managing cluster-wide resources across multiple namespaces. This aligns with the operator's architecture.

@mangelajo mangelajo force-pushed the operator-bundle branch 6 times, most recently from 232cd1b to 757d489 Compare November 21, 2025 16:53
@mangelajo mangelajo marked this pull request as ready for review November 21, 2025 17:07
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 (1)
.github/workflows/check-bundle.yaml (1)

72-92: Timestamp filtering logic is pragmatic but has edge cases.

The workflow attempts to distinguish between expected createdAt timestamp changes and substantive changes using grep filtering (line 78). However, the regex pattern may have edge cases:

  1. The pattern ^[+-].*createdAt:.*[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z could potentially miss timestamps with different formatting.
  2. The logic filters context lines (^---|\+\+\+|@@|index|diff) but the subsequent check for ^[+-] patterns might still catch unintended changes if they happen to match the pattern structure.

Consider documenting the expected timestamp format more explicitly or validating this against actual make bundle output to ensure it works reliably. Otherwise, developers might encounter mysterious CI failures.

To test the robustness of the timestamp filtering, you could run make bundle locally and inspect the git diff output to ensure your regex patterns capture all expected timestamp-only changes.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9d4687c and 3c91135.

⛔ Files ignored due to path filters (1)
  • deploy/operator/dist/install.yaml is excluded by !**/dist/**
📒 Files selected for processing (18)
  • .github/workflows/build.yaml (1 hunks)
  • .github/workflows/check-bundle.yaml (1 hunks)
  • api/v1alpha1/client_types.go (1 hunks)
  • api/v1alpha1/exporter_types.go (1 hunks)
  • api/v1alpha1/exporteraccesspolicy_types.go (1 hunks)
  • deploy/helm/jumpstarter/crds/jumpstarter.dev_clients.yaml (1 hunks)
  • deploy/helm/jumpstarter/crds/jumpstarter.dev_exporters.yaml (1 hunks)
  • deploy/operator/Makefile (1 hunks)
  • deploy/operator/api/v1alpha1/jumpstarter_types.go (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter.dev_clients.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter.dev_exporters.yaml (1 hunks)
  • deploy/operator/cmd/main.go (2 hunks)
  • deploy/operator/config/crd/bases/jumpstarter.dev_clients.yaml (1 hunks)
  • deploy/operator/config/crd/bases/jumpstarter.dev_exporters.yaml (1 hunks)
  • deploy/operator/config/rbac/role.yaml (1 hunks)
  • deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go (2 hunks)
  • deploy/operator/test/e2e/e2e_test.go (1 hunks)
✅ Files skipped from review due to trivial changes (4)
  • api/v1alpha1/exporteraccesspolicy_types.go
  • api/v1alpha1/exporter_types.go
  • api/v1alpha1/client_types.go
  • deploy/operator/config/crd/bases/jumpstarter.dev_exporters.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
  • deploy/operator/bundle/manifests/jumpstarter.dev_clients.yaml
🧰 Additional context used
🧠 Learnings (7)
📚 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:

  • .github/workflows/build.yaml
  • deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go
  • deploy/operator/bundle/manifests/jumpstarter.dev_exporters.yaml
  • deploy/operator/api/v1alpha1/jumpstarter_types.go
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.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/config/rbac/role.yaml
  • deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go
  • deploy/operator/api/v1alpha1/jumpstarter_types.go
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.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/internal/controller/jumpstarter/jumpstarter_controller.go
📚 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/internal/controller/jumpstarter/jumpstarter_controller.go
  • deploy/operator/bundle/manifests/jumpstarter.dev_exporters.yaml
  • deploy/helm/jumpstarter/crds/jumpstarter.dev_exporters.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.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/jumpstarter.dev_exporters.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/bundle/manifests/jumpstarter-operator.clusterserviceversion.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/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml
⏰ 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). (7)
  • GitHub Check: tests
  • GitHub Check: e2e-test-operator
  • GitHub Check: deploy-kind
  • GitHub Check: e2e-tests-release-0-7
  • GitHub Check: e2e-tests (ubuntu-24.04)
  • GitHub Check: e2e-tests (ubuntu-24.04-arm)
  • GitHub Check: lint-go
🔇 Additional comments (19)
deploy/operator/api/v1alpha1/jumpstarter_types.go (1)

513-518: LGTM - Documentation improvement.

The comment update improves clarity over the previous placeholder text. The empty struct is appropriate for a status field that will be expanded in future work.

deploy/operator/test/e2e/e2e_test.go (1)

125-125: LGTM! Cleaner timestamp formatting.

The simplified call to Format() directly on metav1.Time is more idiomatic than accessing .Time explicitly, and both produce identical output.

deploy/operator/Makefile (1)

6-6: Version bump is clean and well-integrated.

The change from 0.0.1 to 0.8.0 is straightforward and properly reflected throughout the Makefile via the VERSION variable. All dependent targets (BUNDLE_IMG, CATALOG_IMG, BUNDLE_GEN_FLAGS) will use this new version consistently.

.github/workflows/build.yaml (1)

29-31: Bundle image matrix entry is properly configured.

The new bundle image entry follows the established pattern with correct context path (deploy/operator) and dockerfile reference. Reuses the shared build step infrastructure cleanly without duplicating logic.

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

49-49: Schema description improved for clarity.

The updated description provides clearer documentation of the credential field's purpose. This is a non-breaking improvement to the OpenAPI schema for better API documentation.

deploy/helm/jumpstarter/crds/jumpstarter.dev_clients.yaml (1)

49-49: Helm CRD in sync with operator CRD.

This change mirrors the description update in the operator's CRD bases, maintaining consistency between Helm and operator-deployed instances.

.github/workflows/check-bundle.yaml (1)

94-97: Consider the order of operations: resetting after bundle validation.

Line 97 (git checkout -- .) resets all changes to ensure a clean state before building the installer. This is correct in sequence but ensure that the bundle generation in the previous step (line 70) doesn't produce any files that need to be committed before this reset. The current logic suggests bundle changes should already be committed, which is enforced by the check in lines 72-92.

deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (5)

1-25: CSV metadata structure is complete.

The ClusterServiceVersion metadata is well-formed with appropriate annotations for OLM (alm-examples, capabilities, builder info). The version 0.8.0 is consistent with the Makefile change.


28-46: CRD ownership declarations are properly specified.

All owned CRDs (Client, ExporterAccessPolicy, Exporter, Jumpstarter, Lease) are declared with correct apiVersion/version and kind. This allows OLM to manage these CRDs as part of the operator installation lifecycle.


59-269: RBAC permissions are comprehensive.

The clusterPermissions cover all required resource types: core Kubernetes resources (ConfigMaps, Secrets, Services), Jumpstarter custom resources, OpenShift Routes, authentication/authorization APIs, monitoring, and networking. The granular verb lists (create, read, update, delete) align with expected operator needs.


271-331: Deployment specification is well-configured.

The deployment includes health probes (liveness/readiness), reasonable resource requests/limits (10m CPU/256Mi memory request, 500m/512Mi limit), security context (non-root, no privilege escalation, drop ALL capabilities), and proper service account binding. The 1 replica and 10-second termination grace period are appropriate for an operator.


367-375: Installation mode correctly restricted to AllNamespaces.

The CSV restricts installation to AllNamespaces mode only, which is appropriate for an operator managing global resources like ClusterRoles and Routes. SingleNamespace and OwnNamespace modes are correctly marked as unsupported.

deploy/helm/jumpstarter/crds/jumpstarter.dev_exporters.yaml (1)

49-49: Helm and operator Exporter CRDs remain synchronized.

The description update to "Exporter status fields" mirrors the change in the operator CRD bases, maintaining consistency across deployment methods.

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

1-50: Bundle Exporter CRD is well-formed and complete.

The CRD properly defines the Exporter resource with comprehensive schema including conditions array, device tracking, credentials, and lease references. Status fields are properly structured with condition metadata (lastTransitionTime, message, reason, observedGeneration, type, status).


51-160: Condition and reference fields follow Kubernetes conventions.

Condition items follow the standard Kubernetes condition pattern with required fields and proper constraints (maxLength, pattern, enum values). LocalObjectReferences for credential and leaseRef use atomic map-type convention and include proper defaults/validation.

deploy/operator/config/rbac/role.yaml (1)

187-195: LGTM! RBAC permissions align with controller annotations.

The new routes/custom-host permissions correctly match the kubebuilder RBAC annotation in deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go at line 87, ensuring consistency between the generated role and controller requirements.

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

87-87: LGTM! RBAC annotation correctly defines routes/custom-host permissions.

The annotation generates the corresponding RBAC rule in deploy/operator/config/rbac/role.yaml (lines 187-195) with matching verbs.


873-873: Fix required: Validation webhook must be implemented and enabled before using fixed ConfigMap name.

The ConfigMap name change from dynamic (fmt.Sprintf("%s-controller", jumpstarter.Name)) to fixed ("jumpstarter-controller") assumes only one Jumpstarter CR per namespace. However, verification shows the validation webhook is currently disabled in deploy/operator/config/default/kustomization.yaml (webhook sections are commented out) and not implemented in the codebase.

If multiple Jumpstarter CRs are created in the same namespace before the webhook is enabled, the second CR will fail permanently during reconciliation at SetControllerReference() because the ConfigMap is already owned by the first CR. This creates a blocking failure with no recovery path short of deleting the first CR.

Required actions:

  1. Implement the Jumpstarter validation webhook to enforce max 1 CR per namespace, OR
  2. Add a controller-level guard to detect and reject multiple CRs with a clear error message, OR
  3. Revert to dynamic ConfigMap naming until the webhook is production-ready

Add an explanatory comment if proceeding:

// Name is fixed to "jumpstarter-controller" assuming only one Jumpstarter CR 
// per namespace, enforced by a validation webhook (currently in progress).
// TODO: Validate webhook is deployed before enabling in production.
Name: "jumpstarter-controller",
⛔ Skipped due to 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: 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.
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.
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 101
File: internal/controller/client_controller_test.go:152-152
Timestamp: 2025-02-17T20:31:08.553Z
Learning: In the jumpstarter-controller repository, the `TokenKey` constant is defined within the `internal/controller` package and is accessible to all files within the same package.
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 101
File: internal/controller/client_controller_test.go:152-152
Timestamp: 2025-02-17T20:31:08.553Z
Learning: In the jumpstarter-controller repository, the `TokenKey` constant is defined in `internal/controller/secret_helpers.go` as `const TokenKey string = "token"` and is used across the controller package for managing secret data.
deploy/operator/cmd/main.go (1)

62-63: No action required—operator properly handles multi-platform scenarios.

The code correctly manages Route API availability through runtime discovery. While the Route API is registered unconditionally in the scheme (line 63, main.go), this is safe—scheme registration only adds type metadata and doesn't validate API existence. The actual Route operations are conditional: the reconciler uses discoverAPIResource() to detect the Route API at initialization, and createRouteForEndpoint() checks r.RouteAvailable before attempting any Route creation (line 82). When unavailable, it gracefully returns nil, allowing the operator to function on both OpenShift and vanilla Kubernetes clusters. No changes needed.

@mangelajo mangelajo force-pushed the operator-bundle branch 3 times, most recently from 51406fc to fe81344 Compare November 21, 2025 17:28
@mangelajo mangelajo requested review from bennyz and bkhizgiy November 21, 2025 17:34
Copy link

@bkhizgiy bkhizgiy left a comment

Choose a reason for hiding this comment

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

looking good overall, just a few nitpicking comments.

- framework
links:
- name: Jumpstarter Operator
url: https://jumpstarter-operator.domain

Choose a reason for hiding this comment

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

I guess it's a placeholder for now, need to remember to change it later

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I thought I had updated this one, good catch!

@@ -0,0 +1,10 @@
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:

Choose a reason for hiding this comment

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

missing labels, I think it will be better to include them for easier cleanup/consistency with other roles.

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

♻️ Duplicate comments (1)
deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (1)

299-299: Acknowledge: Image tag "latest" pending release-time update.

The image still uses the latest tag. As previously noted, this will be updated to a specific version (v0.8.0) at release time—ensure this is included in your release checklist to avoid unpredictable behavior in production deployments.

Also applies to: 299-299

🧹 Nitpick comments (4)
.github/workflows/check-bundle.yaml (3)

32-45: Unreachable code branch: the event_name check is unnecessary.

This workflow only triggers on pull_request events (lines 3-6), so the else branch at lines 42-44 will never execute. The event_name check can be removed for clarity.

-      - name: Get version
+      - name: Determine TAG from base branch
         run: |
-          if [ "${{ github.event_name }}" == "pull_request" ]; then
-            BASE_BRANCH="${{ github.base_ref }}"
-            if [ "$BASE_BRANCH" == "main" ]; then
-              TAG="latest"
-            elif [[ "$BASE_BRANCH" =~ ^release- ]]; then
-              TAG="$BASE_BRANCH"
-            else
-              echo "::error::Unknown base branch: $BASE_BRANCH"
-              exit 1
-            fi
-          else
-            echo "::error::Unsupported event: ${{ github.event_name }}"
-            exit 1
-          fi
+          BASE_BRANCH="${{ github.base_ref }}"
+          if [ "$BASE_BRANCH" == "main" ]; then
+            TAG="latest"
+          elif [[ "$BASE_BRANCH" =~ ^release- ]]; then
+            TAG="$BASE_BRANCH"
+          else
+            echo "::error::Unknown base branch: $BASE_BRANCH"
+            exit 1
+          fi
           echo "TAG=${TAG}" >> $GITHUB_ENV
           echo "TAG=${TAG}"

54-73: Timestamp filtering logic is fragile and assumes specific field names and formats.

The workflow filters out timestamp-only changes by looking for lines containing createdAt: followed by ISO 8601 timestamps. This approach is brittle:

  • If the bundle generation tool changes the field name (e.g., to generatedAt or timestamp), the filter will miss it.
  • If the timestamp format changes from ISO 8601, the filter will fail.
  • If there are other auto-generated fields besides createdAt that should be ignored, they won't be filtered.

Consider documenting why these specific timestamps are expected, or adopt a more robust approach (e.g., regenerating and comparing only semantically significant manifest content, or using a tool-specific option to suppress timestamp generation).


75-78: Silently suppressing git errors could hide real problems.

The || true on line 78 suppresses errors from git checkout, which could hide legitimate issues like file permission problems or corruption. Consider handling errors more explicitly, or add a comment explaining why silent suppression is acceptable here.

       - name: Ensure clean state before build-installer
         run: |
-          # Reset any remaining changes from root
-          git checkout -- . || true
+          # Reset any remaining changes from root (timestamp changes only, already logged)
+          git checkout -- .
deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (1)

54-56: Empty icon metadata should be populated before release.

The icon base64data and mediatype are empty placeholders. While acceptable for development, consider populating these with an actual icon for the final v0.8.0 release to improve the OLM catalog appearance.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3c91135 and 203c786.

📒 Files selected for processing (4)
  • .github/workflows/build.yaml (1 hunks)
  • .github/workflows/check-bundle.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml (1 hunks)
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 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:

  • .github/workflows/build.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml
  • deploy/operator/bundle/manifests/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.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/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.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/bundle/manifests/jumpstarter-operator.clusterserviceversion.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/bundle/manifests/jumpstarter-operator.clusterserviceversion.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/bundle/manifests/jumpstarter-operator.clusterserviceversion.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/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml
⏰ 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: e2e-test-operator
  • GitHub Check: e2e-tests (ubuntu-24.04)
  • GitHub Check: e2e-tests (ubuntu-24.04-arm)
  • GitHub Check: e2e-tests-release-0-7
🔇 Additional comments (6)
.github/workflows/build.yaml (1)

29-31: Bundle image build matrix entry is well-integrated.

The new entry correctly adds the operator bundle to the existing CI/CD pipeline. It follows the established pattern, reuses the versioning and tagging logic for all matrix entries, and will build multi-platform images alongside the controller and operator.

deploy/operator/bundle/manifests/jumpstarter-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml (1)

1-13: ClusterRole manifest is well-formed with appropriate labels.

The manifest correctly defines a metrics reader role for the operator. Labels are properly included (addressing the previous feedback about consistency), and the rule correctly grants GET access to the /metrics endpoint for monitoring integration.

deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (4)

384-393: Documentation link and operator metadata look good.

The link has been updated from the placeholder to a real documentation URL, and operator metadata (maintainer, provider, keywords) are properly configured.


59-269: RBAC permissions are comprehensive and well-structured.

The ClusterPermissions cover all required resources:

  • Standard Kubernetes resources (services, configmaps, secrets, deployments, leases)
  • Custom Jumpstarter API groups (clients, exporters, leases, exporteraccesspolicies)
  • OpenShift routes with appropriate scoping (routes/custom-host subset is correctly restricted)
  • Monitoring and networking resources
  • Authentication/authorization subresources (tokenreviews, subjectaccessreviews)

The permissions align with the operator's responsibilities and the managed Jumpstarter controller's needs (e.g., secret deletion for runtime operations).

Also applies to: 332-365


321-330: Security context and resource configuration is well-hardened.

Security settings are appropriate:

  • Non-root user (runAsNonRoot: true)
  • SecComp profile set to RuntimeDefault
  • No privilege escalation allowed
  • Resource limits (500m/512Mi) and requests (10m/256Mi) are reasonable

374-375: Install mode correctly limited to AllNamespaces.

Only AllNamespaces is supported, which aligns with the operator managing cluster-wide Jumpstarter resources and enforcing single-instance-per-namespace constraints (per learnings on validation webhook enforcement).

@mangelajo mangelajo merged commit ba1e02b into main Nov 25, 2025
10 checks passed
@mangelajo mangelajo deleted the operator-bundle branch November 25, 2025 09:16
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.

3 participants