Reduce min keepalive interval to 1s #121
Conversation
WalkthroughThis pull request removes the hardcoded gRPC keepalive enforcement function and updates configuration handling to return a dynamic gRPC server option. The initialization of the gRPC server in both controller and router services now uses a configurable option provided at startup. The configuration loading in the internal package has been extended to process gRPC settings and include OIDC authentication loading. Additionally, Helm chart templates and values have been updated to include new configuration parameters, annotations, and ConfigMap entries, and new configuration types have been introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant ConfigLoader
participant ControllerSvc
participant RouterSvc
participant gRPCServer
Main->>+ConfigLoader: LoadConfiguration()
ConfigLoader-->>-Main: (auth, prefix, serverOption)
Main->>+ControllerSvc: Initialize with serverOption
Main->>+RouterSvc: Initialize with serverOption
ControllerSvc->>+gRPCServer: Start gRPC Server (using serverOption)
RouterSvc->>+gRPCServer: Start gRPC Server (using serverOption)
gRPCServer-->>-ControllerSvc: Server running
gRPCServer-->>-RouterSvc: Server running
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (8)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/service/grpc.go (1)
12-14: Consider security-performance tradeoffs with this reduced keepalive interval.Reducing the minimum keepalive time from 30 seconds to 1 second will allow for faster detection of broken connections, but as correctly noted in your comments, it does increase DDoS vulnerability. This is because clients could potentially send keepalive pings every second, increasing server load.
Consider the following recommendations:
- Implement rate limiting if not already present
- Monitor the impact of this change in production environments
- Consider using a slightly higher value (e.g., 5s) if 1s proves too aggressive
- Document this decision and its rationale in your architecture documentation
The added comments are valuable as they explicitly acknowledge the security implications, which is good practice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/service/grpc.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/v1alpha1/grpcconfiguration_types.go (1)
16-19: Consider adding validation for MinTimeThe
MinTimeis stored as a string which will need to be parsed. Consider adding validation or setting constraints on the acceptable values to prevent runtime errors.type Keepalive struct { - MinTime string `json:"minTime"` + MinTime string `json:"minTime" validate:"required,duration"` PermitWithoutStream bool `json:"permitWithoutStream"` }If you're using kubebuilder, you could also consider using the CRD validation markers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
api/v1alpha1/grpcconfiguration_types.go(1 hunks)api/v1alpha1/zz_generated.deepcopy.go(2 hunks)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/controller-route.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml(2 hunks)deploy/helm/jumpstarter/values.yaml(2 hunks)internal/config/config.go(3 hunks)internal/config/grpc.go(1 hunks)internal/service/controller_service.go(2 hunks)internal/service/grpc.go(0 hunks)internal/service/router_service.go(2 hunks)
💤 Files with no reviewable changes (1)
- internal/service/grpc.go
🧰 Additional context used
🧬 Code Definitions (2)
internal/service/controller_service.go (2)
internal/service/router_service.go (4) (4)
s(53-79)s(81-111)s(113-147)s(150-152)internal/service/auth/auth.go (2) (2)
s(37-55)s(57-75)
api/v1alpha1/zz_generated.deepcopy.go (1)
api/v1alpha1/grpcconfiguration_types.go (2) (2)
GrpcConfiguration(10-14)Keepalive(16-19)
🔇 Additional comments (22)
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml (1)
8-10: New Annotations for HAProxy Route Timeouts
The addition of the annotations
haproxy.router.openshift.io/timeout: 2dand
haproxy.router.openshift.io/timeout-tunnel: 2d
sets extended timeout values for the HAProxy router. Please verify that these extended values (2 days) are in line with your operational requirements, especially given the PR objective regarding keepalive settings. Confirm that these timeouts won't conflict with the intent to reduce the minimum keepalive interval to 1s in other parts of the system.deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-route.yaml (1)
8-10: Added HAProxy timeout annotations to ensure long-lived connections.The 2-day timeout settings for both regular and tunnel connections align with the PR's goal of supporting reduced keepalive intervals. This ensures the OpenShift HAProxy router won't terminate connections prematurely while the gRPC keepalive mechanism is active.
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml (1)
13-13: Added gRPC configuration to the ConfigMap.The addition of gRPC configuration in the ConfigMap is appropriate as it enables externalized configuration of gRPC server settings, particularly the keepalive parameters needed for this PR.
This was previously noted by mangelajo who mentioned concerns about over-complicating things with many small file config formats nested inside the ConfigMap. While this is a valid architectural consideration, the current approach aligns with existing patterns in the codebase.
deploy/helm/jumpstarter/values.yaml (1)
70-78: Added gRPC configuration with keepalive settings.The implementation properly defines the gRPC configuration with keepalive settings. However, there's a discrepancy between the PR title "Reduce min keepalive interval to 1s" and the configured value of 3s.
The comments appropriately highlight the security implications of reducing the keepalive time. According to the gRPC documentation link provided in the comments, reducing the minTime can potentially make the server vulnerable to DDoS attacks. Please confirm if 3s is the intended value or if it should be 1s as suggested by the PR title.
Additionally, setting
permitWithoutStream: trueenables keepalive pings even when there are no active calls, which aligns with the goal of maintaining long-lived connections.internal/service/router_service.go (2)
43-45: Added ServerOption field to RouterService struct.The addition of the ServerOption field is a good approach that allows external configuration of gRPC server options, particularly for keepalive settings.
128-128: Using configurable ServerOption instead of hardcoded KeepaliveEnforcementPolicy.Replacing the hardcoded KeepaliveEnforcementPolicy with the configurable ServerOption allows for better control over gRPC server behavior through external configuration, which aligns with the PR objectives.
internal/service/controller_service.go (2)
72-72: Appropriate field addition for gRPC configurationAdding the
ServerOptionfield to theControllerServicestruct is a good approach for making gRPC server configuration more flexible. This change aligns with the PR objective to reduce min keepalive interval to 1s, as it allows the keepalive settings to be configurable rather than hardcoded.
664-666: Properly implemented server configurationThe modification to use
s.ServerOptioninstead of a hardcoded keepalive enforcement policy is correct. This change enhances the flexibility of the gRPC server configuration and enables customization of keepalive intervals per the PR objectives.internal/config/grpc.go (1)
14-38: Robust implementation of configuration loaderThe
LoadGrpcConfigurationfunction is well-structured and correctly handles the parsing of gRPC configuration parameters. It properly decodes the configuration, parses the duration, and constructs the appropriate server option.Some observations:
- The error handling is thorough
- The use of
serializer.EnableStrictensures configuration validation- The function returns a proper
grpc.ServerOptionfor the keepalive enforcement policyThis implementation supports the PR objective to make the keepalive interval configurable.
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml (1)
12-13: Good practice for ConfigMap-based deploymentsAdding the
configmap-sha256annotation is a good approach to ensure the deployment is updated when the ConfigMap changes. This ensures that when gRPC configuration is modified, the controller pods will be restarted to apply the new settings.api/v1alpha1/grpcconfiguration_types.go (1)
9-14: Well-structured configuration type with appropriate commentsThe
GrpcConfigurationstruct is correctly defined with the necessary Kubernetes type metadata markers. This ensures it will work properly with the Kubernetes API machinery for serialization and deserialization.cmd/main.go (3)
154-154: Update to LoadConfiguration function signature.The function now returns a
grpc.ServerOptionwhich enables configurable keepalive settings for the gRPC server. This change properly aligns with the PR objective to support more configurable keepalive intervals.
214-214: Addition of ServerOption to ControllerService.The ServerOption field is properly initialized with the option value returned from LoadConfiguration. This ensures that the ControllerService uses the configured gRPC settings.
221-223: Addition of ServerOption to RouterService.The RouterService now correctly includes the ServerOption field, ensuring consistent gRPC server configuration. This is aligned with the changes in ControllerService.
api/v1alpha1/zz_generated.deepcopy.go (3)
419-424: DeepCopyInto method for GrpcConfiguration.This is an auto-generated method that correctly handles deep copying the GrpcConfiguration struct. The implementation copies all fields, including the TypeMeta and Keepalive fields.
427-442: DeepCopy and DeepCopyObject methods for GrpcConfiguration.These auto-generated methods properly implement the deep copy functionality for GrpcConfiguration objects. The code follows the standard pattern for Kubernetes-style resource types.
459-472: DeepCopyInto and DeepCopy methods for Keepalive.These auto-generated methods correctly implement deep copy functionality for the Keepalive struct, which contains the min keepalive interval configuration.
internal/config/config.go (5)
8-8: Added import for grpc package.Correctly added the Google gRPC package import to support the new gRPC server options.
22-22: Updated function signature.The LoadConfiguration function signature has been updated to return an additional grpc.ServerOption parameter, supporting the PR objective of configurable keepalive settings.
25-31: Error handling updated.The error return statements have been properly updated to include nil for the new grpc.ServerOption parameter.
33-43: Added gRPC configuration handling.New code correctly handles fetching and processing gRPC configuration:
- Checks if "grpc" configuration exists in the ConfigMap
- Calls LoadGrpcConfiguration with appropriate parameters
- Returns error if configuration loading fails
This implementation provides a good fallback mechanism when no gRPC configuration is specified.
56-56: Updated return statement.The return statement correctly includes the new option parameter, completing the changes needed for the function signature update.
NickCao
force-pushed
the
short-keepalive
branch
from
54b0ae6 to
6384927
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/config/oidc.go (1)
1-51: Improve resiliency in LoadAuthenticationConfiguration.This new function for loading and appending a default OIDC-based JWT authenticator is well-structured, with a sensible default prefix. However, consider:
- Logging the final Issuer URL, especially if it’s being derived from external config.
- Checking for unexpected empty fields in OIDC config more defensively, as misconfigurations can be tricky to debug.
Otherwise, this setup is coherent and functional.
internal/config/config.go (1)
32-48: Graceful fallback to the embedded authentication configuration.When
authenticationis present in configmap, you return a keepalive enforcement policy. This logic is correct, but double-check possible edge cases—different user assumptions about keepalive intervals or missing fields in the configmap data might require additional validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
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/controller-route.yaml(2 hunks)deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml(2 hunks)deploy/helm/jumpstarter/values.yaml(2 hunks)internal/config/config.go(2 hunks)internal/config/grpc.go(1 hunks)internal/config/oidc.go(1 hunks)internal/config/types.go(1 hunks)internal/service/controller_service.go(2 hunks)internal/service/grpc.go(0 hunks)internal/service/router_service.go(2 hunks)
💤 Files with no reviewable changes (1)
- internal/service/grpc.go
🚧 Files skipped from review as they are similar to previous changes (5)
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-route.yaml
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-route.yaml
- internal/config/grpc.go
- deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/controller-deployment.yaml
- internal/service/router_service.go
🧰 Additional context used
🧬 Code Definitions (2)
internal/service/controller_service.go (1)
internal/service/router_service.go (4) (4)
s(53-79)s(81-111)s(113-147)s(150-152)
cmd/main.go (2)
internal/config/config.go (2) (2)
config(55-55)LoadConfiguration(18-78)internal/service/router_service.go (1) (1)
RouterService(40-46)
🔇 Additional comments (14)
internal/service/controller_service.go (2)
72-72: Good addition of configurable server optionAdding the ServerOption field to the ControllerService struct allows for more flexible configuration of gRPC server parameters, aligning with the PR objective to modify keepalive intervals.
664-666: Correct use of ServerOption for server initializationThe gRPC server is now initialized using the configurable ServerOption instead of a hardcoded value. This implementation correctly uses the server option from the configuration.
internal/config/types.go (1)
7-28: Well-structured configuration typesThe configuration type hierarchy is well organized with proper JSON tags and a logical structure that separates different configuration concerns. The Grpc.Keepalive settings directly support the PR's objective to configure keepalive intervals.
deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/cms/controller-cm.yaml (1)
13-17: Improved ConfigMap structure addressing previous feedbackThe configuration structure has been simplified by using a top-level
configfield as suggested in previous feedback. This approach makes the configuration more maintainable and addresses the concern about over-complication.deploy/helm/jumpstarter/values.yaml (1)
84-107: Well-structured authentication configurationThe authentication configuration has been properly structured as requested in the previous feedback, with individualized parameters instead of a monolithic configuration block. The commented JWT section provides a good example template for users.
cmd/main.go (3)
154-167: Validate environment variable for configuration key.Here, the call to
config.LoadConfigurationrelies on the"NAMESPACE"environment variable. If"NAMESPACE"is missing or set incorrectly, theclient.ObjectKeymight break configuration loading. Consider adding a check or fallback for"NAMESPACE"if not provided, or at least log a warning to facilitate debugging.Would you like me to generate a quick verification script or fallback logic for missing environment variables?
214-214: Good injection of gRPC server options into ControllerService.By passing the newly returned
optionfromLoadConfigurationintoServerOption, you align the controller service with the dynamic gRPC configuration approach. No immediate concerns.
221-223: Consistent usage of ServerOption in RouterService.Assigning
optiontoServerOptionensures that both router and controller services share the same gRPC keepalive policy. This promotes consistent configuration management throughout the application.internal/config/oidc.go (1)
53-87: Solid JWT authenticator construction.The logic in
newJWTAuthenticatorcorrectly handles dynamic CA content and merges multiple JWT authenticators into a single union. This approach is concise and leverages existing Kubernetes libraries effectively. No issues detected.internal/config/config.go (5)
25-28: Function signature extended with grpc.ServerOption.Returning
grpc.ServerOptionalongside the existing tokens is a clean approach that centralizes gRPC configuration. Ensure that callers handle or at least log any nil server option if the load fails early.
50-52: Clear error message for missing config section.Raising a descriptive error is helpful. This ensures misconfigurations are caught early. No further concerns here.
55-60: Strict YAML unmarshal for config.Using
UnmarshalStrictis a great practice to detect extraneous or malformed fields. Good defensive approach.
61-70: LoadAuthenticationConfiguration usage.This call references the newly defined OIDC loading. It’s consistent and straightforward. Be mindful of any potential collisions in
prefixif multiple sources define different internal prefixes.
72-77: LoadGrpcConfiguration integrated properly.The function retrieves the gRPC keepalive configurations from your
Configstruct and returns them. You then pass it along in the final return. This flow is cohesive. No red flags found.
|
e2e side changes: jumpstarter-dev/jumpstarter-e2e#12 |
2 participants
Summary by CodeRabbit
New Features
Refactor