Add proxy protocol support for tunnel servers #172
AI Code Review Results
AI Pull Request Overview
Summary
- Adds support for proxy protocol on tunnel servers to preserve original client IP addresses.
- Introduces
operator.inlets.dev/proxy-protoannotation for per-service configuration with valuesv1,v2, or empty (disabled). - Renames
InletsProConfigtoTunnelConfigfor improved naming consistency. - Updates CRD with new
proxyProtofield in TunnelSpec. - Implements validation for proxy protocol annotation values.
- Adds unit tests for user data generation with proxy protocol.
- Documents the feature in README with important note on immutability.
- Tested E2E with proxy protocol annotation.
Approval rating (1-10)
7 - Solid implementation with good testing, but lacks comprehensive validation and potential for runtime errors if invalid proxyProto values are passed to inlets-pro.
Summary per file
Summary per file
| File path | Summary |
|---|---|
| Dockerfile | Adds copying of new userdata.go and userdata_test.go files. |
| README.md | Documents proxy protocol feature with annotation usage and immutability warning. |
| chart/inlets-operator/crds/operator.inlets.dev_tunnels.yaml | Regenerates CRD with new proxyProto field and updated descriptions. |
| config.go | Renames InletsProConfig to TunnelConfig struct. |
| config_test.go | Updates test cases to use renamed TunnelConfig. |
| controller.go | Adds proxy protocol annotation parsing, validation, and user data generation. |
| image_test.go | Updates tests for renamed TunnelConfig struct. |
| main.go | Updates flag handling and method calls for TunnelConfig. |
| pkg/apis/inletsoperator/v1alpha1/types.go | Adds optional ProxyProto string field to TunnelSpec. |
| userdata.go | New function to generate exit server user data with proxy protocol flag. |
| userdata_test.go | Unit tests for user data generation with and without proxy protocol. |
| validate.go | Minor formatting change (added blank line). |
Overall Assessment
The changes implement proxy protocol support effectively, with opt-in configuration per service. The renaming improves code clarity, and testing covers the new functionality. However, validation is limited to annotation values during tunnel creation, potentially allowing invalid proxyProto values to reach inlets-pro. The immutability of the setting after VM provisioning is clearly documented, reducing migration risks. No regressions expected for existing users as the feature is disabled by default. Security impact is minimal since proxy protocol is opt-in and preserves IPs as intended.
Detailed Review
Detailed Review
Dockerfile
- Change adds new files to the build, ensuring they are included in the container image.
- No issues detected.
README.md
- Documentation clearly explains the feature, annotation usage, and critical immutability constraint.
- Good practice to warn users about the need to recreate services for changes.
chart/inlets-operator/crds/operator.inlets.dev_tunnels.yaml
- CRD regeneration includes the new proxyProto field with proper nullable and optional annotations.
- Controller-gen version updated from v0.11.1 to v0.14.0 - ensure compatibility with the Kubernetes cluster versions supported.
- Descriptions added for APIVersion, Kind, and other fields - verify these are standard and accurate.
config.go
- Renaming InletsProConfig to TunnelConfig improves semantic clarity without functional changes.
- Method renamed accordingly - ensure all references are updated (appears consistent in diff).
config_test.go
- Tests updated to use new struct name - good coverage maintained.
controller.go
- Validation logic for proxyProto annotation:
if ok && v != "" && v != "v1" && v != "v2"correctly rejects invalid non-empty values. - However, if an invalid value slips through (e.g., manual Tunnel CR creation), inlets-pro may fail at runtime. Consider adding CRD validation rules or additional checks.
- In
getHostConfig, proxyProto is retrieved from service annotations without re-validation - relies on validation during tunnel creation, which is acceptable for this use case. - Code simplification in
createTunnelAuthTokenSecretcall (removed unused variable) is minor improvement.
image_test.go
- Consistent updates for struct rename.
main.go
- Flag variables updated to reference TunnelConfig fields.
- No functional changes beyond renaming.
pkg/apis/inletsoperator/v1alpha1/types.go
- New ProxyProto field added with proper kubebuilder annotations for nullability and optionality.
- Comment explains purpose and requirement for upstream support - clear and informative.
userdata.go
- New function
makeExitServerUserdatacorrectly incorporates proxyProto into the systemd service ExecStart command. - Assumes inlets-pro accepts
--proxy-protocol=""for disabled state - verify inlets-pro documentation to confirm empty string disables proxy protocol. - Environment variable PROXY_PROTO is exported and used in ExecStart - ensures runtime configuration.
userdata_test.go
- Comprehensive tests cover both enabled (v1) and disabled ("") proxy protocol scenarios.
- Exact string matching ensures user data format is correct.
validate.go
- Trivial formatting change - no impact.
Risks and Recommendations:
- Validation Gap: Proxy protocol validation only occurs during operator-managed tunnel creation. Manually created Tunnel CRs with invalid proxyProto values could cause inlets-pro startup failures. Add kubebuilder validation to the CRD to enforce
enum: ["", "v1", "v2"]or similar. - Dependency on inlets-pro: Ensure the inlets-pro version used supports
--proxy-protocolflag with empty string for disable. If not, adjust logic to omit the flag when proxyProto is empty. - Security: Proxy protocol preserves source IPs, which is intended. No additional security concerns as it's opt-in.
- Performance: Negligible impact from additional annotation processing.
- Testing: Unit tests added, E2E mentioned - consider adding tests for invalid annotation values to ensure proper error handling.
- Migration: No issues - new field is optional and nullable.
AI agent details.
Agent processing time: 37.205s
Environment preparation time: 7.007s
Total time from webhook: 52.638s