Implement GET /api/v1/node/{name}/configuration #46
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several changes to enhance the node management API. Key modifications include updating the Changes
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (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: 0
🧹 Nitpick comments (3)
nodeman/nodes.py (2)
Line range hint
273-281: Refactor duplicate configuration object creationThe configuration object creation logic is duplicated between
enroll_nodeandget_node_configuration. Consider extracting this into a helper method.+def _create_node_configuration(name: str, request: Request) -> NodeConfiguration: + return NodeConfiguration( + name=name, + mqtt_broker=request.app.settings.nodes.mqtt_broker, + mqtt_topics=request.app.settings.nodes.mqtt_topics, + trusted_jwks=request.app.trusted_jwks, + ) async def enroll_node( name: str, request: Request, ) -> NodeEnrollmentResult: # ... existing code ... return NodeEnrollmentResult( - name=name, - mqtt_broker=request.app.settings.nodes.mqtt_broker, - mqtt_topics=request.app.settings.nodes.mqtt_topics, - trusted_jwks=request.app.trusted_jwks, + **_create_node_configuration(name, request).model_dump(), x509_certificate=node_certificate.x509_certificate, x509_ca_certificate=node_certificate.x509_ca_certificate, x509_certificate_serial_number=node_certificate.x509_certificate_serial_number, x509_certificate_not_valid_after=node_certificate.x509_certificate_not_valid_after, ) async def get_node_configuration( name: str, request: Request, ) -> NodeConfiguration: # ... existing code ... - return NodeConfiguration( - name=name, - mqtt_broker=request.app.settings.nodes.mqtt_broker, - mqtt_topics=request.app.settings.nodes.mqtt_topics, - trusted_jwks=request.app.trusted_jwks, - ) + return _create_node_configuration(name, request)Also applies to: 359-364
351-351: Fix incorrect docstringThe docstring "Enroll new node" is incorrect for the
get_node_configurationendpoint.- """Enroll new node""" + """Get node configuration"""tests/test_api.py (1)
151-155: Enhance configuration endpoint test coverageThe test could be more comprehensive by:
- Verifying all configuration fields (mqtt_broker, mqtt_topics, trusted_jwks)
- Adding test cases for error scenarios (e.g., non-activated node)
response = client.get(f"{node_url}/configuration") assert response.status_code == status.HTTP_200_OK node_information = response.json() print(json.dumps(node_information, indent=4)) assert node_information["name"] == name + assert "mqtt_broker" in node_information + assert "mqtt_topics" in node_information + assert "trusted_jwks" in node_information + + # Test non-activated node + non_activated_name = ".".join(["non-activated", settings.nodes.domain]) + response = admin_client.post( + urljoin(server, "/api/v1/node"), + params={"name": non_activated_name} + ) + assert response.status_code == status.HTTP_201_CREATED + + response = client.get(f"/api/v1/node/{non_activated_name}/configuration") + assert response.status_code == status.HTTP_400_BAD_REQUEST
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
nodeman/models.py(3 hunks)nodeman/nodes.py(5 hunks)tests/test_api.py(2 hunks)
🔇 Additional comments (2)
nodeman/models.py (2)
Line range hint 90-99: LGTM: Good separation of concerns
The change to make NodeConfiguration inherit from BaseModel instead of NodeCertificate improves the design by properly separating configuration data from certificate data.
101-102: LGTM: Well-structured composite model
The NodeEnrollmentResult class effectively combines configuration and certificate data through multiple inheritance, providing a clean way to return both aspects in enrollment responses.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
nodeman/nodes.py (2)
52-58: Add documentation and type hints to the helper function.The function encapsulates configuration creation logic well, but could benefit from:
- A docstring explaining its purpose and parameters
- Type hint for the
requestparameter-def create_node_configuration(name: str, request: Request) -> NodeConfiguration: +def create_node_configuration(name: str, request: Request) -> NodeConfiguration: + """Create a NodeConfiguration instance for a given node. + + Args: + name: The name of the node + request: The FastAPI request object containing application settings + + Returns: + NodeConfiguration: The node configuration instance + """ return NodeConfiguration( name=name, mqtt_broker=request.app.settings.nodes.mqtt_broker, mqtt_topics=request.app.settings.nodes.mqtt_topics, trusted_jwks=request.app.trusted_jwks, )
221-221: Update the docstring to accurately reflect the endpoint's behavior.The current docstring "Enroll new node" is misleading as this endpoint handles both new node enrollment and returns enrollment results for existing nodes.
- """Enroll new node""" + """Enroll a node and return enrollment results. + + This endpoint handles the node enrollment process and returns both + configuration and certificate information in the enrollment result. + """Also applies to: 282-287
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nodeman/nodes.py(6 hunks)
🔇 Additional comments (1)
nodeman/nodes.py (1)
344-365: Implementation successfully meets PR objectives.
The new endpoint is well-integrated with the existing codebase and follows established patterns. It correctly handles error cases and reuses the configuration logic. With the suggested improvements to observability and documentation, this will be a solid addition to the API.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
nodeman/nodes.py (2)
55-61: Add docstring to helper function.The helper function is well-structured but would benefit from documentation explaining its purpose and parameters.
def create_node_configuration(name: str, request: Request) -> NodeConfiguration: + """Create a NodeConfiguration instance for the given node. + + Args: + name: The name of the node + request: FastAPI request object containing application settings + + Returns: + NodeConfiguration: Configuration including MQTT broker details and trusted JWKS + """ return NodeConfiguration( name=name, mqtt_broker=request.app.settings.nodes.mqtt_broker, mqtt_topics=request.app.settings.nodes.mqtt_topics, trusted_jwks=request.app.trusted_jwks, )
347-372: Consider adding response caching.The configuration endpoint could benefit from response caching since the configuration data is likely to change infrequently. This would improve performance for frequently accessed nodes.
@router.get( "/api/v1/node/{name}/configuration", responses={ status.HTTP_200_OK: {"model": NodeConfiguration}, status.HTTP_404_NOT_FOUND: {}, }, tags=["client"], response_model_exclude_none=True, + response_class=CachedResponse, ) async def get_node_configuration( name: str, request: Request, + response: Response, ) -> NodeConfiguration: """Get node configuration""" node = find_node(name) if not node.activated: logging.debug("Node %s not activated", name, extra={"nodename": name}) raise HTTPException(status.HTTP_400_BAD_REQUEST, detail="Node not activated") res = create_node_configuration(name=name, request=request) node_configurations_requested.add(1) + # Cache response for 5 minutes + response.headers["Cache-Control"] = "public, max-age=300" + return res
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nodeman/nodes.py(7 hunks)
🔇 Additional comments (3)
nodeman/nodes.py (3)
22-22: LGTM: Import and metrics changes are well-structured.
The new NodeEnrollmentResult import and node_configurations_requested metric counter follow the established patterns in the codebase.
Also applies to: 40-42
215-215: LGTM: Enrollment response model changes are correct.
The update to use NodeEnrollmentResult and the helper function improves code organization.
Also applies to: 224-224, 285-291
22-22: Verify NodeEnrollmentResult usage across codebase.
Let's ensure the new NodeEnrollmentResult model is properly handled by API clients.
Also applies to: 215-215, 224-224
✅ Verification successful
NodeEnrollmentResult implementation and usage appears correct
The verification shows:
- NodeEnrollmentResult properly inherits from NodeConfiguration and NodeCertificate models
- The model is correctly used in the enroll_node endpoint with proper error handling
- Test coverage exists for both successful and error cases of node enrollment
- The implementation follows a consistent pattern across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NodeEnrollmentResult usage in tests and client code
rg "NodeEnrollmentResult" --type python
# Search for enroll_node function calls to verify proper response handling
ast-grep --pattern 'enroll_node($$$)'
Length of output: 110
Script:
#!/bin/bash
# Search for NodeEnrollmentResult usage without file type restriction
rg "NodeEnrollmentResult"
# Search for enroll_node function calls
rg "enroll_node" -A 2
Length of output: 455
Script:
#!/bin/bash
# Get more context around NodeEnrollmentResult class definition
rg -B 2 -A 5 "class NodeEnrollmentResult"
# Get more context around enroll_node function to see response handling
rg -B 2 -A 10 "async def enroll_node"
Length of output: 826
Script:
#!/bin/bash
# Check NodeConfiguration and NodeCertificate parent classes
rg -B 2 -A 5 "class Node(Configuration|Certificate)"
# Look for any imports of NodeEnrollmentResult
rg "from.*NodeEnrollmentResult|import.*NodeEnrollmentResult"
# Check for any tests related to node enrollment
rg -l "test.*enroll.*node|test.*node.*enroll"
Length of output: 1085
Script:
#!/bin/bash
# Check the enrollment test implementation
rg -B 5 -A 10 "def test.*enroll.*node" tests/test_api.py
# Look for any error handling patterns in tests
rg "HTTP_400_BAD_REQUEST.*enroll" tests/test_api.py
Length of output: 1028
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
nodeman/settings.py (1)
48-48: Add validation constraints for configuration_ttl.While the default value of 300 seconds is reasonable, consider adding validation constraints to ensure the TTL remains within acceptable bounds.
- configuration_ttl: int = Field(default=300) + configuration_ttl: int = Field(default=300, gt=0, le=86400, description="Configuration cache TTL in seconds")nodeman/nodes.py (1)
373-375: Consider adding ETag for caching optimization.While the Cache-Control header is properly implemented using the configuration_ttl setting, consider adding an ETag header for more efficient caching. This would allow clients to use If-None-Match for conditional requests.
# Cache response for 5 minutes max_age = request.app.settings.nodes.configuration_ttl response.headers["Cache-Control"] = f"public, max-age={max_age}" + # Add ETag for conditional requests + config_hash = hashlib.sha256( + f"{res.model_dump_json()}".encode() + ).hexdigest() + response.headers["ETag"] = f'W/"{config_hash}"'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
nodeman/nodes.py(7 hunks)nodeman/settings.py(1 hunks)tests/test_api.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_api.py
🔇 Additional comments (5)
nodeman/nodes.py (5)
22-22: LGTM!
The new import and metric counter are properly integrated, following the established patterns in the codebase.
Also applies to: 40-42
55-62: LGTM!
Good refactoring to extract common configuration creation logic into a reusable function.
215-215: LGTM!
The endpoint now returns a combined NodeEnrollmentResult, properly constructed using the helper function.
Also applies to: 224-224, 285-290
361-361: Fix incorrect docstring.
The docstring needs to be updated to accurately describe the endpoint's purpose.
356-377: Add observability instrumentation.
For consistency with other endpoints, add OpenTelemetry tracing and logging.
Closes #45
Summary by CodeRabbit
NodeEnrollmentResult.configuration_ttlin settings.