Support parsing enrollment file in client #80
Conversation
|
Warning Rate limit exceeded@jschlyter has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 47 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes modify the enrollment logic in Changes
Possibly related PRs
Poem
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
🧹 Nitpick comments (3)
nodeman/client.py (3)
356-356: Add type hints and file validation.Consider enhancing the file argument with type hints and validation:
- enroll_parser.add_argument("--file", metavar="filename", help="Enrollment file") + enroll_parser.add_argument( + "--file", + metavar="filename", + help="JSON file containing enrollment data", + type=argparse.FileType('r'), + )
219-236: Standardize server URL assignment.The server URL assignment is inconsistent across different enrollment paths. Consider standardizing the logic:
def command_enroll(args: argparse.Namespace) -> NodeConfiguration: + server = args.server # Default server from command line + if args.create: - server = args.server node_bootstrap_information = command_create(args) name = node_bootstrap_information.name enrollment_key = JWK(**node_bootstrap_information.key.model_dump()) elif args.file: with open(args.file) as fp: enrollment_data = json.load(fp) try: name = enrollment_data["name"] - server = enrollment_data["nodeman_url"] + # Override server only if specified in file + if "nodeman_url" in enrollment_data: + server = enrollment_data["nodeman_url"] enrollment_key = JWK(**enrollment_data["key"]) except Exception as exc: logging.error("Error parsing enrollment file", exc_info=exc) raise SystemExit(2) from exc else: - server = args.server name = args.name enrollment_key = JWK(kty="oct", k=args.secret, alg="HS256")
232-233: Standardize error codes.Consider using an enum for error codes to maintain consistency throughout the codebase.
+from enum import IntEnum + +class ErrorCode(IntEnum): + SUCCESS = 0 + GENERAL_ERROR = 1 + INVALID_INPUT = 2 + NETWORK_ERROR = 3 + def command_enroll(args: argparse.Namespace) -> NodeConfiguration: ... logging.error("Error parsing enrollment file", exc_info=exc) - raise SystemExit(2) from exc + raise SystemExit(ErrorCode.INVALID_INPUT) from exc
nodeman/client.py
Outdated
| node_bootstrap_information = command_create(args) | ||
| name = node_bootstrap_information.name | ||
| enrollment_key = JWK(**node_bootstrap_information.key.model_dump()) | ||
| elif args.file: | ||
| with open(args.file) as fp: |
There was a problem hiding this comment.
Validate file path to prevent path traversal.
The file path should be validated before opening to prevent path traversal attacks.
+import os.path
+
def command_enroll(args: argparse.Namespace) -> NodeConfiguration:
...
elif args.file:
+ file_path = os.path.abspath(args.file)
+ if not os.path.exists(file_path):
+ logging.error("Enrollment file does not exist: %s", args.file)
+ raise SystemExit(2)
+ if not os.path.isfile(file_path):
+ logging.error("Path is not a file: %s", args.file)
+ raise SystemExit(2)
+
- with open(args.file) as fp:
+ with open(file_path) as fp:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with open(args.file) as fp: | |
| import os.path | |
| def command_enroll(args: argparse.Namespace) -> NodeConfiguration: | |
| ... | |
| elif args.file: | |
| file_path = os.path.abspath(args.file) | |
| if not os.path.exists(file_path): | |
| logging.error("Enrollment file does not exist: %s", args.file) | |
| raise SystemExit(2) | |
| if not os.path.isfile(file_path): | |
| logging.error("Path is not a file: %s", args.file) | |
| raise SystemExit(2) | |
| with open(file_path) as fp: | |
| ... |
nodeman/client.py
Outdated
| with open(args.file) as fp: | ||
| enrollment_data = json.load(fp) | ||
| try: | ||
| name = enrollment_data["name"] | ||
| server = enrollment_data["nodeman_url"] | ||
| enrollment_key = JWK(**enrollment_data["key"]) | ||
| except Exception as exc: | ||
| logging.error("Error parsing enrollment file", exc_info=exc) | ||
| raise SystemExit(2) from exc |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance file parsing error handling.
The current error handling is too broad. Consider:
- Validating the JSON schema
- Providing specific error messages for missing fields
- Using a Pydantic model for parsing
+from typing import TypedDict
+
+class EnrollmentData(TypedDict):
+ name: str
+ nodeman_url: str
+ key: dict
+
def command_enroll(args: argparse.Namespace) -> NodeConfiguration:
...
elif args.file:
with open(args.file) as fp:
- enrollment_data = json.load(fp)
- try:
- name = enrollment_data["name"]
- server = enrollment_data["nodeman_url"]
- enrollment_key = JWK(**enrollment_data["key"])
- except Exception as exc:
- logging.error("Error parsing enrollment file", exc_info=exc)
+ try:
+ enrollment_data = json.load(fp)
+ except json.JSONDecodeError as exc:
+ logging.error("Invalid JSON in enrollment file: %s", str(exc))
+ raise SystemExit(2) from exc
+
+ missing_fields = []
+ for field in ["name", "nodeman_url", "key"]:
+ if field not in enrollment_data:
+ missing_fields.append(field)
+
+ if missing_fields:
+ logging.error("Missing required fields in enrollment file: %s", ", ".join(missing_fields))
raise SystemExit(2) from exc
+
+ try:
+ enrollment_data_typed = EnrollmentData(
+ name=enrollment_data["name"],
+ nodeman_url=enrollment_data["nodeman_url"],
+ key=enrollment_data["key"],
+ )
+ name = enrollment_data_typed["name"]
+ server = enrollment_data_typed["nodeman_url"]
+ enrollment_key = JWK(**enrollment_data_typed["key"])
+ except (KeyError, TypeError) as exc:
+ logging.error("Invalid field types in enrollment file: %s", str(exc))
+ raise SystemExit(2) from excCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nodeman/client.py(4 hunks)
🔇 Additional comments (2)
nodeman/client.py (2)
6-6: LGTM! Good choice using pathlib.Path.Using
pathlib.Pathis the recommended way to handle file paths in modern Python.
225-241: Enhance JSON parsing error handling.The current error handling is too broad and doesn't provide specific error messages for different failure cases.
Previous review comment suggested using a TypedDict for schema validation and providing specific error messages for different failure cases. This suggestion is still valid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
41-42: Utilize File-Based Enrollment in test-client-enroll TargetThe updated commands now use
curlto fetch the enrollment JSON and then pass it to the client via--file enrollment.json. This is well aligned with the PR objective and client changes.- rm -f tls.crt tls-ca.crt tls.key data.json - [...] - poetry run nodeman_client --debug enroll + rm -f tls.crt tls-ca.crt tls.key data.json + curl -X POST --verbose --user username:password -o enrollment.json http://127.0.0.1:8080/api/v1/node + poetry run nodeman_client --debug enroll --file enrollment.jsonSuggestion: Consider parameterizing the credentials (e.g.,
username:password) or endpoint URL via Makefile variables for improved flexibility and maintainability in different testing environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(2 hunks)
🔇 Additional comments (1)
Makefile (1)
16-16: Update CLIENT_FILES Variable to Include enrollment.jsonAdding
enrollment.jsonto theCLIENT_FILESlist is consistent with the new enrollment file parsing feature in the client. This ensures that the enrollment file is also tracked for cleanup and packaging.-CLIENT_FILES= data.json tls.crt tls.key tls-ca.crt +CLIENT_FILES= data.json tls.crt tls.key tls-ca.crt enrollment.json
Summary by CodeRabbit
enrollment.jsonand modified the enrollment process in the test-client target for better data handling.