Improve version checking system #506
Conversation
WalkthroughThis pull request refactors version claim validation by introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant RS as ReplicationServer
participant AS as APIServer
participant RV as RegistryVerifier
participant CV as ClaimValidator
RS->>AS: startAPIServer(ctx, serverVersion)
AS->>RV: NewRegistryVerifier(..., serverVersion)
RV->>CV: NewClaimValidator(serverVersion)
CV-->>RV: Return ClaimValidator instance or error
RV->>CV: ValidateVersionClaimIsCompatible(claims)
CV-->>RV: Return compatibility result (success/error)
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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 (
|
mkysel
force-pushed
the
mkysel/improveClaims
branch
from
2c01f95 to
8912326
Compare
![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
🧹 Nitpick comments (5)
pkg/authn/claims.go (3)
13-15: Add documentation for ClaimValidator type.The type needs documentation explaining its purpose and responsibility.
Add this documentation:
+// ClaimValidator handles version compatibility checks using semver constraints. +// It ensures that client versions are compatible with the server version. type ClaimValidator struct { constraint semver.Constraints }
17-35: Add documentation and improve error messages in NewClaimValidator.The constructor needs documentation and more descriptive error messages.
Consider these improvements:
+// NewClaimValidator creates a new validator that ensures version compatibility +// using the caret range comparison (^) with the given server version. +// It returns an error if the server version is nil or invalid. func NewClaimValidator(serverVersion *semver.Version) (*ClaimValidator, error) { if serverVersion == nil { - return nil, fmt.Errorf("serverVersion is nil") + return nil, fmt.Errorf("server version cannot be nil") } sanitizedVersion, err := serverVersion.SetPrerelease("") if err != nil { - return nil, err + return nil, fmt.Errorf("failed to sanitize server version: %w", err) } // https://github.com/Masterminds/semver?tab=readme-ov-file#caret-range-comparisons-major constraintStr := fmt.Sprintf("^%s", sanitizedVersion.String()) constraint, err := semver.NewConstraint(constraintStr) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create version constraint: %w", err) } return &ClaimValidator{constraint: *constraint}, nil }
36-53: Add documentation for ValidateVersionClaimIsCompatible method.The method needs documentation explaining its behavior and return values.
Add this documentation:
+// ValidateVersionClaimIsCompatible checks if the version in the claims is compatible +// with the server version using semver caret range comparison. +// It returns nil if the claims version is nil or compatible, and an error otherwise. func (cv *ClaimValidator) ValidateVersionClaimIsCompatible(claims *XmtpdClaims) error {pkg/authn/tokenFactory_test.go (1)
29-36: Add more test cases for version compatibility.The current test cases could be expanded to cover more edge cases.
Add these test cases:
tests := []struct { name string version string + wantErr bool }{ {"current-ish", "0.1.3"}, {"future-ish", "11.7.3"}, {"with-git-describe", "0.1.0-15-gdeadbeef"}, + {"invalid-version", "invalid", true}, + {"empty-version", "", true}, + {"pre-release", "1.0.0-alpha", false}, }pkg/server/server.go (1)
175-176: Consider adding nil check for serverVersion.The
serverVersionparameter should be validated before being used inNewRegistryVerifierto prevent potential nil pointer dereferences.func startAPIServer( ctx context.Context, log *zap.Logger, options config.ServerOptions, s *ReplicationServer, writerDB *sql.DB, blockchainPublisher blockchain.IBlockchainPublisher, listenAddress string, serverVersion *semver.Version, ) error { var err error + if serverVersion == nil { + return fmt.Errorf("serverVersion cannot be nil") + }Also applies to: 248-252
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pkg/authn/claims.go(2 hunks)pkg/authn/claims_test.go(5 hunks)pkg/authn/signingMethod_test.go(5 hunks)pkg/authn/tokenFactory_test.go(3 hunks)pkg/authn/verifier.go(3 hunks)pkg/authn/verifier_test.go(10 hunks)pkg/server/server.go(3 hunks)pkg/server/server_test.go(1 hunks)pkg/testutils/api/api.go(1 hunks)pkg/testutils/versioning.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test (Node)
🔇 Additional comments (9)
pkg/authn/signingMethod_test.go (1)
1-76: LGTM! Package qualification changes look good.The changes to use qualified package names improve code organization and clarity.
pkg/authn/verifier.go (2)
28-39: LGTM! Clean error handling in the constructor.The updated constructor properly initializes the version validator and handles potential errors. The use of semver for version handling adds type safety and standardization.
118-118: LGTM! Good separation of concerns.Delegating version validation to a dedicated validator improves modularity and maintainability.
pkg/authn/claims_test.go (1)
66-96: LGTM! Comprehensive version compatibility testing.Excellent test coverage including:
- Current version compatibility
- Pre-release and metadata handling
- Future version incompatibility
- Specific version checks (0.1.3, 0.1.4)
pkg/testutils/api/api.go (1)
118-123: LGTM! Proper error handling in verifier initialization.The changes correctly integrate version checking and handle potential initialization errors.
pkg/authn/verifier_test.go (1)
24-37: LGTM! Well-structured test helper with proper error handling.The buildVerifier function properly integrates version checking and handles initialization errors.
pkg/server/server.go (2)
54-63: LGTM! Good addition of version compatibility checks.The addition of
serverVersionparameter and its propagation to dependent components enhances version compatibility validation.
248-255: LGTM! Good addition of error handling.The error handling for JWT verifier initialization is a good improvement.
pkg/server/server_test.go (1)
78-78: LGTM! Good test coverage with version information.The test setup correctly uses
testutils.GetLatestVersion(t)to provide version information, ensuring proper testing of the version checking system.
| func GetLatestVersion(t *testing.T) *semver.Version { | ||
| tag := GetLatestTag(t) | ||
| v, err := semver.NewVersion(tag) | ||
| require.NoError(t, err) | ||
|
|
||
| return v | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for version parsing in GetLatestVersion.
The function should validate that the tag can be parsed into a valid semver version.
Consider this improvement:
func GetLatestVersion(t *testing.T) *semver.Version {
tag := GetLatestTag(t)
+ // Remove 'v' prefix if present for semver parsing
+ tag = strings.TrimPrefix(tag, "v")
+
v, err := semver.NewVersion(tag)
- require.NoError(t, err)
+ require.NoError(t, err, "Failed to parse version from tag: %s", tag)
return v
}📝 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.
| func GetLatestVersion(t *testing.T) *semver.Version { | |
| tag := GetLatestTag(t) | |
| v, err := semver.NewVersion(tag) | |
| require.NoError(t, err) | |
| return v | |
| } | |
| func GetLatestVersion(t *testing.T) *semver.Version { | |
| tag := GetLatestTag(t) | |
| // Remove 'v' prefix if present for semver parsing | |
| tag = strings.TrimPrefix(tag, "v") | |
| v, err := semver.NewVersion(tag) | |
| require.NoError(t, err, "Failed to parse version from tag: %s", tag) | |
| return v | |
| } |
| func GetLatestTag(t *testing.T) string { | ||
| // Prepare the command | ||
| cmd := exec.Command("git", "describe", "--tags", "--abbrev=0") | ||
|
|
||
| // Capture the output | ||
| var out bytes.Buffer | ||
| cmd.Stdout = &out | ||
| cmd.Stderr = &out | ||
|
|
||
| // Run the command | ||
| err := cmd.Run() | ||
| require.NoError(t, err, out.String()) | ||
| return strings.TrimSpace(out.String()) | ||
| } |
There was a problem hiding this comment.
Improve error handling and security in GetLatestTag.
The function has several potential issues:
- Command execution in tests can be unreliable if git is not available
- No validation of stderr output separately from stdout
- No validation that the tag follows semver format before returning
Consider these improvements:
func GetLatestTag(t *testing.T) string {
+ // Ensure git is available
+ if _, err := exec.LookPath("git"); err != nil {
+ t.Skip("git not available")
+ }
+
// Prepare the command
cmd := exec.Command("git", "describe", "--tags", "--abbrev=0")
// Capture the output
- var out bytes.Buffer
+ var stdout, stderr bytes.Buffer
cmd.Stdout = &out
- cmd.Stderr = &out
+ cmd.Stderr = &stderr
// Run the command
err := cmd.Run()
- require.NoError(t, err, out.String())
- return strings.TrimSpace(out.String())
+ if err != nil {
+ t.Fatalf("Failed to get latest tag: %v\nStderr: %s", err, stderr.String())
+ }
+
+ tag := strings.TrimSpace(stdout.String())
+ // Validate tag format
+ if !strings.HasPrefix(tag, "v") {
+ t.Fatalf("Invalid tag format: %s", tag)
+ }
+ return tag
}Committable suggestion skipped: line range outside the PR's diff.
| "future-patch-accepts-us", | ||
| currentVersion, | ||
| currentVersion.IncPatch(), | ||
| true, | ||
| }, |
There was a problem hiding this comment.
Fix incorrect test expectation.
The test case "future-patch-accepts-us" expects an error (wantErr: true) when the server is on a future patch version. This seems incorrect as patch version differences should be backward compatible according to semver.
Apply this fix:
- "wantErr": true,
+ "wantErr": false,📝 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.
| "future-patch-accepts-us", | |
| currentVersion, | |
| currentVersion.IncPatch(), | |
| true, | |
| }, | |
| { | |
| "future-patch-accepts-us", | |
| currentVersion, | |
| currentVersion.IncPatch(), | |
| wantErr: false, | |
| }, |
| "future-patch-accepts-us", | |
| currentVersion, | |
| currentVersion.IncPatch(), | |
| true, | |
| }, | |
| "future-patch-accepts-us", | |
| currentVersion, | |
| currentVersion.IncPatch(), | |
| false, | |
| }, |
| } | ||
|
|
||
| c, err := semver.NewConstraint(XMTPD_COMPATIBLE_VERSION_CONSTRAINT) | ||
| // https://github.com/Masterminds/semver?tab=readme-ov-file#caret-range-comparisons-major | ||
| constraintStr := fmt.Sprintf("^%s", sanitizedVersion.String()) |
There was a problem hiding this comment.
Probably in the future we might want to move this constraint on-chain to the XMTP Node Registry.
We were depending on a hardcoded value that does not automatically update when we release.
Instead, determine the version internally.
Add more tests.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores