Move router service to its own deployment #137
Conversation
## Walkthrough
The changes introduce a new standalone router service binary and deployment, decoupling router functionality from the manager. The build system and Dockerfile are updated to build and package both manager and router binaries. Kubernetes manifests are adjusted to deploy and expose the router service separately, and the router's code is refactored for independence from controller-runtime. Helm templates and values are restructured to support multiple routers with individual configurations and dynamic hostname resolution.
## Changes
| Files/Paths | Change Summary |
|---------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| Dockerfile, Makefile | Build system updated to compile and package both `manager` and new `router` binaries; added targets for building and running the router. |
| cmd/main.go | Modified `LoadConfiguration` call to return router config; removed explicit setup of `RouterService` from the manager startup. |
| cmd/router/main.go | New main program for the router service: initializes logger, loads config, starts router service, handles signals, and exits gracefully. |
| internal/service/router_service.go | Refactored `RouterService` to remove Kubernetes client and scheme dependencies; updated JWT parsing callback signature. |
| internal/config/config.go | Added `LoadRouterConfiguration` function; updated `LoadConfiguration` to return router config alongside existing values. |
| internal/config/types.go | Added new types `Router` (map) and `RouterEntry` struct to represent router configurations. |
| internal/service/controller_service.go | Added `Router` field to `ControllerService`; implemented logic to select a router endpoint from the router map in `Dial` method based on label matching. |
| internal/service/helpers.go | Added `MatchLabels` function to compare label maps and score matches. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml | New Helm template defining standalone Kubernetes Deployments for each router entry with security, resource, and environment settings. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml | Modified Service manifest to create multiple Services for each router entry with corresponding labels and nodePort settings. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-ingress.yaml | Modified ingress template to generate multiple ingress resources, one per router configuration, with dynamic hostnames and TLS settings. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml | Modified route template to generate multiple OpenShift Route resources per router configuration with unique names and hostnames. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml | Moved configmap SHA256 checksum annotation from Deployment metadata to Pod template metadata; removed `GRPC_ROUTER_ENDPOINT` environment variable from container spec. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_endpoints.tpl | Deleted static Helm template definitions for router and controller endpoints. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml | Added `router` field to ConfigMap data populated from Helm values. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json | Added JSON schema defining `grpc` config with nested `routers` map entries including hostname, endpoint, and optional nodePort. |
| deploy/helm/jumpstarter/values.yaml | Reorganized gRPC router configuration into a nested map structure with multiple named routers, replacing flat hostname and endpoint keys. |
| hack/deploy_with_helm.sh | Updated Helm set parameters for router hostname and endpoint to use nested keys under `grpc.routers`; changed ingress controller installation to Helm chart with updated ports and SSL passthrough settings. |
| hack/kind_cluster.yaml | Updated kind cluster ingress controller port mappings to support HTTP on 5080 and HTTPS on 5443. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml | Refactored service type and nodePort configuration to use unified `.Values.grpc.service` structure instead of `.Values.grpc.nodeport`. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml | Modified Role namespace to default to `.Release.Namespace` if `.Values.namespace` is unset. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml | Modified RoleBinding namespace fields to default to `.Release.Namespace` if `.Values.namespace` is unset. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-ingress.yaml | Changed ingress rendering condition to nested `.Values.grpc.ingress.enabled`; refactored hostname to use Helm helper. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-route.yaml | Changed route rendering condition to nested `.Values.grpc.route.enabled`; refactored hostname to use Helm helper. |
| deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml | Added Helm template helpers for generating gRPC and router hostnames and endpoints with validation and defaulting logic. |
| .github/workflows/e2e.yaml | Updated e2e test workflow to use `router` branch of jumpstarter-e2e action instead of `main`. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Router Service (binary)
participant Kubernetes
participant Manager (binary)
User->>Kubernetes: Deploy router via Helm chart
Kubernetes->>Router Service (Pod): Start container
Router Service (binary)->>Router Service (binary): Initialize logger and config
Router Service (binary)->>Router Service (binary): Start gRPC server
User->>Router Service (Service): Connect to gRPC endpoint
Router Service (binary)->>User: Handle request/response
Note over Manager (binary): No longer initializes RouterServicePossibly related PRs
Suggested reviewers
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (10)
5-5: Avoid hardcoding namespace in chart templates
Using{{ default .Release.Namespace .Values.namespace }}can be confusing—Helm charts typically rely on--namespaceand.Release.Namespace. Consider dropping.Values.namespaceand usingnamespace: {{ .Release.Namespace }}exclusively, or document the override clearly.
9-11: Improve Helm templating for conditional timestamp
YAML linters may error on the raw{{ if }}block. To ensure proper indentation and remove extra blank lines, use the whitespace-trimming syntax and indent inside the block, e.g.:{{- if .Values.global.timestamp }} deployment.timestamp: {{ .Values.global.timestamp | quote }} {{- end }}🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 10-10: syntax error: could not find expected ':'
(syntax)
18-18: Make replicas count configurable
The replica count is hardcoded to1. For high availability or load-driven scaling, expose this viavalues.yaml(e.g.,.Values.router.replicaCount) so users can adjust without editing templates.
29-44: Move nodeAffinity tovalues.yamlinstead of commented code
The commented-out affinity section is useful guidance, but it’s better to template it dynamically. For example:affinity: {{ toYaml .Values.affinity | indent 8 }}and let users configure
affinityinvalues.yaml. This keeps templates clean and maintainable.
49-52: Consider addingfsGroupfor volume permissions
You already enforcerunAsNonRootandseccompProfile. If the router writes to any mounted volumes in the future, addingfsGroupundersecurityContextcan ensure correct file permissions.
58-64: ConsolidateGRPC_ROUTER_ENDPOINTlogic
The multi-branchif/elseforGRPC_ROUTER_ENDPOINTis verbose. You can simplify with a single expression, for instance usingdefaultandprintf:- name: GRPC_ROUTER_ENDPOINT value: {{ default (printf "%s:%d" .Values.routerHostname .Values.grpc.tls.port) .Values.grpc.routerEndpoint | quote }}
74-79: Harden container with read-only root filesystem
You drop all capabilities and disable privilege escalation. For added security, add:readOnlyRootFilesystem: trueto your
securityContext.
79-90: Enable and parameterize health probes
Liveness/readiness probes are commented out. It’s recommended to enable them and make probe settings (path,port,initialDelaySeconds,periodSeconds) configurable viavalues.yaml.
91-97: Parameterize resource requests/limits
Resource values are hardcoded. To cater to different environments, delegateresourcestovalues.yaml(e.g.,.Values.router.resources), then inject withtoYaml:resources: {{ toYaml .Values.router.resources | indent 10 }}
98-98: ExplicitserviceAccountNameconfiguration
TheserviceAccountNameis commented out. For proper RBAC separation, expose this via a Helm value (e.g.,.Values.serviceAccountName) and enable it by default or document its use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Dockerfile(2 hunks)Makefile(1 hunks)cmd/main.go(0 hunks)cmd/router/main.go(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml(2 hunks)internal/service/router_service.go(1 hunks)
💤 Files with no reviewable changes (1)
- cmd/main.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/router/main.go (3)
internal/config/grpc.go (1)
LoadGrpcConfiguration(10-20)internal/config/types.go (2)
Grpc(21-23)Keepalive(25-28)internal/service/router_service.go (1)
RouterService(39-43)
🪛 GitHub Check: lint-go
cmd/router/main.go
[failure] 67-67:
S1000: should use a simple channel send/receive instead of select with a single case (staticcheck)
🪛 GitHub Actions: Linters
cmd/router/main.go
[error] 67-67: staticcheck: should use a simple channel send/receive instead of select with a single case (S1000)
🪛 YAMLlint (1.35.1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml
[error] 10-10: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: deploy-kind
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
🔇 Additional comments (10)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml (1)
5-5: Label selector change looks goodThe updated labels properly target the new router deployment instead of the controller-manager. This is a necessary change for the service to work with the decoupled router deployment.
Also applies to: 24-24
Dockerfile (3)
14-14: LGTM: Copying entire cmd/ directoryCopying the entire
cmd/directory is necessary to build both binaries.
24-24: Build process for router binary looks goodAdding a build command for the router binary with the same build flags as the manager binary ensures consistency.
29-29: LGTM: Copying router binary to final imageThe router binary is properly copied to the final image, making it available for execution.
internal/service/router_service.go (1)
59-59: Modernizing Go code with 'any' typeGood update to use the more modern
anytype alias instead ofinterface{}, which was introduced in Go 1.18.Makefile (2)
90-90: LGTM: Build target for router binaryAdding the build command for the router binary ensures it's built alongside the manager binary.
96-98: LGTM: Run target for router serviceThe new
run-routertarget properly mirrors the existingruntarget for the manager, making local development and testing of the router service easier.cmd/router/main.go (3)
19-32: Imports look goodThe imports are appropriate for the router service's functionality, including context handling, signal handling, and logging.
43-52: LGTM: gRPC configurationThe gRPC configuration is set up with reasonable keepalive settings and proper error handling.
54-63: Router service initialization looks goodThe router service is properly initialized with the configured server option and started with a logger-enriched context.
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (1)
7-8: Inconsistentapp.kubernetes.io/namelabelThe router deployment is labeled
app.kubernetes.io/name: jumpstarter-controller, which conflicts with its purpose. It should bejumpstarter-router(or derived via{{ include "jumpstarter-router.name" . }}) to accurately reflect this component.
🧹 Nitpick comments (3)
internal/config/config.go (1)
18-45: Add documentation forLoadRouterConfigurationThe new function is missing documentation comments that explain its purpose, parameters, and return values. Consider adding a comment similar to the existing
LoadGrpcConfigurationfunction to improve code readability and maintainability.+// LoadRouterConfiguration loads gRPC server options from a Kubernetes ConfigMap. +// It retrieves the ConfigMap using the provided client and key, extracts the "config" data section, +// unmarshals it into a Config struct, and then calls LoadGrpcConfiguration on the Grpc field. +// It returns the resulting grpc.ServerOption or any error encountered. func LoadRouterConfiguration( ctx context.Context, client client.Reader, key client.ObjectKey, ) (grpc.ServerOption, error) {Also, consider refactoring the shared logic between this function and
LoadConfigurationto reduce code duplication, particularly the ConfigMap retrieval and unmarshaling steps.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (2)
103-103: Consider using a dedicated service account for the routerThe router deployment is using the
controller-managerservice account, which may grant more permissions than necessary. Consider creating a dedicated service account with the minimum required permissions for the router service to follow the principle of least privilege.
58-65: Document complex environment variable setupThe
GRPC_ROUTER_ENDPOINTenvironment variable uses a complex conditional with multiple fallbacks. Consider adding comments to explain the purpose and priority of each fallback option to improve maintainability.- name: GRPC_ROUTER_ENDPOINT + # Priority order: + # 1. Use explicit router endpoint if provided + # 2. Use router hostname if provided + # 3. Fallback to constructed domain using baseDomain {{ if .Values.grpc.routerEndpoint }} value: {{ .Values.grpc.routerEndpoint }} {{ else if .Values.routerHostname }} value: {{ .Values.routerHostname }}:{{ .Values.grpc.tls.port }} {{ else }} value: router.{{ .Values.global.baseDomain }}:{{ .Values.grpc.tls.port }} {{ end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/router/main.go(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml(1 hunks)internal/config/config.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/router/main.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/config/config.go (2)
internal/config/types.go (2)
Config(7-10)Grpc(21-23)internal/config/grpc.go (1)
LoadGrpcConfiguration(10-20)
🪛 YAMLlint (1.35.1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml
[error] 10-10: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (2)
9-10: Inconsistentapp.kubernetes.io/namelabel
The router Deployment is still labeled asjumpstarter-controller, which misrepresents this component. It should reference the router chart, e.g.jumpstarter-routeror use the Helm name helper.- app.kubernetes.io/name: jumpstarter-controller + app.kubernetes.io/name: {{ include "jumpstarter-router.name" . }}
86-97: Implement appropriate gRPC health checks
The HTTP health probes are commented out and not suitable for a gRPC service. You’ll need to either integrate the gRPC Health Checking Protocol in your binary or use agrpc_health_probeexec command.
🧹 Nitpick comments (7)
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml (1)
10-14: Add nested gRPC router configuration
The newgrpc.router.defaultblock correctly scopes per-router settings (endpoint,hostname,nodeport) to support multiple router instances. Consider adding a documentation comment for thenodeportfield alongside the existingendpointandhostnameparameters to improve discoverability.hack/deploy_with_helm.sh (2)
43-45: DRY up router hostname assignment in ingress branch
The lineGRPC_ROUTER_HOSTNAME="router.${BASEDOMAIN}"is duplicated in both branches. You can extract the hostname calculation out of the conditional to reduce duplication and simplify future updates.
50-52: DRY up router hostname assignment in nodeport branch
Similarly, the nodeport branch repeatsGRPC_ROUTER_HOSTNAME="router.${BASEDOMAIN}". Consolidating this logic will make the script more maintainable.deploy/helm/jumpstarter/values.yaml (1)
106-111: Define default gRPC router settings
Addinggrpc.router.defaultwithendpoint,hostname, andnodeportmirrors the controller chart changes. For completeness, consider adding@param jumpstarter-controller.grpc.router.<name>.nodeportto the documentation section above.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (3)
98-104: Template resource requests and limits
Hardcoding CPU/memory makes tuning difficult. Consider driving these fromvalues.yamlso operators can override per-environment.resources: limits: cpu: {{ .Values.router.resources.limits.cpu }} memory: {{ .Values.router.resources.limits.memory }} requests: cpu: {{ .Values.router.resources.requests.cpu }} memory: {{ .Values.router.resources.requests.memory }}
105-107: Use a dedicated service account for the router
Reusingcontroller-managerconflates privileges. Define and reference ajumpstarter-routerServiceAccount to enforce least privilege.- serviceAccountName: controller-manager + serviceAccountName: jumpstarter-router
32-48: Move commented nodeAffinity to values-driven block
A large commented-out affinity block clutters the template. If you intend to support multi-arch scheduling, drive this viavalues.router.affinity, using a Helm helper rather than inline comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_endpoints.tpl(0 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-ingress.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml(1 hunks)deploy/helm/jumpstarter/values.yaml(3 hunks)hack/deploy_with_helm.sh(1 hunks)
💤 Files with no reviewable changes (1)
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_endpoints.tpl
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-ingress.yaml
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml
[error] 12-12: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: deploy-kind
🔇 Additional comments (20)
hack/deploy_with_helm.sh (1)
56-57: Correct Helm flags for nested router values
Using--set jumpstarter-controller.grpc.router.default.hostname=${GRPC_ROUTER_HOSTNAME}and--set jumpstarter-controller.grpc.router.default.endpoint=${GRPC_ROUTER_ENDPOINT}aligns with the new nested schema in your Helm chart.deploy/helm/jumpstarter/values.yaml (2)
46-46: Update doc for router hostname parameter
The@param jumpstarter-controller.grpc.router.<name>.hostnameentry matches the new nested map and will guide users to configure custom router hostnames.
56-56: Update doc for router endpoint parameter
The@param jumpstarter-controller.grpc.router.<name>.endpointdoc is now consistent with the chart’s dynamic router configuration.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml (7)
2-3: Enable multi-router iteration for OpenShift Routes
Therange $k, $v := .Values.grpc.routerloop combined with document separators (---) will generate a distinctRouteper router key, supporting the new multi-router deployment.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
13-14: Dynamic naming and namespace resolution
Appending-{{ $k }}to theRoutename and defaulting the namespace via{{ default $.Release.Namespace $.Values.namespace }}ensures each router resource is uniquely identified.
16-21: Conditional host selection
The block checkingif $v.hostnamewith a fallback torouter-{{ $k }}.{{ $.Values.global.baseDomain }}and a required validation message guarantees valid host values for all routes.
26-31: TLS termination logic
Separatingpassthroughandreencryptbranches under the TLS spec is correct and preserves ingress flexibility for different security requirements.
33-36: Optional external certificate configuration
Guarding theexternalCertificatestanza withif $.Values.grpc.tls.routerCertSecretcleanly handles cases where no router-specific cert is provided.
40-40: Service target alignment
Routing tojumpstarter-router-grpc-{{ $k }}for each router key directs traffic to the proper backend service.
44-44: Closing Helm control blocks
The final{{ end }}statements correctly terminate therangeandifconstructs.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-ingress.yaml (10)
2-3: Implement multi-router loop for Ingress resources
Iterating over.Values.grpc.routerwith---separators will produce oneIngressper router key, matching the multi-instance design.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
12-14: Conditional TLS passthrough annotation
Thenginx.ingress.kubernetes.io/ssl-passthrough: "true"annotation under thepassthroughcheck is properly scoped within the router loop.
15-16: Ingress naming and namespace scoping
Naming each Ingressjumpstarter-router-ingress-{{ $k }}and defaulting the namespace prevents naming collisions.
18-19: Optional ingress class configuration
Conditionally settingingressClassNamefrom.Values.grpc.ingress.classallows flexible integration with cluster-specific Ingress controllers.
22-27: Dynamic host routing
Usingif $v.hostnamewith a fallback torouter-{{ $k }}.{{ $.Values.global.baseDomain }}and a required validation ensures every Ingress rule has a valid host.
29-29: HTTP backend path mapping
Defining the HTTP path context (/) under each Ingress properly routes gRPC traffic to the service backend.
35-35: Service backend mapping
Pointing the Ingress backend to servicejumpstarter-router-grpc-{{ $k }}on port8083aligns with the router Service configuration.
40-46: TLS hosts configuration
Iterating overtls.hostswith the same hostname logic ensures each router’s TLS section is correctly populated.
47-48: Conditional TLS secret usage
GuardingsecretName: {{ $.Values.grpc.tls.routerCertSecret }}under anifcheck prevents errors when no certificate is specified.
51-52: Finalize Helm blocks
The closing{{ end }}directives properly terminate therangeand outerifstatements.
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml
Show resolved
Hide resolved
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hack/deploy_with_helm.sh(1 hunks)hack/kind_cluster.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- hack/kind_cluster.yaml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
🔇 Additional comments (2)
hack/deploy_with_helm.sh (2)
38-40: Approve: Introduce router gRPC variables for ingress mode
These new variables correctly capture the router’s hostname and endpoint for the ingress-enabled path. They’ll be consumed by the Helm sets later in the script.
46-48: Approve: Introduce router gRPC variables for NodePort mode
The router hostname and endpoint for the NodePort deployment are correctly configured (port 8083 to avoid collision with the main gRPC service on 8082).
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
hack/deploy_with_helm.sh (1)
114-125:⚠️ Potential issueUpdate loop to check all router endpoints.
The script checks
GRPC_ENDPOINTandGRPC_ROUTER_ENDPOINT, but not the newGRPC_ROUTER_ANOTHER_ENDPOINT.Apply this diff to include all router endpoints in the verification loop:
-for ep in ${GRPC_ENDPOINT} ${GRPC_ROUTER_ENDPOINT}; do +for ep in ${GRPC_ENDPOINT} ${GRPC_ROUTER_ENDPOINT} ${GRPC_ROUTER_ANOTHER_ENDPOINT}; do
♻️ Duplicate comments (2)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (2)
86-97: Health probe implementation reminder.The health probes remain commented out. Remember to implement these health checks as part of the router binary as you mentioned.
10-10: 🛠️ Refactor suggestionInconsistent
app.kubernetes.io/namelabel.The router deployment is labeled
app.kubernetes.io/name: jumpstarter-controller, but since this is now a separate router deployment, consider using a distinct label likejumpstarter-routerto correctly identify this component.
🧹 Nitpick comments (1)
internal/service/controller_service.go (1)
416-428: Simple but effective router selection logic.The implementation uses Go's random map iteration to effectively select a random router, which provides basic load distribution. The error handling for the case when no router is available is comprehensive.
Consider enhancing this in a future update to support more sophisticated selection strategies (round-robin, least connections, etc.) if router load balancing becomes important.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/main.go(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml(0 hunks)hack/deploy_with_helm.sh(1 hunks)internal/config/config.go(3 hunks)internal/config/types.go(1 hunks)internal/service/controller_service.go(3 hunks)
💤 Files with no reviewable changes (1)
- deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml
✅ Files skipped from review due to trivial changes (2)
- deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json
- internal/config/types.go
🚧 Files skipped from review as they are similar to previous changes (3)
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml
- cmd/main.go
- internal/config/config.go
🧰 Additional context used
🧠 Learnings (2)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter-controller#137
File: deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml:23-26
Timestamp: 2025-05-13T19:56:27.876Z
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.
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter-controller#137
File: deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml:23-26
Timestamp: 2025-05-13T19:56:27.876Z
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.
🧬 Code Graph Analysis (1)
internal/service/controller_service.go (1)
internal/config/types.go (1)
Router(30-30)
🪛 YAMLlint (1.35.1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml
[error] 12-12: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: deploy-kind
🔇 Additional comments (8)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml (1)
18-19: Added router configuration section looks good.The addition of the router configuration to the shared ConfigMap provides a clean way to manage both controller and router configurations in one place.
internal/service/controller_service.go (2)
36-36: Import for config package added appropriately.This import is necessary to use the Router type for the newly added field.
74-74: Good addition of Router field to ControllerService struct.Adding this field allows the service to be aware of all available routers.
hack/deploy_with_helm.sh (3)
26-34: Better approach using Helm for ingress-nginx installation.Using Helm for the ingress-nginx installation is cleaner and more maintainable than applying manifests directly.
39-47: Multi-router configuration looks good.The updated endpoint configurations properly support multiple named router endpoints, which aligns with the PR objective to move the router service to its own deployment.
58-59: Router configuration structure aligns with new architecture.The Helm parameters have been updated to reflect the new multi-router architecture, using a map structure with named keys.
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (2)
60-67: Well-structured endpoint configuration with proper quoting.The environment variable configuration handles various fallback options appropriately and properly quotes values containing colons to avoid YAML parsing errors.
52-56: Good security practices implemented.The deployment includes proper security context settings at both pod and container levels, including:
- Running as non-root
- Using a default seccomp profile
- Disabling privilege escalation
- Dropping all Linux capabilities
These are excellent security practices for containerized applications.
Also applies to: 81-85
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (2)
10-10: Update theapp.kubernetes.io/namelabel for router
The label is stilljumpstarter-controller, which conflicts with the router component. It should accurately reflect the router, e.g.:app.kubernetes.io/name: jumpstarter-router
61-61:⚠️ Potential issueQuote environment variable values containing colons
TheGRPC_ROUTER_ENDPOINTvalue can includehost:port. Wrap it in quotes (or use| quote) to render valid YAML:- - name: GRPC_ROUTER_ENDPOINT - value: {{ include "router.endpoint" (list $ $k $v) }} + - name: GRPC_ROUTER_ENDPOINT + value: {{ include "router.endpoint" (list $ $k $v) | quote }}
🧹 Nitpick comments (6)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml (1)
18-19: Ensure consistent root context for Values
For clarity and to avoid scoping issues in deeper templates, use the root context ($) for all.Valuesreferences.- {{ if .Values.grpc.service.nodePort }} - nodePort: {{ .Values.grpc.service.nodePort }} + {{- if $.Values.grpc.service.nodePort }} + nodePort: {{ $.Values.grpc.service.nodePort }} + {{- end }}🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 19-19: syntax error: could not find expected ':'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml (1)
1-3: Flatten nestedifstatements for readability
Combine the two conditionals (.Values.grpc.routersentry exists and.route.enabled) into one to reduce nesting:-{{ range $k, $v := .Values.grpc.routers }} -{{ if $v.route }} -{{ if $v.route.enabled }} +{{- range $k, $v := .Values.grpc.routers }} +{{- if and $v.route $v.route.enabled }} --- apiVersion: route.openshift.io/v1 ... +{{- end }} +{{- end }}This simplifies the template and trims extra indentation.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-ingress.yaml (1)
1-3: Use a combined conditional foringress.enabled
Merge the twoifchecks into one to reduce nesting and improve clarity:-{{ range $k, $v := .Values.grpc.routers }} -{{ if $v.ingress }} -{{ if $v.ingress.enabled }} +{{- range $k, $v := .Values.grpc.routers }} +{{- if and $v.ingress $v.ingress.enabled }} --- apiVersion: networking.k8s.io/v1 ... +{{- end }} +{{- end }}This approach consolidates the logic and removes an extra layer of indentation.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json (2)
2-3: Upgrade to JSON Schema draft-07 and HTTPS
Helm 3 supports draft-07. Update the$schemaURI to"https://json-schema.org/draft-07/schema#"to leverage improved validation and avoid potential deprecation issues.
88-90: Consider requiringgrpcat the top level
Currently only"image"is required. Sincegrpc.endpointis vital, add"grpc"to the top-level"required"array so Helm enforces its presence.hack/deploy_with_helm.sh (1)
26-34: Pin the ingress-nginx chart version for reproducible deployments
You’re installingingress-nginxvia Helm without specifying--version, which can lead to drift when the chart is updated. Pin to a known stable release to keep your deployments consistent:- helm upgrade --wait --install ingress-nginx ingress-nginx \ + helm upgrade --wait --install ingress-nginx ingress-nginx \ --repo https://kubernetes.github.io/ingress-nginx \ --namespace ingress-nginx --create-namespace \ + --version 4.7.1 \ --set controller.service.type=NodePort \ --set controller.service.nodePorts.http=5080 \ --set controller.service.nodePorts.https=5443 \ --set controller.ingressClassResource.default=true \ --set controller.config.worker-processes=2 \ --set controller.extraArgs.enable-ssl-passthrough=true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-ingress.yaml(3 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-route.yaml(3 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-ingress.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml(1 hunks)deploy/helm/jumpstarter/values.yaml(2 hunks)hack/deploy_with_helm.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/helm/jumpstarter/values.yaml
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml
🧰 Additional context used
🧠 Learnings (2)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter-controller#137
File: hack/deploy_with_helm.sh:26-34
Timestamp: 2025-05-13T19:57:56.762Z
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).
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter-controller#137
File: deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml:23-26
Timestamp: 2025-05-13T19:56:27.876Z
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.
🪛 YAMLlint (1.35.1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml
[error] 19-19: syntax error: could not find expected ':'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-ingress.yaml
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-route.yaml
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 3-3: wrong indentation: expected 0 but found 4
(indentation)
[warning] 4-4: wrong indentation: expected 0 but found 2
(indentation)
[warning] 5-5: wrong indentation: expected 0 but found 4
(indentation)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-ingress.yaml
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml
[error] 12-12: syntax error: could not find expected ':'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml
[error] 21-21: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: deploy-kind
🔇 Additional comments (17)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml (1)
8-8: Ensure namespace fallback with Helmdefault.
Switching tonamespace: {{ default .Release.Namespace .Values.namespace }}guarantees that if.Values.namespaceis not set or is empty, the Role will be created in the release’s namespace. This change aligns with the other RBAC templates and improves configuration robustness.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml (1)
22-22: Align configmap checksum annotation to Pod template
Moving theconfigmap-sha256annotation undertemplate.metadata.annotationsensures pods roll out when the configmap changes, which is a best practice for triggering updates.deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml (2)
1-1: Set controller image to the official repository
Pinning the controller image atquay.io/jumpstarter-dev/jumpstarter-controllercentralizes the image reference and avoids ambiguity.
4-5: Introduce unified gRPC service configuration
Replacing flat router-specific keys with a nestedgrpc.service.typemakes the service definition more consistent and flexible across both controller and router charts.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml (1)
10-10: Use dynamic service type for gRPC service
Switchingtypeto{{ $.Values.grpc.service.type }}aligns the controller Service with the new unified.Values.grpc.servicestructure and supports multiple service types.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-route.yaml (3)
1-2: Guard route creation on existence and enabled flags
Adding an outer{{ if .Values.grpc.route }}before checking.Values.grpc.route.enabledprevents rendering whengrpc.routeis undefined, improving chart robustness.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
15-15: Use hostname helper for route host
Leveraging{{ include "grpc.hostname" . }}centralizes hostname logic and avoids duplicated inline templates across resources.
32-32: Close nested route conditionals correctly
Ensuring the outerifis closed prevents the route block from rendering whengrpc.routeis absent, avoiding unintended resource creation.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-ingress.yaml (4)
1-2: Refactor ingress guard to nested checks
Requiring both{{ if .Values.grpc.ingress }}and{{ if .Values.grpc.ingress.enabled }}makes ingress rendering conditions explicit and avoids accidental inclusion.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
21-21: Use helper for ingress host
Replacing inline host logic with{{ include "grpc.hostname" . }}maintains consistency and centralizes domain name computation.
33-33: Use helper for TLS host entries
Aligning TLShostswith the hostname helper simplifies updates and ensures both HTTP and TLS ingress rules use the same value.
38-39: Properly close ingress conditionals
Closing bothifblocks prevents the ingress resource from leaking into the rendered manifest when ingress is disabled.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml (1)
1-3: Ignore false-positive YAMLLint errors in Helm template directives
The linter’s complaintexpected '<document start>', but found '{'occurs on lines with{{but these are valid Helm controls, not literal YAML. It’s safe to disregard these warnings.Also applies to: 39-41
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-ingress.yaml (1)
1-3: Ignore false-positive YAMLLint errors in Helm template directives
Lint errors aboutexpected '<document start>', but found '{'on the Helm directives ({{ … }}) are false positives—these lines are template logic, not actual YAML.Also applies to: 40-42
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml (1)
1-7: Ignore indentation warnings within template helpers
The YAMLLint errors about indentation insidedefineblocks are false positives—Helm’s whitespace control handles these correctly, and they do not appear in the rendered manifests.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 3-3: wrong indentation: expected 0 but found 4
(indentation)
[warning] 4-4: wrong indentation: expected 0 but found 2
(indentation)
[warning] 5-5: wrong indentation: expected 0 but found 4
(indentation)
hack/deploy_with_helm.sh (1)
56-56: Approve addition of router hostname for nodeport mode
DefiningGRPC_ROUTER_HOSTNAMEin the non-ingress branch ensures the subsequent Helm flags can reference it consistently. This change aligns correctly with the new router configuration structure.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (1)
11-13: Ignore YAMLlint false positive on Helm templating
The{{ if }}/{{ end }}block undermetadata.labelstriggers a YAML parse error in standard linters but is valid Helm template logic. You can safely ignore this warning.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: syntax error: could not find expected ':'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml
Outdated
Show resolved
Hide resolved
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml
Outdated
Show resolved
Hide resolved
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json
Outdated
Show resolved
Hide resolved
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json
Outdated
Show resolved
Hide resolved
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/service/helpers.go (2)
3-13: Add documentation to clarify the function's purpose and return valuesThe function logic is clear and correct for label matching, but it lacks documentation that explains its purpose, parameters, and return values. Add a comment block to clarify:
- What the function is used for (router selection based on label matching)
- What the parameters represent (candidate router labels vs target/required labels)
- What the return values mean (-1 for non-match, count of matches otherwise)
package service +// MatchLabels checks if the candidate map contains all key-value pairs from the target map. +// It returns: +// - The count of matching labels (equal to len(target)) if all target labels exist in candidate with matching values +// - -1 if any label in target is missing from candidate or has a different value +// This is used for selecting routers based on label matching criteria. func MatchLabels(candidate map[string]string, target map[string]string) int { count := 0 for k, vt := range target { if vc, ok := candidate[k]; ok && vc == vt { count += 1 } else { return -1 } } return count }
3-13: Consider handling nil or empty map edge cases explicitlyThe current implementation works correctly for most cases, but doesn't explicitly handle nil or empty maps. While the loop would not execute for empty maps (returning 0), adding explicit handling would improve readability and avoid potential confusion.
func MatchLabels(candidate map[string]string, target map[string]string) int { + // If target is empty, all labels match (by definition) + if len(target) == 0 { + return 0 + } + + // If candidate is nil/empty but target has requirements, no match is possible + if candidate == nil && len(target) > 0 { + return -1 + } + count := 0 for k, vt := range target { if vc, ok := candidate[k]; ok && vc == vt { count += 1 } else { return -1 } } return count }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json(1 hunks)hack/deploy_with_helm.sh(1 hunks)internal/config/types.go(1 hunks)internal/service/controller_service.go(3 hunks)internal/service/helpers.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/config/types.go
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml
- internal/service/controller_service.go
- hack/deploy_with_helm.sh
- deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: deploy-kind
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml (1)
20-24: Fix indentation fornodePortconditional to prevent parse errors
Align thenodePortlines with proper whitespace trimming, e.g.:- {{ if $v.service }} - {{ if $v.service.nodePort }} - nodePort: {{ $v.service.nodePort }} - {{ end }} - {{ end }} + {{- if $v.service }} + {{- if $v.service.nodePort }} + nodePort: {{ $v.service.nodePort }} + {{- end }} + {{- end }}This corrects YAMLlint
could not find expected ':'errors by removing extra indentation.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 21-21: syntax error: could not find expected ':'
(syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml(0 hunks)hack/deploy_with_helm.sh(3 hunks)
💤 Files with no reviewable changes (1)
- deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/deploy_with_helm.sh
- deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json
🧰 Additional context used
🪛 YAMLlint (1.35.1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml
[error] 21-21: syntax error: could not find expected ':'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 3-3: wrong indentation: expected 0 but found 4
(indentation)
[warning] 4-4: wrong indentation: expected 0 but found 2
(indentation)
[warning] 5-5: wrong indentation: expected 0 but found 4
(indentation)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml
[error] 19-19: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: deploy-kind
🔇 Additional comments (12)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml (1)
10-10: Leverage Helm helper for dynamic service type
Dynamictype: {{ include "grpc.service.type" . }}correctly centralizes service‐type logic and makes the chart more flexible.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml (6)
1-1: Enable multi-router iteration
Introducingrangeover.Values.grpc.routerscorrectly parameterizes Service resources per router instance.
7-7: Dynamic service label suffix
Usingcontrol-plane: controller-router-{{ $k }}ensures unique labels per router.
9-9: Dynamic Service naming
Appending-{{ $k }}to the Service name prevents clashes when multiple routers are defined.
10-10: Consistent namespace resolution
Switching to top-level context ($.Release.Namespace) within the loop is necessary and correctly applied.
12-12: Leverage Helm helper for dynamic router service type
Usinginclude "router.service.type" (list $ $k $v)centralizes service-type logic per router entry.
26-26: Dynamic selector for multi-router support
Updating the selector to usecontroller-router-{{ $k }}matches the metadata labels and ensures proper pod selection.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml (5)
1-7: Standardize Helm helper for gRPC hostname resolution
Thegrpc.hostnamehelper cleanly defaults togrpc.<baseDomain>if not overridden. Good use ofrequiredto enforce configuration.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 3-3: wrong indentation: expected 0 but found 4
(indentation)
[warning] 4-4: wrong indentation: expected 0 but found 2
(indentation)
[warning] 5-5: wrong indentation: expected 0 but found 4
(indentation)
17-23: Parameterize gRPC service type with default
Thegrpc.service.typehelper correctly abstracts service-type logic and defaults toClusterIPwhen unset, promoting DRY patterns.
25-34: Parameterize router hostname per instance
Therouter.hostnamehelper effectively generates hostnames for each router key with a fallback torouter-<key>.<baseDomain>. Well modularized.
36-45: Parameterize router endpoint per instance
Therouter.endpointhelper mirrorsgrpc.endpointlogic—supports per-router override with fallback to the shared TLS port. Looks solid.
47-56: Parameterize router service type with default
Therouter.service.typehelper cleanly abstracts per-router service-type selection, defaulting toClusterIPif unset. Nicely structured.
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml
Show resolved
Hide resolved
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json (3)
55-64: Defineingressas an object in the schema
Theingressdefinition currently lacks atypeandpropertieswrapper, so its fields aren’t enforced. It should be defined as:"ingress": { "type": "object", "properties": { "enabled": { "type": "boolean", "description": "Whether to enable Ingress for the gRPC endpoint" }, "class": { "type": "string", "description": "IngressClass to use for the gRPC endpoint" } }, "additionalProperties": false }
65-70: Definerouteas an object in the schema
Theroutedefinition also lacks atypewrapper andadditionalProperties, so onlyenabledisn’t enforced. It should be:"route": { "type": "object", "properties": { "enabled": { "type": "boolean", "description": "Whether to enable OpenShift Router for the gRPC endpoint" } }, "additionalProperties": false }
316-373: Explicitly settype: "object"for root-levelgrpc
While you’ve definedpropertiesandadditionalProperties, adding"type": "object"ensures that only an object is accepted:"grpc": { - "additionalProperties": false, + "type": "object", + "additionalProperties": false, "properties": { // … } }
🧹 Nitpick comments (1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json (1)
71-96: Consider enforcing required fields forrouterdefinitions
Currently, therouterobject makes all fields optional, which may lead to incomplete configurations. It’s recommended to require at leasthostnameandendpoint:"router": { "type": "object", "additionalProperties": false, + "required": ["hostname", "endpoint"], "properties": { "hostname": { … }, "endpoint": { … }, // … } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json(1 hunks)deploy/helm/jumpstarter/values.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- deploy/helm/jumpstarter/values.yaml
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: deploy-kind
- GitHub Check: lint-go
- GitHub Check: tests
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
🔇 Additional comments (4)
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json (4)
1-54: Core definitions are correct
Theport,labels,ClusterIP,NodePort, andservicedefinitions correctly validate port ranges and service types.
97-223: JWT & claim definitions look solid
ThePrefixedClaimOrExpression,ClaimOrExpression, andJWTAuthenticatorschemas are correctly constructed with strictadditionalProperties: false.
224-279: Configuration andconfig.grpcschema are well-defined
The top-levelconfigobject and its nestedauthenticationandkeepaliveproperties are properly scoped and type-safe.
282-296: Basic properties validated correctly
Thenamespace,image,tag, andimagePullPolicyfields are all typed correctly as strings.
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml (2)
11-17: Review aggressive gRPC keepalive defaults.Setting
minTime: 3swithpermitWithoutStream: truecan generate frequent HTTP/2 PING frames and expose you to potential DDoS vectors (as noted). Consider:
- Providing more conservative defaults (e.g.,
30s).- Documenting these settings in your values schema and README to guide operators.
21-37: Promote JWT configuration to a real schema block.Rather than leaving JWT settings commented out, define them explicitly in
values.yaml(and updatevalues.schema.json) with defaults disabled. This enhances discoverability viahelm show values:@@ values.yaml - # To trust service account tokens, first execute: - # kubectl create clusterrolebinding oidc-reviewer \ - # --clusterrole=system:service-account-issuer-discovery \ - # --group=system:unauthenticated - # Then uncomment: - # - # jwt: - # - issuer: - # url: https://kubernetes.default.svc.cluster.local - # audiences: - # - https://kubernetes.default.svc.cluster.local - # audienceMatchPolicy: MatchAny - # certificateAuthority: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt - # claimMappings: - # username: - # claim: "sub" - # prefix: "kubernetes:" + # JWT-based authentication configuration. + jwt: + enabled: false + issuers: []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml(1 hunks)deploy/helm/jumpstarter/values.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- deploy/helm/jumpstarter/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
🔇 Additional comments (2)
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml (2)
18-20:✅ Verification successful
Verify the internal authentication prefix format.
You’ve configured
prefix: "internal:". Ensure downstream code or templates:
- Don’t inadvertently double-append the colon.
- Properly strip or append this prefix when constructing service principals.
🏁 Script executed:
#!/usr/bin/env bash # Confirm usage of the internal auth prefix in Go code rg -n 'internal:' -A 2 .Length of output: 747
To verify how the
internal:prefix is consumed in your Helm templates, let’s search for its usage in the chart’s templates:
🏁 Script executed:
#!/usr/bin/env bash # Search for direct use of the internal auth prefix in Helm templates rg -n '.Values.authentication.internal.prefix' -A 3 deploy/helm/jumpstarter/charts/jumpstarter-controller/templates rg -n 'authentication.internal' -A 3 deploy/helm/jumpstarter/charts/jumpstarter-controller/templatesLength of output: 216
To verify that the
internal:prefix isn’t accidentally doubled in code, let’s inspect exactly how it’s consumed in your Go configuration logic:
🏁 Script executed:
#!/usr/bin/env bash # Inspect how Internal.Prefix is used when building service principals rg -n 'Prefix' -A 5 internal/oidc/config.go internal/config/oidc.goLength of output: 2536
Let’s confirm how
newJWTAuthenticatorhandles thePrefix(to ensure no extra colons are added or stripped):
🏁 Script executed:
#!/usr/bin/env bash # Show the implementation of newJWTAuthenticator in both config files rg -n 'func newJWTAuthenticator' -A 30 internal/config/oidc.go internal/oidc/config.goLength of output: 4058
Internal auth prefix usage verified
- The Go config defaults
Internal.Prefixto"internal:"and passes it directly intoPrefixedClaimOrExpressionwithout any extra colon added innewJWTAuthenticator.- No occurrences of
.Values.authentication.internal.prefixwere found in the Helm templates, so there’s no risk of double-appending the colon.No changes required.
8-9:✅ Verification successful
Confirm gRPC TLS passthrough settings propagate correctly.
You’ve set
mode: "passthrough"andport: 443undergrpc.tls. Please verify that all related templates (service, ingress, router) consume these values consistently:
🏁 Script executed:
#!/usr/bin/env bash # Search for grpc.tls usage in your Helm templates rg -n "grpc.tls" -A 3 deploy/helm/jumpstarter/charts/jumpstarter-controller/templatesLength of output: 5585
🏁 Script executed:
#!/usr/bin/env bash # Show Service definitions to verify port handling rg -n "kind: Service" -A5 deploy/helm/jumpstarter/charts/jumpstarter-controller/templates # Check where .Values.grpc.tls.port is referenced across templates rg -n ".Values.grpc.tls.port" deploy/helm/jumpstarter/charts/jumpstarter-controller/templatesLength of output: 6127
🏁 Script executed:
#!/usr/bin/env bash # Print service port definitions for router and controller sed -n '1,200p' deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml sed -n '1,200p' deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml # Print ingress backend port definitions for router and controller sed -n '1,200p' deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-ingress.yaml sed -n '1,200p' deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-ingress.yamlLength of output: 4132
gRPC TLS passthrough is wired correctly
- The
passthroughmode is applied in all route and ingress templates:
router-route.yaml&controller-route.yamlusetermination: passthroughrouter-ingress.yaml&controller-ingress.yamlsetnginx.ingress.kubernetes.io/ssl-passthrough: "true"- The
port: 443value is only consumed by the hostname helpers (_helpers.yaml) for client‐side URL generation, while the Service backends remain on their internal gRPC ports (8083 for routers, 8082 for the controller), which is intentional.No further changes needed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…s/router-deployment.yaml Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml (1)
18-22:⚠️ Potential issueFix YAML indentation for
nodePortconditional to prevent parse errorsThe current templating lines introduce extra leading whitespace, causing YAMLlint
could not find expected ':'. Align theif/endblocks andnodePortentry with proper hyphen whitespace trimming.Apply this diff to fix the YAML indentation:
- {{ if .Values.grpc.service }} - {{ if .Values.grpc.service.nodePort }} - nodePort: {{ .Values.grpc.service.nodePort }} - {{ end }} - {{ end }} +{{- if .Values.grpc.service }} +{{- if .Values.grpc.service.nodePort }} + nodePort: {{ .Values.grpc.service.nodePort }} +{{- end }} +{{- end }}🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 19-19: syntax error: could not find expected ':'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml (2)
1-1: Add conditional rendering for theenabledflagThe
enabled: trueflag was added but chart templates don't reference.Values.enabledfor conditional rendering. All resource templates should include conditional guards.Verify template usage with this script:
#!/bin/bash # Check if .Values.enabled is referenced in templates rg -n "Values\.enabled" deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/
3-4: 🛠️ Refactor suggestionPin the image tag explicitly
The image specification lacks an explicit tag, which can lead to unpredictable deployments using implicit
latesttags.Refactor the image configuration:
-image: quay.io/jumpstarter-dev/jumpstarter-controller -imagePullPolicy: IfNotPresent +image: + repository: quay.io/jumpstarter-dev/jumpstarter-controller + tag: "v0.1.0" # specify appropriate version + pullPolicy: IfNotPresentdeploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml (2)
7-8: Duplicateapp.kubernetes.io/namelabel issue
The Service is still labeledjumpstarter-controllerthough it represents a router. Rename tojumpstarter-routeror leverage a helper like{{ include "router.name" . }}.
20-24:⚠️ Potential issueFix indentation for
nodePortblock
The nestedifstatements aroundnodePortare misaligned, causing YAML parse errors. Consolidate into one conditional aligned with the other port keys:- {{ if $v.service }} - {{ if $v.service.nodePort }} - nodePort: {{ $v.service.nodePort }} - {{ end }} - {{ end }} + {{- if $v.service.nodePort }} + nodePort: {{ $v.service.nodePort }} + {{- end }}🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 21-21: syntax error: could not find expected ':'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (1)
10-10: Duplicateapp.kubernetes.io/namelabel issue
The Deployment retainsapp.kubernetes.io/name: jumpstarter-controllerthough the pod runs the router. It should be updated tojumpstarter-router.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml (1)
9-15: Ensure defaultgrpc.tls.portis declaredThe
grpc.endpointhelper references.Values.grpc.tls.portfor its fallback. Make sure you’ve defined a defaultgrpc.tls.portinvalues.yaml(and the JSON schema) to avoid missing-value errors at deployment.
🧹 Nitpick comments (9)
deploy/helm/jumpstarter/values.yaml (1)
3-4: Document simplified global parameters
The newly reducedtimestampandbaseDomainentries lack context—consider adding brief comments or descriptions to clarify their purpose for future maintainers.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-ingress.yaml (3)
1-3: Consolidate nested ingress conditionals
You can combine the twoifchecks into one for readability:{{- if and $v.ingress $v.ingress.enabled }}🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
16-17: Use Helm fullname helper for resource naming
Instead of hardcoding:name: jumpstarter-router-ingress-{{ $k }}use your chart’s fullname helper to include the release prefix and avoid naming conflicts.
30-30: Apply fullname helper to backend service name
Replace the literal service name with:name: {{ include "jumpstarter-controller.fullname" (dict "name" (printf "router-grpc-%s" $k) "values" $) }}to maintain consistency.
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml (1)
9-9: Use fullname helper for Service name
Swap the hardcoded name for:name: {{ include "jumpstarter-controller.fullname" (dict "name" (printf "router-grpc-%s" $k) "values" $) }}to inherit release naming.
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml (2)
1-3: Consolidate nested route conditionals
Merge{{ if $v.route }}and{{ if $v.route.enabled }}:{{- if and $v.route $v.route.enabled }}for brevity.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
14-15: Use Helm fullname helper for Route name
Avoid hardcoded names; use:name: {{ include "jumpstarter-controller.fullname" (dict "name" (printf "router-route-%s" $k) "values" $) }}deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml (2)
25-34: Refine error message in router hostname helperThe
requiredcall mentionsgrpc.router.<name>.hostnamebut the actual path isValues.grpc.routers.<key>.hostname. Updating the message to reflect the real values path will improve clarity for chart users.
47-56: Extract common service type logic (optional)Both
grpc.service.typeandrouter.service.typefollow the same pattern. Consider creating a genericservice.typehelper to DRY up this duplication and simplify future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.github/workflows/e2e.yaml(1 hunks)Dockerfile(2 hunks)Makefile(1 hunks)cmd/main.go(2 hunks)cmd/router/main.go(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_endpoints.tpl(0 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-ingress.yaml(3 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-route.yaml(3 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-ingress.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml(1 hunks)deploy/helm/jumpstarter/values.kind.yaml(0 hunks)deploy/helm/jumpstarter/values.schema.json(1 hunks)deploy/helm/jumpstarter/values.yaml(1 hunks)hack/deploy_with_helm.sh(3 hunks)hack/kind_cluster.yaml(1 hunks)internal/config/config.go(3 hunks)internal/config/types.go(1 hunks)internal/service/controller_service.go(3 hunks)internal/service/helpers.go(1 hunks)internal/service/router_service.go(1 hunks)
💤 Files with no reviewable changes (2)
- deploy/helm/jumpstarter/values.kind.yaml
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_endpoints.tpl
✅ Files skipped from review due to trivial changes (2)
- internal/config/types.go
- deploy/helm/jumpstarter/values.schema.json
🚧 Files skipped from review as they are similar to previous changes (15)
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role_binding.yaml
- Dockerfile
- cmd/main.go
- hack/kind_cluster.yaml
- .github/workflows/e2e.yaml
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml
- internal/service/router_service.go
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/rbac/leader_election_role.yaml
- cmd/router/main.go
- internal/service/helpers.go
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml
- internal/config/config.go
- deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json
- Makefile
- hack/deploy_with_helm.sh
🧰 Additional context used
🧠 Learnings (2)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter-controller#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).
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter-controller#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.
🪛 YAMLlint (1.37.1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 3-3: wrong indentation: expected 0 but found 4
(indentation)
[warning] 4-4: wrong indentation: expected 0 but found 2
(indentation)
[warning] 5-5: wrong indentation: expected 0 but found 4
(indentation)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-ingress.yaml
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-route.yaml
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml
[error] 19-19: syntax error: could not find expected ':'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml
[error] 12-12: syntax error: could not find expected ':'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-ingress.yaml
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-service.yaml
[error] 21-21: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: deploy-kind
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: tests
- GitHub Check: lint-go
🔇 Additional comments (14)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-service.yaml (1)
10-10: LGTM: Dynamic service type configurationGood use of the helper template for dynamic service type resolution, which supports the new multi-router architecture.
deploy/helm/jumpstarter/charts/jumpstarter-controller/values.yaml (1)
11-37: LGTM: Comprehensive configuration structureThe new config section provides well-structured authentication settings with helpful comments for JWT configuration. The gRPC keepalive settings include appropriate safety warnings.
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-ingress.yaml (2)
21-21: LGTM: Consistent hostname helper usageGood use of the
grpc.hostnamehelper template for consistent hostname resolution across ingress rules and TLS configuration.Also applies to: 33-33
1-2:❓ Verification inconclusive
Verify YAML template syntax
The nested conditional structure looks correct, but static analysis flagged a syntax error. Ensure proper YAML document formatting.
🏁 Script executed:
#!/bin/bash # Test Helm template rendering to verify syntax helm template deploy/helm/jumpstarter/charts/jumpstarter-controller/ --dry-run --debugLength of output: 183
Verify YAML template syntax for nested conditionals
We weren’t able to run
helm templatein the sandbox (Helm not installed), so please install Helm or run a lint step locally to catch any formatting errors:helm lint deploy/helm/jumpstarter/charts/jumpstarter-controller/ # or, once Helm is installed: helm template deploy/helm/jumpstarter/charts/jumpstarter-controller/ --dry-run --debugPoints to double-check:
- Proper indentation and matching
{{ end }}for the nested
if .Values.grpc.ingress→if .Values.grpc.ingress.enabledblocks- No stray or extra
endstatements- Final rendered YAML is valid and parsable
The helper template usage for hostname looks good.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-route.yaml (2)
15-15: LGTM: Consistent hostname resolutionGood use of the
grpc.hostnamehelper template for consistent hostname configuration in the OpenShift Route.
1-2:❓ Verification inconclusive
Verify YAML template syntax for OpenShift Route
The nested conditional structure matches the ingress template pattern, but static analysis flagged a syntax error. Ensure proper YAML formatting.
🏁 Script executed:
#!/bin/bash # Test Route template rendering specifically helm template deploy/helm/jumpstarter/charts/jumpstarter-controller/ --show-only templates/controller-route.yamlLength of output: 209
Manual Check Needed: OpenShift Route Template Syntax
We attempted to render
controller-route.yamlwith Helm in the sandbox, buthelmisn’t installed. Please verify locally that the nested conditional blocks emit valid YAML:
- File: deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-route.yaml
- Focus on lines 1–2:
{{ if .Values.grpc.route }} {{ if .Values.grpc.route.enabled }}- Confirm all
{{ end }}statements line up and indentation produces a well-formed Route.Run locally:
helm template deploy/helm/jumpstarter/charts/jumpstarter-controller/ \ --show-only templates/controller-route.yaml🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 2-2: syntax error: expected '', but found '{'
(syntax)
internal/service/controller_service.go (2)
78-78: LGTM: Router configuration integrationGood addition of the Router field to support the new multi-router architecture with label-based selection.
20-20: LGTM: Well-organized importsGood additions of necessary imports for the new functionality:
cmpandslicesfor sorting router candidatesgolang.org/x/exp/mapsfor working with router mapconfigpackage for router configuration typesAlso applies to: 26-26, 31-32
deploy/helm/jumpstarter/values.yaml (1)
8-9: Ensuregrpc.routersmap is supplied downstream
This minimalvalues.yamlomits thegrpc.routersdefinition introduced in the chart. Users must provide agrpc.routersmap in their environment-specific values; verify upgrade impact on existing deployments.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (2)
23-26: Reference to controller ConfigMap is intentional
Per design, the router shares the controller’s ConfigMap (controller-cm.yaml), so the existing annotation is correct.
80-91: Skip HTTP health probe comments for gRPC
These HTTP-based liveness/readiness probes are commented out deliberately; they’ll be replaced once the router binary implements gRPC health endpoints.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/_helpers.yaml (3)
1-7: Validate hostname fallback logicThe helper correctly prioritizes an explicit
Values.grpc.hostnameand usesrequiredonglobal.baseDomainfor fallback. Good use ofrequiredto surface missing configurations at render time.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 3-3: wrong indentation: expected 0 but found 4
(indentation)
[warning] 4-4: wrong indentation: expected 0 but found 2
(indentation)
[warning] 5-5: wrong indentation: expected 0 but found 4
(indentation)
17-23: Default gRPC service type implementationThis helper sensibly falls back to
"ClusterIP"when nogrpc.service.typeis set. The conditional and default usage is clear and correct.
36-45: Consistent endpoint construction for routersThe
router.endpointhelper mirrors the gRPC logic and correctly falls back to a constructed hostname with the shared TLS port. This aligns well with the multi-router architecture.
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (1)
8-10: Inconsistentapp.kubernetes.io/namelabel
Theapp.kubernetes.io/namelabel is still set tojumpstarter-controllerrather thanjumpstarter-router, which misrepresents this component. It should be updated (or derived via a Helm helper) to accurately reflect the router deployment.
🧹 Nitpick comments (5)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (5)
7-7: Quote namespace value
Wrap the templated namespace in quotes to avoid YAML parsing issues if it contains special characters:- namespace: {{ default $.Release.Namespace $.Values.namespace }} + namespace: "{{ default $.Release.Namespace $.Values.namespace }}"
32-39: Unaddressed TODO for node affinity
The template still contains commented-out node affinity and a TODO. For multi-architecture support, either remove this block or parameterize and enable affinity viavalues.yaml.
80-91: Missing gRPC health probes
HTTP health probes are commented out. Once your router binary implements a gRPC health endpoint (or you include agrpc_health_probe), update these to use anexecprobe or HTTP₂ probe for gRPC:livenessProbe: exec: command: ["/grpc_health_probe", "-addr=localhost:{{ .Values.grpc.tls.port }}"] initialDelaySeconds: 15 periodSeconds: 20 readinessProbe: exec: command: ["/grpc_health_probe", "-addr=localhost:{{ .Values.grpc.tls.port }}"] initialDelaySeconds: 5 periodSeconds: 10
99-99: Decouple ServiceAccount for router
Reusingcontroller-managercouples RBAC for both controller and router. Consider parameterizingserviceAccountNameand creating a dedicated ServiceAccount for the router to enforce least-privilege.
100-100: Review terminationGracePeriodSeconds
The 10 s grace period may not cover in-flight gRPC requests during shutdown. Consider aligning this with your router binary’s shutdown timeout or exposing it as a configurable value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml(1 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json(1 hunks)hack/deploy_with_helm.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml
- deploy/helm/jumpstarter/charts/jumpstarter-controller/values.schema.json
- hack/deploy_with_helm.sh
🧰 Additional context used
🧠 Learnings (1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter-controller#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.
🪛 YAMLlint (1.37.1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml
[error] 12-12: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint-go
- GitHub Check: tests
- GitHub Check: deploy-kind
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
There was a problem hiding this comment.
Nice, my only ask would be that we try to minimize changes to the config entries as much as we can.
I know some are unavoidable if we have multiple routers, also try to keep the top level values.yaml with all the possible configurations and defaults.
This is a change that will break deployers, like @bennyz and I on the lab as we switch from 0.6.0 to 0.7.0
|
|
||
| grpc: | ||
| hostname: "" |
There was a problem hiding this comment.
We should not delete the options in this values.yaml
As it serves as the base to let others understand what are the available overall options and defaults (i.e. argocd)
There was a problem hiding this comment.
They are now in the json schema, I think argocd can visualize that too.
|
|
||
| config: | ||
| grpc: | ||
| keepalive: |
There was a problem hiding this comment.
Can we keep the overall grpc config under the root grpc?
There was a problem hiding this comment.
The keepalive part is handled by the controller itself not the helm chart.
| @@ -1,10 +0,0 @@ | |||
| global: | |||
| baseDomain: jumpstarter.127.0.0.1.nip.io | |||
There was a problem hiding this comment.
we don't need this anymore for kind?
There was a problem hiding this comment.
They are always set by the hack/deploy_with_helm.sh script?
|
|
||
| jumpstarter-controller: | ||
| enabled: true |
There was a problem hiding this comment.
It's generally very useful to see the overall structure and defaults in the values.yaml I see we have json now, which I guess it's much better to eventually generate docs or have other tools understand our structure.
| @@ -1,4 +1,5 @@ | |||
| {{ if eq .Values.grpc.mode "ingress" }} | |||
| {{ if .Values.grpc.ingress }} | |||
There was a problem hiding this comment.
Let's avoid changes to configuration formats unless we have a good reason, of course.
| @@ -1,4 +1,5 @@ | |||
| {{ if eq .Values.grpc.mode "route" }} | |||
| {{ if .Values.grpc.route }} | |||
| {{ if .Values.grpc.route.enabled }} | |||
| @@ -7,38 +10,32 @@ metadata: | |||
| nginx.ingress.kubernetes.io/backend-protocol: "GRPC" | |||
| nginx.ingress.kubernetes.io/proxy-read-timeout: "300" | |||
| nginx.ingress.kubernetes.io/proxy-send-timeout: "300" | |||
| {{ if eq .Values.grpc.tls.mode "passthrough" }} | |||
| {{ if eq $.Values.grpc.tls.mode "passthrough" }} | |||
There was a problem hiding this comment.
We don't need the complexity to individually configure each router in a different form.
Can we keep unified settings for all routers? (image, mode, etc...) only the endpoint name could change potentially. Otherwise it means that you need to finetune each of them.
For tls we may want to use a certificate that covers them all.
| HELM_SETS+=" --set global.baseDomain=${BASEDOMAIN}" | ||
| HELM_SETS+=" --set jumpstarter-controller.grpc.endpoint=${GRPC_ENDPOINT}" | ||
|
|
||
| HELM_SETS+=" --set jumpstarter-controller.grpc.routers.default.hostname=${GRPC_ROUTER_DEFAULT_HOSTNAME}" |
There was a problem hiding this comment.
Will we still get a default one based on baseDomain if we don't provide one here?
|
|
||
| candidates := maps.Values(s.Router) | ||
| slices.SortFunc(candidates, func(a config.RouterEntry, b config.RouterEntry) int { | ||
| return -cmp.Compare(MatchLabels(a.Labels, exporter.Labels), MatchLabels(b.Labels, exporter.Labels)) |
There was a problem hiding this comment.
ah nice, so you can steer exporters to specific routers.
Summary by CodeRabbit
New Features
Improvements
Refactor