operator: ingress support #176
Conversation
WalkthroughIngress reconciliation and creation were added for endpoints; service creation logic now considers Ingress/Route and merges labels/annotations via a new MergeMaps utility. Deployment scripts and env var usage switched from INGRESS_ENABLED to NETWORKING_MODE. Tests added for Ingress behavior and cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler as EndpointReconciler
participant API as K8s API
participant Utils as utils.MergeMaps
Note over Reconciler: ReconcileControllerEndpoint / ReconcileRouterReplicaEndpoint
Reconciler->>API: ensure Service (ClusterIP/NodePort/LoadBalancer) with merged labels/annotations
API-->>Reconciler: Service created/updated
alt Ingress/Route enabled
Reconciler->>Utils: Merge(defaultIngressAnnotations, userAnnotations)
Utils-->>Reconciler: merged annotations
Reconciler->>API: createOrUpdate Ingress (IngressClassName?, Rules, TLS) with ownerRef -> service backend
API-->>Reconciler: Ingress created/updated
else Ingress/Route not enabled
Note right of Reconciler: Skip ingress creation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hack/deploy_with_operator.sh (1)
2-2: Consider if command tracing should remain enabled.The
-xflag enables command tracing, which increases log verbosity. This may be useful for debugging but could clutter logs. Note thathack/deploy_with_helm.shstill usesset -eo pipefailwithout tracing.Consider either:
- Removing the
-xflag if it was added temporarily for debugging- Adding it consistently to other deployment scripts if it's intended to be permanent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go(7 hunks)deploy/operator/internal/controller/jumpstarter/endpoints/endpoints_test.go(5 hunks)hack/deploy_vars(1 hunks)hack/deploy_with_helm.sh(1 hunks)hack/deploy_with_operator.sh(2 hunks)hack/utils(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints_test.go (1)
deploy/operator/api/v1alpha1/jumpstarter_types.go (2)
Endpoint(380-414)IngressConfig(433-451)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (1)
deploy/helm/jumpstarter/charts/jumpstarter-controller/model.py (2)
Ingress(74-82)Route(85-90)
⏰ 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-release-0-7
- GitHub Check: lint-go
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: deploy-with-operator
- GitHub Check: deploy-kind
🔇 Additional comments (15)
hack/utils (1)
15-15: LGTM! Standardized networking mode configuration.The transition from
INGRESS_ENABLEDtoNETWORKING_MODEwith a default of "nodeport" improves clarity and aligns with the broader refactoring across deployment scripts.hack/deploy_with_operator.sh (1)
53-67: LGTM! Appropriate ingress class configuration.Setting the ingress class to "nginx" for both controller and router endpoints aligns well with the nginx ingress installation performed earlier in the script. This ensures the ingress resources will be properly handled by the nginx ingress controller.
hack/deploy_with_helm.sh (1)
16-16: LGTM! Consistent networking mode check.The update from
INGRESS_ENABLED == "true"toNETWORKING_MODE == "ingress"maintains the same logic while aligning with the standardized configuration variable across all deployment scripts.hack/deploy_vars (1)
11-18: LGTM! Clear endpoint configuration based on networking mode.The port mappings are appropriate: 5443 for ingress (typically with TLS termination) and 8082/8083 for nodeport. The logic is consistent with the broader NETWORKING_MODE refactoring.
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (4)
157-171: LGTM! Proper ClusterIP service handling for Ingress/Route.The logic correctly creates a ClusterIP service when needed by Ingress or Route configurations, while safely handling cases where ClusterIP config might be nil. This ensures the backend service exists for Ingress to route traffic.
174-185: LGTM! Proper default service logic.The updated condition correctly avoids creating a default ClusterIP service when Ingress or Route are already enabled and handling service creation. This prevents resource duplication.
214-262: LGTM! Consistent router endpoint handling.The router endpoint reconciliation follows the same patterns as the controller endpoint, ensuring consistency. The Ingress creation and ClusterIP service logic mirror the controller implementation appropriately.
290-291: Theutilspackage referenced in the import does not exist in the codebase.The code at line 291 calls
utils.MergeMaps(baseLabels, extraLabels), but:
- The package
deploy/operator/internal/utilsdoes not exist in the repository- The
MergeMapsfunction is not defined anywhere in the codebase- This code would fail to compile
Either the utils package needs to be created with the
MergeMapsfunction implementation, or the import path is incorrect. Cannot verify the precedence behavior without seeing the actualMergeMapsimplementation.deploy/operator/internal/controller/jumpstarter/endpoints/endpoints_test.go (7)
258-317: LGTM! Comprehensive Ingress test coverage.This test thoroughly validates the Ingress creation flow, including:
- ClusterIP service backend creation
- IngressClassName assignment
- Default nginx annotations for gRPC/TLS
- Ingress rules and host configuration
- TLS configuration
The test coverage is excellent for the Ingress-enabled endpoint scenario.
319-362: LGTM! Proper annotation and label merging validation.This test confirms that user-provided annotations correctly override defaults while preserving non-conflicting default annotations. The label merging is also validated. This is important for ensuring flexibility in Ingress configuration.
364-406: LGTM! Thorough hostname extraction testing.The test validates hostname extraction from various address formats (with and without ports), ensuring the Ingress host configuration is correct regardless of how the address is specified. The inline cleanup is appropriate for this iterative test.
408-436: LGTM! Validates default IngressClass behavior.This test ensures that when no ingress class is specified, the IngressClassName is nil, allowing the cluster's default IngressClass to be used. This is the correct Kubernetes behavior.
440-461: LGTM! Proper test cleanup for Ingress resources.The AfterEach block is correctly updated to clean up the newly created Ingress resources alongside their associated services. This prevents test pollution and resource leaks.
540-590: LGTM! Comprehensive router Ingress testing.The router replica endpoint tests mirror the controller endpoint tests, validating Ingress creation, annotation merging, and proper service selection. The consistency between controller and router test patterns demonstrates good test design.
605-615: LGTM! Router Ingress cleanup.The cleanup properly handles router-specific Ingress resources, maintaining consistency with the controller test cleanup patterns.
mangelajo
force-pushed
the
operator-ingress
branch
2 times, most recently
from
823548d to
d29516f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
hack/deploy_with_helm.sh (2)
16-20: Validate NETWORKING_MODE and fail fast on unknown values.Add a guard to avoid silent misconfig when NETWORKING_MODE is unset or unexpected; default to nodeport or exit with a clear message.
-if [ "${NETWORKING_MODE}" == "ingress" ]; then +case "${NETWORKING_MODE}" in + ingress) ;; + nodeport|"") NETWORKING_MODE="nodeport" ;; + *) echo "Unsupported NETWORKING_MODE=${NETWORKING_MODE} (expected: ingress|nodeport)"; exit 1 ;; +esac + +if [ "${NETWORKING_MODE}" == "ingress" ]; thenAlso applies to: 31-35
43-45: Use precise Helm release detection.Avoid grep false positives; use helm status on the exact release/namespace.
-if helm list -A | grep jumpstarter > /dev/null; then - METHOD=upgrade -fi +if helm status jumpstarter -n jumpstarter-lab >/dev/null 2>&1; then + METHOD=upgrade +fideploy/operator/internal/utils/utils.go (1)
5-19: Preallocate map capacity to reduce allocations.Minor perf win and keeps semantics unchanged.
-func MergeMaps(base, overrides map[string]string) map[string]string { - merged := make(map[string]string) +func MergeMaps(base, overrides map[string]string) map[string]string { + merged := make(map[string]string, len(base)+len(overrides))deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (2)
68-74: Preserve NodePort by port name, not index.Index-based copy breaks if port order changes; match by Name to retain stable NodePorts.
- if service.Spec.Type == corev1.ServiceTypeNodePort || service.Spec.Type == corev1.ServiceTypeLoadBalancer { - for i := range existingService.Spec.Ports { - if existingService.Spec.Ports[i].NodePort != 0 && i < len(service.Spec.Ports) { - service.Spec.Ports[i].NodePort = existingService.Spec.Ports[i].NodePort - } - } - } + if service.Spec.Type == corev1.ServiceTypeNodePort || service.Spec.Type == corev1.ServiceTypeLoadBalancer { + existingByName := make(map[string]int32, len(existingService.Spec.Ports)) + for _, p := range existingService.Spec.Ports { + if p.NodePort != 0 && p.Name != "" { + existingByName[p.Name] = p.NodePort + } + } + for i := range service.Spec.Ports { + if np, ok := existingByName[service.Spec.Ports[i].Name]; ok { + service.Spec.Ports[i].NodePort = np + } + } + }Also applies to: 84-90
131-138: Consider creating the ClusterIP service before the Ingress.Minor ordering nit: reconciling the service first avoids brief ingress backends-not-found windows on first apply. Functionally OK either way.
Also applies to: 156-171
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints_test.go (2)
364-407: Add IPv4 and IPv6 cases to hostname extraction tests.Covers important formats declared in the CRD comments.
testCases := []struct { address string expectedHost string }{ {"grpc.example.com", "grpc.example.com"}, {"grpc.example.com:443", "grpc.example.com"}, {"grpc.example.com:8080", "grpc.example.com"}, + {"192.0.2.10:8443", "192.0.2.10"}, + {"[2001:db8::1]:8443", "2001:db8::1"}, }
258-317: Add a negative test: Ingress enabled but empty address → no Ingress created.Verifies the “skip when no hostname” path.
deploy/operator/internal/controller/jumpstarter/endpoints/ingress.go (2)
106-116: Gate nginx-specific defaults by Ingress class.Avoid applying nginx annotations when using another controller.
- // Build default annotations for TLS passthrough with GRPC with nginx ingress - defaultAnnotations := map[string]string{ - "nginx.ingress.kubernetes.io/ssl-redirect": "true", - "nginx.ingress.kubernetes.io/backend-protocol": "GRPC", - "nginx.ingress.kubernetes.io/proxy-read-timeout": "300", - "nginx.ingress.kubernetes.io/proxy-send-timeout": "300", - "nginx.ingress.kubernetes.io/ssl-passthrough": "true", - } + defaultAnnotations := map[string]string{} + if endpoint.Ingress.Class == "" || endpoint.Ingress.Class == "nginx" { + defaultAnnotations = map[string]string{ + "nginx.ingress.kubernetes.io/ssl-redirect": "true", + "nginx.ingress.kubernetes.io/backend-protocol": "GRPC", + "nginx.ingress.kubernetes.io/proxy-read-timeout": "300", + "nginx.ingress.kubernetes.io/proxy-send-timeout": "300", + "nginx.ingress.kubernetes.io/ssl-passthrough": "true", + } + }Also applies to: 121-126
70-90: Use net.SplitHostPort for robust parsing; keep IPv6 bracket handling.Reduces edge-case bugs (e.g., bare IPv6 with port). Behavior stays backward‑compatible.
+import ( + "context" + "net" + "strings" + ... +) @@ func extractHostname(address string) string { - // Handle IPv6 addresses in brackets - if strings.HasPrefix(address, "[") { - // Find the closing bracket - if idx := strings.Index(address, "]"); idx != -1 { - return address[1:idx] - } - return address - } - - // For hostname or IPv4, strip port if present - if idx := strings.LastIndex(address, ":"); idx != -1 { - // Check if this is part of an IPv6 address (no brackets) - // Count colons - if more than one, likely IPv6 - if strings.Count(address, ":") > 1 { - return address - } - return address[:idx] - } - - return address + if address == "" { + return "" + } + // Bracketed IPv6 with optional port + if strings.HasPrefix(address, "[") { + if host, _, err := net.SplitHostPort(address); err == nil { + return host + } + if idx := strings.Index(address, "]"); idx != -1 { + return address[1:idx] + } + return address + } + // Try host:port for hostname/IPv4 + if host, _, err := net.SplitHostPort(address); err == nil { + return host + } + // Bare hostname, IPv4, or bare IPv6 without port + return address }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go(7 hunks)deploy/operator/internal/controller/jumpstarter/endpoints/endpoints_test.go(5 hunks)deploy/operator/internal/controller/jumpstarter/endpoints/ingress.go(1 hunks)deploy/operator/internal/utils/utils.go(1 hunks)hack/deploy_vars(1 hunks)hack/deploy_with_helm.sh(1 hunks)hack/deploy_with_operator.sh(2 hunks)hack/utils(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- hack/deploy_vars
- hack/deploy_with_operator.sh
- hack/utils
🧰 Additional context used
🧬 Code graph analysis (3)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (1)
deploy/operator/internal/utils/utils.go (1)
MergeMaps(5-19)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints_test.go (2)
deploy/operator/api/v1alpha1/jumpstarter_types.go (2)
Endpoint(380-414)IngressConfig(433-451)deploy/helm/jumpstarter/charts/jumpstarter-controller/model.py (2)
Ingress(74-82)Port(70-71)
deploy/operator/internal/controller/jumpstarter/endpoints/ingress.go (2)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (1)
Reconciler(35-38)deploy/operator/internal/utils/utils.go (1)
MergeMaps(5-19)
⏰ 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-tests-release-0-7
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: deploy-with-operator
🔇 Additional comments (1)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (1)
291-292: Nil-safe merge is fine; confirm extraLabels origin.utils.MergeMaps handles nil. Ensure callers don’t pass huge maps unintentionally (labels copied per reconcile).
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints_test.go
Show resolved
Hide resolved
mangelajo
force-pushed
the
operator-ingress
branch
from
d29516f to
c1f42c4
Compare
mangelajo
force-pushed
the
operator-ingress
branch
from
c1f42c4 to
3e595c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deploy/operator/internal/controller/jumpstarter/endpoints/ingress.go (1)
113-113: TODO documented for status condition propagation.The TODO comment notes that invalid hostname errors should be propagated to status conditions in the future. This is good to track for improved observability.
Would you like me to open an issue to track implementing status condition propagation for invalid hostnames?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go(7 hunks)deploy/operator/internal/controller/jumpstarter/endpoints/endpoints_test.go(5 hunks)deploy/operator/internal/controller/jumpstarter/endpoints/ingress.go(1 hunks)deploy/operator/internal/utils/utils.go(1 hunks)hack/deploy_vars(1 hunks)hack/deploy_with_helm.sh(1 hunks)hack/deploy_with_operator.sh(2 hunks)hack/utils(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- hack/deploy_with_helm.sh
- hack/deploy_with_operator.sh
- deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go
- hack/deploy_vars
🧰 Additional context used
🧬 Code graph analysis (2)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints_test.go (1)
deploy/operator/api/v1alpha1/jumpstarter_types.go (2)
Endpoint(380-414)IngressConfig(433-451)
deploy/operator/internal/controller/jumpstarter/endpoints/ingress.go (2)
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (1)
Reconciler(35-38)deploy/operator/internal/utils/utils.go (1)
MergeMaps(5-19)
⏰ 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). (6)
- GitHub Check: tests
- GitHub Check: deploy-with-operator
- GitHub Check: deploy-kind
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-release-0-7
- GitHub Check: lint-go
🔇 Additional comments (9)
deploy/operator/internal/utils/utils.go (1)
5-19: LGTM! Clean and correct implementation.The function correctly merges two maps with proper precedence (overrides take precedence over base). The implementation is safe for nil inputs since ranging over nil maps in Go is well-defined behavior.
hack/utils (1)
15-15: LGTM! Clean refactoring to mode-based configuration.The change from
INGRESS_ENABLEDtoNETWORKING_MODEwith a default of "nodeport" provides better clarity and aligns with the PR's ingress support objectives.deploy/operator/internal/controller/jumpstarter/endpoints/endpoints_test.go (4)
258-436: Excellent test coverage for Ingress functionality.These tests comprehensively validate:
- ClusterIP service creation with Ingress
- Default nginx annotations for TLS passthrough and GRPC
- User annotation/label merging with correct precedence
- Hostname extraction from various address formats
- IngressClassName handling (both specified and nil/default)
- TLS configuration
The test structure is clear and follows existing patterns.
440-461: Proper cleanup of Ingress resources.The AfterEach cleanup correctly includes the new ingress resources (controller-grpc-ing, test-svc-ing) alongside existing service cleanup.
540-590: Good coverage for router replica Ingress scenarios.These tests properly validate Ingress creation for router replicas, including service creation, ingress configuration, annotation merging, and proper pod selector targeting.
605-615: Cleanup properly handles router ingress resources.The router AfterEach correctly includes cleanup of router-grpc-ing alongside service resources.
deploy/operator/internal/controller/jumpstarter/endpoints/ingress.go (3)
34-67: LGTM! Follows controller-runtime best practices.The function correctly uses CreateOrUpdate to reconcile Ingress resources, updates all mutable fields (Labels, Annotations, IngressClassName, Rules, TLS), sets owner references, and includes proper error handling and logging.
69-92: Hostname extraction logic handles multiple address formats correctly.The function properly handles:
- IPv6 addresses in brackets
[IPv6]and[IPv6]:port- Hostnames and IPv4 addresses with optional ports
- IPv6 addresses without brackets (multiple colons)
94-183: Well-structured ingress creation with proper validation.The function includes several good practices:
- Hostname extraction and validation using
IsDNS1123Subdomain- Default nginx annotations for TLS passthrough and GRPC backend
- Correct precedence when merging user annotations/labels with defaults using
MergeMaps- Proper handling of optional
IngressClassName(nil when empty)- TLS configuration for passthrough mode
Note: The past review comment regarding potential DNS1123 name length validation for the constructed ingress name (serviceName + "-ing") at line 143 remains relevant.
|
github arm runners are down, that's why that shows up in red. |
Summary by CodeRabbit
Refactor
New Features
Tests