Add key package validation #393
Conversation
WalkthroughThe pull request introduces key package validation functionality across multiple components of the system. A new validation service is added to the Changes
Possibly related 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
CodeRabbit Configuration File (
|
|
this will be a massive performance hit if we have to add a roundtrip to the MLS validation service for every request. The good news is that it can be done concurrently and is not on the DB hot path. It might increase latency of package writes, but should not impact throughput. |
![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 (4)
pkg/api/message/service.go (2)
62-68: Initialization of the newvalidationService
The service is correctly assigned in the returnedServiceinstance. Consider adding a nil-check if you expect scenarios where validation might be optional or uninitialized.if err != nil { return nil, err } -return &Service{ +svc := &Service{ ctx: ctx, log: log, registrant: registrant, store: store, publishWorker: publishWorker, subscribeWorker: subscribeWorker, validationService: validationService, } +if svc.validationService == nil { + svc.log.Warn("validationService is nil, skipping key package validation") +} +return svc, nil
411-437: Key package validation workflow
- Checks the payload type before calling the validation service.
- Makes a synchronous request to validate the key packages and examines the result.
- Returns an
InvalidArgumentcode if validation fails.This approach is well-structured. Consider covering these lines with direct unit tests to ensure the correct error codes are returned in various edge cases (e.g., empty or multiple key packages).
pkg/mlsvalidate/service.go (1)
121-126: Consider logging validation failures for debugging.When validation fails, it would be helpful to log the failure reason for debugging purposes. This is especially important since we're silently converting errors into false results.
if !response.IsOk { + log.Debug("Key package validation failed", + zap.String("error", response.ErrorMessage)) out[i] = KeyPackageValidationResult{ IsOk: false, InstallationKey: nil, Credential: nil, Expiration: 0, }pkg/server/server.go (1)
Line range hint
102-115: Consider refactoring duplicate initialization logic.The validation service initialization is duplicated between the indexer and API server code paths. Consider extracting it into a helper method to maintain DRY principles.
+func (s *ReplicationServer) initValidationService(ctx context.Context) error { + if s.validationService != nil { + return nil + } + var err error + s.validationService, err = mlsvalidate.NewMlsValidationService( + ctx, + s.log, + s.options.MlsValidation, + ) + return err +}Then use it in both locations:
if options.Indexer.Enable { - s.validationService, err = mlsvalidate.NewMlsValidationService( - ctx, - log, - options.MlsValidation, - ) - if err != nil { - return nil, err - } + if err := s.initValidationService(ctx); err != nil { + return nil, err + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/api/message/service.go(5 hunks)pkg/mlsvalidate/interface.go(1 hunks)pkg/mlsvalidate/service.go(1 hunks)pkg/server/server.go(4 hunks)pkg/testutils/api/api.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test (Node)
🔇 Additional comments (10)
pkg/api/message/service.go (5)
12-12: New import for validation service
No issues spotted. This import is required for accessing the validation interfaces.
16-16: Import for topic management
This import is consistent with referencing constant definitions intopic. No concerns.
36-42: AddedvalidationServicefield
Introducing a dedicated validation service field is a good architectural approach that clearly segregates validation logic from other concerns. This fosters testability and maintainability.
50-50: AddedvalidationServiceparameter to constructor
Ensuring the validation service is part of the constructor extends the possibility for dependency injection, enabling better control during testing.
330-336: Conditional key package validation
Good approach to invoke enhanced validation only when the topic kind isTOPIC_KIND_KEY_PACKAGES_V1. Ensure that future expansions to other topic kinds also handle or skip validation correctly.pkg/mlsvalidate/interface.go (1)
13-13: AddedIsOkfield in the validation result
Having a dedicated boolean field clarifies success/failure state at a glance. Make sure to maintain consistent usage across all validation paths to avoid ambiguous results.pkg/testutils/api/api.go (3)
17-17: Importing mock validation service
This import effectively supports testing the newly introduced validation logic. No issues spotted.
82-82: Mock validation service creation
Instantiating this mock is an excellent way to isolate the validation logic for unit tests, aiding in comprehensive coverage.
92-92: Wiring the mock intoNewReplicationApiService
This ensures that the replication API tests will use the mock validation service, facilitating robust and isolated testing of your validation flows.pkg/server/server.go (1)
39-47: LGTM! Field placement is appropriate.The validationService field is correctly placed with other service-related fields in the ReplicationServer struct.
|
Would just be requests for uploading key packages, which should be pretty low volume. I would at some point in the project like to move the MLS Validation stuff from a RPC to some sort of embedded FFI call. There are uniffi bindings for Go, so it's possible in theory. |
Summary by CodeRabbit
New Features
IsOkflagBug Fixes
Refactor