-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change ingestion to use the API #128
Conversation
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce new methods and message types to enhance the functionality of the Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 bufapi/v1/service.protoAn unexpected error occurred while running buf. 🔧 golangci-lintlevel=error msg="Timeout exceeded: try increasing it by passing --timeout option" 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 (
|
Codecov ReportAttention: Patch coverage is
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (13)
cmd/ingest/sbom/sbom.go (3)
21-21
: Consider using an interface type for better testabilityThe
ingestServiceClient
field could benefit from using an interface type instead of the concrete implementation.Consider defining and using an interface:
type IngestServiceClient interface { IngestSBOM(context.Context, *connect.Request[apiv1.IngestSBOMRequest]) (*connect.Response[apiv1.IngestSBOMResponse], error) }
28-30
: Consider using a more descriptive flag nameThe flag name "addr" could be more descriptive, such as "server-addr" or "minefield-addr", to better indicate its purpose.
- cmd.Flags().StringVar(&o.addr, "addr", DefaultAddr, "Address of the minefield server") + cmd.Flags().StringVar(&o.addr, "server-addr", DefaultAddr, "Address of the minefield server")
Line range hint
53-55
: Make progress indicator CI/CD friendlyThe current progress indicator uses ANSI escape codes which might not work well in CI/CD environments. Consider making it configurable or detecting the environment.
+ if isInteractive := term.IsTerminal(int(os.Stdout.Fd())); isInteractive { // Clear the line by overwriting with spaces fmt.Printf("\r\033[1;36m%-80s\033[0m", " ") fmt.Printf("\r\033[1;36mIngested %d/%d SBOMs\033[0m | \033[1;34m%s\033[0m", index+1, len(result), tools.TruncateString(data.Path, 50)) + } else { + fmt.Printf("Ingested %d/%d SBOMs: %s\n", index+1, len(result), data.Path) + }api/v1/service.proto (1)
103-107
: Consider enhancing the service interface design.The current design has several limitations that might affect scalability and error handling:
- Using
google.protobuf.Empty
response type limits error reporting- No streaming support for large payloads
- No batch processing capabilities
Consider these improvements:
- Define a proper response type with error details and processing status
- Add streaming support for large payloads
- Add batch processing methods
service IngestService { - rpc IngestSBOM(IngestSBOMRequest) returns (google.protobuf.Empty) {} - rpc IngestVulnerability(IngestVulnerabilityRequest) returns (google.protobuf.Empty) {} - rpc IngestScorecard(IngestScorecardRequest) returns (google.protobuf.Empty) {} + // Single item ingestion with proper response + rpc IngestSBOM(IngestSBOMRequest) returns (IngestResponse) {} + rpc IngestVulnerability(IngestVulnerabilityRequest) returns (IngestResponse) {} + rpc IngestScorecard(IngestScorecardRequest) returns (IngestResponse) {} + + // Streaming support for large payloads + rpc IngestSBOMStream(stream IngestSBOMRequest) returns (IngestResponse) {} + rpc IngestVulnerabilityStream(stream IngestVulnerabilityRequest) returns (IngestResponse) {} + rpc IngestScorecardStream(stream IngestScorecardRequest) returns (IngestResponse) {} + + // Batch processing + rpc BatchIngest(BatchIngestRequest) returns (BatchIngestResponse) {} } + message IngestResponse { + string status = 1; + string error = 2; + } + + message BatchIngestRequest { + repeated IngestSBOMRequest sboms = 1; + repeated IngestVulnerabilityRequest vulnerabilities = 2; + repeated IngestScorecardRequest scorecards = 3; + } + + message BatchIngestResponse { + repeated IngestResponse results = 1; + }cmd/ingest/osv/osv.go (1)
19-21
: Consider adding validation for the server addressWhile the fields are well-documented, consider adding validation for the server address format during initialization to catch configuration errors early.
type options struct { storage graph.Storage addr string // Address of the minefield server + + // validateAddr validates the server address format + validateAddr() error { + if _, err := url.Parse(o.addr); err != nil { + return fmt.Errorf("invalid server address: %w", err) + } + return nil + } ingestServiceClient apiv1connect.IngestServiceClient }cmd/server/server.go (1)
63-64
: Consider implementing a more structured service registration pattern.While the current implementation works, as the number of services grows, managing them individually becomes harder to maintain.
Consider refactoring to use a more structured approach:
- Define a slice of service registrations
- Iterate through them for registration
- Makes it easier to manage service dependencies and startup order
Example implementation:
type serviceRegistration struct { name string registerFn func(service *service.Service) (string, http.Handler) } var services = []serviceRegistration{ {"Query", apiv1connect.NewQueryServiceHandler}, {"Leaderboard", apiv1connect.NewLeaderboardServiceHandler}, // ... other services {"Ingest", apiv1connect.NewIngestServiceHandler}, } // In setupServer for _, svc := range services { path, handler := svc.registerFn(newService) mux.Handle(path, handler) }api/v1/service.go (2)
292-294
: Consider enhancing the health check implementation.While the current implementation is functional, consider enhancing it to provide more meaningful health information such as:
- Storage connectivity status
- System resource metrics
- Version information
Example enhancement:
func (s *Service) Check(ctx context.Context, req *connect.Request[emptypb.Empty]) (*connect.Response[service.HealthCheckResponse], error) { - return connect.NewResponse(&service.HealthCheckResponse{Status: "ok"}), nil + status := "ok" + if err := s.storage.Ping(ctx); err != nil { + status = "storage_error" + } + return connect.NewResponse(&service.HealthCheckResponse{ + Status: status, + Version: "v1", + Timestamp: time.Now().Unix(), + }), nil }
296-318
: Consider introducing a generic ingestion helper.The three ingestion methods share similar patterns. Consider introducing a generic helper function to reduce code duplication and standardize error handling.
Example implementation:
func (s *Service) ingestHelper(ctx context.Context, data []byte, ingestFn func(context.Context, graph.Storage, []byte) error) (*connect.Response[emptypb.Empty], error) { if len(data) == 0 { return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("empty data")) } if err := ingestFn(ctx, s.storage, data); err != nil { return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("ingestion failed: %w", err)) } return connect.NewResponse(&emptypb.Empty{}), nil } // Usage example: func (s *Service) IngestSBOM(ctx context.Context, req *connect.Request[service.IngestSBOMRequest]) (*connect.Response[emptypb.Empty], error) { return s.ingestHelper(ctx, req.Msg.Sbom, ingest.SBOM) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 296-301: api/v1/service.go#L296-L301
Added lines #L296 - L301 were not covered by tests
[warning] 304-309: api/v1/service.go#L304-L309
Added lines #L304 - L309 were not covered by tests
[warning] 312-317: api/v1/service.go#L312-L317
Added lines #L312 - L317 were not covered by testscmd/ingest/scorecard/scorecard.go (5)
25-25
: Consider using HTTPS for the default server address to enhance securityThe default server address is set to use HTTP. If the server supports HTTPS, it's a good practice to use it to secure the communication between the client and the server.
Apply this diff if applicable:
const ( - DefaultAddr = "http://localhost:8089" // Default address of the minefield server + DefaultAddr = "https://localhost:8089" // Default address of the minefield server )Ensure that the server is configured to accept HTTPS connections.
Line range hint
42-42
: Correct the error message to reflect the operation being performedThe error message refers to ingesting an SBOM, but this file handles scorecards. Updating the message will prevent confusion and improve clarity.
Apply this diff to fix the error message:
return fmt.Errorf("failed to ingest SBOM: %w", err)return fmt.Errorf("failed to ingest scorecard: %w", err)
50-50
: Use a context with timeout for network requestsUsing
context.Background()
may lead to indefinite blocking if the server does not respond. Consider using a context with a timeout to prevent potential hangs.For example, you can set a timeout of 10 seconds:
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() if _, err := o.ingestServiceClient.IngestScorecard(ctx, req); err != nil { return fmt.Errorf("failed to ingest Scorecard: %w", err) }Make sure to import the
time
package:import ( "time" )
Line range hint
74-76
: Update the progress message to reflect scorecards instead of SBOMsThe
printProgress
function mentions SBOMs, but this command is ingesting scorecards. Updating the message will improve accuracy and reduce confusion.Apply this diff to correct the progress message:
func printProgress(count int, path string) string { - return fmt.Sprintf("\033[1;36mIngested %d SBOMs\033[0m | \033[1;34m%s\033[0m", count, tools.TruncateString(path, 50)) + return fmt.Sprintf("\033[1;36mIngested %d Scorecards\033[0m | \033[1;34m%s\033[0m", count, tools.TruncateString(path, 50)) }Additionally, consider using this function to print progress in your loop for consistency.
28-30
: Ensure the command-line flag description is clear and user-friendlyThe description for the
--addr
flag could be more user-centric. Providing a clearer description helps users understand the purpose of the flag.Consider rephrasing the flag description:
cmd.Flags().StringVar(&o.addr, "addr", DefaultAddr, - "Address of the minefield server") + "Address of the Minefield server to connect for ingestion")This provides more context about how the address is used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
gen/api/v1/apiv1connect/service.connect.go
is excluded by!**/gen/**
gen/api/v1/service.pb.go
is excluded by!**/*.pb.go
,!**/gen/**
📒 Files selected for processing (7)
api/v1/service.go
(2 hunks)api/v1/service.proto
(2 hunks)cmd/ingest/osv/osv.go
(2 hunks)cmd/ingest/sbom/sbom.go
(3 hunks)cmd/ingest/scorecard/scorecard.go
(3 hunks)cmd/server/server.go
(1 hunks)pkg/tools/ingest/scorecard.go
(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
api/v1/service.go
[warning] 296-301: api/v1/service.go#L296-L301
Added lines #L296 - L301 were not covered by tests
[warning] 304-309: api/v1/service.go#L304-L309
Added lines #L304 - L309 were not covered by tests
[warning] 312-317: api/v1/service.go#L312-L317
Added lines #L312 - L317 were not covered by tests
pkg/tools/ingest/scorecard.go
[warning] 48-48: pkg/tools/ingest/scorecard.go#L48
Added line #L48 was not covered by tests
🔇 Additional comments (9)
cmd/ingest/sbom/sbom.go (1)
4-4
: LGTM: Import changes are appropriate
The new imports are necessary for the client-server functionality and follow Go conventions.
Also applies to: 8-10
cmd/ingest/osv/osv.go (4)
4-4
: LGTM: Imports are appropriate for the new service-based approach
The new imports correctly include the necessary packages for connect RPC and API types.
Also applies to: 8-10
24-26
: LGTM: Well-defined default configuration
The default address is appropriately set for local development and well-documented.
28-30
: LGTM: Flag configuration follows best practices
The server address flag is properly configured with a clear description and default value.
32-37
: Verify service endpoint compatibility
Ensure that the service endpoint implements the expected IngestService interface.
✅ Verification successful
Service endpoint compatibility verified
The service endpoint compatibility is confirmed through the following evidence:
- The
IngestService
interface is properly defined in the proto file with theIngestVulnerability
RPC method - A concrete implementation exists in
api/v1/service.go
that matches the interface - The connect service handler and client are correctly generated in
gen/api/v1/apiv1connect/service.connect.go
- The service is properly wired up in
cmd/server/server.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the service implementation exists and matches the interface
ast-grep --pattern 'type IngestService interface {
$$$
IngestVulnerability(context.Context, *IngestVulnerabilityRequest) (*IngestVulnerabilityResponse, error)
$$$
}'
# Check for the service implementation
ast-grep --pattern 'func (s *Service) IngestVulnerability(ctx context.Context, req *IngestVulnerabilityRequest) (*IngestVulnerabilityResponse, error) {
$$$
}'
Length of output: 336
Script:
#!/bin/bash
# Let's try a different approach to find the service interface and implementation
# Search for IngestService interface definition
rg -A 5 "type IngestService interface"
# Search for IngestVulnerability method implementation
rg -A 5 "func.*IngestVulnerability.*context\.Context"
# Check for connect service definitions
rg -A 5 "IngestServiceHandler"
# Look for proto definitions that might define the service
fd -e proto | xargs cat
Length of output: 8444
pkg/tools/ingest/scorecard.go (1)
Line range hint 48-108
: Consider adapting this function for the new API-based approach.
Given that this PR's objective is to "Change ingestion to use the API", this function still directly interacts with storage. Consider refactoring it to align with the new API-based approach mentioned in the PR description.
Suggestions:
- Consider moving the storage interaction logic to the API service layer
- This function could be refactored to return processed data that the API service can then store
- Document why this component hasn't been migrated to the API-based approach if this is intentional
Let's check for other components that might have been migrated to the API:
#!/bin/bash
# Description: Look for API-related changes in the codebase
# Search for new API service definitions
rg -l "IngestScorecard"
# Search for similar ingestion functions that might have been migrated
ast-grep --pattern 'func $_(storage graph.Storage, data []byte)'
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 48-48: pkg/tools/ingest/scorecard.go#L48
Added line #L48 was not covered by tests
cmd/server/server.go (1)
63-64
: LGTM! Verify IngestService interface implementation.
The IngestService handler registration follows the established pattern and is correctly placed before server initialization.
Let's verify that the Service struct properly implements the IngestService interface:
✅ Verification successful
Interface implementation verified successfully
Based on the search results, the Service struct in api/v1/service.go
correctly implements all three required ingest methods:
IngestSBOM
IngestVulnerability
IngestScorecard
Each method follows the connect-go pattern with proper context and request/response handling. The implementation is complete and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify IngestService interface implementation in the Service struct
# Check for required method implementations
ast-grep --pattern 'type Service struct {
$$$
}
$$$
func (s *Service) IngestSBOM($$$) ($$$) {
$$$
}
func (s *Service) IngestVulnerability($$$) ($$$) {
$$$
}
func (s *Service) IngestScorecard($$$) ($$$) {
$$$
}'
Length of output: 224
Script:
#!/bin/bash
# Let's try a different approach to verify the interface implementation
# First, let's find the IngestService interface definition
ast-grep --pattern 'type IngestService interface {
$$$
}'
# Then, let's search for the Service struct and its methods separately
echo "=== Service struct and methods ==="
rg "type Service struct" -A 5
echo ""
rg "func \(s \*Service\) Ingest" -A 5
Length of output: 1688
Script:
#!/bin/bash
# Let's verify the interface definition in the proto/service files
echo "=== Proto Service Definition ==="
rg "service IngestService" -A 10 --type proto
# Also check the generated connect service interface
echo "=== Generated Connect Service Interface ==="
rg "type IngestService interface" -A 10 --type go
Length of output: 301
api/v1/service.go (1)
296-302
:
Several improvements needed for ingestion methods.
The new ingestion methods need improvements in the following areas:
- Input validation
- Error handling
- Context propagation
- Test coverage
Here's a script to verify the test coverage:
Suggested improvements:
func (s *Service) IngestSBOM(ctx context.Context, req *connect.Request[service.IngestSBOMRequest]) (*connect.Response[emptypb.Empty], error) {
+ if len(req.Msg.Sbom) == 0 {
+ return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("empty SBOM data"))
+ }
- err := ingest.SBOM(s.storage, req.Msg.Sbom)
+ err := ingest.SBOM(ctx, s.storage, req.Msg.Sbom)
if err != nil {
- return nil, err
+ return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("failed to ingest SBOM: %w", err))
}
return connect.NewResponse(&emptypb.Empty{}), nil
}
Similar improvements should be applied to IngestVulnerability
and IngestScorecard
methods.
Would you like me to:
- Generate comprehensive test cases for these methods?
- Create a GitHub issue to track the test coverage task?
Also applies to: 304-310, 312-318
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 296-301: api/v1/service.go#L296-L301
Added lines #L296 - L301 were not covered by tests
cmd/ingest/scorecard/scorecard.go (1)
33-38
: Handle potential errors when creating the ingest service client
While the NewIngestServiceClient
function does not return an error, ensuring the client is properly initialized and considering any potential setup issues can improve robustness.
Ensure that the client initialization does not require additional error handling. If there's a possibility of misconfiguration, consider adding checks or logging to assist in debugging.
Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
50f86c9
to
6934367
Compare
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Bug Fixes