Ausias/fix ports session #247
Conversation
…t/hoprd-operator into ausias/session-port-range
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates to version numbers in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Outside diff range and nitpick comments (5)
charts/hoprd-operator/templates/configmap-config.yaml (1)
Line range hint
33-34: Remove extra blank lines at the end of the fileThere are unnecessary blank lines at the end of the template.
persistence: size: "{{- .Values.operator.persistence.size }}" storage_class_name: "{{- .Values.operator.persistence.storageClassName }}" - -src/hoprd/hoprd_service.rs (3)
Line range hint
124-127: Improve error handling and retry logic.Consider the following improvements:
- Extract retry configuration (max retries, sleep duration) to constants or configuration
- Enhance the error message with more details (e.g., include the service name and current retry count)
- Consider using a backoff strategy for retries
Here's a suggested implementation:
const MAX_RETRIES: u32 = 12; const RETRY_INTERVAL_SECS: u64 = 5; // ... in the function if load_balancer_ip.is_none() { return Err(HoprdError::HoprdStatusError(format!( "Failed to obtain load balancer IP for service {}-p2p-tcp after {} retries", name, MAX_RETRIES ))); }
Line range hint
82-156: Consider architectural improvements for service creation.The current implementation could benefit from the following improvements:
- Extract common service creation logic to reduce duplication
- Implement cleanup on failure (delete TCP service if UDP service creation fails)
- Add structured logging for better observability
Consider implementing a builder pattern for service creation and a cleanup mechanism:
struct ServiceBuilder { name: String, namespace: String, labels: BTreeMap<String, String>, owner_references: Option<Vec<OwnerReference>>, protocol: String, load_balancer_ip: Option<String>, } impl ServiceBuilder { fn new(name: String, namespace: String) -> Self { ... } fn with_labels(mut self, labels: BTreeMap<String, String>) -> Self { ... } fn with_protocol(mut self, protocol: String) -> Self { ... } async fn build(self, api: Api<Service>) -> Result<Service, HoprdError> { ... } }This would make the code more maintainable and easier to test.
Line range hint
157-196: Improve port handling and validation.The current port handling implementation has several areas for improvement:
- The API port (3001) is hardcoded
- There's no validation for the port range
- Port naming could be more consistent
Consider these improvements:
const API_PORT: u16 = 3001; fn build_ports(starting_port: u16, last_port: u16, port_name: Option<&str>) -> Result<Vec<ServicePort>, HoprdError> { if starting_port >= last_port { return Err(HoprdError::ValidationError( format!("Invalid port range: starting_port ({}) must be less than last_port ({})", starting_port, last_port) )); } let mut ports = Vec::new(); let protocols = match port_name { Some(name) if name.contains("udp") => vec!["UDP"], Some(_) => vec!["TCP"], None => vec!["TCP", "UDP"], }.into_iter().map(String::from).collect::<Vec<_>>(); // Rest of the implementation... }src/hoprd/hoprd_ingress.rs (1)
256-256: Consider adding edge case tests.While the current tests are good, consider adding these scenarios:
- When starting_port + allocation would exceed max_port
- When there's exactly enough space for allocation
- When the first port is higher than min_port + allocation
Would you like me to help generate these additional test cases?
Also applies to: 261-261, 267-267, 273-273, 279-279
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.toml(1 hunks)charts/hoprd-operator/Chart.yaml(1 hunks)charts/hoprd-operator/templates/configmap-config.yaml(1 hunks)src/bootstrap_operator.rs(1 hunks)src/constants.rs(0 hunks)src/hoprd/hoprd_ingress.rs(4 hunks)src/hoprd/hoprd_service.rs(1 hunks)src/operator_config.rs(1 hunks)
💤 Files with no reviewable changes (1)
- src/constants.rs
✅ Files skipped from review due to trivial changes (2)
- Cargo.toml
- charts/hoprd-operator/Chart.yaml
🧰 Additional context used
📓 Learnings (3)
src/bootstrap_operator.rs (1)
Learnt from: ausias-armesto
PR: hoprnet/hoprd-operator#246
File: src/bootstrap_operator.rs:29-33
Timestamp: 2024-10-30T17:56:33.685Z
Learning: Ensure that code suggestions for `src/bootstrap_operator.rs`'s `open_nginx_deployment_ports` function maintain functional equivalence, and carefully consider the implications of code changes on functional behavior.
src/hoprd/hoprd_ingress.rs (2)
Learnt from: ausias-armesto
PR: hoprnet/hoprd-operator#246
File: src/hoprd/hoprd_ingress.rs:182-189
Timestamp: 2024-10-30T22:44:03.067Z
Learning: In the file `src/hoprd/hoprd_ingress.rs`, the function `find_next_port` used to find the next available port does not need to implement binary search, as the current sequential approach is sufficient.
Learnt from: ausias-armesto
PR: hoprnet/hoprd-operator#246
File: src/hoprd/hoprd_ingress.rs:37-39
Timestamp: 2024-10-30T23:58:30.915Z
Learning: In the `create_ingress` function in `src/hoprd/hoprd_ingress.rs`, prefer not to handle errors from `open_port` by mapping `HoprError` to `kube::Error`; it's acceptable to use `unwrap()`.
src/operator_config.rs (1)
Learnt from: ausias-armesto
PR: hoprnet/hoprd-operator#246
File: src/operator_config.rs:25-26
Timestamp: 2024-10-30T22:39:35.272Z
Learning: In `src/operator_config.rs`, the `IngressConfig` struct's `port_min` and `port_max` fields are injected via config maps as strings, so they cannot be changed to numeric types like `u16`.
🔇 Additional comments (6)
charts/hoprd-operator/templates/configmap-config.yaml (1)
26-27: Verify port value constraints in values.yaml
The port values are now treated as raw numbers instead of strings. While this aligns with the u16 type changes in the operator, please ensure that the values.yaml file includes proper validation for these ports (e.g., minimum and maximum values, numeric type).
✅ Verification successful
Let me gather more information about the port validation by checking the related Rust code mentioned in the context.
Port validation is handled in the operator code
The port validation is properly implemented in the Rust operator code:
- Ports must be between 1024 and 65535
- Minimum port must be less than maximum port
- Type safety is ensured through u16 types
The removal of quotes in the ConfigMap template is correct as it aligns with:
- The u16 type requirements in operator_config.rs
- The numeric validation in bootstrap_operator.rs
- The numeric port definitions in values.yaml
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if port values in values.yaml have proper constraints
# Expected: Find port min/max definitions with type/range validation
# Search for port definitions and their validation in values.yaml
rg -A 5 "ports:" "charts/hoprd-operator/values.yaml"
Length of output: 352
Script:
#!/bin/bash
# Check port validation in operator_config.rs and bootstrap_operator.rs
rg -A 5 "port_" "src/operator_config.rs" "src/bootstrap_operator.rs"
# Also check for any port-related validation in the Helm chart
ast-grep --pattern 'if not (and (kindIs "float64" .Values.operator.ingress.ports.min) (kindIs "float64" .Values.operator.ingress.ports.max))' charts/hoprd-operator/templates/
Length of output: 2436
src/bootstrap_operator.rs (2)
21-22: 🛠️ Refactor suggestion
Verify type conversion safety and consider explicit error handling.
The direct type conversions from u16 to i32 are safe since:
u16max value (65535) fits withini32range- Port validation (1024-65535) remains intact
- Source type (
u16) guarantees non-negative values
However, consider making the conversions more explicit for better maintainability:
- let min_port: i32 = ingress_config.port_min.into();
- let max_port: i32 = ingress_config.port_max as i32 + 1;
+ // Explicit conversion from u16 to i32 for port numbers
+ let min_port = i32::from(ingress_config.port_min);
+ let max_port = i32::from(ingress_config.port_max) + 1;Let's verify the port type changes across related files:
#!/bin/bash
# Description: Verify port type consistency across files
# Expected: All port-related fields should use u16 type
# Check port type declarations
rg -A 2 'port_.+:\s*(?:String|u16|i32)' src/Line range hint 21-65: Verify deployment patching behavior with the new port type system.
The port allocation and deployment patching logic appears correct, but let's verify:
- Port range validation remains intact
- Container port specifications are correctly generated
- Deployment patching still works as expected
✅ Verification successful
Let me gather more information about the port handling across the codebase to verify the deployment patching behavior thoroughly.
Port type system changes and deployment patching are correctly implemented
The verification confirms:
- Port types are consistently using
u16across the codebase - Port validation (1024-65535) is properly enforced with appropriate error handling
- Deployment patching follows the standard k8s pattern with proper error handling for both TCP and UDP ports
- Container port specifications are correctly generated with type-safe port numbers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify port-related deployment logic
# Expected: Consistent port handling across deployment specs
# Check port usage in deployment specs
rg -A 5 'container_port.*=.*port' src/
# Check port validation patterns
rg -A 3 'contains.*port' src/
# Look for any remaining string-based port parsing
ast-grep --pattern 'parse::<i32>()'
Length of output: 2306
Script:
#!/bin/bash
# Check deployment patching logic and port type consistency
# Look for deployment patch operations
rg -A 5 'patch.*deployment' src/
# Check container port type definitions and usage
ast-grep --pattern 'ContainerPort { $$$container_port: $_, $$$}'
# Look for any port-related error handling
rg -A 3 'HoprError.*port' src/
Length of output: 3730
src/hoprd/hoprd_ingress.rs (3)
126-130: LGTM! Clear port range validation.
The port range validation is now more straightforward with direct u16 comparison, and the error message clearly explains the allocation failure reason.
165-166: LGTM! Simplified port filtering logic.
The port filtering logic is now more efficient with direct u16 comparisons, and the find_next_port call correctly uses the min_port value.
Also applies to: 170-170
177-179: LGTM! Simplified port allocation logic.
The function is now more straightforward with:
- Direct u16 parameter instead of Option
- Clearer empty ports handling
- Simplified gap detection logic
This aligns with the previous feedback about keeping the sequential approach.
Also applies to: 183-183
Fixing the configuration file for hoprd-operator and converting min_port and max_port into u16 instead of String
Summary by CodeRabbit
New Features
hoprd_operatorpackage and Helm chart.Bug Fixes
Refactor
port_minandport_maxfields fromStringtou16for more appropriate data representation.Chores