validate policy against nodes, error if not valid #2089
Conversation
this commit aims to improve the feedback of "runtime" policy errors which would only manifest when the rules are compiled to filter rules with nodes. this change will in; file-based mode load the nodes from the db and try to compile the rules on start up and return an error if they would not work as intended. database-based mode prevent a new ACL being written to the database if it does not compile with the current set of node. Fixes juanfont#2073 Fixes juanfont#2044 Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
WalkthroughThe changes enhance the Changes
Assessment against linked issues
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
Documentation and Community
|
|
@pallabpain What do you think about this? |
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- .github/workflows/test-integration.yaml (1 hunks)
- integration/cli_test.go (1 hunks)
- integration/dockertestutil/execute.go (1 hunks)
- integration/hsic/hsic.go (1 hunks)
Additional comments not posted (4)
integration/dockertestutil/execute.go (1)
65-65: Approve the change in logging level but verify impact.The change to set
HEADSCALE_LOG_LEVELto "info" is approved as it can aid in debugging. However, it's important to verify that this does not lead to excessive logging, especially in production environments, which could affect performance and log storage costs.Run the following script to monitor log file sizes and performance during peak times:
Verification successful
Logging level change is consistent but verify appropriateness for use case.
The change to set
HEADSCALE_LOG_LEVELto "info" inintegration/dockertestutil/execute.goaligns with the existing logging practices in the codebase. Ensure that this level is suitable for the intended environment, as it may increase log verbosity. Consider the impact on performance and log storage, especially in production settings.
- Check if "info" level logging is appropriate for the specific use case in
integration/dockertestutil/execute.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Monitor log file sizes and system performance. # Monitor log file growth over a 24-hour period. find /var/log -type f -name '*.log' -exec ls -lh {} \; | awk '{ print $5, $9 }' # Check system performance. top -b -n1 | head -n 5Length of output: 224
Script:
#!/bin/bash # Search for logging configurations or usages in the codebase that might be affected by the change. rg 'HEADSCALE_LOG_LEVEL' -A 5 # Check for any documentation or test files related to logging. fd -e md -e go | xargs rg 'logging|HEADSCALE_LOG_LEVEL'Length of output: 2499
integration/cli_test.go (1)
1680-1752: Review of the new test functionTestPolicyBrokenConfigCommand.The test function
TestPolicyBrokenConfigCommandis well-structured and addresses the PR objectives effectively by testing the policy command with an invalid action. Here are some specific observations and suggestions:
Error Handling and Assertions:
- The use of
assert.ErrorContainsto check for specific error messages is appropriate and ensures that the system reacts correctly to configuration errors.- It's good to see that the test checks both the failure of setting an invalid policy and the absence of any policy after the attempt, which confirms that no invalid data persists in the system.
Test Isolation and Cleanup:
- The commented out
defer scenario.Shutdown()on line 1686 should typically be enabled to ensure that resources are cleaned up after the test. If there's a specific reason for it being commented out (e.g., for debugging), it would be helpful to include a comment explaining this.Policy Definition Clarity:
- The policy is defined inline within the test. While this is acceptable for small examples, if policies become more complex or need to be reused across tests, consider defining them in a separate helper function or a test data file.
File Handling:
- Writing to a hardcoded file path (
/etc/headscale/policy.json) might lead to conflicts or require elevated permissions depending on the environment. If possible, use a temporary file or a mock file system for testing.Documentation and Comments:
- The comments within the test are clear and provide good documentation of what each step is intended to achieve. This is particularly useful in understanding the purpose of the test and the expected outcomes.
Overall, the test function is a valuable addition to the test suite, enhancing the robustness of the policy management feature by ensuring that only valid configurations are applied.
The implementation of the test function is correct and aligns with the objectives of the PR. Minor improvements could be made regarding resource cleanup and file handling to enhance the test's robustness and maintainability.
.github/workflows/test-integration.yaml (1)
40-40: Approval of the new test case addition.The addition of
TestPolicyBrokenConfigCommandis a positive step towards enhancing the robustness of policy command functionalities. It is crucial to ensure that this test covers all scenarios where policy configurations might fail, providing comprehensive feedback and preventing runtime errors.The code changes are approved.
integration/hsic/hsic.go (1)
554-554: Enhanced error reporting in theExecutemethod.The modification to include both stdout and stderr in the error message when command execution fails is a significant improvement. This change will aid in debugging and provide clearer context on what went wrong during command execution.
The code changes are approved.
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- integration/cli_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- integration/cli_test.go
Sorry, I did not get a chance to look at this. I was thinking more along the lines of validating the incoming policy bytes at the time of unmarshaling it into the ACL type. But this looks good. 👍🏼 |
this commit aims to improve the feedback of "runtime" policy errors which would only manifest when the rules are compiled to filter rules with nodes.
this change will in;
file-based mode load the nodes from the db and try to compile the rules on start up and return an error if they would not work as intended.
database-based mode prevent a new ACL being written to the database if it does not compile with the current set of node.
Fixes #2073
Fixes #2044
Summary by CodeRabbit
New Features
Bug Fixes