Fix 764 #2093
Fix 764 #2093
Conversation
When a zero time value is loaded from JSON or a DB in a way that assigns it the local timezone, it does not roudtrip in JSON as a value for which IsZero returns true. This causes KeyExpiry to be treated as a far past value instead of a nilish value. See golang/go#57040
|
Thank you, much appreciated, for this change I would need a integration test that first verify the problem, so something that when ran in sqlite/postgres mode returns pass/fail respectively, and then when this change is applied change to pass/pass. If you have time to look at it would be great, or else I can see what I have time to do next week. I recon that a fix for this issue also would merit a changelog entry. |
|
The unit test I added would fail prior to this change, and now passes. It really actually has nothing to do with the database, but rather the |
|
Integration tests to ensure full roundtrip via both drivers would be necessary. Both for this change and to ensure regressions, we have had libraries introduce silent failures before. |
|
I guess it really depends on the timezone the tests run in - the existing integration tests should trip it as long as the timezone the headscale server runs in is non-UTC. I suspect on the integration tests the timezone is set to UTC and hence doesn't happen. I can take a look next week and see how hard it would be to introduce that, I'm not familiar with your integration test setup. |
WalkthroughThe changes encompass the addition of new test cases and functions to enhance the testing framework, modifications to timestamp handling for consistency in UTC, and improvements in configuration options for PostgreSQL integration. The updates address a specific bug related to client activation with a PostgreSQL backend while broadening test coverage for time-sensitive operations across different time zones. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Headscale
participant PostgreSQL
Client->>Headscale: Authenticate with payload
Headscale->>PostgreSQL: Query for client data
PostgreSQL-->>Headscale: Return client data
Headscale-->>Client: Respond with authentication result
Client->>Headscale: Send additional payload
Headscale->>PostgreSQL: Validate key expiry
PostgreSQL-->>Headscale: Return key expiry status
Headscale-->>Client: Provide key expiry information
Assessment against linked issues
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
Documentation and Community
|
|
OK, added an integration test for this, as I suspected it was enough to ensure that the headscale container was in an alternate timezone using existing test scenarios. I added a test method that forces the container into an alternate TZ and runs it that fails without this change. Also found that not all integration tests respected the env variable to use postgresql instead of sqlite, so fixed that as well, and added changelog entries. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- .github/workflows/test-integration.yaml (1 hunks)
- CHANGELOG.md (1 hunks)
- hscontrol/mapper/tail.go (2 hunks)
- hscontrol/mapper/tail_test.go (2 hunks)
- integration/general_test.go (2 hunks)
- integration/hsic/hsic.go (1 hunks)
- integration/run.sh (1 hunks)
- integration/scenario.go (2 hunks)
Additional comments not posted (8)
hscontrol/mapper/tail.go (1)
96-96: Approve the conversion of timestamps to UTC.The changes to convert the
KeyExpiryandCreatedtimestamps to UTC are crucial for maintaining consistency and avoiding potential issues related to time zone discrepancies. This adjustment ensures that time-related data is handled robustly and is a best practice for systems that operate across multiple time zones.Also applies to: 105-105
.github/workflows/test-integration.yaml (1)
48-48: Approve the addition of the new test case.The inclusion of
TestEphemeralInAlternateTimezonein the test matrix is a valuable addition, enhancing the robustness of the testing framework by ensuring that the application behaves correctly across different time zones. This is particularly important for applications that rely on time-sensitive operations.hscontrol/mapper/tail_test.go (2)
4-4: Approved: Import statement for JSON operations.The addition of the
encoding/jsonpackage is necessary for the JSON operations performed in the new test function.
210-273: Approved: New test functionTestNodeExpiry.The function
TestNodeExpiryis well-implemented and enhances the test coverage for the expiry logic in node management. It effectively handles various scenarios for theExpiryfield of aNodeobject. Consider adding more test cases if other expiry scenarios are identified in the future.integration/general_test.go (3)
218-219: Refactor: Utilize new test function for flexibility.The modification of
TestEphemeralto usetestEphemeralWithOptionsenhances the flexibility of the test by allowing variadic parameters to be passed. This change supports different configurations and is a good use of function abstraction to avoid code duplication.
221-223: New Test Function: Verify timezone handling.
TestEphemeralInAlternateTimezoneis a new test function that specifically checks the ephemeral functionality in a different timezone ("America/Los_Angeles"). This is a valuable addition as it tests the system's behavior under timezone variations, ensuring robustness across different environments.
Line range hint
225-238: New Utility Function: Enhance test configurability.The introduction of
testEphemeralWithOptionsallows for more dynamic configuration of tests through variadic parameters. This function is used to set up scenarios with specific options, making the tests more adaptable and easier to extend with additional parameters in the future.Also applies to: 238-238
CHANGELOG.md (1)
73-74: Changelog Update: Document significant fixes and enhancements.The new entries in the changelog correctly document the fix for registering nodes using preauthkeys with a PostgreSQL database in a non-UTC timezone, addressing issue #764. Additionally, the enhancement to ensure integration tests cover PostgreSQL for all scenarios is well-noted, reflecting a commitment to improve test coverage and reliability.
When a zero time value is loaded from JSON or a DB in a way that assigns it the local timezone, it does not roudtrip in JSON as a value for which IsZero returns true. This causes KeyExpiry to be treated as a far past value instead of a nilish value.
See golang/go#57040
Fixes #764
Summary by CodeRabbit
New Features
Bug Fixes
Documentation