Add Zero Touch Satellite Registration #54
Conversation
WalkthroughThe pull request introduces significant modifications across multiple files, focusing on enhancing the data models, API routes, and SQL queries related to satellite and group management. Key changes include the addition of new structures for groups and robot accounts, the introduction of new API handlers for satellite registration and group synchronization, and the restructuring of SQL schemas to accommodate new fields. Additionally, several obsolete files related to image and label management have been removed, reflecting a shift in functionality. The configuration management has also been improved with new parameters and error handling enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant Satellite
participant GroundControl
participant Database
Satellite->>GroundControl: Register Satellite
GroundControl->>Database: Create Satellite Entry
Database-->>GroundControl: Confirmation
GroundControl-->>Satellite: Registration Success
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range and nitpick comments (9)
ground-control/sql/schema/012_robot_projects.sql (1)
1-10: Summary: New many-to-many relationship table addedThis migration introduces a
robot_projectstable, establishing a many-to-many relationship between robots and projects. The implementation is correct and follows best practices for SQL schema design.Key points:
- Proper foreign key constraints with CASCADE delete.
- Composite primary key ensures uniqueness.
- Both Up and Down migrations are present and correct.
Impact on the system:
- Enables associating multiple robots with multiple projects and vice versa.
- Provides flexibility in managing robot-project relationships.
- Ensures referential integrity with cascading deletes.
As the system evolves, consider the following:
- Monitor query performance, especially for project-related queries, and add indexes if needed.
- If you need to track when robot-project associations are made, consider adding a timestamp column.
- Ensure that application logic is updated to utilize this new relationship table when managing robots and projects.
ground-control/sql/schema/011_projects.sql (2)
1-1: Remove unnecessary empty linesConsider removing the empty lines at the beginning and end of the file to keep it concise.
Apply this diff to remove unnecessary empty lines:
- -- +goose Up +-- +goose Up CREATE TABLE projects ( id SERIAL PRIMARY KEY, projects VARCHAR(255) NOT NULL, created_at TIMESTAMP DEFAULT NOW() NOT NULL, updated_at TIMESTAMP DEFAULT NOW() NOT NULL ); -- +goose Down DROP TABLE projects; -Also applies to: 13-13
4-9: Clarify the purpose of the projects table and consider additional columnsThe current schema for the
projectstable is quite minimal. Could you provide more context on the purpose of this table and how it fits into the overall database schema?Depending on the requirements, you might want to consider adding more columns to store project-specific information. Some suggestions:
description: TEXTstatus: VARCHAR(50)owner_id: INTEGER (foreign key to a users table, if applicable)start_date: DATEend_date: DATEAlso, consider how this table will relate to other tables in your schema. Will there be foreign keys referencing this table from other tables?
ground-control/sql/schema/010_robots.sql (2)
3-11: Consider adding a comment to explain the table's purposeWhile the table creation syntax is correct, it would be helpful to add a comment explaining the purpose of the
robotstable and its relation to Zero Touch Satellite Registration. This will improve the maintainability of the schema.Consider adding a comment like this before the CREATE TABLE statement:
-- This table stores information about robots used in Zero Touch Satellite Registration
13-14: Consider data preservation in the "Down" migrationThe current "Down" migration simply drops the
robotstable. Depending on your requirements, you might want to consider preserving the data before dropping the table, especially if this table will be used in a production environment.If data preservation is necessary, consider adding a step to backup the data before dropping the table:
-- +goose Down CREATE TABLE robots_backup AS SELECT * FROM robots; DROP TABLE robots;This way, you can recover the data if needed after rolling back the migration.
ground-control/internal/database/models.go (2)
45-50: Consider renaming the 'Projects' fieldThe
Projectstruct looks good overall, but theProjectsfield name is plural, which is unusual for a single project entity. Consider renaming it toNameorProjectNamefor clarity.type Project struct { ID int32 - Projects string + Name string CreatedAt time.Time UpdatedAt time.Time }
61-64: Consider adding timestamp fields to RobotProjectThe
RobotProjectstruct looks good as a join table for the many-to-many relationship between Robot and Project. However, for consistency with other structs in this file, consider addingCreatedAtandUpdatedAtfields.type RobotProject struct { RobotID int32 ProjectID int32 + CreatedAt time.Time + UpdatedAt time.Time }ground-control/internal/server/routes.go (1)
35-36: LGTM. Consider removing the commented-out route.The new route for listing satellites follows RESTful conventions and is consistent with other handlers.
Remove the commented-out route
/satellites/createas it appears to be redundant with the/satellites/registerroute:- // r.HandleFunc("/satellites/create", s.addSatelliteHandler).Methods("POST") r.HandleFunc("/satellites/list", s.listSatelliteHandler).Methods("GET")ground-control/internal/server/handlers.go (1)
225-250: Clarify the purpose and differences betweenregisterSatelliteHandlerandztrHandler.Both handlers appear to perform the same operation. If they are intended for different endpoints or have subtle differences not reflected in the current code, consider adding comments to clarify their purposes. Otherwise, consider combining them.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (10)
- dagger.json (1 hunks)
- go.mod (6 hunks)
- ground-control/internal/database/models.go (1 hunks)
- ground-control/internal/server/handlers.go (2 hunks)
- ground-control/internal/server/routes.go (1 hunks)
- ground-control/sql/schema/001_satellites.sql (1 hunks)
- ground-control/sql/schema/006_images.sql (1 hunks)
- ground-control/sql/schema/010_robots.sql (1 hunks)
- ground-control/sql/schema/011_projects.sql (1 hunks)
- ground-control/sql/schema/012_robot_projects.sql (1 hunks)
Files skipped from review due to trivial changes (2)
- ground-control/sql/schema/001_satellites.sql
- ground-control/sql/schema/006_images.sql
Additional comments not posted (19)
dagger.json (1)
5-5: LGTM. Verify impact of Dagger engine update.The update of the Dagger engine version from v0.13.1 to v0.13.3 looks good. This minor version update likely includes bug fixes and small improvements.
To ensure this update doesn't introduce any unexpected changes, please:
- Review the changelog for versions v0.13.2 and v0.13.3 to be aware of any changes that might affect your project.
- Test the project with this new engine version to confirm compatibility.
Verification successful
Dagger engine version update verified.
The update from v0.13.1 to v0.13.3 includes minor additions and bug fixes. No significant changes are expected to impact the project.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Fetch the changelog for Dagger versions v0.13.2 and v0.13.3 # Test: Retrieve changelogs for v0.13.2 and v0.13.3 echo "Changelog for v0.13.2:" gh release view v0.13.2 -R dagger/dagger echo "\nChangelog for v0.13.3:" gh release view v0.13.3 -R dagger/daggerLength of output: 2188
ground-control/sql/schema/012_robot_projects.sql (1)
9-10: LGTM! Down migration is correct.The Down migration properly reverses the changes made in the Up migration by dropping the
robot_projectstable. This is sufficient, as the foreign key constraints with CASCADE delete ensure that no orphaned records will be left in other tables.ground-control/sql/schema/011_projects.sql (1)
11-12: Down migration looks goodThe Down migration correctly drops the
projectstable, which matches the table created in the Up migration.ground-control/sql/schema/010_robots.sql (3)
1-15: LGTM: Well-structured migration fileThe overall structure of the migration file is well-organized with clear "Up" and "Down" sections. The formatting and indentation are consistent, which enhances readability.
7-8: ReconsiderON DELETE SET NULLand verifysatellitestable existence
The
ON DELETE SET NULLbehavior forsatellite_idmeans that if a satellite is deleted, the robot will remain in the database but without a satellite association. Consider if this is the desired behavior or ifON DELETE CASCADEwould be more appropriate.This schema assumes the existence of a
satellitestable. Please ensure that thesatellitestable is created before this migration runs.Run the following script to verify the existence of the
satellitestable:
6-6: Review security implications of storingsecretas plain textThe
secretcolumn is currently defined asVARCHAR(255). Consider whether this sensitive information should be encrypted or hashed before storage. Also, evaluate if the length of 255 characters is sufficient for your use case.To ensure this is the only instance of potentially unsecured secret storage, run the following script:
ground-control/internal/database/models.go (2)
Line range hint
1-3: Caution: This is a generated filePlease note that this file is generated by sqlc and should not be edited manually. Any changes to the database schema should be made in the SQL files used by sqlc to generate this code.
To ensure this file is properly generated, run the following script:
#!/bin/bash # Description: Verify the presence of sqlc.yaml and SQL files # Test: Check for sqlc.yaml if [ -f "sqlc.yaml" ]; then echo "sqlc.yaml found" else echo "sqlc.yaml not found" fi # Test: Check for SQL files sql_files=$(fd -e sql) if [ -n "$sql_files" ]; then echo "SQL files found:" echo "$sql_files" else echo "No SQL files found" fi
52-59: Address potential issues with the Robot structThe
Robotstruct looks good overall, but there are two potential issues to address:
The
Secretfield might contain sensitive information. Consider implementing additional protection or encryption for this field.The
sqlpackage is not imported, which will cause a compilation error for thesql.NullInt32type.To resolve the compilation error, add the following import at the top of the file:
import ( "time" + "database/sql" )To ensure the
Secretfield is properly handled, run the following script:ground-control/internal/server/routes.go (4)
37-38: LGTM. Ensure proper error handling for non-existent satellites.The routes for getting and deleting a satellite by ID follow RESTful conventions and use appropriate HTTP methods. The handler names are clear and consistent.
Ensure that the handlers implement proper error handling, especially for cases where the satellite ID doesn't exist. Let's check if there's any standard error handling pattern in the codebase:
#!/bin/bash # Description: Check for error handling patterns in satellite-related handlers # Test: Search for error handling in satellite handlers rg --type go 'func \(s \*Server\) (get|delete)SatelliteByID' -A 10 ground-control/internal/server/
39-41: LGTM. Consider implementing GET method and restructuring routes.The routes for assigning and removing images from satellites follow RESTful conventions and use appropriate HTTP methods. The handler names are clear and consistent.
- Implement the commented-out GET method to retrieve images for a satellite.
- Consider restructuring the routes to better follow RESTful conventions:
- // r.HandleFunc("/satellites/images", s.GetImagesForSatellite).Methods("GET") - r.HandleFunc("/satellites/image", s.assignImageToSatellite).Methods("POST") - r.HandleFunc("/satellites/image", s.removeImageFromSatellite).Methods("DELETE") + r.HandleFunc("/satellites/{satellite}/images", s.GetImagesForSatellite).Methods("GET") + r.HandleFunc("/satellites/{satellite}/images", s.assignImageToSatellite).Methods("POST") + r.HandleFunc("/satellites/{satellite}/images/{image}", s.removeImageFromSatellite).Methods("DELETE")This structure more clearly represents the relationship between satellites and their images, and allows for more specific operations on individual images.
Let's check if there are any similar RESTful patterns already in use in the codebase:
#!/bin/bash # Description: Check for nested resource patterns in routes # Test: Search for nested resource patterns in route definitions rg --type go 'r\.HandleFunc\("/.+/.+/.+"' ground-control/internal/server/routes.go
33-41: Overall good additions, with some suggestions for improvementThe new routes for satellite management are a good addition to the API. They generally follow RESTful conventions and are consistent with the existing codebase. Here are some overall recommendations:
- Ensure that all new routes have proper authentication and authorization checks.
- Consider implementing pagination for the
/satellites/listendpoint to handle potentially large numbers of satellites.- Implement proper error handling and validation for all new handlers.
- Add comprehensive unit tests for each new handler.
- Update API documentation to reflect these new endpoints and their usage.
To improve the overall architecture and maintainability of the codebase:
- Consider grouping related routes under a common prefix (e.g.,
/api/v1/satellites/) to allow for easier versioning and organization.- Implement a middleware for common operations like authentication, logging, and error handling to keep the handlers focused on their primary logic.
- Consider using a dependency injection pattern for the
Serverstruct to make it easier to test and mock dependencies.Let's check if there's any existing API documentation that needs to be updated:
#!/bin/bash # Description: Check for API documentation files # Test: Search for API documentation files fd -e md -e yaml -e json 'api|swagger|openapi' ground-control/
33-33: LGTM. Consider adding authentication middleware.The new route for registering satellites follows RESTful conventions and naming consistency with other handlers. However, ensure that appropriate authentication and authorization checks are implemented in the handler or middleware to prevent unauthorized satellite registrations.
Let's verify if authentication middleware is applied elsewhere in the file:
go.mod (5)
Line range hint
1-5: LGTM: Module and Go version declarations are up-to-date.The module name, Go version (1.22.3), and toolchain version (1.22.4) are all current and appropriate. This ensures that the project is using the latest features and security updates from Go.
408-414: LGTM: Core Go package updates look good, but some might have newer versions available.The following core Go packages have been updated:
- golang.org/x/crypto to v0.27.0
- golang.org/x/net to v0.29.0
- golang.org/x/sys to v0.25.0
- golang.org/x/term to v0.24.0
- golang.org/x/text to v0.18.0
These updates are crucial for security and performance. However, it's worth checking if even newer versions are available for some packages, particularly crypto and text, which seem to be slightly behind the others.
To verify if newer versions are available, please run the following script:
#!/bin/bash # Description: Check for the latest versions of core Go packages # Test: Compare current versions with the latest available versions for pkg in "crypto" "net" "sys" "term" "text"; do echo "Checking golang.org/x/$pkg:" current_version=$(grep "golang.org/x/$pkg" go.mod | awk '{print $2}') latest_version=$(go list -m -versions golang.org/x/$pkg | awk '{print $NF}') echo "Current: $current_version, Latest: $latest_version" if [ "$current_version" != "$latest_version" ]; then echo "Newer version available" else echo "Up to date" fi echo done
420-422: LGTM: Google package updates look good, but keep an eye on genproto packages.The following Google packages have been updated:
- google.golang.org/genproto/googleapis/api to v0.0.0-20240814211410-ddb44dafa142
- google.golang.org/genproto/googleapis/rpc to v0.0.0-20240814211410-ddb44dafa142
- google.golang.org/grpc to v1.65.0
The gRPC update to a stable version is good. The genproto packages are using specific commit hashes, which is common but may make version tracking more difficult. Consider monitoring these packages for stable releases in the future.
To verify the status of these packages, please run the following script:
#!/bin/bash # Description: Check for stable releases of genproto packages and latest gRPC version # Test: Check for stable releases of genproto packages for pkg in "api" "rpc"; do echo "Checking google.golang.org/genproto/googleapis/$pkg:" go list -m -versions google.golang.org/genproto/googleapis/$pkg | grep -v "-" done # Test: Check latest gRPC version echo "Checking latest gRPC version:" go list -m -versions google.golang.org/grpc | awk '{print $NF}'
Line range hint
466-474: Note the use of replace directives for OpenTelemetry packages.The file includes several replace directives for OpenTelemetry packages:
- go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc
- go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp
- go.opentelemetry.io/otel/log
- go.opentelemetry.io/otel/sdk/log
These directives ensure specific versions or commit hashes are used, which can be helpful for consistency. However, this approach may prevent automatic updates to these packages.
To keep these packages up-to-date, periodically run the following script to check for newer versions:
#!/bin/bash # Description: Check for newer versions of replaced OpenTelemetry packages # Test: Compare current versions with the latest available versions for pkg in "otel/exporters/otlp/otlplog/otlploggrpc" "otel/exporters/otlp/otlplog/otlploghttp" "otel/log" "otel/sdk/log"; do echo "Checking go.opentelemetry.io/$pkg:" current_version=$(grep "go.opentelemetry.io/$pkg" go.mod | awk '{print $4}') latest_version=$(go list -m -versions go.opentelemetry.io/$pkg | awk '{print $NF}') echo "Current: $current_version, Latest: $latest_version" if [ "$current_version" != "$latest_version" ]; then echo "Newer version available" else echo "Up to date" fi echo done
12-12: LGTM: Dependency updates look good, but ensure thorough testing.The following key dependencies have been updated:
- golang.org/x/sync to v0.8.0
- golang.org/x/oauth2 to v0.22.0
- github.com/adrg/xdg to v0.5.0
- github.com/cespare/xxhash/v2 to v2.3.0
- github.com/grpc-ecosystem/grpc-gateway/v2 to v2.22.0
These updates likely include important bug fixes, performance improvements, and security patches. However, it's crucial to thoroughly test the project with these new versions to ensure no breaking changes have been introduced.
To verify the impact of these changes, please run the following script:
Also applies to: 23-23, 67-67, 121-121, 223-223
ground-control/internal/server/handlers.go (2)
238-240: Handle the case wherereq.Namemight be empty.Similar to the previous comment, validate
Namebefore using it.Use the same script as above to ensure
Nameis validated.
211-213: Handle the case wherereq.Namemight be empty.If
Nameis a required field for creating a satellite, ensure you validate it before proceeding.Run the following script to check if
Nameis validated elsewhere:
bupd
changed the title
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (44)
ground-control/sql/queries/satellite_labels.sql (1)
1-4: Add a comment explaining the query's purposeWhile the query is straightforward, it would be beneficial to add a brief comment explaining its purpose and the significance of the
ON CONFLICT DO NOTHINGclause. This can help other developers understand the intent behind the query.Consider adding a comment like this:
-- name: AddLabelToSatellite :exec +-- Adds a label to a satellite. If the association already exists, the operation is silently ignored. INSERT INTO satellite_labels (satellite_id, label_id) VALUES ($1, $2) ON CONFLICT DO NOTHING;ground-control/sql/queries/labels.sql (4)
6-8: LGTM: GetLabelByID query is correctly implemented.The query effectively retrieves a label by its ID. Good use of parameterized query for security.
Consider specifying the exact columns needed instead of using * if not all columns are required by the application. This can improve query performance and reduce data transfer.
10-12: LGTM with suggestions: GetLabelByName query is implemented correctly.The query effectively retrieves a label by its name, with good use of a parameterized query for security.
Consider the following improvements:
- Specify the exact columns needed instead of using * if not all columns are required.
- Ensure there's a uniqueness constraint on label_name in the table definition, or handle potential multiple results in the application layer.
- If multiple labels with the same name are possible and intended, consider changing the return type to
:manyinstead of:one.
14-16: LGTM with a suggestion: DeleteLabel query is implemented correctly.The query effectively deletes a label by its ID, with good use of a parameterized query for security.
Consider adding error handling in the application layer to check if any rows were affected by the DELETE operation. This can help distinguish between successful deletions and attempts to delete non-existent labels.
Line range hint
1-16: Overall, the label management queries are well-implemented and align with the PR objectives.The file provides a solid foundation for label management in the context of Zero Touch Registration (ZTR) for satellites. All queries are secure and cover basic CRUD operations.
To further enhance ZTR capabilities, consider the following future improvements:
- Add a query to update existing labels.
- Implement a query to retrieve multiple labels (e.g., for a specific satellite or category).
- Add support for label hierarchies or relationships, if applicable to the ZTR process.
- Include a query to check for label existence without retrieving all data.
These enhancements could provide more flexibility and efficiency in managing labels for satellite registration.
ground-control/sql/schema/014_satellite_token.sql (2)
1-9: LGTM! Consider adding an index on satellite_id.The table structure is well-designed and includes appropriate constraints:
- The
idas a SERIAL PRIMARY KEY is a good choice for an auto-incrementing identifier.- The
satellite_idforeign key with CASCADE delete maintains referential integrity.- The
tokencolumn's UNIQUE constraint ensures no duplicate tokens.created_atandupdated_atcolumns are useful for auditing.Consider adding an index on the
satellite_idcolumn to improve query performance when searching tokens by satellite:CREATE INDEX idx_satellite_token_satellite_id ON satellite_token(satellite_id);
1-12: Consider additional security measures for Zero Touch Registration.The
satellite_tokentable is a crucial component for implementing Zero Touch Registration (ZTR) for satellites. It provides a mechanism to associate unique tokens with specific satellites, which can be used for authentication during the automatic registration process.To enhance security for ZTR, consider the following suggestions:
- Implement token expiration: Add an
expires_atcolumn to allow for token rotation and reduce the risk of token abuse.- Use a cryptographically secure method for generating tokens.
- Implement rate limiting on token usage to prevent brute force attacks.
- Consider encrypting the token column for an extra layer of security.
Here's an example of how you might add an expiration column:
ALTER TABLE satellite_token ADD COLUMN expires_at TIMESTAMP;Also, ensure that your application logic includes proper token validation and handles token expiration gracefully.
ground-control/sql/queries/group_images.sql (1)
6-8: LGTM! Consider adding affected rows check.The new
RemoveImageFromGroupquery is well-structured and secure, using parameterized inputs. It correctly removes the specified image from the group.For improved error handling and feedback, consider modifying the query to return the number of affected rows. This can help detect cases where the specified group_id and image_id combination doesn't exist. Here's a suggested modification:
-- name: RemoveImageFromGroup :one DELETE FROM group_images WHERE group_id = $1 AND image_id = $2 RETURNING (xmax <> 0) AS deleted;This modification will return a boolean indicating whether a row was actually deleted. You can then handle this in your application logic to provide appropriate feedback or error handling.
ground-control/sql/schema/013_robot_accounts.sql (2)
3-10: Table structure looks good, but consider enhancing security for robot_secret.The table structure and constraints are well-defined:
- Appropriate use of SERIAL PRIMARY KEY for the id column.
- UNIQUE constraint on robot_name ensures no duplicate robot names.
- Foreign key relationship with ON DELETE CASCADE maintains referential integrity.
- Timestamp columns for created_at and updated_at with DEFAULT NOW() are good for tracking.
However, there are a couple of points to consider:
- For the robot_secret column, consider using a more secure data type or additional measures to protect sensitive information. For example, you could use pgcrypto for encryption or implement application-level encryption.
- Verify if VARCHAR(255) is sufficient for all columns, especially robot_name and robot_secret. Adjust if necessary based on your specific requirements.
Consider implementing one of these security enhancements for the robot_secret column:
- Use pgcrypto extension (if available):
CREATE EXTENSION IF NOT EXISTS pgcrypto; CREATE TABLE robot_accounts ( -- other columns remain the same robot_secret TEXT NOT NULL, -- use this in your application code: -- INSERT INTO robot_accounts (robot_secret) VALUES (crypt('secret', gen_salt('bf'))); );
- Or, if pgcrypto is not available, implement application-level encryption and store the encrypted value in the database.
1-13: Overall, the migration aligns with PR objectives, but consider clarifying ZTR implementation details.This migration introduces a
robot_accountstable, which is a step towards implementing Zero Touch Registration (ZTR) for satellites as outlined in the PR objectives. The table structure provides a foundation for managing robot accounts associated with satellites, which is crucial for automated registration processes.However, to fully address the objectives of issues #41 and #36, consider the following:
Clarify how this table will be used in the ZTR process. Will these robot accounts be automatically created during satellite registration? How will they interact with ground control?
Document the overall ZTR flow, explaining how this table fits into the broader implementation of automatic satellite registration.
Ensure that the security measures for
robot_secretare sufficient for your use case, considering the sensitive nature of satellite communications.Plan for how this table will be populated and managed as part of the zero-touch process.
By addressing these points, you'll strengthen the connection between this database change and the overall ZTR implementation, ensuring that the PR fully meets its objectives.
ground-control/sql/queries/satellite_token.sql (3)
1-4: LGTM! Consider adding a UNIQUE constraint on the token column.The
AddTokencommand is well-structured and secure, using parameterized queries to prevent SQL injection. It correctly sets timestamps and efficiently returns the inserted token.To enhance data integrity, consider adding a UNIQUE constraint on the
tokencolumn in thesatellite_tokentable. This would prevent duplicate tokens and align with the apparent 1:1 relationship between tokens and satellites. You can add this constraint with an ALTER TABLE statement:ALTER TABLE satellite_token ADD CONSTRAINT unique_token UNIQUE (token);
11-13: *LGTM! Consider specifying required columns instead of using .The
GetTokencommand is secure, using a parameterized query. The :one annotation correctly indicates that it returns a single row.For better performance and to make the query's intent clearer, consider explicitly listing the required columns instead of using
*. This approach fetches only the necessary data and makes the query's purpose more evident. For example:SELECT id, satellite_id, token, created_at, updated_at FROM satellite_token WHERE id = $1;Adjust the selected columns based on your specific needs.
18-20: LGTM! Consider returning the number of affected rows.The
DeleteTokencommand is secure, using a parameterized query. The deletion based on the token value is appropriate, assuming tokens are unique.To provide more information about the operation's result, consider returning the number of affected rows. This can be useful for verifying whether the deletion was successful. You can modify the command as follows:
-- name: DeleteToken :one DELETE FROM satellite_token WHERE token = $1 RETURNING 1;This will return 1 if a row was deleted, or no rows if no matching token was found. Update the annotation to
:oneto reflect this change.ground-control/internal/database/satellite_labels.sql.go (2)
12-16: LGTM: Consistent naming for SQL constantThe rename from
addSatelliteToLabeltoaddLabelToSatelliteimproves consistency with the function name change. The SQL query itself is correct for inserting a record into the satellite_labels table.Consider adding a comment explaining the purpose of
ON CONFLICT DO NOTHING. For example:const addLabelToSatellite = `-- name: AddLabelToSatellite :exec INSERT INTO satellite_labels (satellite_id, label_id) VALUES ($1, $2) +-- Ignore if the satellite-label association already exists ON CONFLICT DO NOTHING `
23-26: LGTM: Function rename and implementationThe function rename from
AddSatelliteToLabeltoAddLabelToSatelliteis consistent with the other changes. The implementation correctly uses the SQL constant and struct defined earlier.Consider wrapping the returned error with additional context:
func (q *Queries) AddLabelToSatellite(ctx context.Context, arg AddLabelToSatelliteParams) error { _, err := q.db.ExecContext(ctx, addLabelToSatellite, arg.SatelliteID, arg.LabelID) - return err + if err != nil { + return fmt.Errorf("failed to add label to satellite: %w", err) + } + return nil }This change would provide more context when debugging potential issues.
ground-control/sql/queries/satellites.sql (2)
2-3: LGTM! Consider adding a comment explaining the ZTR approach.The removal of the
tokenparameter and the use ofNOW()for timestamps align well with the Zero Touch Registration (ZTR) objective. This simplifies the satellite creation process and ensures consistent timestamp recording.Consider adding a comment above the query to explain the ZTR approach and why the
tokenis no longer needed. This will help future maintainers understand the rationale behind this change.
17-19: Consider removing or explaining the commented-out GetSatelliteByToken query.While commenting out this query aligns with the move towards Zero Touch Registration, leaving commented-out code in the file might lead to confusion in the future.
Consider one of the following options:
- Remove the commented-out query entirely if it's no longer needed.
- If there's a possibility of needing this query during a transition period, add a comment explaining its deprecation and when it can be safely removed. For example:
-- Deprecated: GetSatelliteByToken query -- This query is no longer used in the Zero Touch Registration (ZTR) approach. -- It is kept temporarily for reference and will be removed in version X.X. -- -- name: GetSatelliteByToken :one -- SELECT * FROM satellites -- WHERE token = $1 LIMIT 1;ground-control/sql/queries/robot_accounts.sql (6)
1-4: LGTM with a minor suggestion for data integrity.The
AddRobotAccountcommand is well-structured and correctly inserts a new robot account. It appropriately usesNOW()for timestamps and returns the full record.Consider adding a
UNIQUEconstraint on thesatellite_idcolumn in therobot_accountstable to ensure each satellite has only one robot account. This can be done in a separate migration script:ALTER TABLE robot_accounts ADD CONSTRAINT unique_satellite_id UNIQUE (satellite_id);
10-12: LGTM with a suggestion for handling potential multiple results.The
GetRobotAccBySatelliteIDcommand efficiently retrieves therobot_nameandrobot_secretfor a givensatellite_id.Consider changing the query type from
:oneto:manyto handle potential scenarios where multiple robot accounts might be associated with a single satellite:-- name: GetRobotAccBySatelliteID :many SELECT robot_name, robot_secret FROM robot_accounts WHERE satellite_id = $1;This change would make the query more flexible if the relationship between satellites and robot accounts changes in the future.
14-15: LGTM with suggestions for scalability.The
ListRobotAccountscommand effectively retrieves all robot accounts.For improved scalability, consider the following enhancements:
- Implement pagination to limit the number of records returned in a single query:
-- name: ListRobotAccounts :many SELECT * FROM robot_accounts ORDER BY id LIMIT $1 OFFSET $2;
- Add sorting options:
-- name: ListRobotAccounts :many SELECT * FROM robot_accounts ORDER BY $1 $2 LIMIT $3 OFFSET $4;These changes would allow the application to retrieve data in manageable chunks and provide sorting flexibility.
17-19: LGTM with a suggestion for deletion confirmation.The
DeleteRobotAccountcommand correctly deletes a robot account by its id.Consider returning the deleted row for confirmation:
-- name: DeleteRobotAccount :one DELETE FROM robot_accounts WHERE id = $1 RETURNING *;This change would allow the application to confirm the deletion and potentially use the deleted data if needed.
21-26: LGTM with suggestions for improvement.The
UpdateRobotAccountcommand correctly updates the specified fields and theupdated_attimestamp.Consider the following improvements:
- Return the updated record for confirmation:
-- name: UpdateRobotAccount :one UPDATE robot_accounts SET robot_name = $2, robot_secret = $3, updated_at = NOW() WHERE id = $1 RETURNING *;
- Add an option to update the
satellite_idif needed:-- name: UpdateRobotAccount :one UPDATE robot_accounts SET robot_name = $2, robot_secret = $3, satellite_id = COALESCE($4, satellite_id), updated_at = NOW() WHERE id = $1 RETURNING *;These changes would provide more flexibility and confirmation of the update operation.
1-26: Overall, well-structured CRUD operations with room for minor enhancements.The SQL file provides a comprehensive set of operations for managing robot accounts. The queries are generally well-structured and functional. Consider the following overall suggestions:
Implement the suggested improvements for individual queries to enhance scalability, flexibility, and data integrity.
For consistency, consider adding a
created_atfield to theUpdateRobotAccountquery:-- name: UpdateRobotAccount :one UPDATE robot_accounts SET robot_name = $2, robot_secret = $3, updated_at = NOW(), created_at = COALESCE($4, created_at) WHERE id = $1 RETURNING *;This change would allow for potential correction of the
created_attimestamp if needed, maintaining consistency across all fields.
- Consider adding appropriate indexes to the
robot_accountstable to optimize query performance, especially for thesatellite_idfield which is used in theGetRobotAccBySatelliteIDquery.ground-control/sql/queries/images.sql (2)
5-6: Consider adding pagination toListImagesquery.The new
ListImagesquery is a straightforward way to retrieve all images. However, if theimagestable grows large, this query might return an extensive dataset, potentially causing performance issues.Consider adding pagination to this query to improve performance and reduce memory usage. For example:
-- name: ListImages :many SELECT * FROM images ORDER BY id LIMIT $1 OFFSET $2;This allows for retrieving images in smaller batches, where
$1is the limit and$2is the offset.
17-34: ApproveGetReposOfSatellitequery with suggestions for improvement.The new
GetReposOfSatellitequery effectively retrieves repository names associated with a satellite, considering both satellite groups and labels. This comprehensive approach is commendable.Consider the following improvements:
Add an ORDER BY clause to ensure consistent ordering of results:
ORDER BY repositoryIf performance becomes an issue with large datasets, consider using a UNION ALL instead of UNION, as UNION ALL doesn't remove duplicates and is generally faster:
UNION ALLHowever, only do this if duplicate repositories are acceptable in the result set.
To potentially improve query performance, consider using a Common Table Expression (CTE) to avoid repeating the satellite_id condition:
WITH satellite_data AS ( SELECT $1 AS satellite_id ) SELECT i.repository FROM satellite_data sd JOIN satellite_groups sg ON sd.satellite_id = sg.satellite_id JOIN group_images gi ON sg.group_id = gi.group_id JOIN images i ON gi.image_id = i.id UNION SELECT i.repository FROM satellite_data sd JOIN satellite_labels sl ON sd.satellite_id = sl.satellite_id JOIN label_images li ON sl.label_id = li.label_id JOIN images i ON li.image_id = i.id ORDER BY repository;These suggestions aim to improve the query's performance and result consistency.
ground-control/internal/database/labels.sql.go (2)
37-45: LGTM with a minor suggestion.The
DeleteLabelfunction is well-implemented. It correctly uses a parameterized query to prevent SQL injection and properly handles the context and error.Consider adding a comment to explain the function's behavior, especially regarding what happens if the label doesn't exist. For example:
// DeleteLabel removes a label from the database by its ID. // If the label doesn't exist, it returns nil (no error). func (q *Queries) DeleteLabel(ctx context.Context, id int32) error { // ... (existing code) }
47-62: LGTM with a minor suggestion.The
GetLabelByIDfunction is well-implemented. It uses a parameterized query to prevent SQL injection, properly handles the context, and correctly scans the result into a Label struct.Consider adding a comment to explain the function's behavior, especially regarding what happens if the label doesn't exist. For example:
// GetLabelByID retrieves a label from the database by its ID. // If the label doesn't exist, it returns an error (likely sql.ErrNoRows). func (q *Queries) GetLabelByID(ctx context.Context, id int32) (Label, error) { // ... (existing code) }ground-control/internal/database/group_images.sql.go (1)
86-94: LGTM: New Go function and struct for removing an image from a group.The new
RemoveImageFromGroupParamsstruct andRemoveImageFromGroupmethod are well-implemented and consistent with the existing code style. The method correctly uses the database context and handles error reporting.Consider adding a brief comment above the
RemoveImageFromGroupmethod to describe its purpose and any important details. For example:+// RemoveImageFromGroup removes the specified image from the given group. +// It returns an error if the operation fails. func (q *Queries) RemoveImageFromGroup(ctx context.Context, arg RemoveImageFromGroupParams) error { _, err := q.db.ExecContext(ctx, removeImageFromGroup, arg.GroupID, arg.ImageID) return err }ground-control/internal/database/satellites.sql.go (4)
13-15: *LGTM! Consider using RETURNING .The changes to the createSatellite query look good. Removing the token field and using NOW() for timestamps are appropriate modifications.
Consider using
RETURNING *instead of explicitly listing the columns. This can make the query more maintainable as it will automatically adapt to any future column additions:INSERT INTO satellites (name, created_at, updated_at) VALUES ($1, NOW(), NOW()) RETURNING *
40-42: LGTM! Consider adding an index on the id column.The changes to the getSatellite query look good. Retrieving a satellite by ID is more efficient and less prone to errors than using names.
Consider adding an index on the id column if it doesn't already exist. This will improve the performance of this query, especially as the number of satellites grows:
CREATE INDEX IF NOT EXISTS idx_satellites_id ON satellites(id);
57-59: LGTM! Consider adding an index on the name column.The addition of the getSatelliteByName query is good. It provides flexibility in how satellites can be retrieved.
Consider adding a unique index on the name column if it doesn't already exist. This will improve the performance of this query and ensure that satellite names are unique:
CREATE UNIQUE INDEX IF NOT EXISTS idx_satellites_name ON satellites(name);
Line range hint
1-120: Overall, the changes align well with the Zero Touch Registration objectives.The modifications to this file, including the removal of token-based operations and the addition of name-based retrieval methods, appear to support a more streamlined satellite registration process. These changes align with the PR objectives of implementing Zero Touch Registration (ZTR) for satellites.
However, to ensure full alignment with the ZTR objectives:
- Verify that these database changes are accompanied by corresponding changes in the application logic to support automatic satellite registration.
- Consider adding documentation or comments explaining how these database operations support the ZTR process.
- Ensure that proper error handling and logging are implemented in the application code that uses these database operations to facilitate troubleshooting of the ZTR process.
ground-control/internal/server/handlers.go (9)
141-161: Improved error handling and image URL parsing.The changes in this function enhance error handling and introduce proper parsing of the image URL. These improvements are beneficial for better error reporting and data validation.
Consider making the error message for invalid artifact URL more specific by including the actual URL:
err = &AppError{ - Message: "Error: Invalid Artifact URL", + Message: fmt.Sprintf("Error: Invalid Artifact URL: %s", req.Image), Code: http.StatusBadRequest, }
210-364: Well-implemented registerSatelliteHandler with room for refactoring.The function correctly handles satellite registration with proper transaction management and error handling. The use of a transaction ensures data consistency across multiple database operations.
While the implementation is correct, the function is quite long and complex. Consider refactoring it into smaller, more focused functions to improve readability and maintainability. For example:
- Create a separate function for group association:
func (s *Server) associateGroups(ctx context.Context, q *Queries, satelliteID int32, groups []string) error { // Implementation here }
- Create a separate function for label association:
func (s *Server) associateLabels(ctx context.Context, q *Queries, satelliteID int32, labels []string) error { // Implementation here }
- Create a separate function for robot account creation:
func (s *Server) createRobotAccount(ctx context.Context, q *Queries, satelliteID int32, satelliteName string) error { // Implementation here }Then, call these functions from the main
registerSatelliteHandler. This approach would make the main function more concise and easier to understand.
366-427: Enhance clarity and documentation in ztrHandler.The function correctly implements Zero Touch Registration (ZTR) for satellites with proper transaction management and error handling.
To improve code clarity and maintainability:
- Add a comment explaining the ZTR process at the beginning of the function.
- Use more descriptive variable names. For example, change
qtotxQueries.- Add comments explaining the purpose of each major step in the function.
Example:
// ztrHandler handles Zero Touch Registration (ZTR) for satellites. // It validates the provided token, retrieves the associated satellite, // and returns the robot account information for the satellite. func (s *Server) ztrHandler(w http.ResponseWriter, r *http.Request) { // ... (existing code) // Start a new transaction tx, err := s.db.BeginTx(r.Context(), nil) if err != nil { log.Println("Failed to start transaction:", err) HandleAppError(w, err) return } txQueries := s.dbQueries.WithTx(tx) // ... (existing code) // Validate the token and get the associated satellite ID satelliteID, err := txQueries.GetSatelliteIDByToken(r.Context(), token) if err != nil { log.Println("Invalid Satellite Token:", err) // ... (existing error handling) } // Delete the used token err = txQueries.DeleteToken(r.Context(), token) if err != nil { log.Println("Failed to delete token:", err) // ... (existing error handling) } // Retrieve the robot account for the satellite robot, err := txQueries.GetRobotAccBySatelliteID(r.Context(), satelliteID) if err != nil { log.Println("Robot Account Not Found:", err) // ... (existing error handling) } // ... (rest of the existing code) }These changes will make the function easier to understand and maintain.
429-437: Consider adding pagination to listSatelliteHandler.The function correctly retrieves and returns the list of satellites with proper error handling.
To improve scalability and performance, consider implementing pagination for this endpoint. This is especially important if the number of satellites could grow large over time. Here's an example of how you might implement this:
func (s *Server) listSatelliteHandler(w http.ResponseWriter, r *http.Request) { limit := 50 // Default limit offset := 0 // Default offset // Parse limit and offset from query parameters if limitStr := r.URL.Query().Get("limit"); limitStr != "" { if parsedLimit, err := strconv.Atoi(limitStr); err == nil { limit = parsedLimit } } if offsetStr := r.URL.Query().Get("offset"); offsetStr != "" { if parsedOffset, err := strconv.Atoi(offsetStr); err == nil { offset = parsedOffset } } result, err := s.dbQueries.ListSatellitesWithPagination(r.Context(), database.ListSatellitesWithPaginationParams{ Limit: int32(limit), Offset: int32(offset), }) if err != nil { HandleAppError(w, err) return } WriteJSONResponse(w, http.StatusOK, result) }This change would require adding a new
ListSatellitesWithPaginationmethod to your database queries, but it would allow clients to request smaller chunks of data, improving performance for large datasets.
Line range hint
439-456: Implement getSatelliteByID and deleteSatelliteByID correctly.Both
getSatelliteByIDanddeleteSatelliteByIDfunctions currently have the same implementation aslistSatelliteHandler, which doesn't match their intended purposes.These functions need to be implemented correctly:
- For
getSatelliteByID:func (s *Server) getSatelliteByID(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) idStr := vars["id"] id, err := strconv.ParseInt(idStr, 10, 32) if err != nil { HandleAppError(w, &AppError{Message: "Invalid satellite ID", Code: http.StatusBadRequest}) return } satellite, err := s.dbQueries.GetSatelliteByID(r.Context(), int32(id)) if err != nil { if err == sql.ErrNoRows { HandleAppError(w, &AppError{Message: "Satellite not found", Code: http.StatusNotFound}) } else { HandleAppError(w, err) } return } WriteJSONResponse(w, http.StatusOK, satellite) }
- For
deleteSatelliteByID:func (s *Server) deleteSatelliteByID(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) idStr := vars["id"] id, err := strconv.ParseInt(idStr, 10, 32) if err != nil { HandleAppError(w, &AppError{Message: "Invalid satellite ID", Code: http.StatusBadRequest}) return } err = s.dbQueries.DeleteSatellite(r.Context(), int32(id)) if err != nil { if err == sql.ErrNoRows { HandleAppError(w, &AppError{Message: "Satellite not found", Code: http.StatusNotFound}) } else { HandleAppError(w, err) } return } WriteJSONResponse(w, http.StatusOK, map[string]string{"message": "Satellite deleted successfully"}) }These implementations assume the existence of
GetSatelliteByIDandDeleteSatellitemethods in your database queries. If these methods don't exist, you'll need to add them to your database layer as well.
Line range hint
481-501: Approve AddLabelToSatellite with minor suggestion.The renaming of the function to
AddLabelToSatellitemakes it exported, which is fine if this is the intended behavior. The implementation correctly handles adding a label to a satellite.Consider improving the error handling slightly:
func (s *Server) AddLabelToSatellite(w http.ResponseWriter, r *http.Request) { var req AddLabelToSatelliteParams if err := DecodeRequestBody(r, &req); err != nil { HandleAppError(w, err) return } params := database.AddLabelToSatelliteParams{ SatelliteID: int32(req.SatelliteID), LabelID: int32(req.LabelID), } err := s.dbQueries.AddLabelToSatellite(r.Context(), params) if err != nil { - log.Printf("Error: Failed to add satellite to label: %v", err) - HandleAppError(w, err) + log.Printf("Error: Failed to add label to satellite: %v", err) + HandleAppError(w, &AppError{ + Message: "Failed to add label to satellite", + Code: http.StatusInternalServerError, + }) return } WriteJSONResponse(w, http.StatusOK, map[string]string{}) }This change provides a more specific error message and ensures that an
AppErroris always passed toHandleAppError.
547-567: Improve error handling in deleteImageFromGroup.The function correctly handles removing an image from a group, but the error handling can be improved.
Consider enhancing the error handling:
func (s *Server) deleteImageFromGroup(w http.ResponseWriter, r *http.Request) { var req AssignImageToGroupParams if err := DecodeRequestBody(r, &req); err != nil { HandleAppError(w, err) return } params := database.RemoveImageFromGroupParams{ GroupID: int32(req.GroupID), ImageID: int32(req.ImageID), } err := s.dbQueries.RemoveImageFromGroup(r.Context(), params) if err != nil { - log.Printf("Error: Failed to delete image from group: %v", err) - HandleAppError(w, err) + if err == sql.ErrNoRows { + log.Printf("Image (ID: %d) not found in group (ID: %d)", req.ImageID, req.GroupID) + HandleAppError(w, &AppError{ + Message: "Image not found in group", + Code: http.StatusNotFound, + }) + } else { + log.Printf("Error: Failed to delete image from group: %v", err) + HandleAppError(w, &AppError{ + Message: "Failed to delete image from group", + Code: http.StatusInternalServerError, + }) + } return } WriteJSONResponse(w, http.StatusOK, map[string]string{}) }This change provides more specific error messages and handles the case where the image is not found in the group.
569-592: Enhance error handling in listGroupImages.The function correctly retrieves images for a specific group, but the error handling can be improved.
Consider enhancing the error handling and adding a check for empty results:
func (s *Server) listGroupImages(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) groupID := vars["groupID"] id, err := strconv.ParseInt(groupID, 10, 32) if err != nil { - log.Printf("Error: Invalid groupID: %v", err) - err := &AppError{ - Message: "Error: Invalid GroupID", - Code: http.StatusBadRequest, - } - HandleAppError(w, err) + log.Printf("Error: Invalid groupID: %s - %v", groupID, err) + HandleAppError(w, &AppError{ + Message: fmt.Sprintf("Invalid group ID: %s", groupID), + Code: http.StatusBadRequest, + }) return } result, err := s.dbQueries.GetImagesForGroup(r.Context(), int32(id)) if err != nil { - log.Printf("Error: Failed to get image for group: %v", err) - HandleAppError(w, err) + if err == sql.ErrNoRows { + log.Printf("No images found for group (ID: %d)", id) + WriteJSONResponse(w, http.StatusOK, []database.Image{}) + } else { + log.Printf("Error: Failed to get images for group (ID: %d): %v", id, err) + HandleAppError(w, &AppError{ + Message: "Failed to retrieve images for group", + Code: http.StatusInternalServerError, + }) + } return } + if len(result) == 0 { + log.Printf("No images found for group (ID: %d)", id) + } WriteJSONResponse(w, http.StatusOK, result) }These changes provide more specific error messages, handle the case where no images are found for the group, and add a log message when the result is empty.
621-623: Enhance environment variable checks in regListHandler.The use of environment variables for Harbor credentials is a good security practice. The check for the URL's presence is appropriate.
Consider adding checks for the username and password as well:
func (s *Server) regListHandler(w http.ResponseWriter, r *http.Request) { username := os.Getenv("HARBOR_USERNAME") password := os.Getenv("HARBOR_PASSWORD") url := os.Getenv("HARBOR_URL") - if url == "" { + if username == "" || password == "" || url == "" { err := &AppError{ - Message: "Missing URL in Request", + Message: "Missing Harbor credentials or URL in environment variables", Code: http.StatusBadRequest, } HandleAppError(w, err) return } result, err := reg.FetchRepos(username, password, url) if err != nil { HandleAppError(w, err) return } WriteJSONResponse(w, http.StatusOK, result) }This change ensures that all necessary credentials are present before attempting to fetch repositories.
Also applies to: 645-651
ground-control/internal/utils/helper.go (1)
91-95: Ensure uniqueness of project names when extracting from repositoriesWhile extracting project names from the repositories, there is a possibility of duplicate project names being added to the
projectsslice. If duplicates can cause issues in later processing, consider ensuring that the list of projects is unique.You can use a map to track unique project names:
projectSet := make(map[string]struct{}) for _, repo := range repos { proj := strings.Split(repo, "/") projectSet[proj[0]] = struct{}{} } var projects []string for project := range projectSet { projects = append(projects, project) }ground-control/internal/database/images.sql.go (2)
114-116: Remove redundant call torows.Close()inGetReposOfSatelliteYou have already deferred
rows.Close()at line 105. Explicitly callingrows.Close()again at lines 114-116 is unnecessary and can be removed to streamline the code.Apply this diff to remove the redundant call:
- if err := rows.Close(); err != nil { - return nil, err - }
Line range hint
149-151: Remove redundant call torows.Close()inListImagesIn the
ListImagesfunction,rows.Close()is already deferred at line 132. The explicit call torows.Close()at lines 149-151 is unnecessary and can be removed to clean up the code.Apply this diff to remove the redundant call:
- if err := rows.Close(); err != nil { - return nil, err - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (28)
- ground-control/internal/database/db.go (1 hunks)
- ground-control/internal/database/group_images.sql.go (2 hunks)
- ground-control/internal/database/groups.sql.go (3 hunks)
- ground-control/internal/database/images.sql.go (3 hunks)
- ground-control/internal/database/label_images.sql.go (1 hunks)
- ground-control/internal/database/labels.sql.go (2 hunks)
- ground-control/internal/database/models.go (3 hunks)
- ground-control/internal/database/robot_accounts.sql.go (1 hunks)
- ground-control/internal/database/satellite_groups.sql.go (1 hunks)
- ground-control/internal/database/satellite_labels.sql.go (2 hunks)
- ground-control/internal/database/satellite_token.sql.go (1 hunks)
- ground-control/internal/database/satellites.sql.go (3 hunks)
- ground-control/internal/server/handlers.go (10 hunks)
- ground-control/internal/server/routes.go (1 hunks)
- ground-control/internal/utils/helper.go (1 hunks)
- ground-control/reg/adapter.go (1 hunks)
- ground-control/sql/queries/group_images.sql (1 hunks)
- ground-control/sql/queries/groups.sql (2 hunks)
- ground-control/sql/queries/images.sql (1 hunks)
- ground-control/sql/queries/labels.sql (1 hunks)
- ground-control/sql/queries/robot_accounts.sql (1 hunks)
- ground-control/sql/queries/satellite_labels.sql (1 hunks)
- ground-control/sql/queries/satellite_token.sql (1 hunks)
- ground-control/sql/queries/satellites.sql (1 hunks)
- ground-control/sql/schema/001_satellites.sql (1 hunks)
- ground-control/sql/schema/011_projects.sql (1 hunks)
- ground-control/sql/schema/013_robot_accounts.sql (1 hunks)
- ground-control/sql/schema/014_satellite_token.sql (1 hunks)
✅ Files skipped from review due to trivial changes (5)
- ground-control/internal/database/db.go
- ground-control/internal/database/label_images.sql.go
- ground-control/internal/database/robot_accounts.sql.go
- ground-control/internal/database/satellite_groups.sql.go
- ground-control/internal/database/satellite_token.sql.go
🚧 Files skipped from review as they are similar to previous changes (3)
- ground-control/internal/server/routes.go
- ground-control/sql/schema/001_satellites.sql
- ground-control/sql/schema/011_projects.sql
🔇 Additional comments (41)
ground-control/sql/queries/satellite_labels.sql (2)
1-4: Summary: Changes align with ZTR implementationThe modifications in this file, particularly the improved operation naming, contribute to the overall goal of implementing Zero Touch Registration (ZTR) for satellites. The existing conflict resolution strategy (
ON CONFLICT DO NOTHING) is well-suited for an automated system, as it prevents duplicate entries without raising errors.While this change alone doesn't fully implement ZTR, it's a step in the right direction. As you continue to work on this feature, consider how this query will be used in the broader context of automatic satellite registration and labeling.
1-1: Approve operation name changeThe change from
AddSatelliteToLabeltoAddLabelToSatelliteis a good improvement. It more accurately describes the operation being performed, which is adding a label to a satellite rather than the other way around.To ensure this change doesn't break any existing code, please run the following script to check for any references to the old operation name:
✅ Verification successful
Adjusting Reference Search Script
The previous script had an error with the
pythonfile type. Here's the corrected version to ensure all references toAddSatelliteToLabelare properly searched:
Operation Name Change Verified
No remaining references to
AddSatelliteToLabelwere found in the codebase. The operation name change toAddLabelToSatelliteis safe and does not impact existing functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to the old operation name # Test: Search for the old operation name. Expect: No results. rg --type sql --type go --type python "AddSatelliteToLabel"Length of output: 90
Script:
#!/bin/bash # Description: Search for any remaining references to the old operation name # Test: Search for the old operation name. Expect: No results. rg --type sql --type go --type py "AddSatelliteToLabel"Length of output: 55
ground-control/sql/queries/labels.sql (1)
Line range hint
1-4: LGTM: CreateLabel query is well-implemented.The CreateLabel query is correctly implemented with the following good practices:
- Use of parameterized queries to prevent SQL injection.
- Inclusion of created_at and updated_at fields for proper record keeping.
- Returns all columns, allowing the application to access any auto-generated fields.
ground-control/sql/schema/014_satellite_token.sql (1)
11-12: LGTM! The "Down" migration is correct.The "Down" migration properly reverses the "Up" migration by dropping the
satellite_tokentable. This ensures that the migration can be rolled back if needed.ground-control/sql/queries/group_images.sql (1)
Line range hint
1-15: Overall, the changes enhance group image management capabilities.The addition of the
RemoveImageFromGroupquery complements the existing operations, providing a complete set of functionalities for managing group images. This aligns well with the PR objectives of implementing Zero Touch Registration for satellites, potentially allowing for dynamic updates of satellite imagery groups during the registration process.ground-control/sql/queries/groups.sql (2)
3-3: Approve: Improved timestamp handling in CreateGroup queryThe change from
$3toNOW()for theupdated_atfield is a good improvement. It ensures that the timestamp is always set to the current time when a group is created, reducing the chance of inconsistencies and simplifying the API by removing one parameter.
17-19: Approve: Addition of DeleteGroup queryThe new DeleteGroup query provides necessary functionality to remove groups from the database. This is a standard operation in data management and the implementation is straightforward.
Consider potential improvements to the delete strategy
While the current implementation is functional, consider the following suggestions:
- Add a check for the existence of the group before deletion to provide more informative feedback.
- Consider implementing a soft delete mechanism if retaining historical data is important for your use case.
- Ensure that this delete operation aligns with the intended functionality for Zero Touch Satellite Registration as mentioned in the PR objectives.
Here's a potential improvement that includes an existence check:
-- name: DeleteGroup :execrows WITH deleted AS ( DELETE FROM groups WHERE id = $1 RETURNING id ) SELECT COUNT(*) FROM deleted;This version returns the number of rows deleted, which will be 0 if the group didn't exist.
To ensure we're not overlooking any potential issues with foreign key relationships, let's run a check:
This will help us identify if there are any tables with foreign key constraints referencing the groups table, which might need to be considered in the delete strategy.
ground-control/sql/schema/013_robot_accounts.sql (2)
1-3: LGTM: Up migration structure is correct.The Up migration is properly defined using the
+goose Upcomment. The migration file name follows the sequential naming convention, which is good for maintaining order in migrations.
12-13: LGTM: Down migration is correct, but be cautious about data loss.The Down migration is properly defined using the
+goose Downcomment and correctly drops therobot_accountstable. This is an appropriate way to revert the Up migration.However, it's important to note that executing this Down migration in a production environment will result in permanent data loss. Ensure that proper backups are in place and that the team is aware of this risk. Consider running the following verification script to check for existing data before applying the migration in production:
This script will help verify the existence of the table and presence of data before applying the migration.
ground-control/sql/queries/satellite_token.sql (2)
6-9: LGTM! Efficient and secure token lookup.The
GetSatelliteIDByTokencommand is well-implemented. It uses a parameterized query for security and selects only the necessary column for efficiency. The :one annotation correctly indicates that it returns a single row.
1-20: Overall, good implementation of satellite token management for Zero Touch Registration.This file successfully implements the necessary SQL queries for managing satellite tokens, which is crucial for the Zero Touch Registration (ZTR) system mentioned in the PR objectives. The queries cover creation, retrieval, listing, and deletion of tokens, providing a solid foundation for the ZTR implementation.
A few suggestions for improvement have been made:
- Adding a UNIQUE constraint on the token column.
- Implementing pagination for the ListToken query.
- Explicitly listing required columns instead of using * in select statements.
- Returning affected rows count for the DeleteToken operation.
These changes will enhance performance, security, and usability of the satellite token management system, further supporting the goals outlined in issues #41 and #36.
ground-control/internal/database/satellite_labels.sql.go (3)
3-3: LGTM: sqlc version updateThe update to sqlc v1.27.0 is a good practice to ensure you're using the latest features and bug fixes.
18-21: LGTM: Struct rename for consistencyThe rename of the struct from
AddSatelliteToLabelParamstoAddLabelToSatelliteParamsmaintains consistency with the function name change. The struct fields correctly match the SQL query parameters.
Line range hint
1-26: Summary: Consistent renaming supports Zero Touch Registration implementationThe changes in this file, particularly the renaming of the
AddLabelToSatellitefunction and related components, contribute to the implementation of Zero Touch Registration (ZTR) for satellites. These updates improve code consistency and readability, which is crucial for maintaining a robust automatic registration system.While the core functionality remains unchanged, these naming improvements lay a foundation for a more intuitive API that could be used in the ZTR process. As you continue to implement the ZTR system, consider how this function will be integrated into the automatic registration flow and ensure that error handling and logging are comprehensive throughout the process.
ground-control/sql/queries/satellites.sql (2)
9-11: LGTM! The new GetSatellite query is a valuable addition.This query provides a straightforward way to retrieve a satellite by its
id, which is essential for satellite management in the ZTR system. The use ofLIMIT 1is appropriate asidshould be unique.
Line range hint
1-28: Overall, the changes effectively support Zero Touch Registration (ZTR) for satellites.The modifications in this file align well with the PR objectives:
- Removal of token-based registration in
CreateSatellite.- Introduction of
GetSatellitequery for id-based retrieval.- Deprecation of
GetSatelliteByToken.These changes collectively contribute to implementing ZTR as outlined in issues #41 and #36, simplifying the satellite registration process and moving away from manual token-based systems.
ground-control/sql/queries/robot_accounts.sql (1)
6-8: LGTM! Simple and effective query.The
GetRobotAccountcommand efficiently retrieves a single robot account by its id. It's concise and assumes 'id' is the primary key, which is a common and appropriate practice.ground-control/sql/queries/images.sql (2)
13-15: Approve rename ofDeleteImageListtoDeleteImage.The rename from
DeleteImageListtoDeleteImageis appropriate as it more accurately describes the query's function of deleting a single image by its ID.
36-50: Clarify the replacement ofGetImagesForSatellitewithGetReposOfSatellite.The previous
GetImagesForSatellitequery has been commented out and effectively replaced by the newGetReposOfSatellitequery. This change represents a significant shift in functionality:
- The old query used a satellite token for identification, while the new one uses a satellite_id.
- The old query retrieved all image data, whereas the new query only fetches repository names.
Could you please clarify the reasons for this change? Is it intentional to only retrieve repository names instead of full image data? Also, how will this affect the systems that were previously using the
GetImagesForSatellitequery?To verify the impact of this change, we can run the following script:
This script will help us identify any code that might be affected by this change.
✅ Verification successful
Verification Successful: The
GetImagesForSatellitequery has been effectively replaced byGetReposOfSatellite. No active references to the old query were found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of the old GetImagesForSatellite query in the codebase # Search for GetImagesForSatellite usages echo "Searching for GetImagesForSatellite usages:" rg --type go "GetImagesForSatellite" # Search for potential new usages of GetReposOfSatellite echo "Searching for GetReposOfSatellite usages:" rg --type go "GetReposOfSatellite"Length of output: 999
ground-control/reg/adapter.go (3)
19-24: Consider renaming the struct to singular form for clarity.The struct name
Imagesis plural, which might be confusing as it represents a single image configuration. Consider renaming it toImageorImageConfigfor better clarity.Clarify the relationship between
ImagesandImagestructs.There's an existing
Imagestruct in the file. Could you please clarify the relationship between the newImagesstruct and the existingImagestruct? Are they meant to be used together or for different purposes?Explain how this struct contributes to Zero Touch Registration.
Given that this PR is about implementing Zero Touch Registration for satellites, could you provide some context on how this new struct contributes to that goal? It would be helpful to understand its role in the registration process.
19-24: Provide information on the planned usage of the newImagesstruct.The new
Imagesstruct is not used anywhere in the existing code provided. Could you please explain how this struct is intended to be used? Are there planned updates to existing functions or new functions that will utilize this struct?
Line range hint
31-78: Clarify the role ofFetchReposin satellite registration.The
FetchReposfunction fetches repositories from a Docker registry. Could you please explain how this function relates to the Zero Touch Registration process for satellites? Understanding its role in the larger context would be helpful.Consider making the repository limit configurable.
The URL in the
FetchReposfunction includes a hardcoded limit of 1000 repositories (n=1000). Consider making this limit configurable to allow for more flexibility in different scenarios.You could modify the function signature to include a limit parameter:
func FetchRepos(username, password, url string, limit int) ([]string, error) { // ... url = fmt.Sprintf("%s/v2/_catalog?n=%d", url, limit) // ... }ground-control/internal/database/labels.sql.go (3)
Line range hint
1-79: Summary of changes and relation to PR objectivesThe changes in this file introduce new database operations for managing labels, which appear to be consistent with the PR objectives of implementing Zero Touch Registration (ZTR) for satellites. The new functions (
DeleteLabel,GetLabelByID, andGetLabelByName) provide the necessary database interactions to support label management, which could be a part of the satellite registration process.However, it's important to note:
- This file is generated by sqlc and should not be manually edited.
- The direct connection between these label operations and ZTR is not immediately clear from this file alone.
To ensure these changes align with the ZTR implementation, please run the following script:
#!/bin/bash # Description: Check for ZTR-related code in the codebase # Test: Search for ZTR-related functions or comments rg -i "zero.?touch|ztr|satellite.?registration" --type go # Test: Check if labels are used in any ZTR-related code rg -i "zero.?touch|ztr|satellite.?registration" -C 5 | rg "label"Please review the results to confirm that these label operations are indeed being used in the context of Zero Touch Registration for satellites.
64-79: LGTM with suggestions and a question.The
GetLabelByNamefunction is well-implemented. It uses a parameterized query to prevent SQL injection, properly handles the context, and correctly scans the result into a Label struct.Consider adding a comment to explain the function's behavior, especially regarding what happens if the label doesn't exist. For example:
// GetLabelByName retrieves a label from the database by its name. // If the label doesn't exist, it returns an error (likely sql.ErrNoRows). func (q *Queries) GetLabelByName(ctx context.Context, labelName string) (Label, error) { // ... (existing code) }This function is very similar to
GetLabelByID. Could we potentially refactor to reduce duplication? For example, we could have a single internal function that takes a query string and a parameter, and then haveGetLabelByIDandGetLabelByNameas thin wrappers around this internal function.To verify if this refactoring is feasible and beneficial, please run the following script:
#!/bin/bash # Description: Check for similar functions in the codebase # Test: Search for functions with similar structure ast-grep --lang go --pattern $'func (q *Queries) Get$_($_) (Label, error) { row := q.db.QueryRowContext(ctx, $_, $_) var i Label err := row.Scan( &i.ID, &i.LabelName, &i.CreatedAt, &i.UpdatedAt, ) return i, err }' # Test: Count the number of such functions ast-grep --lang go --pattern $'func (q *Queries) Get$_($_) (Label, error) { row := q.db.QueryRowContext(ctx, $_, $_) var i Label err := row.Scan( &i.ID, &i.LabelName, &i.CreatedAt, &i.UpdatedAt, ) return i, err }' | wc -lPlease review the results to determine if refactoring would be beneficial.
3-3: Verify the impact of sqlc version update.The sqlc version has been updated from v1.26.0 to v1.27.0. While this is likely beneficial, it's important to ensure that this update doesn't introduce any breaking changes or unexpected behavior.
Run the following script to check for any significant changes or known issues in the new sqlc version:
Please review the changelog and any reported issues to ensure that the update is safe and beneficial for your project.
✅ Verification successful
sqlc version update is safe.
No breaking changes or known issues found in sqlc v1.27.0.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for significant changes or known issues in the new sqlc version # Test: Fetch the changelog for sqlc v1.27.0 curl -s https://api.github.com/repos/kyleconroy/sqlc/releases/tags/v1.27.0 | jq -r '.body' # Test: Search for any reported issues related to v1.27.0 gh issue list -R kyleconroy/sqlc -S "v1.27.0 in:body" --json number,title,stateLength of output: 936
Script:
#!/bin/bash # Description: Verify sqlc version update from v1.26.0 to v1.27.0 # Check if required tools are installed command -v curl >/dev/null 2>&1 || { echo "curl is required but it's not installed. Exiting."; exit 1; } command -v jq >/dev/null 2>&1 || { echo "jq is required but it's not installed. Exiting."; exit 1; } command -v gh >/dev/null 2>&1 || { echo "GitHub CLI (gh) is required but it's not installed. Exiting."; exit 1; } # Fetch the changelog for sqlc v1.27.0 echo "Fetching changelog for sqlc v1.27.0..." curl -s https://api.github.com/repos/kyleconroy/sqlc/releases/tags/v1.27.0 | jq -r '.body' > sqlc_v1.27.0_changelog.txt if [ $? -ne 0 ]; then echo "Failed to fetch or parse the changelog." exit 1 fi echo "Changelog fetched successfully." # Search for any reported issues related to v1.27.0 echo "Searching for issues related to sqlc v1.27.0..." gh issue list -R kyleconroy/sqlc --search "v1.27.0 in:body" --json number,title,state > sqlc_v1.27.0_issues.json if [ $? -ne 0 ]; then echo "Failed to fetch issues from GitHub." exit 1 fi echo "Issue search completed successfully."Length of output: 612
ground-control/internal/database/group_images.sql.go (3)
81-84: LGTM: New SQL command for removing an image from a group.The new SQL command
removeImageFromGroupis well-structured and follows the existing naming conventions. It correctly uses parameterized queries to prevent SQL injection, which is a good security practice.
Line range hint
1-94: Summary: New functionality aligns with PR objectives.The changes made to this file introduce a new capability to remove an image from a group, which could be a part of the Zero Touch Registration (ZTR) process for satellites. The implementation is consistent with the existing code style and follows good practices for SQL operations and error handling.
3-3: LGTM: sqlc version update.The update to sqlc v1.27.0 is a good practice to keep the tool up-to-date. However, it's important to ensure that this update doesn't introduce any breaking changes.
To verify the compatibility of the new sqlc version with your project, please run the following commands:
If the
git diffcommand shows no differences, it indicates that the new version is compatible with your current setup.ground-control/internal/database/groups.sql.go (4)
15-15: Improvement in timestamp handling for group creation.The modification to use
NOW()for theupdated_atfield in thecreateGroupSQL command is a positive change. This approach ensures that theupdated_attimestamp is always set to the current time when a group is created, improving data consistency and simplifying the API.
25-25: API simplification for CreateGroup function.The removal of the
UpdatedAtparameter from theCreateGroupfunction is consistent with the SQL command changes and simplifies the API. This is a positive change that reduces the potential for errors when creating groups.Consider updating any relevant documentation or comments to reflect this change in the API.
36-44: New DeleteGroup functionality added.The addition of the
deleteGroupSQL command and the correspondingDeleteGroupfunction is a valuable feature that allows for the removal of groups from the database. The implementation is correct and consistent with the existing code structure.Consider adding a check for the existence of the group before deletion to provide more informative error handling. This could be done by returning the number of affected rows and checking if it's zero:
func (q *Queries) DeleteGroup(ctx context.Context, id int32) error { - _, err := q.db.ExecContext(ctx, deleteGroup, id) - return err + result, err := q.db.ExecContext(ctx, deleteGroup, id) + if err != nil { + return err + } + rowsAffected, err := result.RowsAffected() + if err != nil { + return err + } + if rowsAffected == 0 { + return fmt.Errorf("no group found with id %d", id) + } + return nil }This change would provide more meaningful feedback when attempting to delete a non-existent group.
Line range hint
1-110: Overall assessment of changes in groups.sql.goThe changes made to this file are well-aligned with the PR objectives and represent improvements in the API for group management. Key points:
- The sqlc version update should be verified for any potential impacts.
- Timestamp handling for group creation has been improved.
- The API for creating groups has been simplified.
- A new feature for deleting groups has been added, with a suggestion for improved error handling.
These changes contribute positively to the implementation of Zero Touch Registration for satellites by enhancing the group management capabilities of the system.
ground-control/internal/database/satellites.sql.go (4)
18-19: LGTM! Function signature simplified.The changes to the CreateSatellite function look good. The simplified signature with a single 'name' parameter aligns well with the modifications made to the SQL query.
45-46: LGTM! Function updated to use ID for retrieval.The changes to the GetSatellite function look good. Using ID for satellite retrieval is more efficient and aligns well with typical CRUD operations.
62-63: LGTM! New function added for name-based retrieval.The addition of the GetSatelliteByName function is good. It complements the new getSatelliteByName SQL query and provides a clear way to retrieve satellites by name.
3-3: Verify sqlc version update impact.The sqlc version has been updated from v1.26.0 to v1.27.0. While this is likely a beneficial update, it's important to ensure that it doesn't introduce any breaking changes or unexpected behavior.
Run the following script to check for any significant changes or known issues in the new version:
ground-control/internal/server/handlers.go (1)
Line range hint
1-674: Overall assessment of changes in handlers.goThe changes in this file represent significant improvements and additions to the functionality of the server. New handlers for satellite registration, image management, and group handling have been implemented, and existing functions have been enhanced.
Key strengths:
- Improved error handling in many functions.
- Use of database transactions for complex operations.
- Addition of new functionality for satellite registration and management.
Areas for further improvement:
- Some long functions (e.g., registerSatelliteHandler) could benefit from refactoring into smaller, more focused functions.
- Consistency in error handling across all functions could be improved.
- Some functions (getSatelliteByID, deleteSatelliteByID) need proper implementation.
- Consider adding pagination to list operations for better scalability.
Overall, the changes are positive and enhance the capabilities of the server. Addressing the suggested improvements will further increase the code's maintainability and robustness.
ground-control/internal/utils/helper.go (1)
16-55: Review ofParseArtifactURLfunctionThe
ParseArtifactURLfunction is well-structured and effectively parses the artifact URL into its components. Apart from the issue noted below regarding the handling of both tag and digest, the function logic is sound.ground-control/internal/database/images.sql.go (3)
3-3: Update sqlc to version v1.27.0The sqlc code generator has been updated to
v1.27.0. Ensure that this version is compatible with your project and that any new features or changes are accounted for.
127-128: Verify that replacingGetImagesForSatellitewithListImagesis intentionalThe function
GetImagesForSatellitehas been replaced withListImages, which now retrieves all images without filtering by satellite. Please confirm that this change is intentional and verify that all call sites and dependent functionality have been updated accordingly to prevent unintended behavior.Run the following script to identify all usages of
GetImagesForSatellite:#!/bin/bash # Description: Find all references to the old `GetImagesForSatellite` function. # Test: Search for usages of `GetImagesForSatellite` in the codebase. rg --type go --word-regexp 'GetImagesForSatellite'
55-56: Ensure all references toDeleteImageListare updated toDeleteImageThe function
DeleteImageListhas been renamed toDeleteImage. Please verify that all references to the old function name have been updated throughout the codebase to prevent any undefined references or runtime errors.Run the following script to find any occurrences of
DeleteImageList:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
ground-control/internal/server/handlers.go (10)
142-162: Improved error handling and logging in addImageHandler.The changes to this function enhance error handling and logging, which is a good practice. The use of
ParseArtifactURLand customAppErrortype allows for more specific error responses.For consistency with other error handling in this file, consider wrapping the error returned by
ParseArtifactURLwithfmt.Errorf:image, err := utils.ParseArtifactURL(req.Image) if err != nil { log.Println(err) + err = fmt.Errorf("error: invalid artifact URL: %v", err) err = &AppError{ Message: "Error: Invalid Artifact URL", Code: http.StatusBadRequest, } HandleAppError(w, err) return }
177-187: Implement listImageHandler with proper error handling.The new
listImageHandlerfunction correctly retrieves and returns the list of images. The error handling follows the pattern used in other parts of the file.Based on a previous learning, consider modifying the error handling to create an
AppError:func (s *Server) listImageHandler(w http.ResponseWriter, r *http.Request) { result, err := s.dbQueries.ListImages(r.Context()) if err != nil { - err = fmt.Errorf("error: list images failed: %v", err) log.Println(err) + err = &AppError{ + Message: fmt.Sprintf("Failed to list images: %v", err), + Code: http.StatusInternalServerError, + } HandleAppError(w, err) return } WriteJSONResponse(w, http.StatusOK, result) }This change aligns with the
AppErrorusage in other parts of the file and provides more structured error information.
189-216: Well-implemented removeImageHandler with proper error handling.The
removeImageHandlerfunction is well-implemented with appropriate error handling for invalid image IDs and database operations. The specific handling ofsql.ErrNoRowsis a good practice.For consistency with other error handling in this file, consider using
AppErrorfor the invalid imageID case:id, err := strconv.ParseInt(imageID, 10, 32) if err != nil { - err = fmt.Errorf("error: invalid imageID: %v", err) + err = &AppError{ + Message: fmt.Sprintf("Invalid imageID: %v", err), + Code: http.StatusBadRequest, + } log.Println(err) HandleAppError(w, err) return }This change would make the error handling more consistent throughout the file.
374-435: Well-implemented ztrHandler with proper transaction management.The
ztrHandlerfunction is well-implemented with appropriate transaction management and error handling. It correctly handles the Zero Touch Registration (ZTR) process for satellites.For consistency with other error handling in this file, consider using
AppErrorfor the "Robot Account Not Found" case:if err != nil { log.Println("Robot Account Not Found") log.Println(err) - err := &AppError{ + err = &AppError{ Message: "Error: Robot Account Not Found", Code: http.StatusInternalServerError, } HandleAppError(w, err) tx.Rollback() return }This change would make the error handling more consistent throughout the file.
Line range hint
457-465: Incorrect implementation of deleteSatelliteByID.The
deleteSatelliteByIDfunction is incorrectly implemented. It's currently using theListSatellitesquery instead of deleting a specific satellite by ID.Implement the correct logic for deleting a satellite by ID:
func (s *Server) deleteSatelliteByID(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) idStr := vars["id"] id, err := strconv.ParseInt(idStr, 10, 32) if err != nil { HandleAppError(w, &AppError{ Message: "Invalid satellite ID", Code: http.StatusBadRequest, }) return } err = s.dbQueries.DeleteSatellite(r.Context(), int32(id)) if err != nil { if err == sql.ErrNoRows { HandleAppError(w, &AppError{ Message: "Satellite not found", Code: http.StatusNotFound, }) } else { HandleAppError(w, err) } return } WriteJSONResponse(w, http.StatusOK, map[string]string{"message": "Satellite deleted successfully"}) }This implementation assumes the existence of a
DeleteSatellitemethod in your database queries. If it doesn't exist, you'll need to add it to your database layer.
577-600: Well-implemented listGroupImages function with proper error handling.The
listGroupImagesfunction is correctly implemented with appropriate error handling for invalid group IDs and database operations. It follows the established patterns in the file for parameter parsing, error handling, and response writing.For consistency with other error handling in this file, consider using
HandleAppErrorfor the invalid groupID case:id, err := strconv.ParseInt(groupID, 10, 32) if err != nil { - log.Printf("Error: Invalid groupID: %v", err) - err := &AppError{ + log.Println(err) + err = &AppError{ Message: "Error: Invalid GroupID", Code: http.StatusBadRequest, } HandleAppError(w, err) return }This change would make the error handling more consistent throughout the file.
602-616: Remove commented-out GetImagesForSatellite function.The
GetImagesForSatellitefunction is currently commented out. Keeping commented-out code in the codebase can lead to confusion and clutter.If this function is no longer needed, consider removing it entirely. If it's intended for future use, consider moving it to a separate file or adding a TODO comment explaining why it's kept and when it might be used.
-// func (s *Server) GetImagesForSatellite(w http.ResponseWriter, r *http.Request) { -// token, err := GetAuthToken(r) -// if err != nil { -// HandleAppError(w, err) -// return -// } -// result, err := s.dbQueries.GetImagesForSatellite(r.Context(), token) -// if err != nil { -// log.Printf("Error: Failed to get image for satellite: %v", err) -// HandleAppError(w, err) -// return -// } -// -// WriteJSONResponse(w, http.StatusOK, result) -// }
629-631: Improved security by using environment variables for Harbor credentials.The use of environment variables for Harbor credentials and URL is a good security practice. It prevents hardcoding sensitive information in the source code.
Consider adding validation for the username and password to ensure they are not empty:
username := os.Getenv("HARBOR_USERNAME") password := os.Getenv("HARBOR_PASSWORD") url := os.Getenv("HARBOR_URL") + if username == "" || password == "" { + err := &AppError{ + Message: "Missing Harbor credentials", + Code: http.StatusInternalServerError, + } + HandleAppError(w, err) + return + } if url == "" { err := &AppError{ Message: "Missing URL in Request", Code: http.StatusBadRequest, } HandleAppError(w, err) return }This addition will ensure that the function fails gracefully if the Harbor credentials are not properly set in the environment.
655-661: Improved getGroupHandler with proper group ID parsing.The changes to
getGroupHandlercorrectly parse the group ID from the URL parameters, which is an improvement. However, the error handling can be made more consistent with the rest of the file.Consider updating the error handling to use
AppErrorandHandleAppErrorfor consistency:groupID, err := strconv.ParseInt(group, 10, 32) if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + log.Println(err) + HandleAppError(w, &AppError{ + Message: "Invalid group ID", + Code: http.StatusBadRequest, + }) return } result, err := s.dbQueries.GetGroupByID(r.Context(), int32(groupID)) if err != nil { - http.Error(w, err.Error(), http.StatusNotFound) + log.Println(err) + if err == sql.ErrNoRows { + HandleAppError(w, &AppError{ + Message: "Group not found", + Code: http.StatusNotFound, + }) + } else { + HandleAppError(w, err) + } return }These changes will make the error handling in
getGroupHandlermore consistent with the rest of the file.
Line range hint
1-707: Overall assessment of changes in handlers.goThe changes in this file significantly enhance the functionality of the server, particularly in areas of satellite registration, image management, and group handling. Here's a summary of the key points:
- Improved security practices, such as using environment variables for sensitive information.
- Consistent use of transactions for complex operations, ensuring data integrity.
- Generally good error handling practices, with some room for improvement in consistency.
- New functions that expand the server's capabilities in managing satellites, images, and groups.
While the individual functions are generally well-implemented, there are opportunities to improve the overall architecture:
- Consider breaking down large functions (e.g.,
registerSatelliteHandler) into smaller, more focused functions to improve maintainability.- Implement a consistent error handling strategy across all functions, preferably using the
AppErrortype andHandleAppErrorfunction.- Consider creating a separate package for database operations to better separate concerns and improve modularity.
- Implement comprehensive logging throughout the handlers for better observability and debugging.
These architectural improvements will make the codebase more maintainable, easier to test, and more scalable as the project grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- ground-control/internal/server/handlers.go (10 hunks)
- ground-control/internal/utils/helper.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ground-control/internal/utils/helper.go
🧰 Additional context used
📓 Learnings (1)
ground-control/internal/server/handlers.go (1)
Learnt from: bupd PR: container-registry/harbor-satellite#54 File: ground-control/internal/server/handlers.go:176-186 Timestamp: 2024-09-30T06:15:48.512Z Learning: In `listImageHandler`, the error should be wrapped with `fmt.Errorf` before being passed to `HandleAppError`.
🔇 Additional comments (4)
ground-control/internal/server/handlers.go (4)
437-445: Correctly implemented listSatelliteHandler.The
listSatelliteHandlerfunction is correctly implemented, retrieving and returning the list of satellites with proper error handling.
555-575: Well-implemented deleteImageFromGroup function.The
deleteImageFromGroupfunction is correctly implemented with proper error handling and database interaction. It follows the established patterns in the file for request decoding, error handling, and response writing.
671-684: Well-implemented GenerateRandomToken function.The
GenerateRandomTokenfunction is correctly implemented, usingcrypto/randfor secure random number generation. It properly handles the conversion of bytes to a hex-encoded string, ensuring the generated token has the specified character length.This implementation is secure and follows best practices for generating random tokens.
33-36: 🛠️ Refactor suggestionConsider using value types instead of pointer types for slices.
The
GroupsandLabelsfields in theRegisterSatelliteParamsstruct are defined as pointers to slices (*[]string). In Go, slices are already reference types, so using pointers to slices is usually unnecessary and can complicate the code.Consider simplifying the struct definition:
type RegisterSatelliteParams struct { Name string `json:"satellite_name"` - Groups *[]string `json:"groups,omitempty"` - Labels *[]string `json:"labels,omitempty"` + Groups []string `json:"groups,omitempty"` + Labels []string `json:"labels,omitempty"` }This change will simplify the code and reduce the need for nil checks when accessing these fields.
Likely invalid or redundant comment.
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 39
🧹 Outside diff range and nitpick comments (26)
ground-control/sql/schema/012_robot_projects.sql (1)
3-7: Consider enhancing the table for Zero Touch Registration.
The table structure is well-designed with appropriate foreign key constraints and composite primary key. However, for a Zero Touch Registration system, consider these enhancements:
- Add audit timestamps to track automated registrations:
CREATE TABLE robot_projects (
robot_id INT REFERENCES robots(id) ON DELETE CASCADE,
project_id INT REFERENCES projects(id) ON DELETE CASCADE,
created_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP,
registered_by VARCHAR(255), -- to track if registration was automatic or manual
PRIMARY KEY (robot_id, project_id)
);- Add an index to optimize project-based queries:
CREATE INDEX idx_robot_projects_project_id ON robot_projects(project_id);ground-control/sql/schema/002_groups.sql (1)
Line range hint 1-13: Document schema design decisions.
Please add comments to document:
- The purpose of the
registry_url(format expectations, usage) - The structure of the
projectsarray (format, limitations) - The relationship between groups, projects, and the Zero Touch Registration flow
-- +goose Up
+ -- groups table stores satellite group configurations for Zero Touch Registration
+ -- registry_url: The URL of the registry service (must be valid HTTP/HTTPS URL)
+ -- projects: Array of project identifiers associated with this group
CREATE TABLE groups (ground-control/.env (1)
6-7: Document the purpose of ZOT Registry
Since this PR implements Zero Touch Registration, it would be helpful to document how the ZOT Registry relates to this feature.
Add a comment explaining:
- The role of ZOT Registry in the Zero Touch Registration process
- Whether this is a required component for ZTR
ground-control/sql/queries/satellite_groups.sql (1)
Line range hint 1-4: LGTM! The insert operation handles conflicts appropriately.
The ON CONFLICT DO NOTHING clause ensures idempotency, which is crucial for zero-touch registration scenarios where multiple registration attempts might occur.
Consider adding foreign key constraints to ensure referential integrity:
ALTER TABLE satellite_groups
ADD CONSTRAINT fk_satellite
FOREIGN KEY (satellite_id)
REFERENCES satellites(id),
ADD CONSTRAINT fk_group
FOREIGN KEY (group_id)
REFERENCES groups(id);ground-control/sql/queries/satellite_token.sql (3)
6-9: Add index on token column for performance.
Since this query filters by token, which is likely to be frequently used for lookups, consider adding an index on the token column in your schema file.
CREATE INDEX idx_satellite_token_token ON satellite_token(token);18-20: Consider implementing soft delete.
For Zero Touch Registration (ZTR) systems, maintaining an audit trail is crucial. Consider implementing soft delete by adding a deleted_at timestamp column instead of physically removing the records.
-DELETE FROM satellite_token
-WHERE token = $1;
+UPDATE satellite_token
+SET deleted_at = NOW()
+WHERE token = $1 AND deleted_at IS NULL;This would require schema modification to add the deleted_at column and updating other queries to include WHERE deleted_at IS NULL.
1-20: Add constraints for token column.
Since this is part of an authentication system, consider adding these constraints to your schema:
- Make token column unique to prevent duplicates
- Add a minimum length requirement
- Consider adding a check constraint for token format if you're using a specific pattern
ALTER TABLE satellite_token
ADD CONSTRAINT unique_token UNIQUE (token),
ADD CONSTRAINT check_token_length CHECK (length(token) >= 32);ground-control/sql/queries/satellites.sql (1)
Line range hint 1-28: Consider documenting the new satellite authentication flow.
The changes show a shift from token-based to a new authentication mechanism for Zero Touch Registration. Consider:
- Documenting the new authentication flow in the README
- Adding migration guides for existing satellite registrations
- Updating any API documentation to reflect these changes
Would you like me to help create documentation templates for these changes?
ground-control/internal/models/models.go (3)
3-7: Consider improving field naming clarity in StateArtifact struct.
The struct fields could be more descriptive to better reflect their roles in satellite registration:
Group- Is this a satellite group? Consider a more specific name likeSatelliteGroupRegistry- The purpose isn't immediately clear. ConsiderRegistryURLorArtifactRegistryif it's a container registry
18-21: Clarify the purpose of States field in ZtrResult.
The States field name is ambiguous. If these are registration states or status messages, consider renaming to be more specific (e.g., RegistrationStates or StatusMessages).
23-26: Enhance security measures for Account struct.
The Account struct handles sensitive data but lacks validation constraints. Consider:
- Adding field validation tags (e.g., using
validatorpackage) - Adding documentation about secret handling
- Implementing methods to safely handle the secret (e.g., masking in logs)
Example implementation with validation:
type Account struct {
// Name represents the account identifier for satellite registration
Name string `json:"name" validate:"required,min=3,max=64"`
// Secret is the authentication token. Should be handled securely and never logged.
Secret string `json:"secret" validate:"required,min=32"`
}
// String implements Stringer interface to prevent accidental secret logging
func (a Account) String() string {
return fmt.Sprintf("Account{Name: %s, Secret: [REDACTED]}", a.Name)
}ground-control/reg/harbor/project.go (1)
20-22: Enhance error context
The error message could provide more context about what was being attempted.
- return nil, fmt.Errorf("error: listing projects: %v", err)
+ return nil, fmt.Errorf("error: failed to list Harbor projects: %w", err)ground-control/reg/harbor/replication.go (2)
14-28: Add parameter validation and consider pagination limits.
The function accepts ListParams without validation. Consider adding checks for:
- Negative page numbers
- Reasonable page size limits
- Valid sort parameters
Here's a suggested improvement:
func ListReplication(ctx context.Context, opts ListParams, client *v2client.HarborAPI) ([]*models.ReplicationPolicy, error) {
+ if opts.Page < 1 {
+ opts.Page = 1
+ }
+ if opts.PageSize < 1 || opts.PageSize > 100 {
+ opts.PageSize = 100
+ }
+
response, err := client.Replication.ListReplicationPolicies(24-26: Enhance error context for debugging.
The error wrapping could provide more context about the operation and parameters.
- return nil, fmt.Errorf("error: listing replication policies: %v", err)
+ return nil, fmt.Errorf("failed to list replication policies (page: %d, size: %d, query: %s): %w",
+ opts.Page, opts.PageSize, opts.Q, err)ground-control/sql/queries/robot_accounts.sql (1)
22-24: Consider implementing soft delete.
For audit purposes and data recovery, consider implementing soft delete instead of hard delete. This is especially important for robot accounts that might have historical data or audit requirements.
Consider adding a deleted_at timestamp:
- DELETE FROM robot_accounts
- WHERE id = $1;
+ UPDATE robot_accounts
+ SET deleted_at = NOW()
+ WHERE id = $1 AND deleted_at IS NULL;ground-control/internal/server/routes.go (1)
24-25: Implement the group satellite listing functionality.
The TODO comment indicates a missing feature for listing satellites in a group, but the commented route already provides the solution.
Consider implementing this endpoint as it's a crucial functionality for group management. Would you like me to:
- Generate the handler implementation for this endpoint?
- Open a GitHub issue to track this task?
ground-control/reg/adapter.go (1)
19-24: LGTM! Structure follows container image naming conventions.
The Images struct correctly represents container image metadata with appropriate serialization tags and optional fields.
Consider adding documentation to explain the purpose and usage of this struct:
+// Images represents the components of a container image location,
+// including the registry host, repository path, and optional tag or digest.
+// This is used for managing container images during satellite registration.
type Images struct {ground-control/internal/database/satellite_token.sql.go (1)
40-51: Implement rate limiting for token lookups.
Frequent token lookups could be used for brute force attacks to discover valid tokens. Consider implementing rate limiting at the database or application level.
Consider:
- Adding rate limiting middleware
- Implementing token lookup caching
- Adding logging for security audits (but ensure tokens are not logged in plaintext)
ground-control/internal/database/satellite_groups.sql.go (1)
Line range hint 1-97: Architecture aligns well with Zero Touch Registration requirements.
The implementation provides a solid foundation for satellite group management, which is crucial for Zero Touch Registration (ZTR). The queries support:
- Dynamic group assignment during automatic registration
- Flexible group management with add/remove capabilities
- Efficient group membership lookups in both directions (satellite→groups and group→satellites)
These features align well with the ZTR objectives from issues #41 and #36.
ground-control/internal/database/satellites.sql.go (2)
57-63: LGTM! Consider adding an index on the name column.
The implementation looks correct. Since this function performs lookups by name, consider adding an index on the name column if not already present, especially if this query is frequently used.
80-82: Remove commented query from source SQL file.
These comments appear in the generated code, suggesting they exist in the source SQL file. Since the token-based lookup has been removed, these comments should be cleaned up from the source SQL file to maintain clarity.
ground-control/reg/harbor/robot.go (3)
88-99: Add input validation for robot ID.
Consider validating that the id parameter is positive before making the API call.
func GetRobotAccount(ctx context.Context, id int64, client *v2client.HarborAPI) (*models.Robot, error) {
+ if id <= 0 {
+ return nil, fmt.Errorf("invalid robot ID: must be positive")
+ }
response, err := client.Robot.GetRobotByID(Line range hint 128-144: Consider making access permissions configurable.
The hardcoded permissions array could be made configurable to support different robot account types with varying access levels.
+// RobotAccessLevel defines the set of permissions for a robot account
+type RobotAccessLevel struct {
+ Action string
+ Resource string
+}
+
+// DefaultRobotAccess provides the default minimal permissions
+var DefaultRobotAccess = []RobotAccessLevel{
+ {Action: "read", Resource: "artifact"},
+ {Action: "read", Resource: "repository"},
+ {Action: "pull", Resource: "repository"},
+}
+
-func GenRobotPerms(projects []string) []*models.RobotPermission {
- robotAccess := []*models.Access{
- {Action: "read", Resource: "artifact"},
- {Action: "read", Resource: "repository"},
- {Action: "pull", Resource: "repository"},
- }
+func GenRobotPerms(projects []string, access []RobotAccessLevel) []*models.RobotPermission {
+ robotAccess := make([]*models.Access, len(access))
+ for i, a := range access {
+ robotAccess[i] = &models.Access{Action: a.Action, Resource: a.Resource}
+ }Line range hint 1-144: Consider documenting the ZTR workflow.
This file plays a crucial role in Zero Touch Registration by managing Harbor robot accounts. Consider adding documentation that explains:
- The lifecycle of robot accounts in the ZTR process
- The relationship between satellites and robot accounts
- The security model and access control strategy
ground-control/internal/database/robot_accounts.sql.go (2)
60-78: Use consistent naming for 'RobotAccount', avoid abbreviations
The function GetRobotAccBySatelliteID and the constant getRobotAccBySatelliteID use the abbreviation 'Acc'. For clarity and consistency with other functions like GetRobotAccount, consider using 'Account' instead of 'Acc'.
Apply this diff to rename the function and constant:
-const getRobotAccBySatelliteID = `-- name: GetRobotAccBySatelliteID :one
+const getRobotAccountBySatelliteID = `-- name: GetRobotAccountBySatelliteID :one
SELECT id, robot_name, robot_secret, robot_id, satellite_id, created_at, updated_at FROM robot_accounts
WHERE satellite_id = $1
`
-func (q *Queries) GetRobotAccBySatelliteID(ctx context.Context, satelliteID int32) (RobotAccount, error) {
- row := q.db.QueryRowContext(ctx, getRobotAccBySatelliteID, satelliteID)
+func (q *Queries) GetRobotAccountBySatelliteID(ctx context.Context, satelliteID int32) (RobotAccount, error) {
+ row := q.db.QueryRowContext(ctx, getRobotAccountBySatelliteID, satelliteID)
var i RobotAccount
err := row.Scan(
&i.ID,
&i.RobotName,
&i.RobotSecret,
&i.RobotID,
&i.SatelliteID,
&i.CreatedAt,
&i.UpdatedAt,
)
return i, err
}135-159: Consider allowing satellite_id to be updated in UpdateRobotAccount
The current updateRobotAccount query does not update the satellite_id. If there's a requirement to modify the satellite_id of an existing robot account, include it in the SET clause and adjust the parameters accordingly.
Apply this diff to include satellite_id in the update:
UPDATE robot_accounts
SET robot_name = $2,
robot_secret = $3,
robot_id = $4,
+ satellite_id = $5,
updated_at = NOW()
WHERE id = $1Update the UpdateRobotAccountParams struct to include SatelliteID:
type UpdateRobotAccountParams struct {
ID int32
RobotName string
RobotSecret string
RobotID string
+ SatelliteID int32
}Adjust the function call to pass the new parameter:
func (q *Queries) UpdateRobotAccount(ctx context.Context, arg UpdateRobotAccountParams) error {
_, err := q.db.ExecContext(ctx, updateRobotAccount,
arg.ID,
arg.RobotName,
arg.RobotSecret,
arg.RobotID,
+ arg.SatelliteID,
)
return err
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
ground-control/go.sumis excluded by!**/*.sum
📒 Files selected for processing (44)
- ground-control/.env (1 hunks)
- ground-control/go.mod (2 hunks)
- ground-control/internal/database/db.go (1 hunks)
- ground-control/internal/database/group_images.sql.go (0 hunks)
- ground-control/internal/database/groups.sql.go (4 hunks)
- ground-control/internal/database/images.sql.go (0 hunks)
- ground-control/internal/database/label_images.sql.go (0 hunks)
- ground-control/internal/database/labels.sql.go (0 hunks)
- ground-control/internal/database/models.go (2 hunks)
- ground-control/internal/database/robot_accounts.sql.go (1 hunks)
- ground-control/internal/database/satellite_groups.sql.go (2 hunks)
- ground-control/internal/database/satellite_labels.sql.go (0 hunks)
- ground-control/internal/database/satellite_token.sql.go (1 hunks)
- ground-control/internal/database/satellites.sql.go (3 hunks)
- ground-control/internal/models/models.go (1 hunks)
- ground-control/internal/server/handlers.go (4 hunks)
- ground-control/internal/server/routes.go (1 hunks)
- ground-control/internal/utils/helper.go (1 hunks)
- ground-control/reg/adapter.go (1 hunks)
- ground-control/reg/harbor/project.go (1 hunks)
- ground-control/reg/harbor/replication.go (1 hunks)
- ground-control/reg/harbor/robot.go (5 hunks)
- ground-control/sql/queries/group_images.sql (0 hunks)
- ground-control/sql/queries/groups.sql (2 hunks)
- ground-control/sql/queries/images.sql (0 hunks)
- ground-control/sql/queries/label_images.sql (0 hunks)
- ground-control/sql/queries/labels.sql (0 hunks)
- ground-control/sql/queries/robot_accounts.sql (1 hunks)
- ground-control/sql/queries/satellite_groups.sql (1 hunks)
- ground-control/sql/queries/satellite_labels.sql (0 hunks)
- ground-control/sql/queries/satellite_token.sql (1 hunks)
- ground-control/sql/queries/satellites.sql (1 hunks)
- ground-control/sql/schema/001_satellites.sql (1 hunks)
- ground-control/sql/schema/002_groups.sql (1 hunks)
- ground-control/sql/schema/003_labels.sql (0 hunks)
- ground-control/sql/schema/005_satellite_labels.sql (0 hunks)
- ground-control/sql/schema/006_images.sql (0 hunks)
- ground-control/sql/schema/007_group_images.sql (0 hunks)
- ground-control/sql/schema/008_label_images.sql (0 hunks)
- ground-control/sql/schema/010_robots.sql (1 hunks)
- ground-control/sql/schema/011_projects.sql (1 hunks)
- ground-control/sql/schema/012_robot_projects.sql (1 hunks)
- ground-control/sql/schema/013_robot_accounts.sql (1 hunks)
- ground-control/sql/schema/014_satellite_token.sql (1 hunks)
💤 Files with no reviewable changes (15)
- ground-control/internal/database/group_images.sql.go
- ground-control/internal/database/images.sql.go
- ground-control/internal/database/label_images.sql.go
- ground-control/internal/database/labels.sql.go
- ground-control/internal/database/satellite_labels.sql.go
- ground-control/sql/queries/group_images.sql
- ground-control/sql/queries/images.sql
- ground-control/sql/queries/label_images.sql
- ground-control/sql/queries/labels.sql
- ground-control/sql/queries/satellite_labels.sql
- ground-control/sql/schema/003_labels.sql
- ground-control/sql/schema/005_satellite_labels.sql
- ground-control/sql/schema/006_images.sql
- ground-control/sql/schema/007_group_images.sql
- ground-control/sql/schema/008_label_images.sql
✅ Files skipped from review due to trivial changes (1)
- ground-control/internal/database/db.go
🚧 Files skipped from review as they are similar to previous changes (5)
- ground-control/internal/database/models.go
- ground-control/sql/schema/001_satellites.sql
- ground-control/sql/schema/010_robots.sql
- ground-control/sql/schema/011_projects.sql
- ground-control/sql/schema/014_satellite_token.sql
🔇 Additional comments (41)
ground-control/sql/schema/012_robot_projects.sql (1)
9-10: LGTM!
The down migration correctly drops the table.
ground-control/sql/schema/002_groups.sql (1)
Line range hint 1-13: Verify the impact on existing data and handlers.
The addition of non-nullable columns without defaults requires verification:
- Ensure existing data migration strategy is in place
- Verify handlers are updated to provide these new required fields
✅ Verification successful
Let me check the content of the found files to better understand the situation.
Migration and handlers are properly aligned with the schema changes
The verification shows that:
- This is the first migration creating the groups table (002_groups.sql is the only file with CREATE TABLE groups)
- All required handlers are properly implemented in the generated sqlc code (groups.sql.go) with correct field mappings
- The queries (groups.sql) handle all the new fields appropriately, including an upsert strategy for handling conflicts
No data migration is needed since this is the initial creation of the groups table, and all handlers are correctly generated to work with the new schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing groups table and verify handler updates
# Test 1: Check if groups table exists and has data
echo "Checking for existing groups table..."
find . -type f -name "*.sql" -exec grep -l "INSERT INTO groups" {} \;
# Test 2: Verify handlers are updated to handle new fields
echo "Checking handlers for new fields..."
rg -l "registry_url|projects.*groups" --type go
# Test 3: Check for migration scripts
echo "Checking for data migration scripts..."
find . -type f -name "*migrate*" -o -name "*upgrade*" -exec grep -l "groups" {} \;
Length of output: 550
Script:
#!/bin/bash
# Check the content of found files
echo "Content of groups.sql:"
cat ./ground-control/sql/queries/groups.sql
echo -e "\nContent of groups.sql.go:"
cat ./ground-control/internal/database/groups.sql.go
# Check if this is the first migration creating the groups table
echo -e "\nChecking for previous groups table migrations:"
find . -type f -name "*.sql" -exec grep -l "CREATE TABLE groups" {} \;
Length of output: 3768
ground-control/.env (1)
13-14: Review the synchronization interval for production readiness
A 10-second sync interval might be too aggressive for production use, potentially causing unnecessary load on both the Ground Control system and the registry.
Let's check if this interval is used in any critical operations:
Consider:
- Making this value environment-specific (higher in production)
- Adding documentation about its purpose and impact
- Implementing exponential backoff for failed operations
ground-control/sql/schema/013_robot_accounts.sql (1)
3-11: LGTM on the overall table structure.
The table design effectively supports Zero Touch Registration (ZTR) for satellites with:
- Proper primary key and unique constraints
- Foreign key relationship with cascading deletes
- Audit timestamps for tracking records
ground-control/sql/queries/satellite_groups.sql (1)
14-16: LGTM! The delete operation is precise.
The compound WHERE clause ensures accurate removal of the specific satellite-group association.
Let's verify the table structure and constraints:
✅ Verification successful
The delete operation is safe and well-designed
The verification confirms that the RemoveSatelliteFromGroup query is properly designed:
- The table has a composite primary key (satellite_id, group_id)
- Both columns are foreign keys with CASCADE delete behavior
- The WHERE clause in the delete operation matches the primary key structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify satellite_groups table structure and constraints
# Test: Check if the table exists and its structure
rg -A 10 "CREATE TABLE.*satellite_groups" ground-control/sql/schema/
# Test: Check for existing foreign key constraints
rg "FOREIGN KEY.*satellite_groups" ground-control/sql/schema/
Length of output: 781
ground-control/sql/queries/satellite_token.sql (1)
15-16: Previous review comment about pagination and column specification still applies.
ground-control/sql/queries/groups.sql (2)
22-24: Consider data consistency implications of group deletion.
The DELETE operation might leave orphaned satellite records. Consider implementing one of these solutions:
- Add ON DELETE CASCADE to the foreign key constraints in related tables
- Implement a transaction that handles cleanup of related records
Let's check for related tables and constraints:
✅ Verification successful
The previous output showed a partial result indicating there's at least one table satellite_groups with ON DELETE CASCADE, but let's get the complete picture of the database schema to make a proper assessment.
ON DELETE CASCADE is properly implemented - no data consistency issues
The schema shows that the satellite_groups junction table, which is the only table directly referencing the groups table, has the appropriate ON DELETE CASCADE constraint:
CREATE TABLE satellite_groups (
satellite_id INT REFERENCES satellites(id) ON DELETE CASCADE,
group_id INT REFERENCES groups(id) ON DELETE CASCADE,
PRIMARY KEY (satellite_id, group_id)
);This ensures that when a group is deleted, all related records in the satellite_groups table will be automatically removed, maintaining referential integrity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related tables that might be affected by group deletion
# Expected: Find foreign key constraints referencing groups table
# Search for table definitions referencing groups
fd -e sql -x rg -A 10 'REFERENCES\s+groups'
# Search for existing transaction handling
rg -A 5 'BEGIN|START TRANSACTION'
Length of output: 319
Script:
#!/bin/bash
# Let's search more thoroughly for table definitions and foreign keys
# Look for any SQL files that might contain table definitions
fd -e sql -x cat {} \; | grep -A 10 -B 2 'CREATE TABLE'
# Also specifically look for satellite_groups table definition
fd -e sql -x rg -l 'satellite_groups'
Length of output: 2666
2-8: LGTM! Verify registry_url format validation.
The UPSERT logic for groups with registry URL and projects looks good. However, ensure that registry URL validation is implemented in the application layer.
Let's verify the validation implementation:
✅ Verification successful
Registry URL validation is implemented correctly
The codebase has proper URL validation through the IsValidURL function in internal/utils/utils.go which validates URLs using Go's net/url package. Additionally, the ParseArtifactURL function in ground-control/internal/utils/helper.go provides thorough URL parsing and validation for registry URLs.
Key validations in place:
- URL scheme verification
- Registry host presence check
- Repository path validation
- Tag/digest validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for registry URL validation in the codebase
# Expected: Find validation logic for registry URL format
# Search for registry URL validation patterns
rg -A 5 'registry.*url.*valid|valid.*registry.*url'
# Search for URL validation utilities/helpers
ast-grep --pattern 'func $_(url string) bool {
$$$
}'
Length of output: 319
Script:
#!/bin/bash
# Let's try a different approach to find URL validation logic
# Search for URL-related validation patterns in Go files
rg -t go "validate.*url|isValid.*url|checkURL|validateURL" -A 5
# Search for URL parsing or validation imports
rg -t go "net/url|validator"
# Look for struct field tags related to URL validation
rg -t go 'RegistryURL.*`.*validate:".*"`'
# Check for URL-related constants or patterns
rg "(?i)(registry.*url.*pattern|url.*pattern|url.*regex)"
# Look for group-related validation logic
fd -e go -x rg -l "group.*validate|validate.*group"
Length of output: 340
Script:
#!/bin/bash
# Let's check the content of files that import net/url to see how it's used
# Check utils.go content
echo "=== Content of internal/utils/utils.go ==="
cat internal/utils/utils.go
# Check helper.go content
echo "=== Content of ground-control/internal/utils/helper.go ==="
cat ground-control/internal/utils/helper.go
# Look for any group-related structs and their validation
rg -t go "type.*Group.*struct" -A 10
Length of output: 15597
ground-control/sql/queries/satellites.sql (3)
9-11: LGTM! Well-structured query for satellite retrieval.
The query follows best practices with:
- Proper parameter usage for safe querying
- LIMIT 1 for efficient single record retrieval
2-3: Consider adding name validation and uniqueness constraints.
While removing the token field aligns with the new ZTR approach, consider:
- Adding input validation for the satellite name
- Ensuring uniqueness constraints are in place to prevent duplicate satellite names
Let's verify if these constraints exist in the schema:
17-19: Verify token functionality migration before removing commented code.
The commented-out token query suggests a transition in the authentication mechanism. Let's verify that the token functionality has been properly migrated to the new system.
Let's check the new token implementation:
✅ Verification successful
Safe to remove the commented code - token functionality has been properly migrated
The token functionality has been successfully migrated to a dedicated satellite_token table with proper queries implemented in satellite_token.sql. The new implementation includes:
- A dedicated table with proper foreign key constraints
- Complete set of CRUD operations for tokens
GetSatelliteIDByTokenquery that replaces the oldGetSatelliteByTokenfunctionality
The commented code in satellites.sql can be safely removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for the new satellite_token table and related queries
echo "Checking for satellite_token table:"
rg -A 10 "CREATE TABLE satellite_token" ground-control/sql/schema/
echo "Checking for token-related queries:"
rg -l "satellite_token" ground-control/sql/queries/
Length of output: 1135
Script:
#!/bin/bash
# Let's check the content of satellite_token.sql to verify the token-related queries
echo "Content of satellite_token.sql:"
cat ground-control/sql/queries/satellite_token.sql
# Also check if there are any remaining token-related queries in satellites.sql
echo -e "\nRemaining token references in satellites.sql:"
cat ground-control/sql/queries/satellites.sql
Length of output: 1306
ground-control/internal/models/models.go (1)
1-27: Verify integration with satellite registration workflow.
The models seem to be missing explicit satellite identification or registration status fields. Let's verify how these models integrate with the satellite registration process.
ground-control/reg/harbor/project.go (2)
1-10: LGTM: Clean package structure and imports
The imports are well-organized and use specific versioning for the Harbor client.
12-30: Verify integration with Zero Touch Registration flow
The function is part of the ZTR implementation but its relationship to the satellite registration process isn't immediately clear. We should verify how this integrates with the overall ZTR flow.
Let's check for references to this function in the codebase:
✅ Verification successful
Integration with Zero Touch Registration is verified and properly implemented
The ListProjects function is correctly integrated into the ZTR flow through the following chain:
- The
registerSatelliteHandlerinitiates satellite registration - It calls
CreateRobotAccForSatellitewhich creates Harbor robot accounts for satellites - The robot account creation process uses
RobotAccountTemplateto set up proper permissions - While
ListProjectsis available, it's intentionally not used in the registration flow as projects are derived from the provided repositories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to ListProjects in the codebase
rg -l "ListProjects.*harbor" --type go
# Look for ZTR-related code that might use this function
rg -l "zero.*touch|ZTR" --type go
Length of output: 76
Script:
#!/bin/bash
# Let's try a broader search for ListProjects function usage
rg "ListProjects" --type go -A 3
# Search for harbor project related imports and usage
rg "harbor.*project" --type go
# Search for registration or robot related code that might use this
rg "RobotCreate|registration" --type go
Length of output: 1706
Script:
#!/bin/bash
# Let's examine the robot account creation flow and its relationship with ListProjects
rg -A 10 "CreateRobotAccForSatellite" --type go
# Look at the robot account template implementation
rg -A 10 "RobotAccountTemplate" --type go
# Check for satellite registration related code
rg -A 5 "satellite.*registration|register.*satellite" --type go -i
Length of output: 4917
ground-control/reg/harbor/replication.go (1)
14-28: Verify the role of replication policies in ZTR architecture.
This function appears to be part of the Zero Touch Registration implementation, potentially managing satellite image distribution.
Let's verify the usage of replication policies in the codebase:
ground-control/sql/queries/robot_accounts.sql (1)
1-32: Consider implementing comprehensive data security and audit measures.
For a Zero Touch Registration system handling robot accounts, consider implementing these security measures across all queries:
- Add appropriate indexes for
robot_idandsatellite_id - Implement row-level security policies
- Add audit logging for sensitive operations
- Consider encryption for
robot_secret
Let's verify the current security measures:
ground-control/internal/server/routes.go (2)
30-31: Verify the dual registration flow implementation.
The code implements two registration paths: regular registration and Zero Touch Registration (ZTR). Let's verify that both flows are properly implemented.
#!/bin/bash
# Description: Verify the implementation of both registration flows
# Test 1: Check the registration handler implementations
echo "Checking registration handler implementations:"
ast-grep --pattern 'func $_(s *Server) registerSatelliteHandler($$$) {
$$$
}'
echo "\nChecking ZTR handler implementations:"
ast-grep --pattern 'func $_(s *Server) ztrHandler($$$) {
$$$
}'
# Test 2: Look for registration-related database operations
echo "\nChecking database operations:"
rg -A 5 "INSERT INTO.*satellites"The previous review comment about missing ZTR documentation is still valid. Please add documentation explaining:
- The purpose and process of ZTR
- How it differs from regular registration
- Any specific requirements or prerequisites
32-34: LGTM! The satellite management endpoints follow REST conventions.
The endpoints for listing, getting, and deleting satellites are well-structured and follow REST best practices:
- Use of plural resource names
- Proper HTTP methods
- Consistent URL patterns
ground-control/reg/adapter.go (1)
19-24: Verify integration with satellite registration flow.
The Images struct appears to be part of the Zero Touch Registration (ZTR) process for satellites. Let's verify how it's being used in the registration flow.
✅ Verification successful
The Images struct is properly integrated with the satellite workflow
The verification shows that the Images struct is well-integrated into the system through the ParseArtifactURL utility function, which properly validates:
- Required fields (repository and registry)
- Presence of either tag or digest
- URL format and structure
The struct is used as part of the artifact URL parsing flow, which is a critical component for satellite image management. The implementation includes appropriate error handling and validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how Images struct is used in the registration process
# Look for registration handlers or services using this struct
echo "Searching for registration-related code using Images struct..."
rg -l "reg\.Images" | grep -i "registration\|register"
# Look for tests to ensure proper validation
echo "Searching for related tests..."
rg -l "reg\.Images" | grep "_test.go"
Length of output: 300
Script:
#!/bin/bash
# Let's try a broader search to understand the usage of Images struct
# Search for direct struct usage without the package prefix
echo "Searching for Images struct usage..."
rg -A 5 "type Images struct"
# Search for any files importing the reg package
echo "Searching for imports of reg package..."
rg -l "\".*ground-control/reg\""
# Search for files containing "Images" in ground-control directory
echo "Searching for files with Images references..."
rg -l "Images" ./ground-control/
# Look for any registration or satellite related files
echo "Searching for registration/satellite related files..."
fd -t f "registration|satellite|reg" ./ground-control/
Length of output: 1627
Script:
#!/bin/bash
# Let's examine the actual usage of Images struct in the found files
# Check helper.go for Images usage
echo "Checking helper.go implementation..."
rg -A 10 "Images" ./ground-control/internal/utils/helper.go
# Check routes.go for Images usage
echo "Checking routes.go implementation..."
rg -A 10 "Images" ./ground-control/internal/server/routes.go
# Check for any satellite registration handlers
echo "Checking for satellite registration handlers..."
rg -A 10 "func.*Register.*Satellite" ./ground-control/internal/server/
Length of output: 1798
ground-control/go.mod (4)
33-33: Verify logging configuration.
The addition of klauspost/compress and sirupsen/logrus suggests compression and logging capabilities. Ensure proper log levels and compression settings are configured for production use.
#!/bin/bash
# Check for logging configuration
rg -A 5 "logrus.*New|logrus.*Level"Also applies to: 43-43
7-7: Review the container registry integration requirements.
The addition of github.com/google/go-containerregistry suggests container image operations will be part of the Zero Touch Registration flow. This is a solid choice for container operations, but ensure proper error handling is implemented when using this library.
#!/bin/bash
# Check for error handling patterns in container registry operations
rg -A 3 "go-containerregistry" | rg -B 2 "error|err"Line range hint 3-3: Verify Go version compatibility with deployment environment.
The module specifies Go 1.22.4, which is a very recent version. Ensure your deployment environment supports this version.
✅ Verification successful
Let me gather more information about Go version requirements by searching through deployment and configuration files.
Go version 1.22.4 is compatible with CI/CD requirements
The Go version 1.22.4 specified in go.mod is compatible with the deployment environment requirements. CI/CD workflows in .github/workflows/ specify Go version >=1.22, which is satisfied by version 1.22.4.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if README or deployment docs specify Go version requirements
rg -i "go.*version|golang.*version" README.md
Length of output: 2622
Script:
#!/bin/bash
# Search for Go version requirements in deployment files and documentation
fd -e yaml -e yml -e md | xargs rg -i "go.*version|golang.*version"
# Check if there's a Dockerfile that specifies Go version
fd Dockerfile | xargs cat
# Check if there are any CI configuration files that specify Go version
fd -e yml -e yaml . .github/ | xargs rg -i "go-version|golang-version"
Length of output: 515
16-17: Review compatibility of Docker and OCI dependencies.
Several Docker and OCI-related packages are marked as incompatible or using release candidates:
github.com/docker/cli v27.1.1+incompatiblegithub.com/opencontainers/image-spec v1.1.0-rc3
Consider:
- Upgrading to compatible versions if available
- Verifying these versions work well together
- Testing container operations thoroughly
Also applies to: 38-39
✅ Verification successful
This will help us understand:
- All related Docker/OCI dependencies and their versions
- Where these packages are actually being used
- The scope of container-related functionality in the codebase
This will help us understand:
- How these dependencies are actually being used in critical paths
- If there are any existing issues with these dependencies
- The architectural decisions that led to these dependency choices
Based on the gathered information, I can now provide a final response:
Docker and OCI dependencies are appropriately configured for the project's needs
The current dependency setup is intentional and working as designed:
- The project uses Crane (Google's container registry library) as the primary tool for image operations, as documented in ADR 0001
- The Docker/OCI dependencies are indirect requirements from these core libraries
- No errors or issues related to these dependencies are found in the codebase
- The image operations (replication, listing, deletion) are working as expected
The +incompatible markers are normal for Go modules when using packages that haven't fully adopted semantic import versioning, which is common in the Docker ecosystem. The current versions are stable releases, not release candidates as initially suspected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any reported issues with these dependencies
rg -l "docker/cli|image-spec" | xargs rg -B 2 "TODO|FIXME|BUG|HACK"
Length of output: 131
Script:
#!/bin/bash
# Check go.mod for all Docker and OCI-related dependencies
rg "github.com/(docker|opencontainers)" go.mod
# Check for actual usage of these packages in the codebase
rg -l "github.com/docker/cli|github.com/docker/distribution|github.com/opencontainers/image-spec"
# Look for container-related functionality that might be affected
rg -l "container|docker|oci|image"
Length of output: 2014
Script:
#!/bin/bash
# Check actual container/image operations in key files
rg -A 5 "(NewClient|Pull|Push|Image|Container)" internal/images/get-images.go internal/satellite/satellite.go ground-control/reg/adapter_test.go
# Check if there are any error logs or issues related to Docker/OCI operations
rg -B 2 "err.*docker|err.*container|err.*image" internal/images/get-images.go internal/satellite/satellite.go ground-control/reg/adapter_test.go
# Look at architecture decisions for context
cat docs/decisions/0002-zot-vs-docker-registries.md docs/decisions/0001-skopeo-vs-crane.md
Length of output: 7407
ground-control/internal/database/satellite_token.sql.go (3)
71-102:
Add pagination to prevent memory issues.
The current implementation loads all tokens into memory, which could cause issues with large datasets.
Consider modifying the query to include pagination:
const listToken = `-- name: ListToken :many
SELECT id, satellite_id, token, created_at, updated_at FROM satellite_token
+LIMIT $1 OFFSET $2
`
-func (q *Queries) ListToken(ctx context.Context) ([]SatelliteToken, error) {
+func (q *Queries) ListToken(ctx context.Context, limit int32, offset int32) ([]SatelliteToken, error) {
- rows, err := q.db.QueryContext(ctx, listToken)
+ rows, err := q.db.QueryContext(ctx, listToken, limit, offset)Also, there's a redundant rows.Close() call at line 95 since it's already handled by the defer statement.
Let's check the potential impact:
#!/bin/bash
# Check how many tokens might be affected
rg -A 2 "ListToken.*\(" --type go1-102: Verify secure token handling across the system.
As this code handles authentication tokens for satellite registration, it's crucial to ensure secure token handling throughout the system.
Let's check for potential security concerns:
#!/bin/bash
# Check for token exposure in logs
rg -B 2 -A 2 "log.*token|print.*token" --type go
# Check for token transmission in clear text
rg -B 2 -A 2 "http.*token|grpc.*token" --type go
# Look for token generation implementation
ast-grep --pattern 'func $$$Token$$$'Consider implementing:
- Token encryption at rest
- Audit logging for token operations
- Token rotation mechanism
- Rate limiting for token operations
12-28: Consider adding token validation and uniqueness constraints.
The token insertion lacks validation for token strength and uniqueness. This could lead to security vulnerabilities or conflicts in satellite identification.
Let's verify if there are any uniqueness constraints in the schema:
Consider implementing:
- Token strength validation before insertion
- A unique constraint on the
tokencolumn if not already present - Token expiration mechanism for enhanced security
ground-control/internal/database/satellite_groups.sql.go (2)
56-69: LGTM!
The implementation is clean, secure, and follows best practices with proper parameter handling and error checking.
3-3: Verify sqlc version update impact.
The sqlc version has been updated from 1.26.0 to 1.27.0. Please ensure this update is intentional and documented in the dependency management files.
✅ Verification successful
sqlc version update is consistent across the codebase
The sqlc version has been updated to v1.27.0 consistently across all generated files in the database package. This indicates a proper and complete regeneration of the database code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if sqlc version is consistently updated across the codebase
rg -l "sqlc v1\.2[67]\.0"
Length of output: 375
Script:
#!/bin/bash
# Check the actual sqlc version in each file to ensure consistency
for file in $(rg -l 'sqlc v1\.2[67]\.0'); do
echo "=== $file ==="
rg "sqlc v1\.2[67]\.0" "$file"
done
Length of output: 1849
ground-control/internal/database/satellites.sql.go (3)
40-46: LGTM! Good improvement in database access pattern.
The change from name-based to ID-based lookup is a good practice as it:
- Uses the primary key for efficient lookups
- Avoids potential issues with name changes
- Follows proper database normalization principles
91-91: LGTM! Good practice using explicit column selection.
The change from SELECT * to explicit column selection is a good practice as it:
- Makes the code more maintainable
- Prevents issues with schema changes
- Improves query performance
13-19: Verify token handling mechanism for satellite authentication.
The removal of the token parameter from CreateSatellite suggests a change in the authentication mechanism. While this aligns with the Zero Touch Registration objectives, we need to ensure secure token handling is implemented elsewhere.
✅ Verification successful
Token handling has been properly refactored into a dedicated system
The removal of the token parameter from CreateSatellite is justified as token management has been properly moved to a dedicated satellite_token table with appropriate schema and queries:
- A new table
satellite_tokenmaintains the relationship between satellites and their tokens - Foreign key constraint ensures referential integrity with the satellites table
- Comprehensive token management queries are implemented:
AddTokenfor token creationGetSatelliteIDByTokenfor token validationGetTokenandListTokenfor token retrievalDeleteTokenfor token revocation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for satellite token handling implementation
# Expected: Find evidence of token management in the new satellite_token table
# Check for satellite token related schema
fd -e sql -e go . | xargs rg -l "satellite.*token"
# Look for token-related functions
ast-grep --pattern 'func $_(ctx context.Context, token string)'
Length of output: 355
Script:
#!/bin/bash
# Let's examine the token-related schema and implementation
# Check the token table schema
cat ground-control/sql/schema/014_satellite_token.sql
# Check the token-related queries
cat ground-control/sql/queries/satellite_token.sql
# Look for token validation in handlers
rg -A 5 "func.*ValidateSatelliteToken|func.*VerifyToken" ground-control/internal/server/handlers.go
Length of output: 970
ground-control/reg/harbor/robot.go (2)
14-16: LGTM! Clean struct definition for list parameters.
The renamed ListParams struct provides a well-structured set of fields for pagination and filtering, making it more generic and reusable.
115-124: Review security implications of system-level access.
Several security considerations to address:
- The
level: "system"grants broad access. Consider if project-level access would be sufficient. - The infinite duration (
-1) prevents automatic credential rotation. Consider setting a reasonable expiration. - The description could include more metadata (e.g., creation timestamp, purpose).
Let's check if system-level access is truly needed:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
System-level access is appropriate for this robot account's purpose
The implementation shows that the robot account is granted minimal permissions despite being at system level:
- Only read/pull access to artifacts and repositories
- No write/push capabilities
- Permissions are scoped to specific projects passed as parameters
- The infinite duration (-1) is justified as this is a managed service account
The description "managed by ground-control should not edit" is also appropriate as it clearly indicates the account's purpose and management authority.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any uses of the robot account that require system-level access
rg -A 5 "level.*system" --type go
Length of output: 35
Script:
#!/bin/bash
# Let's try a broader search to understand the robot account usage and permissions
# First, let's find where GenRobotPerms is defined and used
ast-grep --pattern 'func GenRobotPerms($$$) {
$$$
}'
# Also search for any references to robot permissions or access levels
rg -B 2 -A 5 'Level.*:.*' --type go
# And look for any documentation or comments about robot account permissions
rg -l 'robot.*permission|permission.*robot' --type go
Length of output: 618
Script:
#!/bin/bash
# Let's check the handlers.go file to understand how robot permissions are used
cat ground-control/internal/server/handlers.go
# Let's also search for GenRobotPerms with ripgrep to see its implementation
rg -B 5 -A 10 "GenRobotPerms" --type go
# And check for any configuration or documentation about robot accounts
rg -l "robot.*config|config.*robot" --type go
fd "README|CONTRIBUTING" --type f --exec cat {}
Length of output: 26979
Script:
#!/bin/bash
# Let's check the GenRobotPerms implementation to understand the actual permissions being granted
rg -B 2 -A 10 "func GenRobotPerms" ground-control/reg/harbor/robot.go
# Also check for any robot-related tests that might provide insights
rg -l "_test.*robot|robot.*_test" --type go
# And look for any configuration files that might define robot permissions
fd "config|settings" --type yaml --type json --exec cat {}
Length of output: 786
ground-control/internal/utils/helper.go (2)
37-74: LGTM! Well-structured URL parsing with proper validation
The function has good error handling and validation checks for all required components.
124-149: LGTM! Well-structured robot account update logic
The function has good error handling and proper validation of inputs.
ground-control/internal/database/groups.sql.go (5)
3-3: Generated code updated with sqlc v1.27.0
The file has been regenerated using sqlc v1.27.0, addressing previous concerns about outdated generated code.
15-22: CreateGroup SQL query updated to handle new fields
The INSERT INTO groups statement now includes registry_url and projects, along with an ON CONFLICT clause to update existing entries. This ensures that group records are up-to-date without inserting duplicates.
26-28: CreateGroupParams and CreateGroup function correctly handle new fields
The CreateGroupParams struct and CreateGroup function have been updated to include RegistryUrl and Projects, matching the changes in the SQL query. The use of pq.Array for the Projects slice is appropriate for handling PostgreSQL array types.
Also applies to: 32-38
45-53: DeleteGroup function added for group deletion
The new DeleteGroup function allows for the deletion of groups by their ID, enhancing the CRUD capabilities for group management.
Line range hint 56-67: Query functions updated to include new fields
The GetGroupByID, GetGroupByName, and ListGroups functions now select and scan the registry_url and projects fields, ensuring that the additional group information is retrieved as expected.
Also applies to: 75-86, 94-110
ground-control/internal/database/robot_accounts.sql.go (1)
30-48: Proper implementation of AddRobotAccount function
The AddRobotAccount function correctly prepares the statement, executes the query with parameters, and scans the result into the RobotAccount struct. Error handling is appropriately implemented.
| @@ -3,6 +3,8 @@ | |||
| CREATE TABLE groups ( | |||
| id SERIAL PRIMARY KEY, | |||
| group_name VARCHAR(255) UNIQUE NOT NULL, | |||
| registry_url VARCHAR(255) NOT NULL, | |||
| projects TEXT[] NOT NULL, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding array constraints and a GIN index.
For the projects array column:
- Consider adding constraints to ensure non-empty elements
- Add a GIN index if you plan to perform array operations like containment searches
id SERIAL PRIMARY KEY,
group_name VARCHAR(255) UNIQUE NOT NULL,
registry_url VARCHAR(255) NOT NULL,
- projects TEXT[] NOT NULL,
+ projects TEXT[] NOT NULL CHECK (array_length(projects, 1) > 0 AND NOT EXISTS (SELECT 1 FROM unnest(projects) AS p WHERE p = '')),
created_at TIMESTAMP DEFAULT NOW() NOT NULL,
updated_at TIMESTAMP DEFAULT NOW() NOT NULL
);
+ CREATE INDEX idx_groups_projects ON groups USING GIN (projects);📝 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.
| projects TEXT[] NOT NULL, | |
| projects TEXT[] NOT NULL CHECK (array_length(projects, 1) > 0 AND NOT EXISTS (SELECT 1 FROM unnest(projects) AS p WHERE p = '')), | |
| CREATE INDEX idx_groups_projects ON groups USING GIN (projects); |
| @@ -3,6 +3,8 @@ | |||
| CREATE TABLE groups ( | |||
| id SERIAL PRIMARY KEY, | |||
| group_name VARCHAR(255) UNIQUE NOT NULL, | |||
| registry_url VARCHAR(255) NOT NULL, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding URL format validation and an index.
The registry_url column might benefit from:
- A CHECK constraint to validate URL format
- An index for faster lookups if queried frequently
id SERIAL PRIMARY KEY,
group_name VARCHAR(255) UNIQUE NOT NULL,
- registry_url VARCHAR(255) NOT NULL,
+ registry_url VARCHAR(255) NOT NULL CHECK (registry_url ~ '^https?://'),
projects TEXT[] NOT NULL,
created_at TIMESTAMP DEFAULT NOW() NOT NULL,
updated_at TIMESTAMP DEFAULT NOW() NOT NULL
);
+ CREATE INDEX idx_groups_registry_url ON groups(registry_url);📝 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.
| registry_url VARCHAR(255) NOT NULL, | |
| id SERIAL PRIMARY KEY, | |
| group_name VARCHAR(255) UNIQUE NOT NULL, | |
| registry_url VARCHAR(255) NOT NULL CHECK (registry_url ~ '^https?://'), | |
| projects TEXT[] NOT NULL, | |
| created_at TIMESTAMP DEFAULT NOW() NOT NULL, | |
| updated_at TIMESTAMP DEFAULT NOW() NOT NULL | |
| ); | |
| CREATE INDEX idx_groups_registry_url ON groups(registry_url); |
| # Ground Control PORT | ||
| PORT=8080 | ||
| APP_ENV=local |
There was a problem hiding this comment.
Environment configuration inconsistency
The file mixes production URLs (registry.bupd.xyz) with local environment settings (APP_ENV=local), which could lead to accidental production data manipulation during development.
Consider:
- Creating separate
.envfiles for different environments - Using environment-specific URLs:
-APP_ENV=local
+APP_ENV=production # or keep local and change HARBOR_URL to localCommittable suggestion was skipped due to low confidence.
ground-control/.env
Outdated
| # Source Registry | ||
| HARBOR_USERNAME=admin | ||
| HARBOR_PASSWORD=Harbor12345 | ||
| HARBOR_URL=https://demo.goharbor.io | ||
| HARBOR_URL=https://registry.bupd.xyz |
There was a problem hiding this comment.
Security: Avoid committing credentials to version control
The file contains hard-coded admin credentials for Harbor registry. This poses a significant security risk, especially since it's pointing to what appears to be a production URL.
Consider:
- Using environment-specific credential management
- Adding this file to
.gitignore - Providing a template file (e.g.,
.env.example) with placeholder values instead
-HARBOR_USERNAME=admin
-HARBOR_PASSWORD=Harbor12345
+HARBOR_USERNAME=<your-username>
+HARBOR_PASSWORD=<your-password>Committable suggestion was skipped due to low confidence.
| robot_name VARCHAR(255) UNIQUE NOT NULL, | ||
| robot_secret VARCHAR(255) NOT NULL, | ||
| robot_id VARCHAR(255) UNIQUE NOT NULL, | ||
| satellite_id INT NOT NULL REFERENCES satellites(id) ON DELETE CASCADE, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add an index for foreign key performance.
Consider adding an index on satellite_id to improve query performance when looking up robot accounts by satellite.
Add this after the table creation:
CREATE INDEX idx_robot_accounts_satellite_id ON robot_accounts(satellite_id);| func GenerateRandomToken(charLength int) (string, error) { | ||
| // The number of bytes needed to generate a token with the required number of hex characters | ||
| byteLength := charLength / 2 | ||
|
|
||
| // Create a byte slice of the required length | ||
| token := make([]byte, byteLength) | ||
| _, err := rand.Read(token) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| // Return the token as a hex-encoded string |
There was a problem hiding this comment.
Improve token generation security.
The GenerateRandomToken function should handle the case where charLength is odd, and it should validate the input to prevent potential issues.
Apply this diff to improve the implementation:
func GenerateRandomToken(charLength int) (string, error) {
+ if charLength <= 0 {
+ return "", fmt.Errorf("token length must be positive")
+ }
+
// The number of bytes needed to generate a token with the required number of hex characters
- byteLength := charLength / 2
+ byteLength := (charLength + 1) / 2 // Handle odd lengths correctly
// Create a byte slice of the required length
token := make([]byte, byteLength)
_, err := rand.Read(token)
if err != nil {
return "", err
}
// Return the token as a hex-encoded string
- return hex.EncodeToString(token), nil
+ hexToken := hex.EncodeToString(token)
+ // Ensure exact length for odd charLength
+ if len(hexToken) > charLength {
+ hexToken = hexToken[:charLength]
+ }
+ return hexToken, nil📝 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 GenerateRandomToken(charLength int) (string, error) { | |
| // The number of bytes needed to generate a token with the required number of hex characters | |
| byteLength := charLength / 2 | |
| // Create a byte slice of the required length | |
| token := make([]byte, byteLength) | |
| _, err := rand.Read(token) | |
| if err != nil { | |
| return "", err | |
| } | |
| // Return the token as a hex-encoded string | |
| func GenerateRandomToken(charLength int) (string, error) { | |
| if charLength <= 0 { | |
| return "", fmt.Errorf("token length must be positive") | |
| } | |
| // The number of bytes needed to generate a token with the required number of hex characters | |
| byteLength := (charLength + 1) / 2 // Handle odd lengths correctly | |
| // Create a byte slice of the required length | |
| token := make([]byte, byteLength) | |
| _, err := rand.Read(token) | |
| if err != nil { | |
| return "", err | |
| } | |
| // Return the token as a hex-encoded string | |
| hexToken := hex.EncodeToString(token) | |
| // Ensure exact length for odd charLength | |
| if len(hexToken) > charLength { | |
| hexToken = hexToken[:charLength] | |
| } | |
| return hexToken, nil |
| func (s *Server) getSatelliteByID(w http.ResponseWriter, r *http.Request) { | ||
| result, err := s.dbQueries.ListSatellites(r.Context()) | ||
| if err != nil { | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
|
|
||
| params := database.AssignImageToLabelParams{ | ||
| LabelID: int32(req.LabelID), | ||
| ImageID: int32(req.ImageID), | ||
| } | ||
| WriteJSONResponse(w, http.StatusOK, result) | ||
| } |
There was a problem hiding this comment.
Fix incorrect implementation of getSatelliteByID.
The getSatelliteByID handler is incorrectly using ListSatellites instead of retrieving a specific satellite by ID.
Apply this diff to fix the implementation:
func (s *Server) getSatelliteByID(w http.ResponseWriter, r *http.Request) {
- result, err := s.dbQueries.ListSatellites(r.Context())
+ vars := mux.Vars(r)
+ idStr := vars["id"]
+
+ id, err := strconv.ParseInt(idStr, 10, 32)
+ if err != nil {
+ HandleAppError(w, &AppError{
+ Message: "Invalid satellite ID",
+ Code: http.StatusBadRequest,
+ })
+ return
+ }
+
+ result, err := s.dbQueries.GetSatelliteByID(r.Context(), int32(id))
if err != nil {
+ if err == sql.ErrNoRows {
+ HandleAppError(w, &AppError{
+ Message: "Satellite not found",
+ Code: http.StatusNotFound,
+ })
+ return
+ }
HandleAppError(w, err)
return
}📝 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 (s *Server) getSatelliteByID(w http.ResponseWriter, r *http.Request) { | |
| result, err := s.dbQueries.ListSatellites(r.Context()) | |
| if err != nil { | |
| HandleAppError(w, err) | |
| return | |
| } | |
| params := database.AssignImageToLabelParams{ | |
| LabelID: int32(req.LabelID), | |
| ImageID: int32(req.ImageID), | |
| } | |
| WriteJSONResponse(w, http.StatusOK, result) | |
| } | |
| func (s *Server) getSatelliteByID(w http.ResponseWriter, r *http.Request) { | |
| vars := mux.Vars(r) | |
| idStr := vars["id"] | |
| id, err := strconv.ParseInt(idStr, 10, 32) | |
| if err != nil { | |
| HandleAppError(w, &AppError{ | |
| Message: "Invalid satellite ID", | |
| Code: http.StatusBadRequest, | |
| }) | |
| return | |
| } | |
| result, err := s.dbQueries.GetSatelliteByID(r.Context(), int32(id)) | |
| if err != nil { | |
| if err == sql.ErrNoRows { | |
| HandleAppError(w, &AppError{ | |
| Message: "Satellite not found", | |
| Code: http.StatusNotFound, | |
| }) | |
| return | |
| } | |
| HandleAppError(w, err) | |
| return | |
| } | |
| WriteJSONResponse(w, http.StatusOK, result) | |
| } |
| func (s *Server) groupsSyncHandler(w http.ResponseWriter, r *http.Request) { | ||
| var req models.StateArtifact | ||
| if err := DecodeRequestBody(r, &req); err != nil { | ||
| log.Println(err) | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
|
|
||
| projects := utils.GetProjectNames(&req.Artifacts) | ||
| params := database.CreateGroupParams{ | ||
| GroupName: req.GroupName, | ||
| CreatedAt: time.Now(), | ||
| UpdatedAt: time.Now(), | ||
| GroupName: req.Group, | ||
| RegistryUrl: os.Getenv("HARBOR_URL"), | ||
| Projects: projects, | ||
| } | ||
|
|
||
| // Call the database query to create Group | ||
| result, err := s.dbQueries.CreateGroup(r.Context(), params) | ||
| if err != nil { | ||
| err = &AppError{ | ||
| Message: err.Error(), | ||
| Code: http.StatusBadRequest, | ||
| } | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
|
|
||
| WriteJSONResponse(w, http.StatusCreated, result) | ||
| } | ||
|
|
||
| func (s *Server) createLabelHandler(w http.ResponseWriter, r *http.Request) { | ||
| var req LabelRequestParams | ||
| if err := DecodeRequestBody(r, &req); err != nil { | ||
| satellites, err := s.dbQueries.GroupSatelliteList(r.Context(), result.ID) | ||
| if err != nil { | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
| params := database.CreateLabelParams{ | ||
| LabelName: req.LabelName, | ||
| CreatedAt: time.Now(), | ||
| UpdatedAt: time.Now(), | ||
|
|
||
| for _, satellite := range satellites { | ||
| robotAcc, err := s.dbQueries.GetRobotAccBySatelliteID(r.Context(), satellite.SatelliteID) | ||
| if err != nil { | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
| // update robot account projects permission | ||
| _, err = utils.UpdateRobotProjects(r.Context(), projects, robotAcc.RobotName, robotAcc.RobotID) | ||
| if err != nil { | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| result, err := s.dbQueries.CreateLabel(r.Context(), params) | ||
| // Create State Artifact for the group | ||
| err = utils.CreateStateArtifact(&req) | ||
| if err != nil { | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
|
|
||
| WriteJSONResponse(w, http.StatusCreated, result) | ||
| WriteJSONResponse(w, http.StatusOK, result) |
There was a problem hiding this comment.
Add validation for empty projects list.
The groupsSyncHandler should validate that the projects list is not empty before creating a group. Additionally, error messages should be more descriptive.
Add validation after line 64:
projects := utils.GetProjectNames(&req.Artifacts)
+if len(projects) == 0 {
+ err := &AppError{
+ Message: "No valid projects found in artifacts",
+ Code: http.StatusBadRequest,
+ }
+ HandleAppError(w, err)
+ return
+}📝 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 (s *Server) groupsSyncHandler(w http.ResponseWriter, r *http.Request) { | |
| var req models.StateArtifact | |
| if err := DecodeRequestBody(r, &req); err != nil { | |
| log.Println(err) | |
| HandleAppError(w, err) | |
| return | |
| } | |
| projects := utils.GetProjectNames(&req.Artifacts) | |
| params := database.CreateGroupParams{ | |
| GroupName: req.GroupName, | |
| CreatedAt: time.Now(), | |
| UpdatedAt: time.Now(), | |
| GroupName: req.Group, | |
| RegistryUrl: os.Getenv("HARBOR_URL"), | |
| Projects: projects, | |
| } | |
| // Call the database query to create Group | |
| result, err := s.dbQueries.CreateGroup(r.Context(), params) | |
| if err != nil { | |
| err = &AppError{ | |
| Message: err.Error(), | |
| Code: http.StatusBadRequest, | |
| } | |
| HandleAppError(w, err) | |
| return | |
| } | |
| WriteJSONResponse(w, http.StatusCreated, result) | |
| } | |
| func (s *Server) createLabelHandler(w http.ResponseWriter, r *http.Request) { | |
| var req LabelRequestParams | |
| if err := DecodeRequestBody(r, &req); err != nil { | |
| satellites, err := s.dbQueries.GroupSatelliteList(r.Context(), result.ID) | |
| if err != nil { | |
| HandleAppError(w, err) | |
| return | |
| } | |
| params := database.CreateLabelParams{ | |
| LabelName: req.LabelName, | |
| CreatedAt: time.Now(), | |
| UpdatedAt: time.Now(), | |
| for _, satellite := range satellites { | |
| robotAcc, err := s.dbQueries.GetRobotAccBySatelliteID(r.Context(), satellite.SatelliteID) | |
| if err != nil { | |
| HandleAppError(w, err) | |
| return | |
| } | |
| // update robot account projects permission | |
| _, err = utils.UpdateRobotProjects(r.Context(), projects, robotAcc.RobotName, robotAcc.RobotID) | |
| if err != nil { | |
| HandleAppError(w, err) | |
| return | |
| } | |
| } | |
| result, err := s.dbQueries.CreateLabel(r.Context(), params) | |
| // Create State Artifact for the group | |
| err = utils.CreateStateArtifact(&req) | |
| if err != nil { | |
| HandleAppError(w, err) | |
| return | |
| } | |
| WriteJSONResponse(w, http.StatusCreated, result) | |
| WriteJSONResponse(w, http.StatusOK, result) | |
| func (s *Server) groupsSyncHandler(w http.ResponseWriter, r *http.Request) { | |
| var req models.StateArtifact | |
| if err := DecodeRequestBody(r, &req); err != nil { | |
| log.Println(err) | |
| HandleAppError(w, err) | |
| return | |
| } | |
| projects := utils.GetProjectNames(&req.Artifacts) | |
| if len(projects) == 0 { | |
| err := &AppError{ | |
| Message: "No valid projects found in artifacts", | |
| Code: http.StatusBadRequest, | |
| } | |
| HandleAppError(w, err) | |
| return | |
| } | |
| params := database.CreateGroupParams{ | |
| GroupName: req.Group, | |
| RegistryUrl: os.Getenv("HARBOR_URL"), | |
| Projects: projects, | |
| } | |
| result, err := s.dbQueries.CreateGroup(r.Context(), params) | |
| if err != nil { | |
| HandleAppError(w, err) | |
| return | |
| } | |
| satellites, err := s.dbQueries.GroupSatelliteList(r.Context(), result.ID) | |
| if err != nil { | |
| HandleAppError(w, err) | |
| return | |
| } | |
| for _, satellite := range satellites { | |
| robotAcc, err := s.dbQueries.GetRobotAccBySatelliteID(r.Context(), satellite.SatelliteID) | |
| if err != nil { | |
| HandleAppError(w, err) | |
| return | |
| } | |
| // update robot account projects permission | |
| _, err = utils.UpdateRobotProjects(r.Context(), projects, robotAcc.RobotName, robotAcc.RobotID) | |
| if err != nil { | |
| HandleAppError(w, err) | |
| return | |
| } | |
| } | |
| // Create State Artifact for the group | |
| err = utils.CreateStateArtifact(&req) | |
| if err != nil { | |
| HandleAppError(w, err) | |
| return | |
| } | |
| WriteJSONResponse(w, http.StatusOK, result) |
| const addRobotAccount = `-- name: AddRobotAccount :one | ||
| INSERT INTO robot_accounts (robot_name, robot_secret, robot_id, satellite_id, created_at, updated_at) | ||
| VALUES ($1, $2, $3, $4, NOW(), NOW()) | ||
| ON CONFLICT (robot_id) | ||
| DO UPDATE SET | ||
| robot_name = EXCLUDED.robot_name, | ||
| robot_secret = EXCLUDED.robot_secret, | ||
| updated_at = NOW() | ||
| RETURNING id, robot_name, robot_secret, robot_id, satellite_id, created_at, updated_at | ||
| ` |
There was a problem hiding this comment.
Consider including 'satellite_id' in the ON CONFLICT DO UPDATE clause
Currently, if there's a conflict on robot_id, the satellite_id is not updated. If a robot account's satellite_id can change, consider including it in the DO UPDATE SET clause to ensure it reflects the latest association.
Apply this diff to include satellite_id in the update:
INSERT INTO robot_accounts (robot_name, robot_secret, robot_id, satellite_id, created_at, updated_at)
VALUES ($1, $2, $3, $4, NOW(), NOW())
ON CONFLICT (robot_id)
DO UPDATE SET
robot_name = EXCLUDED.robot_name,
robot_secret = EXCLUDED.robot_secret,
+ satellite_id = EXCLUDED.satellite_id,
updated_at = NOW()
RETURNING id, robot_name, robot_secret, robot_id, satellite_id, created_at, updated_at📝 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.
| const addRobotAccount = `-- name: AddRobotAccount :one | |
| INSERT INTO robot_accounts (robot_name, robot_secret, robot_id, satellite_id, created_at, updated_at) | |
| VALUES ($1, $2, $3, $4, NOW(), NOW()) | |
| ON CONFLICT (robot_id) | |
| DO UPDATE SET | |
| robot_name = EXCLUDED.robot_name, | |
| robot_secret = EXCLUDED.robot_secret, | |
| updated_at = NOW() | |
| RETURNING id, robot_name, robot_secret, robot_id, satellite_id, created_at, updated_at | |
| ` | |
| const addRobotAccount = `-- name: AddRobotAccount :one | |
| INSERT INTO robot_accounts (robot_name, robot_secret, robot_id, satellite_id, created_at, updated_at) | |
| VALUES ($1, $2, $3, $4, NOW(), NOW()) | |
| ON CONFLICT (robot_id) | |
| DO UPDATE SET | |
| robot_name = EXCLUDED.robot_name, | |
| robot_secret = EXCLUDED.robot_secret, | |
| satellite_id = EXCLUDED.satellite_id, | |
| updated_at = NOW() | |
| RETURNING id, robot_name, robot_secret, robot_id, satellite_id, created_at, updated_at | |
| ` |
| func (q *Queries) ListRobotAccounts(ctx context.Context) ([]RobotAccount, error) { | ||
| rows, err := q.db.QueryContext(ctx, listRobotAccounts) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer rows.Close() | ||
| var items []RobotAccount | ||
| for rows.Next() { | ||
| var i RobotAccount | ||
| if err := rows.Scan( | ||
| &i.ID, | ||
| &i.RobotName, | ||
| &i.RobotSecret, | ||
| &i.RobotID, | ||
| &i.SatelliteID, | ||
| &i.CreatedAt, | ||
| &i.UpdatedAt, | ||
| ); err != nil { | ||
| return nil, err | ||
| } | ||
| items = append(items, i) | ||
| } | ||
| if err := rows.Close(); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := rows.Err(); err != nil { | ||
| return nil, err | ||
| } | ||
| return items, nil | ||
| } |
There was a problem hiding this comment.
Redundant call to rows.Close(), already deferred
The call to rows.Close() is deferred at line 109, so the explicit call to rows.Close() at line 126 is unnecessary. Removing the extra rows.Close() call prevents potential double-closing of the rows resource.
Apply this diff to remove the redundant call:
defer rows.Close()
var items []RobotAccount
for rows.Next() {
// Processing each row
}
- if err := rows.Close(); err != nil {
- return nil, err
- }
if err := rows.Err(); err != nil {
return nil, err
}📝 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 (q *Queries) ListRobotAccounts(ctx context.Context) ([]RobotAccount, error) { | |
| rows, err := q.db.QueryContext(ctx, listRobotAccounts) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() | |
| var items []RobotAccount | |
| for rows.Next() { | |
| var i RobotAccount | |
| if err := rows.Scan( | |
| &i.ID, | |
| &i.RobotName, | |
| &i.RobotSecret, | |
| &i.RobotID, | |
| &i.SatelliteID, | |
| &i.CreatedAt, | |
| &i.UpdatedAt, | |
| ); err != nil { | |
| return nil, err | |
| } | |
| items = append(items, i) | |
| } | |
| if err := rows.Close(); err != nil { | |
| return nil, err | |
| } | |
| if err := rows.Err(); err != nil { | |
| return nil, err | |
| } | |
| return items, nil | |
| } | |
| func (q *Queries) ListRobotAccounts(ctx context.Context) ([]RobotAccount, error) { | |
| rows, err := q.db.QueryContext(ctx, listRobotAccounts) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() | |
| var items []RobotAccount | |
| for rows.Next() { | |
| var i RobotAccount | |
| if err := rows.Scan( | |
| &i.ID, | |
| &i.RobotName, | |
| &i.RobotSecret, | |
| &i.RobotID, | |
| &i.SatelliteID, | |
| &i.CreatedAt, | |
| &i.UpdatedAt, | |
| ); err != nil { | |
| return nil, err | |
| } | |
| items = append(items, i) | |
| } | |
| if err := rows.Err(); err != nil { | |
| return nil, err | |
| } | |
| return items, nil | |
| } |
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (2)
ground-control/internal/server/routes.go (2)
15-15: Remove commented out code.
Instead of keeping commented code, consider removing it entirely. If this functionality might be needed later, it should be tracked in version control history or as a GitHub issue.
24-25: Convert TODO into a GitHub issue.
Instead of keeping inline TODOs, create a GitHub issue to track the missing functionality for listing satellites attached to a group. This will help with project tracking and visibility.
Would you like me to create a GitHub issue for tracking this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- ground-control/.env (1 hunks)
- ground-control/internal/models/models.go (1 hunks)
- ground-control/internal/server/handlers.go (4 hunks)
- ground-control/internal/server/routes.go (1 hunks)
- ground-control/internal/utils/helper.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ground-control/.env
- ground-control/internal/models/models.go
🔇 Additional comments (4)
ground-control/internal/server/handlers.go (4)
259-360: LGTM! Well-implemented Zero Touch Registration handler.
The implementation includes proper:
- Transaction management
- Token validation and cleanup
- Error handling
- Group state assembly
460-471:
Improve token generation for odd character lengths.
The current implementation might generate tokens shorter than requested when charLength is odd. Also, add input validation.
Apply these improvements:
func GenerateRandomToken(charLength int) (string, error) {
+ if charLength <= 0 {
+ return "", fmt.Errorf("token length must be positive")
+ }
+
// The number of bytes needed to generate a token with the required number of hex characters
- byteLength := charLength / 2
+ byteLength := (charLength + 1) / 2 // Handle odd lengths correctly
// Create a byte slice of the required length
token := make([]byte, byteLength)
_, err := rand.Read(token)
if err != nil {
return "", err
}
// Return the token as a hex-encoded string
- return hex.EncodeToString(token), nil
+ hexToken := hex.EncodeToString(token)
+ // Ensure exact length for odd charLength
+ if len(hexToken) > charLength {
+ hexToken = hexToken[:charLength]
+ }
+ return hexToken, nil
}Likely invalid or redundant comment.
187-239: 🛠️ Refactor suggestion
Optimize group validation to reduce database calls.
The current implementation makes multiple database and API calls within a loop. Consider validating all groups upfront to reduce the number of database calls.
Consider this optimization:
if req.Groups != nil {
+ // Validate all groups first
+ validGroups := make(map[string]database.Group)
+ for _, groupName := range req.Groups {
+ group, err := q.GetGroupByName(r.Context(), groupName)
+ if err != nil {
+ HandleAppError(w, &AppError{
+ Message: fmt.Sprintf("Invalid group name: %v", groupName),
+ Code: http.StatusBadRequest,
+ })
+ tx.Rollback()
+ return
+ }
+ validGroups[groupName] = group
+ }
+
+ // Process validated groups
+ for _, group := range validGroups {
+ err = q.AddSatelliteToGroup(r.Context(), database.AddSatelliteToGroupParams{
+ SatelliteID: satellite.ID,
+ GroupID: group.ID,
+ })
+ if err != nil {
+ HandleAppError(w, err)
+ tx.Rollback()
+ return
+ }
+ }
}Likely invalid or redundant comment.
372-380:
Fix incorrect implementation of getSatelliteByID.
The handler incorrectly uses ListSatellites instead of retrieving a specific satellite by ID.
Apply this fix:
func (s *Server) getSatelliteByID(w http.ResponseWriter, r *http.Request) {
- result, err := s.dbQueries.ListSatellites(r.Context())
+ vars := mux.Vars(r)
+ idStr := vars["id"]
+
+ id, err := strconv.ParseInt(idStr, 10, 32)
+ if err != nil {
+ HandleAppError(w, &AppError{
+ Message: "Invalid satellite ID",
+ Code: http.StatusBadRequest,
+ })
+ return
+ }
+
+ result, err := s.dbQueries.GetSatelliteByID(r.Context(), int32(id))
if err != nil {
+ if err == sql.ErrNoRows {
+ HandleAppError(w, &AppError{
+ Message: "Satellite not found",
+ Code: http.StatusNotFound,
+ })
+ return
+ }
HandleAppError(w, err)
return
}
WriteJSONResponse(w, http.StatusOK, result)
}Likely invalid or redundant comment.
| // r.HandleFunc("/images", s.addImageListHandler).Methods("POST") | ||
| // r.HandleFunc("/group", s.deleteGroupHandler).Methods("DELETE") | ||
| r.HandleFunc("/satellites/register", s.registerSatelliteHandler).Methods("POST") | ||
| r.HandleFunc("/satellites/ztr/{token}", s.ztrHandler).Methods("GET") |
There was a problem hiding this comment.
Consider using POST instead of GET for ZTR endpoint.
Using GET with tokens in the URL path can expose sensitive information in server logs and browser history. Consider using POST method instead:
-r.HandleFunc("/satellites/ztr/{token}", s.ztrHandler).Methods("GET")
+r.HandleFunc("/satellites/ztr", s.ztrHandler).Methods("POST")The token can be sent in the request body or as an Authorization header.
Committable suggestion was skipped due to low confidence.
| r.HandleFunc("/groups/sync", s.groupsSyncHandler).Methods("POST") | ||
| r.HandleFunc("/groups/list", s.listGroupHandler).Methods("GET") | ||
| r.HandleFunc("/groups/{group}", s.getGroupHandler).Methods("GET") | ||
| r.HandleFunc("/groups/satellite", s.addSatelliteToGroup).Methods("PUT") | ||
| r.HandleFunc("/groups/satellite", s.removeSatelliteFromGroup).Methods("PUT") | ||
|
|
||
| r.HandleFunc("/group", s.createGroupHandler).Methods("POST") | ||
| r.HandleFunc("/label", s.createLabelHandler).Methods("POST") | ||
| r.HandleFunc("/image", s.addImageHandler).Methods("POST") | ||
| r.HandleFunc("/satellite", s.addSatelliteHandler).Methods("POST") | ||
| // to-do: listing functionality to list satellites attached to group | ||
| // r.HandleFunc("/groups/{group}/list", s.groupSatelliteHandler).Methods("GET") | ||
|
|
||
| r.HandleFunc("/group/satellite", s.addSatelliteToGroup).Methods("POST") | ||
| r.HandleFunc("/label/satellite", s.addSatelliteToLabel).Methods("POST") | ||
|
|
||
| r.HandleFunc("/group/images", s.assignImageToGroup).Methods("POST") | ||
| r.HandleFunc("/label/images", s.assignImageToLabel).Methods("POST") | ||
| // r.HandleFunc("/satellites", s.addSatelliteHandler).Methods("POST") | ||
|
|
||
| r.HandleFunc("/satellite/images", s.GetImagesForSatellite).Methods("GET") | ||
|
|
||
| // Satellite based routes | ||
| // r.HandleFunc("/images", s.getImageListHandler).Methods("GET") | ||
| // r.HandleFunc("/images", s.addImageListHandler).Methods("POST") | ||
| // r.HandleFunc("/group", s.deleteGroupHandler).Methods("DELETE") | ||
| r.HandleFunc("/satellites/register", s.registerSatelliteHandler).Methods("POST") | ||
| r.HandleFunc("/satellites/ztr/{token}", s.ztrHandler).Methods("GET") | ||
| r.HandleFunc("/satellites/list", s.listSatelliteHandler).Methods("GET") | ||
| r.HandleFunc("/satellites/{satellite}", s.getSatelliteByID).Methods("GET") | ||
| r.HandleFunc("/satellites/{satellite}", s.deleteSatelliteByID).Methods("DELETE") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider organizing routes by resource type.
For better maintainability, consider grouping related routes together and adding comments to separate different resource types:
// Health check routes
r.HandleFunc("/ping", s.Ping).Methods("GET")
r.HandleFunc("/health", s.healthHandler).Methods("GET")
+// Group management routes
r.HandleFunc("/groups/sync", s.groupsSyncHandler).Methods("POST")
r.HandleFunc("/groups/list", s.listGroupHandler).Methods("GET")
r.HandleFunc("/groups/{group}", s.getGroupHandler).Methods("GET")
r.HandleFunc("/groups/satellite", s.addSatelliteToGroup).Methods("PUT")
r.HandleFunc("/groups/satellite", s.removeSatelliteFromGroup).Methods("PUT")
+// Satellite management routes
r.HandleFunc("/satellites/register", s.registerSatelliteHandler).Methods("POST")
r.HandleFunc("/satellites/ztr", s.ztrHandler).Methods("POST")
r.HandleFunc("/satellites/list", s.listSatelliteHandler).Methods("GET")
r.HandleFunc("/satellites/{satellite}", s.getSatelliteByID).Methods("GET")
r.HandleFunc("/satellites/{satellite}", s.deleteSatelliteByID).Methods("DELETE")Committable suggestion was skipped due to low confidence.
| r.HandleFunc("/satellites/register", s.registerSatelliteHandler).Methods("POST") | ||
| r.HandleFunc("/satellites/ztr/{token}", s.ztrHandler).Methods("GET") | ||
| r.HandleFunc("/satellites/list", s.listSatelliteHandler).Methods("GET") | ||
| r.HandleFunc("/satellites/{satellite}", s.getSatelliteByID).Methods("GET") | ||
| r.HandleFunc("/satellites/{satellite}", s.deleteSatelliteByID).Methods("DELETE") |
There was a problem hiding this comment.
💡 Codebase verification
ZTR implementation needs additional security measures
The current Zero Touch Registration (ZTR) implementation has several gaps compared to standard security requirements:
- No token expiration mechanism in the schema or handler implementation
- Missing token validation beyond simple existence check
- No rate limiting for token verification attempts
- Token storage lacks a revocation status field
- Missing secure token transmission guidelines
Required changes:
- Add token expiration timestamp to
satellite_tokentable - Implement token expiration check in
ztrHandler - Add rate limiting for
/satellites/ztr/{token}endpoint - Include token revocation status in schema
- Document secure token transmission requirements
🔗 Analysis chain
Verify ZTR implementation against requirements.
Let's verify if the current implementation aligns with the Zero Touch Registration requirements from issues #41 and #36.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the ZTR implementation
# Test 1: Check the ZTR handler implementation
echo "Checking ZTR handler implementation..."
ast-grep --pattern 'func $_($$) {
$$$
ztrHandler
$$$
}'
# Test 2: Look for any configuration or environment variables related to ZTR
echo "Checking ZTR configuration..."
rg -l "ZTR|ZERO_TOUCH" --type=env --type=yaml --type=json
# Test 3: Check for any database schema changes related to ZTR
echo "Checking database schema for ZTR..."
fd -e sql -x rg -l "token|registration"
Length of output: 575
Script:
#!/bin/bash
# Let's analyze the ZTR implementation more thoroughly
# Check the content of schema and query files
echo "=== Schema file content ==="
cat ./ground-control/sql/schema/014_satellite_token.sql
echo -e "\n=== Satellite token queries ==="
cat ./ground-control/sql/queries/satellite_token.sql
# Check the ZTR handler implementation
echo -e "\n=== ZTR Handler Implementation ==="
rg -A 10 "func.*ztrHandler" ./ground-control/internal/server/
# Check for any configuration related to token expiry or validation
echo -e "\n=== Token Configuration ==="
rg "token" ./ground-control/internal/config/ -A 3
# Look for token validation logic
echo -e "\n=== Token Validation Logic ==="
rg "ValidateToken|verifyToken|validateToken" ./ground-control/internal/
Length of output: 2266
Script:
#!/bin/bash
# Let's see the complete ZTR handler implementation and related code
# Get the full ZTR handler implementation with more context
echo "=== Complete ZTR Handler ==="
rg -A 30 "func.*ztrHandler" ./ground-control/internal/server/handlers.go
# Check for satellite registration related code
echo -e "\n=== Satellite Registration Handler ==="
rg -A 20 "func.*registerSatelliteHandler" ./ground-control/internal/server/handlers.go
# Look for any token generation or validation utilities
echo -e "\n=== Token Utils ==="
rg -A 10 "func.*[Tt]oken" ./ground-control/internal/
# Check for any ZTR-related types/structs
echo -e "\n=== ZTR Related Types ==="
ast-grep --pattern 'type $_ struct {
$$$
[Tt]oken
$$$
}'
Length of output: 8148
| // Add "https://" scheme if missing | ||
| if !strings.Contains(rawURL, "://") { | ||
| rawURL = "https://" + rawURL | ||
| } |
There was a problem hiding this comment.
Add URL validation for security
The automatic addition of "https://" scheme could lead to security issues if the URL contains malicious characters or schemes.
Consider this safer implementation:
- if !strings.Contains(rawURL, "://") {
- rawURL = "https://" + rawURL
- }
+ // Validate URL scheme
+ if !strings.HasPrefix(rawURL, "https://") {
+ if strings.Contains(rawURL, "://") {
+ return reg.Images{}, fmt.Errorf("only https scheme is allowed")
+ }
+ rawURL = "https://" + rawURL
+ }📝 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.
| // Add "https://" scheme if missing | |
| if !strings.Contains(rawURL, "://") { | |
| rawURL = "https://" + rawURL | |
| } | |
| // Validate URL scheme | |
| if !strings.HasPrefix(rawURL, "https://") { | |
| if strings.Contains(rawURL, "://") { | |
| return reg.Images{}, fmt.Errorf("only https scheme is allowed") | |
| } | |
| rawURL = "https://" + rawURL | |
| } |
| func CreateRobotAccForSatellite(ctx context.Context, repos []string, name string) (*models.RobotCreated, error) { | ||
| // get harbor client | ||
| harborClient, err := harbor.GetClient() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting Harbor client: %w", err) | ||
| } | ||
|
|
||
| var projects []string | ||
| // get projects from repos | ||
| for _, project := range repos { | ||
| proj := strings.Split(project, "/") | ||
| projects = append(projects, proj[0]) | ||
| } | ||
|
|
||
| robotTemp := harbor.RobotAccountTemplate(name, projects) | ||
|
|
||
| robot, err := harbor.CreateRobotAccount(ctx, robotTemp, harborClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating robot account: %w", err) | ||
| } | ||
|
|
||
| return robot.Payload, nil | ||
| } |
There was a problem hiding this comment.
Improve context handling in CreateRobotAccForSatellite
The function accepts a context but doesn't propagate it to Harbor client operations.
Modify the function to use context in Harbor client operations:
func CreateRobotAccForSatellite(ctx context.Context, repos []string, name string) (*models.RobotCreated, error) {
- harborClient, err := harbor.GetClient()
+ harborClient, err := harbor.GetClient(ctx)
if err != nil {
return nil, fmt.Errorf("error getting Harbor client: %w", err)
}Committable suggestion was skipped due to low confidence.
| err = crane.Push(img, destinationRepo, options...) | ||
| if err != nil { | ||
| return fmt.Errorf("push image failed: %v", err) | ||
| } | ||
| err = crane.Tag(destinationRepo, fmt.Sprintf("%d", time.Now().Unix()), options...) | ||
| if err != nil { | ||
| return fmt.Errorf("tag image failed: %v", err) | ||
| } | ||
| err = crane.Tag(destinationRepo, "latest", options...) | ||
| if err != nil { | ||
| return fmt.Errorf("tag image failed: %v", err) | ||
| } |
There was a problem hiding this comment.
Ensure atomic operations for state updates
The current implementation has a race condition where the state update is not atomic. If multiple updates occur simultaneously, they could interfere with each other.
Consider this atomic implementation:
+ // Generate unique timestamp for atomic operation
+ timestamp := fmt.Sprintf("%d", time.Now().UnixNano())
+ timestampTag := fmt.Sprintf("%s:%s", destinationRepo, timestamp)
+
- err = crane.Push(img, destinationRepo, options...)
+ // Push with timestamp tag first
+ err = crane.Push(img, timestampTag, options...)
if err != nil {
return fmt.Errorf("push image failed: %v", err)
}
- err = crane.Tag(destinationRepo, fmt.Sprintf("%d", time.Now().Unix()), options...)
- if err != nil {
- return fmt.Errorf("tag image failed: %v", err)
- }
- err = crane.Tag(destinationRepo, "latest", options...)
+
+ // Atomically update latest tag
+ err = crane.Tag(timestampTag, destinationRepo+":latest", options...)
if err != nil {
+ // Cleanup on failure
+ _ = crane.Delete(timestampTag, options...)
return fmt.Errorf("tag image failed: %v", err)
}📝 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.
| err = crane.Push(img, destinationRepo, options...) | |
| if err != nil { | |
| return fmt.Errorf("push image failed: %v", err) | |
| } | |
| err = crane.Tag(destinationRepo, fmt.Sprintf("%d", time.Now().Unix()), options...) | |
| if err != nil { | |
| return fmt.Errorf("tag image failed: %v", err) | |
| } | |
| err = crane.Tag(destinationRepo, "latest", options...) | |
| if err != nil { | |
| return fmt.Errorf("tag image failed: %v", err) | |
| } | |
| // Generate unique timestamp for atomic operation | |
| timestamp := fmt.Sprintf("%d", time.Now().UnixNano()) | |
| timestampTag := fmt.Sprintf("%s:%s", destinationRepo, timestamp) | |
| // Push with timestamp tag first | |
| err = crane.Push(img, timestampTag, options...) | |
| if err != nil { | |
| return fmt.Errorf("push image failed: %v", err) | |
| } | |
| // Atomically update latest tag | |
| err = crane.Tag(timestampTag, destinationRepo+":latest", options...) | |
| if err != nil { | |
| // Cleanup on failure | |
| _ = crane.Delete(timestampTag, options...) | |
| return fmt.Errorf("tag image failed: %v", err) | |
| } |
| type RegisterSatelliteParams struct { | ||
| Name string `json:"satellite_name"` | ||
| Groups *[]string `json:"groups,omitempty"` | ||
| } |
There was a problem hiding this comment.
Remove unnecessary pointer to slice in RegisterSatelliteParams.
Using a pointer to a slice (*[]string) adds unnecessary complexity since slices in Go are already reference types.
Apply this diff to simplify the struct:
type RegisterSatelliteParams struct {
Name string `json:"satellite_name"`
- Groups *[]string `json:"groups,omitempty"`
+ Groups []string `json:"groups,omitempty"`
}📝 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.
| type RegisterSatelliteParams struct { | |
| Name string `json:"satellite_name"` | |
| Groups *[]string `json:"groups,omitempty"` | |
| } | |
| type RegisterSatelliteParams struct { | |
| Name string `json:"satellite_name"` | |
| Groups []string `json:"groups,omitempty"` | |
| } |
| projects := utils.GetProjectNames(&req.Artifacts) | ||
| params := database.CreateGroupParams{ | ||
| GroupName: req.GroupName, | ||
| CreatedAt: time.Now(), | ||
| UpdatedAt: time.Now(), | ||
| GroupName: req.Group, | ||
| RegistryUrl: os.Getenv("HARBOR_URL"), | ||
| Projects: projects, |
There was a problem hiding this comment.
Add validation for empty projects list.
The handler should validate that the projects list is not empty before creating a group.
Add validation after retrieving project names:
projects := utils.GetProjectNames(&req.Artifacts)
+if len(projects) == 0 {
+ HandleAppError(w, &AppError{
+ Message: "No valid projects found in artifacts",
+ Code: http.StatusBadRequest,
+ })
+ return
+}📝 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.
| projects := utils.GetProjectNames(&req.Artifacts) | |
| params := database.CreateGroupParams{ | |
| GroupName: req.GroupName, | |
| CreatedAt: time.Now(), | |
| UpdatedAt: time.Now(), | |
| GroupName: req.Group, | |
| RegistryUrl: os.Getenv("HARBOR_URL"), | |
| Projects: projects, | |
| projects := utils.GetProjectNames(&req.Artifacts) | |
| if len(projects) == 0 { | |
| HandleAppError(w, &AppError{ | |
| Message: "No valid projects found in artifacts", | |
| Code: http.StatusBadRequest, | |
| }) | |
| return | |
| } | |
| params := database.CreateGroupParams{ | |
| GroupName: req.Group, | |
| RegistryUrl: os.Getenv("HARBOR_URL"), | |
| Projects: projects, |
| func (s *Server) deleteSatelliteByID(w http.ResponseWriter, r *http.Request) { | ||
| result, err := s.dbQueries.ListSatellites(r.Context()) | ||
| if err != nil { | ||
| log.Printf("Error: Failed to assign image to label: %v", err) | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
|
|
||
| WriteJSONResponse(w, http.StatusOK, map[string]string{}) | ||
| WriteJSONResponse(w, http.StatusOK, result) |
There was a problem hiding this comment.
Fix incorrect implementation of deleteSatelliteByID.
Similar to getSatelliteByID, this handler incorrectly uses ListSatellites instead of deleting a specific satellite.
Apply this fix:
func (s *Server) deleteSatelliteByID(w http.ResponseWriter, r *http.Request) {
- result, err := s.dbQueries.ListSatellites(r.Context())
+ vars := mux.Vars(r)
+ idStr := vars["id"]
+
+ id, err := strconv.ParseInt(idStr, 10, 32)
+ if err != nil {
+ HandleAppError(w, &AppError{
+ Message: "Invalid satellite ID",
+ Code: http.StatusBadRequest,
+ })
+ return
+ }
+
+ err = s.dbQueries.DeleteSatellite(r.Context(), int32(id))
if err != nil {
+ if err == sql.ErrNoRows {
+ HandleAppError(w, &AppError{
+ Message: "Satellite not found",
+ Code: http.StatusNotFound,
+ })
+ return
+ }
HandleAppError(w, err)
return
}
- WriteJSONResponse(w, http.StatusOK, result)
+ WriteJSONResponse(w, http.StatusOK, map[string]string{"status": "deleted"})
}📝 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 (s *Server) deleteSatelliteByID(w http.ResponseWriter, r *http.Request) { | |
| result, err := s.dbQueries.ListSatellites(r.Context()) | |
| if err != nil { | |
| log.Printf("Error: Failed to assign image to label: %v", err) | |
| HandleAppError(w, err) | |
| return | |
| } | |
| WriteJSONResponse(w, http.StatusOK, map[string]string{}) | |
| WriteJSONResponse(w, http.StatusOK, result) | |
| func (s *Server) deleteSatelliteByID(w http.ResponseWriter, r *http.Request) { | |
| vars := mux.Vars(r) | |
| idStr := vars["id"] | |
| id, err := strconv.ParseInt(idStr, 10, 32) | |
| if err != nil { | |
| HandleAppError(w, &AppError{ | |
| Message: "Invalid satellite ID", | |
| Code: http.StatusBadRequest, | |
| }) | |
| return | |
| } | |
| err = s.dbQueries.DeleteSatellite(r.Context(), int32(id)) | |
| if err != nil { | |
| if err == sql.ErrNoRows { | |
| HandleAppError(w, &AppError{ | |
| Message: "Satellite not found", | |
| Code: http.StatusNotFound, | |
| }) | |
| return | |
| } | |
| HandleAppError(w, err) | |
| return | |
| } | |
| WriteJSONResponse(w, http.StatusOK, map[string]string{"status": "deleted"}) | |
| } |
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 32
🧹 Outside diff range and nitpick comments (9)
ground-control/sql/schema/008_satellite_token.sql (2)
3-9: Enhance security measures for token management.Consider implementing these security improvements:
- Add token expiration mechanism (
expires_at TIMESTAMP)- Consider hashing tokens before storage
- Add a status field to enable token revocation
CREATE TABLE satellite_token ( id SERIAL PRIMARY KEY, satellite_id INT NOT NULL REFERENCES satellites(id) ON DELETE CASCADE, token VARCHAR(64) UNIQUE NOT NULL, + token_hash VARCHAR(128) UNIQUE NOT NULL, + status VARCHAR(16) NOT NULL DEFAULT 'active', + expires_at TIMESTAMP NOT NULL, created_at TIMESTAMP NOT NULL DEFAULT NOW(), updated_at TIMESTAMP NOT NULL DEFAULT NOW() ); + +CREATE INDEX idx_satellite_token_expires_at ON satellite_token(expires_at); + +-- Ensure only valid status values +ALTER TABLE satellite_token ADD CONSTRAINT chk_status + CHECK (status IN ('active', 'revoked', 'expired'));
11-12: Consider data preservation in down migration.The current down migration will permanently delete all token data. For production safety, consider adding a data preservation mechanism before dropping the table.
-- +goose Down +-- Archive existing tokens before dropping +CREATE TABLE IF NOT EXISTS archived_satellite_tokens AS +SELECT * FROM satellite_token; + DROP TABLE satellite_token;ground-control/sql/schema/004_robots.sql (1)
13-14: Consider data preservation in rollback strategyThe current rollback strategy will permanently delete all robot data. For a production system, consider:
- Adding a data export step before dropping the table
- Cleaning up any dependent objects
- Documenting the rollback implications
Consider adding these safety measures:
-- +goose Down +-- Archive robots data before dropping +CREATE TABLE IF NOT EXISTS archived_robots AS SELECT * FROM robots; DROP TABLE robots;ground-control/sql/schema/007_robot_accounts.sql (1)
8-8: Consider soft deletes for audit purposes.The ON DELETE CASCADE might lead to loss of historical data. Consider implementing soft deletes if audit trail is important for security or compliance.
- satellite_id INT NOT NULL REFERENCES satellites(id) ON DELETE CASCADE, + satellite_id INT NOT NULL REFERENCES satellites(id), + deleted_at TIMESTAMP,ground-control/reg/harbor/client.go (1)
1-26: Consider implementing a more robust Harbor client factory for Zero Touch Registration.For a critical component like Zero Touch Registration, the Harbor client initialization should be more robust. Consider implementing a proper client factory pattern with:
- Connection pooling
- Circuit breaker pattern for failure handling
- Metrics/monitoring for client operations
- Graceful shutdown handling
This would improve reliability and observability of the satellite registration process.
Would you like assistance in designing this improved architecture?
ground-control/sql/queries/satellites.sql (2)
2-3: LGTM! Architectural change noted.The removal of the token field aligns well with the Zero Touch Registration implementation, shifting away from token-based authentication. Using
NOW()for timestamps is a good practice.
17-19: Remove commented-out code if token-based auth is deprecated.If token-based authentication is being fully replaced by Zero Touch Registration, it would be cleaner to remove this code entirely rather than keeping it commented out. This prevents confusion and maintains cleaner code.
ground-control/reg/harbor/project.go (1)
1-41: Consider architectural improvements for better maintainabilityTo better support Zero Touch Registration (ZTR) for satellites, consider these architectural improvements:
- Create a configuration structure for Harbor settings:
type HarborConfig struct { ProjectName string IsPublic bool StorageLimit int64 RequestTimeout time.Duration }
- Implement a proper client management pattern:
type HarborManager struct { client *v2client.HarborAPI config *HarborConfig }
- Add metrics and structured logging for better observability
This would make the code more maintainable and configurable for different deployment scenarios.
ground-control/internal/server/routes.go (1)
27-31: Standardize handler naming convention.The satellite routes mix different naming conventions:
- Some use camelCase:
GetSatelliteByName,DeleteSatelliteByName- Others use lowerCase:
registerSatelliteHandler,listSatelliteHandlerConsider standardizing to one convention:
r.HandleFunc("/satellites/register", s.registerSatelliteHandler).Methods("POST") r.HandleFunc("/satellites/ztr/{token}", s.ztrHandler).Methods("GET") r.HandleFunc("/satellites/list", s.listSatelliteHandler).Methods("GET") -r.HandleFunc("/satellites/{satellite}", s.GetSatelliteByName).Methods("GET") -r.HandleFunc("/satellites/{satellite}", s.DeleteSatelliteByName).Methods("DELETE") +r.HandleFunc("/satellites/{satellite}", s.getSatelliteHandler).Methods("GET") +r.HandleFunc("/satellites/{satellite}", s.deleteSatelliteHandler).Methods("DELETE")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
ground-control/.env(1 hunks)ground-control/internal/database/satellites.sql.go(3 hunks)ground-control/internal/models/models.go(1 hunks)ground-control/internal/server/handlers.go(4 hunks)ground-control/internal/server/routes.go(1 hunks)ground-control/internal/utils/helper.go(1 hunks)ground-control/reg/adapter.go(0 hunks)ground-control/reg/adapter_test.go(0 hunks)ground-control/reg/harbor/client.go(1 hunks)ground-control/reg/harbor/project.go(1 hunks)ground-control/reg/harbor/replication.go(1 hunks)ground-control/reg/harbor/robot.go(7 hunks)ground-control/sql/queries/satellites.sql(1 hunks)ground-control/sql/schema/004_robots.sql(1 hunks)ground-control/sql/schema/005_projects.sql(1 hunks)ground-control/sql/schema/006_robot_projects.sql(1 hunks)ground-control/sql/schema/007_robot_accounts.sql(1 hunks)ground-control/sql/schema/008_satellite_token.sql(1 hunks)
💤 Files with no reviewable changes (2)
- ground-control/reg/adapter.go
- ground-control/reg/adapter_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- ground-control/.env
- ground-control/internal/database/satellites.sql.go
- ground-control/internal/models/models.go
- ground-control/reg/harbor/replication.go
- ground-control/reg/harbor/robot.go
🔇 Additional comments (9)
ground-control/sql/schema/005_projects.sql (1)
10-11: LGTM: Down migration looks correct
The down migration properly removes the table, which is appropriate for this schema change.
ground-control/sql/schema/006_robot_projects.sql (1)
9-10: LGTM!
The down migration correctly drops the table, which is appropriate for a new table migration.
ground-control/sql/schema/008_satellite_token.sql (1)
1-12: Verify integration with codebase.
Let's verify the implementation of SQL queries using this table and ensure proper error handling.
✅ Verification successful
SQL implementation is properly integrated with appropriate error handling
The verification shows that:
- The table is properly integrated with generated SQLC code
- Complete CRUD operations are implemented with proper error handling:
AddToken: Inserts new tokens with satellite associationDeleteToken: Removes tokensGetSatelliteIDByToken: Retrieves satellite ID from tokenGetToken: Fetches complete token recordListToken: Lists all tokens
- Each operation includes context support and error handling
- Foreign key constraint to satellites table is enforced at database level
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for SQL query implementations and error handling
echo "Checking for SQL query implementations..."
rg -l "satellite_token" --type go
echo "Checking for proper error handling in token operations..."
ast-grep --pattern 'err := $$.QueryRow("INSERT INTO satellite_token' -l
Length of output: 487
Script:
#!/bin/bash
# Let's check the content of the generated SQL file and any related Go files
echo "=== Checking generated SQL file ==="
cat ground-control/internal/database/satellite_token.sql.go
echo -e "\n=== Checking for token-related operations in Go files ==="
rg -g '*.go' -B 2 -A 2 'satellite_token'
echo -e "\n=== Looking for token validation or error handling patterns ==="
ast-grep --pattern 'if err := $$.QueryRow($$$); err != nil {
$$$
}' --lang go
Length of output: 5246
ground-control/sql/schema/007_robot_accounts.sql (2)
6-6: Consider encrypting sensitive data.
The robot_secret column stores sensitive credentials. Consider using pgcrypto extension to encrypt this data at rest.
[security]
- robot_secret VARCHAR(255) NOT NULL,
+ robot_secret TEXT NOT NULL CHECK (robot_secret = crypt(robot_secret, gen_salt('bf'))),5-5: Clarify the distinction between robot_name and robot_id.
Both fields are unique identifiers. Could you clarify the intended use case for each? If they serve similar purposes, consider consolidating them to reduce redundancy.
Also applies to: 7-7
ground-control/reg/harbor/client.go (1)
21-26:
Improve error handling and add connection validation for Harbor client.
Converting errors to panics in a service component is risky, especially for Zero Touch Registration where reliability is crucial. Consider:
- Proper error handling instead of panic
- Connection validation
- Retry mechanism for transient failures
Here's a suggested improvement:
-func GetClientByConfig(clientConfig *harbor.ClientSetConfig) *v2client.HarborAPI {
+func GetClientByConfig(clientConfig *harbor.ClientSetConfig) (*v2client.HarborAPI, error) {
cs, err := harbor.NewClientSet(clientConfig)
if err != nil {
- panic(err)
+ return nil, fmt.Errorf("failed to create Harbor client: %w", err)
}
- return cs.V2()
+ client := cs.V2()
+
+ // Validate connection
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ if _, err := client.System.GetSystemInfo(ctx); err != nil {
+ return nil, fmt.Errorf("failed to validate Harbor connection: %w", err)
+ }
+
+ return client, nil
}Let's check if there are any direct dependencies on this client that need to be updated:
#!/bin/bash
# Search for GetClient or GetClientByConfig usage
ast-grep --pattern 'GetClient()'
ast-grep --pattern 'GetClientByConfig($_)'ground-control/sql/queries/satellites.sql (1)
25-27: Verify safety measures for satellite deletion.
The name-based deletion operation raises several concerns:
- Ensure satellite names are unique to prevent accidental multiple deletions
- Consider implementing soft delete for audit purposes
- Add safety checks for active connections or dependencies
✅ Verification successful
Satellite deletion is properly constrained but requires documentation
The verification shows that the implementation is actually well-designed:
- The
namecolumn has aUNIQUEconstraint, preventing duplicate satellite names - All foreign key references have appropriate
ON DELETEclauses:satellite_token:CASCADE- tokens are deleted with the satellitesatellite_groups:CASCADE- group associations are removedrobots:SET NULL- robots are preserved but disassociatedrobot_accounts:CASCADE- associated accounts are removed
While the technical implementation is sound, consider documenting these cascading effects in comments for maintainers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if satellite names have a UNIQUE constraint
echo "Checking for UNIQUE constraint on satellite names..."
rg -A 5 "CREATE TABLE satellites"
# Check for potential multiple references to a satellite
echo "Checking for foreign key references to satellites..."
rg "REFERENCES satellites"
Length of output: 1197
ground-control/reg/harbor/project.go (1)
1-11: LGTM! Clean and focused imports
The imports are appropriate for the Harbor project management functionality.
ground-control/internal/server/routes.go (1)
22-24: Implement the group satellite listing functionality.
The TODO comment indicates a missing feature for listing satellites within a group. This functionality would be valuable for ground control admins and follows good REST practices.
Consider implementing this endpoint as it would complement the existing group management features.
Would you like me to help implement the groupSatelliteHandler or create an issue to track this task?
| CREATE TABLE projects ( | ||
| id SERIAL PRIMARY KEY, | ||
| projects VARCHAR(255) NOT NULL, | ||
| created_at TIMESTAMP DEFAULT NOW() NOT NULL, | ||
| updated_at TIMESTAMP DEFAULT NOW() NOT NULL | ||
| ); |
There was a problem hiding this comment.
Add trigger for updated_at timestamp
The updated_at column should be automatically updated whenever a row is modified. Add a trigger to handle this.
CREATE OR REPLACE FUNCTION update_updated_at_column()
RETURNS TRIGGER AS $$
BEGIN
NEW.updated_at = NOW();
RETURN NEW;
END;
$$ language 'plpgsql';
CREATE TRIGGER update_projects_updated_at
BEFORE UPDATE ON projects
FOR EACH ROW
EXECUTE FUNCTION update_updated_at_column();🛠️ Refactor suggestion
Consider adding a unique constraint on the name column
Since this appears to be a project management table, it would be logical to prevent duplicate project names.
CREATE TABLE projects (
id SERIAL PRIMARY KEY,
- projects VARCHAR(255) NOT NULL,
+ projects VARCHAR(255) NOT NULL UNIQUE,
created_at TIMESTAMP DEFAULT NOW() NOT NULL,
updated_at TIMESTAMP DEFAULT NOW() NOT NULL
);📝 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.
| CREATE TABLE projects ( | |
| id SERIAL PRIMARY KEY, | |
| projects VARCHAR(255) NOT NULL, | |
| created_at TIMESTAMP DEFAULT NOW() NOT NULL, | |
| updated_at TIMESTAMP DEFAULT NOW() NOT NULL | |
| ); | |
| CREATE TABLE projects ( | |
| id SERIAL PRIMARY KEY, | |
| projects VARCHAR(255) NOT NULL UNIQUE, | |
| created_at TIMESTAMP DEFAULT NOW() NOT NULL, | |
| updated_at TIMESTAMP DEFAULT NOW() NOT NULL | |
| ); |
Review column naming convention
The column name projects (plural) in a table named projects is unusual and potentially confusing. Consider renaming it to name or project_name (singular) to better represent a single project's name.
CREATE TABLE projects (
id SERIAL PRIMARY KEY,
- projects VARCHAR(255) NOT NULL,
+ name VARCHAR(255) NOT NULL,
created_at TIMESTAMP DEFAULT NOW() NOT NULL,
updated_at TIMESTAMP DEFAULT NOW() NOT NULL
);📝 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.
| CREATE TABLE projects ( | |
| id SERIAL PRIMARY KEY, | |
| projects VARCHAR(255) NOT NULL, | |
| created_at TIMESTAMP DEFAULT NOW() NOT NULL, | |
| updated_at TIMESTAMP DEFAULT NOW() NOT NULL | |
| ); | |
| CREATE TABLE projects ( | |
| id SERIAL PRIMARY KEY, | |
| name VARCHAR(255) NOT NULL, | |
| created_at TIMESTAMP DEFAULT NOW() NOT NULL, | |
| updated_at TIMESTAMP DEFAULT NOW() NOT NULL | |
| ); |
💡 Codebase verification
Consider adding indexes for robot_projects and projects tables
Based on the codebase analysis, here are the recommended indexes:
- For the
projectstable:CREATE INDEX idx_projects_name ON projects(projects);
- For the
robot_projectstable:CREATE INDEX idx_robot_projects_project_id ON robot_projects(project_id); CREATE INDEX idx_robot_projects_robot_id ON robot_projects(robot_id);
The indexes are recommended because:
- Projects are frequently looked up by name during robot account creation and group operations
- The
robot_projectsjunction table will be queried frequently to find project associations - Both robot_id and project_id are used in JOIN operations and foreign key relationships
🔗 Analysis chain
Consider adding indexes based on access patterns
Given this is part of a Zero Touch Satellite Registration system, you might want to add indexes based on how you'll query this table. For example, if you'll be frequently looking up projects by name:
CREATE INDEX idx_projects_name ON projects(projects);Please verify the query patterns in your application code to determine if additional indexes would be beneficial.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SQL queries involving the projects table to understand access patterns
rg -A 3 "SELECT.*FROM.*projects" --type sql
Length of output: 45
Script:
#!/bin/bash
# Let's search for any SQL queries involving the projects table, including JOINs and WHERE clauses
rg -i "projects.*where|from.*projects|join.*projects" --type-add 'code:*.{js,ts,go,rb,py}' --type code -A 3
# Also search for repository files that might contain SQL queries
fd -e sql -e js -e ts -e go -e rb -e py -x grep -l -i "projects" {}
Length of output: 576
Script:
#!/bin/bash
# Let's examine the content of these files to understand the query patterns
cat ./ground-control/sql/queries/groups.sql
cat ./ground-control/internal/database/groups.sql.go
cat ./ground-control/sql/schema/006_robot_projects.sql
# Also check for any model definitions or structs related to projects
rg -A 5 "type.*Project" ./ground-control/internal/database/models.go
Length of output: 3981
Script:
#!/bin/bash
# Let's check for any direct queries on the projects table
rg -A 3 "SELECT.*FROM.*projects|INSERT.*INTO.*projects|UPDATE.*projects" ./ground-control/sql/queries/
# Let's also check handlers to see how projects are accessed
cat ./ground-control/internal/server/handlers.go
Length of output: 16651
| CREATE TABLE robot_projects ( | ||
| robot_id INT REFERENCES robots(id) ON DELETE CASCADE, | ||
| project_id INT REFERENCES projects(id) ON DELETE CASCADE, | ||
| PRIMARY KEY (robot_id, project_id) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding audit fields and indexes for better performance.
The table structure correctly implements the many-to-many relationship between robots and projects with proper foreign key constraints. However, consider the following improvements:
- Add timestamp fields for auditing:
created_atandupdated_at - Add an index on
project_idfor queries filtering by project - Consider adding a
statusfield for soft deletion capability
Here's the enhanced table definition:
CREATE TABLE robot_projects (
robot_id INT REFERENCES robots(id) ON DELETE CASCADE,
project_id INT REFERENCES projects(id) ON DELETE CASCADE,
- PRIMARY KEY (robot_id, project_id)
+ PRIMARY KEY (robot_id, project_id),
+ created_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP,
+ updated_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP
);
+
+CREATE INDEX idx_robot_projects_project_id ON robot_projects(project_id);
+
+-- Add trigger to update updated_at timestamp
+CREATE TRIGGER update_robot_projects_updated_at
+ BEFORE UPDATE ON robot_projects
+ FOR EACH ROW
+ EXECUTE FUNCTION update_updated_at_column();Committable suggestion was skipped due to low confidence.
| -- +goose Up | ||
|
|
||
| CREATE TABLE robot_projects ( | ||
| robot_id INT REFERENCES robots(id) ON DELETE CASCADE, | ||
| project_id INT REFERENCES projects(id) ON DELETE CASCADE, | ||
| PRIMARY KEY (robot_id, project_id) | ||
| ); | ||
|
|
||
| -- +goose Down | ||
| DROP TABLE robot_projects; |
There was a problem hiding this comment.
💡 Codebase verification
Schema changes need adjustments for Zero Touch Registration
Based on the verification results, there are several issues that need to be addressed:
- The
projectstable has a column namedprojects(possibly a typo) instead of an expectednamecolumn - The
projectstable lacks essential columns for access control (likeowner_idor role-related fields) - No existing robot registration endpoints or handlers were found in the codebase
- The satellite-related endpoints are limited to a ping endpoint in test code
The current schema changes for robot_projects are foundational but insufficient for Zero Touch Registration without the proper supporting infrastructure.
🔗 Analysis chain
Verify integration with Zero Touch Registration flow.
The schema changes support the Zero Touch Registration requirements by enabling satellite-to-project associations. However, we should verify:
- The registration flow's ability to create these associations
- The existence of required permissions/roles in the projects table
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if projects table has necessary columns for ZTR
echo "Verifying projects table structure..."
rg -A 10 "CREATE TABLE projects"
# Check for any existing robot registration related functions
echo "Checking for robot registration procedures..."
rg -l "FUNCTION.*register.*robot"
Length of output: 903
Script:
#!/bin/bash
# Let's check for any robot registration related code in Go files
echo "Checking for robot registration handlers..."
rg -A 5 "func.*Register.*Robot"
# Check for any robot-related API endpoints
echo "Checking for robot API endpoints..."
rg -A 5 "(/api/.*robot|/api/.*satellite)"
# Check for any robot registration related migrations or procedures
echo "Checking for robot registration migrations..."
fd -e sql -x grep -l -i "robot.*registration"
# Let's also check the projects table column name
echo "Verifying projects column name..."
rg "projects\s+VARCHAR" ground-control/sql/schema/005_projects.sql
Length of output: 781
| CREATE TABLE satellite_token ( | ||
| id SERIAL PRIMARY KEY, | ||
| satellite_id INT NOT NULL REFERENCES satellites(id) ON DELETE CASCADE, | ||
| token VARCHAR(64) UNIQUE NOT NULL, | ||
| created_at TIMESTAMP NOT NULL DEFAULT NOW(), | ||
| updated_at TIMESTAMP NOT NULL DEFAULT NOW() | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing table structure for better security and performance.
While the basic structure is sound, consider the following improvements:
- Increase token length to accommodate more secure tokens (recommend VARCHAR(128))
- Add an explicit index on satellite_id for better query performance
- Add table/column comments to document the purpose
CREATE TABLE satellite_token (
id SERIAL PRIMARY KEY,
satellite_id INT NOT NULL REFERENCES satellites(id) ON DELETE CASCADE,
- token VARCHAR(64) UNIQUE NOT NULL,
+ token VARCHAR(128) UNIQUE NOT NULL,
created_at TIMESTAMP NOT NULL DEFAULT NOW(),
updated_at TIMESTAMP NOT NULL DEFAULT NOW()
);
+
+CREATE INDEX idx_satellite_token_satellite_id ON satellite_token(satellite_id);
+
+COMMENT ON TABLE satellite_token IS 'Stores authentication tokens for zero-touch satellite registration';
+COMMENT ON COLUMN satellite_token.token IS 'Unique authentication token used during satellite registration';📝 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.
| CREATE TABLE satellite_token ( | |
| id SERIAL PRIMARY KEY, | |
| satellite_id INT NOT NULL REFERENCES satellites(id) ON DELETE CASCADE, | |
| token VARCHAR(64) UNIQUE NOT NULL, | |
| created_at TIMESTAMP NOT NULL DEFAULT NOW(), | |
| updated_at TIMESTAMP NOT NULL DEFAULT NOW() | |
| ); | |
| CREATE TABLE satellite_token ( | |
| id SERIAL PRIMARY KEY, | |
| satellite_id INT NOT NULL REFERENCES satellites(id) ON DELETE CASCADE, | |
| token VARCHAR(128) UNIQUE NOT NULL, | |
| created_at TIMESTAMP NOT NULL DEFAULT NOW(), | |
| updated_at TIMESTAMP NOT NULL DEFAULT NOW() | |
| ); | |
| CREATE INDEX idx_satellite_token_satellite_id ON satellite_token(satellite_id); | |
| COMMENT ON TABLE satellite_token IS 'Stores authentication tokens for zero-touch satellite registration'; | |
| COMMENT ON COLUMN satellite_token.token IS 'Unique authentication token used during satellite registration'; |
| CREATE TABLE robots ( | ||
| id SERIAL PRIMARY KEY, | ||
| robot_name VARCHAR(255) UNIQUE NOT NULL, | ||
| secret VARCHAR(255) NOT NULL, | ||
| satellite_id INT UNIQUE | ||
| REFERENCES satellites(id) ON DELETE SET NULL, | ||
| created_at TIMESTAMP DEFAULT NOW() NOT NULL, | ||
| updated_at TIMESTAMP DEFAULT NOW() NOT NULL | ||
| ); |
There was a problem hiding this comment.
Enhance security and add missing database features
Several improvements could be made to strengthen security and follow best practices:
-
The
secretcolumn stores sensitive data and should have:- Minimum length requirement
- Hash format validation
- Consider using a more appropriate type like CHAR(60) for bcrypt hashes
-
Add an update trigger for
updated_at -
Consider adding appropriate indexes for common query patterns
Apply these improvements:
CREATE TABLE robots (
id SERIAL PRIMARY KEY,
robot_name VARCHAR(255) UNIQUE NOT NULL,
- secret VARCHAR(255) NOT NULL,
+ secret CHAR(60) NOT NULL CHECK (LENGTH(secret) = 60),
satellite_id INT UNIQUE
REFERENCES satellites(id) ON DELETE SET NULL,
created_at TIMESTAMP DEFAULT NOW() NOT NULL,
updated_at TIMESTAMP DEFAULT NOW() NOT NULL
);
+
+CREATE INDEX idx_robots_robot_name ON robots(robot_name);
+
+CREATE TRIGGER update_robots_updated_at
+ BEFORE UPDATE ON robots
+ FOR EACH ROW
+ EXECUTE FUNCTION update_updated_at_column();Committable suggestion was skipped due to low confidence.
| // Update robot account | ||
| func UpdateRobotProjects(ctx context.Context, projects []string, name string, id string) (*robot.UpdateRobotOK, error) { | ||
| ID, err := strconv.ParseInt(id, 10, 64) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error invalid ID: %w", err) | ||
| } | ||
| robot, err := harbor.GetRobotAccount(ctx, ID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting robot account: %w", err) | ||
| } | ||
|
|
||
| robot.Permissions = harbor.GenRobotPerms(projects) | ||
|
|
||
| updated, err := harbor.UpdateRobotAccount(ctx, robot) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error updating robot account: %w", err) | ||
| } | ||
|
|
||
| return updated, nil | ||
| } |
There was a problem hiding this comment.
Add input validation and improve error messages
The function needs input validation for projects array and name.
Add input validation:
func UpdateRobotProjects(ctx context.Context, projects []string, name string, id string) (*robot.UpdateRobotOK, error) {
+ if len(projects) == 0 {
+ return nil, fmt.Errorf("projects list cannot be empty")
+ }
+ if name == "" {
+ return nil, fmt.Errorf("robot name cannot be empty")
+ }
+ if id == "" {
+ return nil, fmt.Errorf("robot ID cannot be empty")
+ }
+
ID, err := strconv.ParseInt(id, 10, 64)
if err != nil {
- return nil, fmt.Errorf("error invalid ID: %w", err)
+ return nil, fmt.Errorf("invalid robot ID %q: %w", id, err)
}📝 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.
| // Update robot account | |
| func UpdateRobotProjects(ctx context.Context, projects []string, name string, id string) (*robot.UpdateRobotOK, error) { | |
| ID, err := strconv.ParseInt(id, 10, 64) | |
| if err != nil { | |
| return nil, fmt.Errorf("error invalid ID: %w", err) | |
| } | |
| robot, err := harbor.GetRobotAccount(ctx, ID) | |
| if err != nil { | |
| return nil, fmt.Errorf("error getting robot account: %w", err) | |
| } | |
| robot.Permissions = harbor.GenRobotPerms(projects) | |
| updated, err := harbor.UpdateRobotAccount(ctx, robot) | |
| if err != nil { | |
| return nil, fmt.Errorf("error updating robot account: %w", err) | |
| } | |
| return updated, nil | |
| } | |
| // Update robot account | |
| func UpdateRobotProjects(ctx context.Context, projects []string, name string, id string) (*robot.UpdateRobotOK, error) { | |
| if len(projects) == 0 { | |
| return nil, fmt.Errorf("projects list cannot be empty") | |
| } | |
| if name == "" { | |
| return nil, fmt.Errorf("robot name cannot be empty") | |
| } | |
| if id == "" { | |
| return nil, fmt.Errorf("robot ID cannot be empty") | |
| } | |
| ID, err := strconv.ParseInt(id, 10, 64) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid robot ID %q: %w", id, err) | |
| } | |
| robot, err := harbor.GetRobotAccount(ctx, ID) | |
| if err != nil { | |
| return nil, fmt.Errorf("error getting robot account: %w", err) | |
| } | |
| robot.Permissions = harbor.GenRobotPerms(projects) | |
| updated, err := harbor.UpdateRobotAccount(ctx, robot) | |
| if err != nil { | |
| return nil, fmt.Errorf("error updating robot account: %w", err) | |
| } | |
| return updated, nil | |
| } |
| // check if project satellite exists and if does not exist create project satellite | ||
| satExist, err := harbor.GetProject(r.Context(), "satellite") | ||
| if err != nil { | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: fmt.Sprintf("Error: Checking satellite project: %v", err), | ||
| Code: http.StatusBadGateway, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } | ||
| if !satExist { | ||
| _, err := harbor.CreateSatelliteProject(r.Context()) | ||
| if err != nil { | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: fmt.Sprintf("Error: creating satellite project: %v", err), | ||
| Code: http.StatusBadGateway, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract satellite project creation logic into a shared function.
This code block is duplicated in both groupsSyncHandler and registerSatelliteHandler. Consider extracting it into a shared function to improve maintainability.
func (s *Server) ensureSatelliteProjectExists(ctx context.Context) error {
satExist, err := harbor.GetProject(ctx, "satellite")
if err != nil {
return fmt.Errorf("checking satellite project: %w", err)
}
if !satExist {
if _, err := harbor.CreateSatelliteProject(ctx); err != nil {
return fmt.Errorf("creating satellite project: %w", err)
}
}
return nil
}Also applies to: 117-140
| func (s *Server) ztrHandler(w http.ResponseWriter, r *http.Request) { | ||
| vars := mux.Vars(r) | ||
| token := vars["token"] | ||
|
|
||
| // Start a new transaction | ||
| tx, err := s.db.BeginTx(r.Context(), nil) | ||
| if err != nil { | ||
| log.Println(err) | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
|
|
||
| params := database.AddSatelliteToGroupParams{ | ||
| SatelliteID: int32(req.SatelliteID), | ||
| GroupID: int32(req.GroupID), | ||
| q := s.dbQueries.WithTx(tx) | ||
|
|
||
| defer func() { | ||
| if p := recover(); p != nil { | ||
| // If there's a panic, rollback the transaction | ||
| tx.Rollback() | ||
| panic(p) // Re-throw the panic after rolling back | ||
| } else if err != nil { | ||
| tx.Rollback() // Rollback transaction on error | ||
| } | ||
| }() | ||
|
|
||
| satelliteID, err := q.GetSatelliteIDByToken(r.Context(), token) | ||
| if err != nil { | ||
| log.Println("Invalid Satellite Token") | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: "Error: Invalid Token", | ||
| Code: http.StatusBadRequest, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return |
There was a problem hiding this comment.
Improve token validation security.
Several security concerns in the token validation process:
- Tokens in URLs can be logged by proxies
- No rate limiting for validation attempts
- Token is deleted before verifying the complete registration chain
Consider these improvements:
- Accept token in request body or header instead of URL
- Implement rate limiting for token validation
- Move token deletion to after successful validation
Example rate limiting implementation:
func (s *Server) rateLimitedZTR(token string) error {
key := fmt.Sprintf("ztr_attempts:%s", token)
attempts, err := s.cache.Incr(key)
if err != nil {
return err
}
// Set key expiration on first attempt
if attempts == 1 {
s.cache.Expire(key, 1*time.Hour)
}
if attempts > 5 {
return fmt.Errorf("too many validation attempts")
}
return nil
}| func (s *Server) listSatelliteHandler(w http.ResponseWriter, r *http.Request) { | ||
| result, err := s.dbQueries.ListSatellites(r.Context()) | ||
| if err != nil { | ||
| log.Printf("Error: Failed to add satellite to label: %v", err) | ||
| log.Printf("Error: Failed to List Satellites: %v", err) | ||
| err := &AppError{ | ||
| Message: "Error: Failed to List Satellites", | ||
| Code: http.StatusInternalServerError, | ||
| } | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
|
|
||
| WriteJSONResponse(w, http.StatusOK, map[string]string{}) | ||
| WriteJSONResponse(w, http.StatusOK, result) |
There was a problem hiding this comment.
Add pagination to satellite listing.
The current implementation loads all satellites at once, which could cause performance issues with a large number of satellites.
Add pagination support:
type ListSatellitesParams struct {
Limit int32 `json:"limit"`
Offset int32 `json:"offset"`
}
func (s *Server) listSatelliteHandler(w http.ResponseWriter, r *http.Request) {
params := ListSatellitesParams{
Limit: 20, // Default limit
Offset: 0,
}
if limit := r.URL.Query().Get("limit"); limit != "" {
if l, err := strconv.ParseInt(limit, 10, 32); err == nil {
params.Limit = int32(l)
}
}
if offset := r.URL.Query().Get("offset"); offset != "" {
if o, err := strconv.ParseInt(offset, 10, 32); err == nil {
params.Offset = int32(o)
}
}
result, total, err := s.dbQueries.ListSatellitesPaginated(r.Context(), params)
// ... error handling ...
response := struct {
Items []database.Satellite `json:"items"`
Total int `json:"total"`
}{
Items: result,
Total: total,
}
WriteJSONResponse(w, http.StatusOK, response)
}| func (s *Server) addSatelliteToGroup(w http.ResponseWriter, r *http.Request) { | ||
| var req SatelliteGroupParams | ||
| if err := DecodeRequestBody(r, &req); err != nil { | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
|
|
||
| params := database.AssignImageToGroupParams{ | ||
| GroupID: int32(req.GroupID), | ||
| ImageID: int32(req.ImageID), | ||
| sat, err := s.dbQueries.GetSatelliteByName(r.Context(), req.Satellite) | ||
| if err != nil { | ||
| log.Printf("Error: Satellite Not Found: %v", err) | ||
| err := &AppError{ | ||
| Message: "Error: Satellite Not Found", | ||
| Code: http.StatusBadRequest, | ||
| } | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
| grp, err := s.dbQueries.GetGroupByName(r.Context(), req.Group) | ||
| if err != nil { | ||
| log.Printf("Error: Group Not Found: %v", err) | ||
| err := &AppError{ | ||
| Message: "Error: Group Not Found", | ||
| Code: http.StatusBadRequest, | ||
| } | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
|
|
||
| params := database.AddSatelliteToGroupParams{ | ||
| SatelliteID: int32(sat.ID), | ||
| GroupID: int32(grp.ID), | ||
| } | ||
|
|
||
| err := s.dbQueries.AssignImageToGroup(r.Context(), params) | ||
| err = s.dbQueries.AddSatelliteToGroup(r.Context(), params) | ||
| if err != nil { | ||
| log.Printf("Error: Failed to assign image to group: %v", err) | ||
| log.Printf("Error: Failed to Add Satellite to Group: %v", err) | ||
| err := &AppError{ | ||
| Message: "Error: Failed to Add Satellite to Group", | ||
| Code: http.StatusInternalServerError, | ||
| } |
There was a problem hiding this comment.
Add validation for group relationships.
The current implementation doesn't validate group relationships, which could lead to:
- Circular dependencies between groups
- Orphaned resources when groups are deleted
Consider adding these validations:
func (s *Server) validateGroupRelationship(ctx context.Context, satelliteID, groupID int32) error {
// Check for circular dependencies
visited := make(map[int32]bool)
var checkCycle func(int32) error
checkCycle = func(gID int32) error {
if visited[gID] {
return fmt.Errorf("circular dependency detected")
}
visited[gID] = true
parentGroups, err := s.dbQueries.GetParentGroups(ctx, gID)
if err != nil {
return err
}
for _, parent := range parentGroups {
if err := checkCycle(parent.ID); err != nil {
return err
}
}
return nil
}
return checkCycle(groupID)
}Signed-off-by: bupd <[email protected]>
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (5)
ground-control/sql/schema/005_satellite_token.sql (1)
3-9: Consider enhancing the schema with additional security and performance features.The table structure is well-designed with appropriate constraints and relationships. However, consider the following improvements:
- Add a token expiration timestamp for enhanced security:
CREATE TABLE satellite_token ( id SERIAL PRIMARY KEY, satellite_id INT NOT NULL REFERENCES satellites(id) ON DELETE CASCADE, token VARCHAR(64) UNIQUE NOT NULL, + expires_at TIMESTAMPTZ, - created_at TIMESTAMP NOT NULL DEFAULT NOW(), - updated_at TIMESTAMP NOT NULL DEFAULT NOW() + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() );
- Add an index to optimize token lookups:
CREATE INDEX idx_satellite_token_token ON satellite_token(token);config.toml (2)
Line range hint
1-1: Fix typo in comment: "Wether" should be "Whether".-# Wether to us the built-in Zot registry or not +# Whether to use the built-in Zot registry or not
Line range hint
1-19: Improve configuration documentation and validation.Consider enhancing the configuration file with:
- Type hints and validation rules for each setting
- Consistent use of quotes (some values use double quotes, others don't)
- Grouping related settings together (e.g., all registry settings)
- Documentation about which settings are required for ZTR
Would you like me to propose a fully documented and structured version of this configuration file?
main.go (1)
33-36: Consider enhancing Zero Touch Registration robustness.To improve the Zero Touch Registration implementation, consider these architectural enhancements:
- Add validation for required ZTR-specific configurations
- Implement health checks to verify ZTR readiness
- Add retry logic for handling transient initialization failures
I can help implement these enhancements. Would you like me to:
- Generate code for ZTR configuration validation?
- Implement health check endpoints for ZTR status?
- Add retry logic with backoff for initialization?
internal/config/config.go (1)
49-81: Enhance error messages with more contextThe error messages in the
ztrfunction are generic and may not provide enough context when issues arise. Including additional information can aid in debugging.Refine error messages to include request details:
if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("unexpected response from Ground Control: %v", resp.Status) + return nil, fmt.Errorf("unexpected response from Ground Control at %s: %v", url, resp.Status) } // Decode the JSON response var res ZtrResult if err := json.NewDecoder(resp.Body).Decode(&res); err != nil { - return nil, fmt.Errorf("failed to decode JSON response: %v", err) + return nil, fmt.Errorf("failed to decode JSON response from %s: %v", url, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
ground-control/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
config.toml(1 hunks)ground-control/go.mod(2 hunks)ground-control/internal/database/models.go(3 hunks)ground-control/sql/schema/004_robot_accounts.sql(1 hunks)ground-control/sql/schema/005_satellite_token.sql(1 hunks)internal/config/config.go(5 hunks)main.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ground-control/go.mod
🔇 Additional comments (7)
ground-control/sql/schema/005_satellite_token.sql (1)
11-12: LGTM!
The rollback command appropriately drops the table, maintaining a clean migration history.
ground-control/sql/schema/004_robot_accounts.sql (2)
1-2: LGTM: Migration structure follows best practices
The migration file is properly structured with both Up and Down migrations, allowing for proper version control and rollback capabilities.
Also applies to: 13-14
5-7: Clarify the relationship between robot_name and robot_id
Both robot_name and robot_id are unique identifiers. Please clarify:
- What's the semantic difference between these fields?
- Could one of them be redundant?
- Should there be any specific format validation?
Let's check if these fields are used consistently across the codebase:
✅ Verification successful
Let me search for any additional context about these fields in the codebase, particularly in Go files that might show their usage patterns:
Based on the code analysis, I can now provide a clear explanation about the fields:
robot_name and robot_id serve distinct purposes - both fields are necessary
The codebase reveals that:
robot_nameis a human-readable identifier used for Harbor robot account creation (e.g., satellite name)robot_idstores Harbor's internal numeric ID as a string (converted from int64), used for API operations- Both fields are used in different contexts:
robot_nameis used for template creation and displayrobot_idis used for Harbor API operations (delete, refresh, update)
Example from the code:
// Creation
RobotName: rbt.Name, // Human readable name
RobotID: strconv.Itoa(int(rbt.ID)), // Harbor's internal IDFormat validation should be added:
robot_nameshould follow Harbor's naming conventionsrobot_idshould be a valid numeric string
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage patterns of robot_name and robot_id
echo "Searching for robot_name usage:"
rg "robot_name" -A 3
echo -e "\nSearching for robot_id usage:"
rg "robot_id" -A 3
Length of output: 7413
Script:
#!/bin/bash
# Search for robot account related structs and types
ast-grep --pattern 'type $_ struct {
$$$
RobotName $_
$$$
}'
echo -e "\nSearching for robot account related code comments:"
rg -i "robot.*name|robot.*id" --type go -A 2
Length of output: 13509
config.toml (1)
18-19: 🛠️ Refactor suggestion
Reconsider error-only logging for new feature deployment.
Setting log_level to "error" might make it difficult to debug Zero Touch Registration issues during initial deployment. Consider using "info" level initially and documenting the recommended production level.
# Set logging level: debug, info, warn, error, fatal, panic
-log_level = "error"
+# Development: Use "info" or "debug" for better visibility during ZTR implementation
+# Production: Use "error" for minimal logging once stable
+log_level = "info"ground-control/internal/database/models.go (1)
Line range hint 1-47: Models support Zero Touch Registration requirements.
The new and modified data structures align well with the PR objectives for implementing Zero Touch Registration (ZTR) for satellites:
Groupstructure supports satellite group managementRobotAccountstructure enables automated authenticationSatelliteTokenstructure facilitates secure registration
Note: This is an autogenerated file by sqlc, so no direct code modifications should be made here.
Let's verify if the corresponding SQL migrations exist for these model changes:
✅ Verification successful
Generated models match their SQL schema definitions perfectly
The autogenerated models in models.go correctly reflect all the SQL schema migrations:
- All tables (
satellites,groups,satellite_groups,robot_accounts,satellite_token) have corresponding migrations - Field types and names in the Go structs match their SQL counterparts
- Foreign key relationships are properly represented
- Timestamps (
created_at,updated_at) are consistently defined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related migration files that define these models
# Expected: Find SQL files containing CREATE TABLE statements for the new structures
# Look for migration files
fd -e sql | while read -r file; do
echo "=== Checking $file ==="
rg -A 3 'CREATE TABLE.*(groups|robot_accounts|satellite_tokens)' "$file"
done
Length of output: 3102
Script:
#!/bin/bash
# Let's check for satellite_token table definition which wasn't found in the previous search
# and verify the complete schema for all tables
# First, let's check for satellite_token table specifically
echo "=== Checking for satellite_token table ==="
rg -p "CREATE TABLE.*satellite_token" ground-control/sql/schema/
# Now let's verify all table definitions match our models
echo -e "\n=== Checking schema details for all related tables ==="
for file in $(fd -e sql . ground-control/sql/schema/); do
echo -e "\n--- Content of $file ---"
cat "$file"
done
Length of output: 3209
main.go (2)
21-21: LGTM! Good addition for Zero Touch Registration.
The automatic loading of environment variables via godotenv/autoload aligns well with the Zero Touch Registration requirements, ensuring configuration is loaded without manual intervention.
Line range hint 21-35: Verify environment variable usage and error handling patterns.
Let's verify the consistency of environment variable handling and error patterns across the codebase.
✅ Verification successful
Let me gather more specific information about the environment variable usage patterns to ensure consistency.
Environment variable handling and error patterns are consistent across the codebase
The verification shows:
- Consistent use of
godotenv/autoloadfor environment variable loading across multiple packages - Proper config initialization pattern in
main.gothat aligns with the implementation ininternal/config/config.go - Consistent error wrapping patterns using
fmt.Errorfwith appropriate context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check environment variable usage and error handling patterns
# Check for environment variable usage patterns
echo "Checking environment variable usage patterns..."
rg -l "os\.Getenv|godotenv"
# Check error handling patterns
echo -e "\nChecking error handling patterns..."
ast-grep --pattern 'if err := $_; err != nil {
return fmt.Errorf($_, err)
}'
Length of output: 1175
Script:
#!/bin/bash
# Check specific environment variable usage patterns in config initialization
echo "Checking config initialization and env patterns..."
rg -A 5 "InitConfig|LoadConfig"
# Check godotenv usage patterns
echo -e "\nChecking godotenv usage patterns..."
rg -A 3 "godotenv"
# Check error wrapping patterns in config related files
echo -e "\nChecking error handling in config files..."
rg -A 2 "fmt\.Errorf.*config" internal/
Length of output: 4525
| robot_name VARCHAR(255) UNIQUE NOT NULL, | ||
| robot_secret VARCHAR(255) NOT NULL, | ||
| robot_id VARCHAR(255) UNIQUE NOT NULL, | ||
| satellite_id INT NOT NULL REFERENCES satellites(id) ON DELETE CASCADE, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add index for foreign key performance
The satellite_id foreign key should be indexed to improve query performance, especially for lookups and joins.
Add this after the table creation:
CREATE INDEX idx_robot_accounts_satellite_id ON robot_accounts(satellite_id);| created_at TIMESTAMP NOT NULL DEFAULT NOW(), | ||
| updated_at TIMESTAMP NOT NULL DEFAULT NOW() |
There was a problem hiding this comment.
Add trigger for updated_at timestamp
The updated_at timestamp won't automatically update on record changes.
Add this after the table creation:
CREATE OR REPLACE FUNCTION update_updated_at_column()
RETURNS TRIGGER AS $$
BEGIN
NEW.updated_at = NOW();
RETURN NEW;
END;
$$ language 'plpgsql';
CREATE TRIGGER update_robot_accounts_updated_at
BEFORE UPDATE ON robot_accounts
FOR EACH ROW
EXECUTE FUNCTION update_updated_at_column();| CREATE TABLE robot_accounts ( | ||
| id SERIAL PRIMARY KEY, | ||
| robot_name VARCHAR(255) UNIQUE NOT NULL, | ||
| robot_secret VARCHAR(255) NOT NULL, | ||
| robot_id VARCHAR(255) UNIQUE NOT NULL, | ||
| satellite_id INT NOT NULL REFERENCES satellites(id) ON DELETE CASCADE, | ||
| created_at TIMESTAMP NOT NULL DEFAULT NOW(), | ||
| updated_at TIMESTAMP NOT NULL DEFAULT NOW() | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider additional schema improvements
A few suggestions to enhance the schema:
- Add CHECK constraints for field formats if applicable
- Consider adding a comment to the table explaining its purpose
- Consider adding a status field for account lifecycle management
Example improvements:
COMMENT ON TABLE robot_accounts IS 'Stores robot account credentials for zero-touch satellite registration';
-- Add format validation if there's a specific format requirement
ALTER TABLE robot_accounts
ADD CONSTRAINT check_robot_id_format
CHECK (robot_id ~ '^[A-Za-z0-9_-]+$');
-- Add status field for account lifecycle
ALTER TABLE robot_accounts
ADD COLUMN status VARCHAR(50) NOT NULL
DEFAULT 'active'
CHECK (status IN ('active', 'inactive', 'suspended'));| CREATE TABLE robot_accounts ( | ||
| id SERIAL PRIMARY KEY, | ||
| robot_name VARCHAR(255) UNIQUE NOT NULL, | ||
| robot_secret VARCHAR(255) NOT NULL, |
There was a problem hiding this comment.
Security concern: Sensitive data storage
The robot_secret column stores secrets in plaintext. Consider using a more secure approach.
Apply this diff to hash the secrets:
- robot_secret VARCHAR(255) NOT NULL,
+ robot_secret_hash VARCHAR(255) NOT NULL,Additionally, ensure that the application layer hashes the secrets before storage.
Committable suggestion skipped: line range outside the PR's diff.
| # ground control | ||
| ground_control_url = "http://localhost:8080" | ||
| # eg. ground_control = "https://ground-control.bupd.xyz" |
There was a problem hiding this comment.
Security: Use HTTPS for ground control communication.
The default ground control URL uses HTTP, which is insecure for satellite communication. Consider:
- Making HTTPS mandatory for production environments
- Adding a comment about security implications
# ground control
-ground_control_url = "http://localhost:8080"
+ground_control_url = "https://localhost:8080"
-# eg. ground_control = "https://ground-control.bupd.xyz"
+# Production deployments must use HTTPS, eg: "https://ground-control.bupd.xyz"
+# Note: Using HTTP in production is insecure and not recommended for satellite communication📝 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.
| # ground control | |
| ground_control_url = "http://localhost:8080" | |
| # eg. ground_control = "https://ground-control.bupd.xyz" | |
| # ground control | |
| ground_control_url = "https://localhost:8080" | |
| # Production deployments must use HTTPS, eg: "https://ground-control.bupd.xyz" | |
| # Note: Using HTTP in production is insecure and not recommended for satellite communication |
internal/config/config.go
Outdated
| // Construct the request URL | ||
| url := fmt.Sprintf("%s/satellites/ztr/%s", gcUrl, token) |
There was a problem hiding this comment.
Ensure ground_control_url is not empty
The gcUrl parameter should be validated to prevent constructing invalid URLs, which could cause HTTP request failures.
Add a check for gcUrl:
// Construct the request URL
+if gcUrl == "" {
+ return nil, fmt.Errorf("ground_control_url is empty")
+}
url := fmt.Sprintf("%s/satellites/ztr/%s", gcUrl, token)📝 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.
| // Construct the request URL | |
| url := fmt.Sprintf("%s/satellites/ztr/%s", gcUrl, token) | |
| // Construct the request URL | |
| if gcUrl == "" { | |
| return nil, fmt.Errorf("ground_control_url is empty") | |
| } | |
| url := fmt.Sprintf("%s/satellites/ztr/%s", gcUrl, token) |
| viper.Set("auth.name", res.Auth.Name) // Set auth name in config | ||
| viper.Set("auth.secret", res.Auth.Secret) // Set auth secret in config | ||
| viper.Set("auth.registry", res.Auth.Registry) // Set auth registry in config | ||
| viper.Set("states", res.States) // Set states array in config | ||
| viper.WriteConfigAs("config.gen.json") |
There was a problem hiding this comment.
Handle errors when writing the configuration file
The viper.WriteConfigAs("config.gen.json") call does not check for errors. If writing the configuration file fails, the application might proceed with incomplete or incorrect settings without any warning.
Modify the code to handle potential errors:
viper.Set("auth.name", res.Auth.Name) // Set auth name in config
viper.Set("auth.secret", res.Auth.Secret) // Set auth secret in config
viper.Set("auth.registry", res.Auth.Registry) // Set auth registry in config
viper.Set("states", res.States) // Set states array in config
- viper.WriteConfigAs("config.gen.json")
+ if err := viper.WriteConfigAs("config.gen.json"); err != nil {
+ return nil, fmt.Errorf("failed to write configuration file: %v", err)
+ }Committable suggestion skipped: line range outside the PR's diff.
Potential concurrency issues with Viper's global instance
If your application is or becomes concurrent, using Viper's global instance (viper.Set, viper.WriteConfigAs) without proper synchronization can lead to race conditions.
Consider creating a new Viper instance within the ztr function:
func ztr(gcUrl string) (*ZtrResult, error) {
+ localViper := viper.New()
// Get environment variables
token := os.Getenv("TOKEN")
// Construct the request URL
url := fmt.Sprintf("%s/satellites/ztr/%s", gcUrl, token)
// [Rest of the code]
- viper.Set("auth.name", res.Auth.Name)
- viper.Set("auth.secret", res.Auth.Secret)
- viper.Set("auth.registry", res.Auth.Registry)
- viper.Set("states", res.States)
- if err := viper.WriteConfigAs("config.gen.json"); err != nil {
+ localViper.Set("auth.name", res.Auth.Name)
+ localViper.Set("auth.secret", res.Auth.Secret)
+ localViper.Set("auth.registry", res.Auth.Registry)
+ localViper.Set("states", res.States)
+ if err := localViper.WriteConfigAs("config.gen.json"); err != nil {
return nil, fmt.Errorf("failed to write configuration file: %v", err)
}This isolates configurations and prevents unintended side effects.
Committable suggestion skipped: line range outside the PR's diff.
| ground_control_url string | ||
| input string | ||
| zot_url string | ||
| registry string | ||
| repository string | ||
| user_input string | ||
| scheme string | ||
| api_version string | ||
| image string | ||
| token string | ||
| states []string | ||
| harbor_password string | ||
| harbor_username string | ||
| env string | ||
| use_unsecure bool | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Export Config struct fields for external package access
All fields in the Config struct are unexported due to starting with lowercase letters. This restricts access to these fields from other packages, which could limit the configurability of your application.
Consider exporting the fields by capitalizing the first letter of each field:
type Config struct {
- log_level string
- own_registry bool
- own_registry_adr string
- own_registry_port string
- zot_config_path string
- ground_control_url string
- input string
- zot_url string
- registry string
- repository string
- user_input string
- scheme string
- api_version string
- image string
- token string
- states []string
- harbor_password string
- harbor_username string
- env string
- use_unsecure bool
+ LogLevel string
+ OwnRegistry bool
+ OwnRegistryAdr string
+ OwnRegistryPort string
+ ZotConfigPath string
+ GroundControlURL string
+ Input string
+ ZotURL string
+ Registry string
+ Repository string
+ UserInput string
+ Scheme string
+ APIVersion string
+ Image string
+ Token string
+ States []string
+ HarborPassword string
+ HarborUsername string
+ Env string
+ UseUnsecure bool
}Committable suggestion skipped: line range outside the PR's diff.
Sample
|
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
ground-control/internal/server/handlers.go (3)
294-306: Consider moving robot account creation before satellite creation.If robot account creation fails, we'll have an orphaned satellite record. Consider creating the robot account first or implementing a cleanup mechanism.
457-469: Add pagination to satellite listing.Loading all satellites at once could cause performance issues with a large number of satellites.
Add pagination support:
type ListSatellitesParams struct { Limit int32 `json:"limit"` Offset int32 `json:"offset"` } func (s *Server) listSatelliteHandler(w http.ResponseWriter, r *http.Request) { params := ListSatellitesParams{ Limit: 20, // Default limit Offset: 0, } if limit := r.URL.Query().Get("limit"); limit != "" { if l, err := strconv.ParseInt(limit, 10, 32); err == nil { params.Limit = int32(l) } } if offset := r.URL.Query().Get("offset"); offset != "" { if o, err := strconv.ParseInt(offset, 10, 32); err == nil { params.Offset = int32(o) } } result, total, err := s.dbQueries.ListSatellitesPaginated(r.Context(), params) // ... error handling ... response := struct { Items []database.Satellite `json:"items"` Total int `json:"total"` }{ Items: result, Total: total, } WriteJSONResponse(w, http.StatusOK, response) }
490-507: Improve error handling in DeleteSatelliteByName.The function should handle
sql.ErrNoRowsto provide a better error message when the satellite doesn't exist.err := s.dbQueries.DeleteSatelliteByName(r.Context(), satellite) if err != nil { + if err == sql.ErrNoRows { + HandleAppError(w, &AppError{ + Message: fmt.Sprintf("Satellite '%s' not found", satellite), + Code: http.StatusNotFound, + }) + return + } log.Printf("error: failed to delete satellite: %v", err) err := &AppError{ Message: "Error: Failed to Delete Satellite", Code: http.StatusInternalServerError, } HandleAppError(w, err) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ground-control/internal/server/handlers.go(4 hunks)ground-control/reg/harbor/robot.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ground-control/reg/harbor/robot.go
| // check if project satellite exists and if does not exist create project satellite | ||
| satExist, err := harbor.GetProject(r.Context(), "satellite") | ||
| if err != nil { | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: fmt.Sprintf("Error: Checking satellite project: %v", err), | ||
| Code: http.StatusBadGateway, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } | ||
| params := database.CreateLabelParams{ | ||
| LabelName: req.LabelName, | ||
| CreatedAt: time.Now(), | ||
| UpdatedAt: time.Now(), | ||
| if !satExist { | ||
| _, err := harbor.CreateSatelliteProject(r.Context()) | ||
| if err != nil { | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: fmt.Sprintf("Error: creating satellite project: %v", err), | ||
| Code: http.StatusBadGateway, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicated satellite project creation logic.
The satellite project creation logic is duplicated in both groupsSyncHandler and registerSatelliteHandler. This violates the DRY principle and makes maintenance harder.
Extract the logic into a shared function:
func (s *Server) ensureSatelliteProjectExists(ctx context.Context) error {
satExist, err := harbor.GetProject(ctx, "satellite")
if err != nil {
return fmt.Errorf("checking satellite project: %w", err)
}
if !satExist {
if _, err := harbor.CreateSatelliteProject(ctx); err != nil {
return fmt.Errorf("creating satellite project: %w", err)
}
}
return nil
}Also applies to: 268-291
| if len(req.Name) < 1 { | ||
| log.Println("name should be atleast one character long.") | ||
| err := &AppError{ | ||
| Message: fmt.Sprintf("Error: name should be atleast one character long."), | ||
| Code: http.StatusBadRequest, | ||
| } | ||
| HandleAppError(w, err) | ||
| return |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance satellite name validation.
The current validation only checks for length. Consider adding more comprehensive validation:
- Check for valid characters (e.g., alphanumeric and hyphens only)
- Prevent reserved names
- Enforce naming conventions
func validateSatelliteName(name string) error {
if len(name) < 1 {
return fmt.Errorf("name should be at least one character long")
}
if !regexp.MustCompile(`^[a-z0-9][a-z0-9-]*[a-z0-9]$`).MatchString(name) {
return fmt.Errorf("name must contain only lowercase letters, numbers, and hyphens, and cannot start or end with a hyphen")
}
reservedNames := map[string]bool{"system": true, "admin": true}
if reservedNames[name] {
return fmt.Errorf("name '%s' is reserved", name)
}
return nil
}| panic(p) // Re-throw the panic after rolling back | ||
| } else if err != nil { | ||
| tx.Rollback() // Rollback transaction on error | ||
| } | ||
| }() | ||
|
|
||
| satelliteID, err := q.GetSatelliteIDByToken(r.Context(), token) | ||
| if err != nil { | ||
| log.Println("Invalid Satellite Token") | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: "Error: Invalid Token", | ||
| Code: http.StatusBadRequest, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } | ||
|
|
||
| err := s.dbQueries.AddSatelliteToLabel(r.Context(), params) | ||
| err = q.DeleteToken(r.Context(), token) | ||
| if err != nil { | ||
| log.Printf("Error: Failed to add satellite to label: %v", err) | ||
| log.Println("error deleting token") | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: "Error: Error deleting token", | ||
| Code: http.StatusInternalServerError, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } | ||
|
|
||
| WriteJSONResponse(w, http.StatusOK, map[string]string{}) | ||
| } | ||
| robot, err := q.GetRobotAccBySatelliteID(r.Context(), satelliteID) | ||
| if err != nil { | ||
| log.Println("Robot Account Not Found") | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: "Error: Robot Account Not Found for Satellite", | ||
| Code: http.StatusInternalServerError, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } | ||
|
|
||
| func (s *Server) assignImageToLabel(w http.ResponseWriter, r *http.Request) { | ||
| var req AssignImageToLabelParams | ||
| if err := DecodeRequestBody(r, &req); err != nil { | ||
| // groups attached to satellite | ||
| groups, err := q.SatelliteGroupList(r.Context(), satelliteID) | ||
| if err != nil { | ||
| log.Printf("failed to list groups for satellite: %v, %v", satelliteID, err) | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: "Error: Satellite Groups List Failed", | ||
| Code: http.StatusInternalServerError, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } | ||
|
|
||
| params := database.AssignImageToLabelParams{ | ||
| LabelID: int32(req.LabelID), | ||
| ImageID: int32(req.ImageID), | ||
| var states []string | ||
| for _, group := range groups { | ||
| grp, err := q.GetGroupByID(r.Context(), group.GroupID) | ||
| if err != nil { | ||
| log.Printf("failed to get group by ID: %v, %v", group.GroupID, err) | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: "Error: Get Group By ID Failed", | ||
| Code: http.StatusInternalServerError, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } | ||
| state := utils.AssembleGroupState(grp.GroupName) | ||
| states = append(states, state) | ||
| } | ||
|
|
||
| err := s.dbQueries.AssignImageToLabel(r.Context(), params) | ||
| result := models.ZtrResult{ | ||
| States: states, | ||
| Auth: models.Account{ | ||
| Name: robot.RobotName, | ||
| Secret: robot.RobotSecret, | ||
| Registry: os.Getenv("HARBOR_URL"), | ||
| }, | ||
| } | ||
|
|
||
| tx.Commit() | ||
| WriteJSONResponse(w, http.StatusOK, result) | ||
| } |
There was a problem hiding this comment.
Improve token validation security.
Several security concerns in the token validation process:
- Tokens in URLs can be logged by proxies
- No rate limiting for validation attempts
- Token is deleted before verifying the complete registration chain
Consider these improvements:
- Accept token in request body or header
- Implement rate limiting
- Move token deletion after successful validation
Example rate limiting implementation:
func (s *Server) rateLimitedZTR(token string) error {
key := fmt.Sprintf("ztr_attempts:%s", token)
attempts, err := s.cache.Incr(key)
if err != nil {
return err
}
// Set key expiration on first attempt
if attempts == 1 {
s.cache.Expire(key, 1*time.Hour)
}
if attempts > 5 {
return fmt.Errorf("too many validation attempts")
}
return nil
}| func (s *Server) addSatelliteToGroup(w http.ResponseWriter, r *http.Request) { | ||
| var req SatelliteGroupParams | ||
| if err := DecodeRequestBody(r, &req); err != nil { | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
|
|
||
| sat, err := s.dbQueries.GetSatelliteByName(r.Context(), req.Satellite) | ||
| if err != nil { | ||
| log.Printf("Error: Satellite Not Found: %v", err) | ||
| err := &AppError{ | ||
| Message: "Error: Satellite Not Found", | ||
| Code: http.StatusBadRequest, | ||
| } | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
| result, err := s.dbQueries.GetImagesForSatellite(r.Context(), token) | ||
| grp, err := s.dbQueries.GetGroupByName(r.Context(), req.Group) | ||
| if err != nil { | ||
| log.Printf("Error: Failed to get image for satellite: %v", err) | ||
| log.Printf("Error: Group Not Found: %v", err) | ||
| err := &AppError{ | ||
| Message: "Error: Group Not Found", | ||
| Code: http.StatusBadRequest, | ||
| } | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
|
|
||
| WriteJSONResponse(w, http.StatusOK, result) | ||
| } | ||
| params := database.AddSatelliteToGroupParams{ | ||
| SatelliteID: int32(sat.ID), | ||
| GroupID: int32(grp.ID), | ||
| } | ||
|
|
||
| func (s *Server) listGroupHandler(w http.ResponseWriter, r *http.Request) { | ||
| result, err := s.dbQueries.ListGroups(r.Context()) | ||
| err = s.dbQueries.AddSatelliteToGroup(r.Context(), params) | ||
| if err != nil { | ||
| log.Printf("Error: Failed to Add Satellite to Group: %v", err) | ||
| err := &AppError{ | ||
| Message: "Error: Failed to Add Satellite to Group", | ||
| Code: http.StatusInternalServerError, | ||
| } | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
|
|
||
| WriteJSONResponse(w, http.StatusOK, result) | ||
| WriteJSONResponse(w, http.StatusOK, map[string]string{}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for group relationships.
The current implementation doesn't validate group relationships, which could lead to:
- Circular dependencies between groups
- Orphaned resources when groups are deleted
func (s *Server) validateGroupRelationship(ctx context.Context, satelliteID, groupID int32) error {
// Check for circular dependencies
visited := make(map[int32]bool)
var checkCycle func(int32) error
checkCycle = func(gID int32) error {
if visited[gID] {
return fmt.Errorf("circular dependency detected")
}
visited[gID] = true
parentGroups, err := s.dbQueries.GetParentGroups(ctx, gID)
if err != nil {
return err
}
for _, parent := range parentGroups {
if err := checkCycle(parent.ID); err != nil {
return err
}
}
return nil
}
return checkCycle(groupID)
}Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
internal/config/config.go (1)
222-235: Consider using environment variables consistentlyThe configuration mixes values from environment variables and the config file inconsistently. For example,
tokenuses an environment variable whileground_control_urluses the config file.Consider implementing a clear precedence:
- Environment variables (override everything)
- Generated config (config.gen.json)
- Base config (config.toml)
This could be implemented by adding a helper function:
func getConfigValue(envKey, configKey string) string { if value := os.Getenv(envKey); value != "" { return value } return viper.GetString(configKey) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
internal/config/config.go(5 hunks)
🔇 Additional comments (3)
internal/config/config.go (3)
38-47: LGTM! Well-structured response types
The ZtrResult and Account structs are well-defined with appropriate JSON tags for serialization/deserialization.
104-106: LGTM! Consistent getter implementations
The new getter methods follow the existing pattern and are properly implemented.
Also applies to: 172-178
75-79:
Add error handling for configuration writing
The configuration writing operation lacks error handling.
Add error handling for the Viper operations:
viper.Set("auth.name", res.Auth.Name)
viper.Set("auth.secret", res.Auth.Secret)
viper.Set("auth.registry", res.Auth.Registry)
viper.Set("states", res.States)
- viper.WriteConfigAs("config.gen.json")
+ if err := viper.WriteConfigAs("config.gen.json"); err != nil {
+ return nil, fmt.Errorf("failed to write configuration: %v", err)
+ }Likely invalid or redundant comment.
| // Check if config.gen.json exists | ||
| if _, err := os.Stat("config.gen.json"); err == nil { | ||
| // If config.gen.json exists, set it as the configuration file | ||
| viper.SetConfigFile("config.gen.json") | ||
| fmt.Println("Using config.gen.json for configuration.") | ||
| } else if os.IsNotExist(err) { | ||
| // fall back to config.toml | ||
| viper.SetConfigName("config") | ||
| viper.SetConfigType("toml") | ||
| viper.AddConfigPath(".") | ||
|
|
||
| if err := viper.ReadInConfig(); err != nil { | ||
| return nil, fmt.Errorf("error reading config file at path '%s': %w", viper.ConfigFileUsed(), err) | ||
| } | ||
|
|
||
| gcURL := viper.GetString("ground_control_url") | ||
| // Call ztr function to fetch and set the necessary configuration | ||
| _, err := ztr(gcURL) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Error in ztr function: %v\n", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor config loading logic for better separation of concerns
The configuration loading logic has become complex and mixes multiple responsibilities. Consider splitting the functionality and improving error messages.
Suggested refactor:
+func loadGeneratedConfig() error {
+ if _, err := os.Stat("config.gen.json"); err != nil {
+ return err
+ }
+ viper.SetConfigFile("config.gen.json")
+ return viper.ReadInConfig()
+}
+
+func loadAndGenerateConfig() error {
+ viper.SetConfigName("config")
+ viper.SetConfigType("toml")
+ viper.AddConfigPath(".")
+
+ if err := viper.ReadInConfig(); err != nil {
+ return fmt.Errorf("failed to read config.toml: %w", err)
+ }
+
+ gcURL := viper.GetString("ground_control_url")
+ if gcURL == "" {
+ return fmt.Errorf("ground_control_url not set in config.toml")
+ }
+
+ if _, err := ztr(gcURL); err != nil {
+ return fmt.Errorf("failed to generate config: %w", err)
+ }
+ return nil
+}
func LoadConfig() (*Config, error) {
- if _, err := os.Stat("config.gen.json"); err == nil {
- viper.SetConfigFile("config.gen.json")
- fmt.Println("Using config.gen.json for configuration.")
- } else if os.IsNotExist(err) {
- viper.SetConfigName("config")
- // ... existing fallback code ...
- } else {
- return nil, fmt.Errorf("Error checking config file: %v\n", err)
+ if err := loadGeneratedConfig(); err != nil {
+ if !os.IsNotExist(err) {
+ return nil, fmt.Errorf("failed to check config.gen.json: %w", err)
+ }
+ if err := loadAndGenerateConfig(); err != nil {
+ return nil, err
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| log_level string | ||
| own_registry bool | ||
| own_registry_adr string | ||
| own_registry_port string | ||
| zot_config_path string | ||
| ground_control_url string | ||
| input string | ||
| zot_url string | ||
| registry string | ||
| repository string | ||
| user_input string | ||
| scheme string | ||
| api_version string | ||
| image string | ||
| token string | ||
| states []string | ||
| harbor_password string | ||
| harbor_username string | ||
| env string | ||
| use_unsecure bool |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize field naming convention and consider field visibility
The Config struct fields use inconsistent naming conventions (snake_case) which differs from Go's standard practice of using camelCase. Additionally, all fields are unexported which may limit the struct's usability in other packages.
Consider standardizing the field names and selectively exporting fields:
type Config struct {
- log_level string
- own_registry bool
- own_registry_adr string
- own_registry_port string
- zot_config_path string
- ground_control_url string
+ LogLevel string
+ OwnRegistry bool
+ OwnRegistryAddr string
+ OwnRegistryPort string
+ ZotConfigPath string
+ GroundControlURL string
// ... similar changes for other fields
}Committable suggestion skipped: line range outside the PR's diff.
| func ztr(gcUrl string) (*ZtrResult, error) { | ||
| // Get environment variables | ||
| token := os.Getenv("TOKEN") | ||
|
|
||
| // Construct the request URL | ||
| gcUrl = fmt.Sprintf("%s/satellites/ztr/%s", gcUrl, token) | ||
|
|
||
| // Make the GET request | ||
| resp, err := http.Get(gcUrl) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to fetch data from Ground Control: %v", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
There was a problem hiding this comment.
Add input validation and response size limit
The function lacks validation for critical inputs and doesn't limit the response size, which could lead to security issues.
Add the following safeguards:
func ztr(gcUrl string) (*ZtrResult, error) {
+ if gcUrl == "" {
+ return nil, fmt.Errorf("ground control URL cannot be empty")
+ }
token := os.Getenv("TOKEN")
+ if token == "" {
+ return nil, fmt.Errorf("TOKEN environment variable is not set")
+ }
gcUrl = fmt.Sprintf("%s/satellites/ztr/%s", gcUrl, token)
- resp, err := http.Get(gcUrl)
+ client := &http.Client{}
+ req, err := http.NewRequest("GET", gcUrl, nil)
+ if err != nil {
+ return nil, fmt.Errorf("failed to create request: %v", err)
+ }
+ resp, err := client.Do(req)
if err != nil {
return nil, fmt.Errorf("failed to fetch data from Ground Control: %v", err)
}
defer resp.Body.Close()
+
+ // Limit response size to prevent memory exhaustion
+ const maxSize = 1 << 20 // 1 MB
+ resp.Body = http.MaxBytesReader(nil, resp.Body, maxSize)📝 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 ztr(gcUrl string) (*ZtrResult, error) { | |
| // Get environment variables | |
| token := os.Getenv("TOKEN") | |
| // Construct the request URL | |
| gcUrl = fmt.Sprintf("%s/satellites/ztr/%s", gcUrl, token) | |
| // Make the GET request | |
| resp, err := http.Get(gcUrl) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to fetch data from Ground Control: %v", err) | |
| } | |
| defer resp.Body.Close() | |
| func ztr(gcUrl string) (*ZtrResult, error) { | |
| if gcUrl == "" { | |
| return nil, fmt.Errorf("ground control URL cannot be empty") | |
| } | |
| token := os.Getenv("TOKEN") | |
| if token == "" { | |
| return nil, fmt.Errorf("TOKEN environment variable is not set") | |
| } | |
| gcUrl = fmt.Sprintf("%s/satellites/ztr/%s", gcUrl, token) | |
| client := &http.Client{} | |
| req, err := http.NewRequest("GET", gcUrl, nil) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create request: %v", err) | |
| } | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to fetch data from Ground Control: %v", err) | |
| } | |
| defer resp.Body.Close() | |
| // Limit response size to prevent memory exhaustion | |
| const maxSize = 1 << 20 // 1 MB | |
| resp.Body = http.MaxBytesReader(nil, resp.Body, maxSize) |
Signed-off-by: bupd <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
ground-control/internal/server/handlers.go (1)
458-471: Add pagination to satellite listing.The current implementation loads all satellites at once, which could cause performance issues with a large number of satellites.
Add pagination support:
type ListSatellitesParams struct { Limit int32 `json:"limit"` Offset int32 `json:"offset"` } func (s *Server) listSatelliteHandler(w http.ResponseWriter, r *http.Request) { params := ListSatellitesParams{ Limit: 20, Offset: 0, } if limit := r.URL.Query().Get("limit"); limit != "" { if l, err := strconv.ParseInt(limit, 10, 32); err == nil { params.Limit = int32(l) } } if offset := r.URL.Query().Get("offset"); offset != "" { if o, err := strconv.ParseInt(offset, 10, 32); err == nil { params.Offset = int32(o) } } result, total, err := s.dbQueries.ListSatellitesPaginated(r.Context(), params) if err != nil { HandleAppError(w, err) return } response := struct { Items []database.Satellite `json:"items"` Total int `json:"total"` }{ Items: result, Total: total, } WriteJSONResponse(w, http.StatusOK, response) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ground-control/internal/server/handlers.go(4 hunks)ground-control/internal/utils/helper.go(1 hunks)
🔇 Additional comments (9)
ground-control/internal/utils/helper.go (5)
117-121: 🛠️ Refactor suggestion
Improve URL handling
The current URL handling is fragile and could be improved:
Consider using the url package for proper URL parsing:
- destinationRepo := fmt.Sprintf("%s/%s/%s", stateArtifact.Registry, repo, "state")
- if strings.Contains(destinationRepo, "://") {
- destinationRepo = strings.SplitN(destinationRepo, "://", 2)[1]
- }
+ // Parse and normalize the registry URL
+ u, err := url.Parse(stateArtifact.Registry)
+ if err != nil {
+ return fmt.Errorf("invalid registry URL: %w", err)
+ }
+ // Use only the host part of the URL
+ destinationRepo := fmt.Sprintf("%s/%s/%s", u.Host, repo, "state")Likely invalid or redundant comment.
42-51:
Add input validation and improve error context
The function needs input validation and better error context:
Apply these improvements:
func CreateRobotAccForSatellite(ctx context.Context, projects []string, name string) (*models.RobotCreated, error) {
+ if len(projects) == 0 {
+ return nil, fmt.Errorf("projects list cannot be empty")
+ }
+ if name == "" {
+ return nil, fmt.Errorf("robot name cannot be empty")
+ }
+ if ctx == nil {
+ return nil, fmt.Errorf("context cannot be nil")
+ }
robotTemp := harbor.RobotAccountTemplate(name, projects)
robt, err := harbor.CreateRobotAccount(ctx, robotTemp)
if err != nil {
- return nil, fmt.Errorf("error creating robot account: %w", err)
+ return nil, fmt.Errorf("failed to create robot account for %q: %w", name, err)
}
return robt.Payload, nil
}Likely invalid or redundant comment.
128-134:
Implement atomic tag updates
The current implementation of tag updates is not atomic and could lead to race conditions:
Apply these improvements:
- // Tag the image with timestamp and latest tags
- tags := []string{fmt.Sprintf("%d", time.Now().Unix()), "latest"}
- for _, tag := range tags {
- if err := crane.Tag(destinationRepo, tag, options...); err != nil {
- return fmt.Errorf("failed to tag image with %s: %v", tag, err)
- }
- }
+ // Generate unique timestamp for atomic operation
+ timestamp := fmt.Sprintf("%d", time.Now().UnixNano())
+ timestampTag := fmt.Sprintf("%s:%s", destinationRepo, timestamp)
+
+ // Tag with timestamp first
+ if err := crane.Tag(destinationRepo, timestamp, options...); err != nil {
+ return fmt.Errorf("failed to tag image with timestamp: %w", err)
+ }
+
+ // Atomically update latest tag
+ if err := crane.Tag(timestampTag, destinationRepo+":latest", options...); err != nil {
+ // Cleanup on failure
+ _ = crane.Delete(timestampTag, options...)
+ return fmt.Errorf("failed to update latest tag: %w", err)
+ }Likely invalid or redundant comment.
78-81:
Add environment and input validation
The function needs validation for both environment variables and input:
Apply these improvements:
-func AssembleGroupState(groupName string) string {
+func AssembleGroupState(groupName string) (string, error) {
+ if groupName == "" {
+ return "", fmt.Errorf("group name cannot be empty")
+ }
+
+ harborURL := os.Getenv("HARBOR_URL")
+ if harborURL == "" {
+ return "", fmt.Errorf("HARBOR_URL environment variable is not set")
+ }
+
+ // Validate group name format to prevent path traversal
+ if strings.Contains(groupName, "..") || strings.Contains(groupName, "/") {
+ return "", fmt.Errorf("invalid group name format: %s", groupName)
+ }
+
- state := fmt.Sprintf("%s/satellite/%s/state:latest", os.Getenv("HARBOR_URL"), groupName)
- return state
+ state := fmt.Sprintf("%s/satellite/%s/state:latest", harborURL, groupName)
+ return state, nil
}Likely invalid or redundant comment.
83-95:
Add input validation and improve error handling
The function needs input validation and better error messages:
Apply these improvements:
func CreateStateArtifact(stateArtifact *m.StateArtifact) error {
+ if stateArtifact == nil {
+ return fmt.Errorf("state artifact cannot be nil")
+ }
+ if stateArtifact.Group == "" {
+ return fmt.Errorf("group name cannot be empty")
+ }
// Set the registry URL from environment variable
stateArtifact.Registry = os.Getenv("HARBOR_URL")
if stateArtifact.Registry == "" {
return fmt.Errorf("HARBOR_URL environment variable is not set")
}
// Marshal the state artifact to JSON format
data, err := json.Marshal(stateArtifact)
if err != nil {
- return fmt.Errorf("failed to marshal state artifact to JSON: %v", err)
+ return fmt.Errorf("failed to marshal state artifact for group %q: %w", stateArtifact.Group, err)
}Likely invalid or redundant comment.
ground-control/internal/server/handlers.go (4)
21-24: Remove unnecessary pointer to slice in RegisterSatelliteParams.
The Groups field is declared as a pointer to slice (*[]string), but slices in Go are already reference types. Using a pointer to a slice adds unnecessary complexity.
374-397: Improve token validation security.
The token is deleted before verifying the complete registration chain, which could lead to orphaned satellites if subsequent operations fail.
768-779: Improve token generation security.
The function should handle odd character lengths correctly and validate input.
83-88:
Add validation for empty projects list.
The handler should validate that the projects list is not empty before creating a group.
projects := utils.GetProjectNames(&req.Artifacts)
+if len(projects) == 0 {
+ HandleAppError(w, &AppError{
+ Message: "No valid projects found in artifacts",
+ Code: http.StatusBadRequest,
+ })
+ return
+}Likely invalid or redundant comment.
| // GetProjectNames parses artifacts & returns project names | ||
| func GetProjectNames(artifacts *[]m.Artifact) []string { | ||
| uniqueProjects := make(map[string]struct{}) // Map to track unique project names | ||
| var projects []string | ||
|
|
||
| for _, artifact := range *artifacts { | ||
| if artifact.Deleted { | ||
| continue | ||
| } | ||
| // Extract project name from repository | ||
| project := strings.Split(artifact.Repository, "/")[0] | ||
|
|
||
| // Check if the project is already added | ||
| if _, exists := uniqueProjects[project]; !exists { | ||
| uniqueProjects[project] = struct{}{} | ||
| projects = append(projects, project) | ||
| } | ||
| } | ||
|
|
||
| return projects | ||
| } |
There was a problem hiding this comment.
Add input validation and improve error handling
The function needs several improvements for robustness and efficiency:
- Missing nil check for input parameter
- Potential panic when splitting malformed repository paths
- Inefficient storage using both map and slice
- No error handling for invalid repository formats
Consider this improved implementation:
-func GetProjectNames(artifacts *[]m.Artifact) []string {
+func GetProjectNames(artifacts *[]m.Artifact) ([]string, error) {
+ if artifacts == nil {
+ return nil, fmt.Errorf("artifacts cannot be nil")
+ }
+
uniqueProjects := make(map[string]struct{})
- var projects []string
for _, artifact := range *artifacts {
if artifact.Deleted {
continue
}
- project := strings.Split(artifact.Repository, "/")[0]
+ parts := strings.SplitN(artifact.Repository, "/", 2)
+ if len(parts) == 0 {
+ return nil, fmt.Errorf("invalid repository format: %s", artifact.Repository)
+ }
+ project := parts[0]
if _, exists := uniqueProjects[project]; !exists {
uniqueProjects[project] = struct{}{}
- projects = append(projects, project)
}
}
- return projects
+ projects := make([]string, 0, len(uniqueProjects))
+ for project := range uniqueProjects {
+ projects = append(projects, project)
+ }
+ return projects, nil
}📝 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.
| // GetProjectNames parses artifacts & returns project names | |
| func GetProjectNames(artifacts *[]m.Artifact) []string { | |
| uniqueProjects := make(map[string]struct{}) // Map to track unique project names | |
| var projects []string | |
| for _, artifact := range *artifacts { | |
| if artifact.Deleted { | |
| continue | |
| } | |
| // Extract project name from repository | |
| project := strings.Split(artifact.Repository, "/")[0] | |
| // Check if the project is already added | |
| if _, exists := uniqueProjects[project]; !exists { | |
| uniqueProjects[project] = struct{}{} | |
| projects = append(projects, project) | |
| } | |
| } | |
| return projects | |
| } | |
| // GetProjectNames parses artifacts & returns project names | |
| func GetProjectNames(artifacts *[]m.Artifact) ([]string, error) { | |
| if artifacts == nil { | |
| return nil, fmt.Errorf("artifacts cannot be nil") | |
| } | |
| uniqueProjects := make(map[string]struct{}) // Map to track unique project names | |
| for _, artifact := range *artifacts { | |
| if artifact.Deleted { | |
| continue | |
| } | |
| // Extract project name from repository | |
| parts := strings.SplitN(artifact.Repository, "/", 2) | |
| if len(parts) == 0 { | |
| return nil, fmt.Errorf("invalid repository format: %s", artifact.Repository) | |
| } | |
| project := parts[0] | |
| if _, exists := uniqueProjects[project]; !exists { | |
| uniqueProjects[project] = struct{}{} | |
| } | |
| } | |
| projects := make([]string, 0, len(uniqueProjects)) | |
| for project := range uniqueProjects { | |
| projects = append(projects, project) | |
| } | |
| return projects, nil | |
| } |
| // Update robot account | ||
| func UpdateRobotProjects(ctx context.Context, projects []string, id string) (*robot.UpdateRobotOK, error) { | ||
| ID, err := strconv.ParseInt(id, 10, 64) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error invalid ID: %w", err) | ||
| } | ||
| robot, err := harbor.GetRobotAccount(ctx, ID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting robot account: %w", err) | ||
| } | ||
|
|
||
| // satellites should always have permission to satellite project by default | ||
| // to get state artifacts | ||
| projects = append(projects, "satellite") | ||
|
|
||
| robot.Permissions = harbor.GenRobotPerms(projects) | ||
|
|
||
| updated, err := harbor.UpdateRobotAccount(ctx, robot) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error updating robot account: %w", err) | ||
| } | ||
|
|
||
| return updated, nil | ||
| } |
There was a problem hiding this comment.
Add input validation and clarify satellite project addition
The function has several areas for improvement:
- Missing input validation
- Implicit behavior of adding "satellite" project
- Error messages could be more descriptive
Consider these improvements:
-func UpdateRobotProjects(ctx context.Context, projects []string, id string) (*robot.UpdateRobotOK, error) {
+func UpdateRobotProjectsWithSatellite(ctx context.Context, projects []string, id string) (*robot.UpdateRobotOK, error) {
+ if ctx == nil {
+ return nil, fmt.Errorf("context cannot be nil")
+ }
+ if len(projects) == 0 {
+ return nil, fmt.Errorf("projects list cannot be empty")
+ }
+ if id == "" {
+ return nil, fmt.Errorf("robot ID cannot be empty")
+ }
ID, err := strconv.ParseInt(id, 10, 64)
if err != nil {
- return nil, fmt.Errorf("error invalid ID: %w", err)
+ return nil, fmt.Errorf("invalid robot ID %q: %w", id, err)
}
robot, err := harbor.GetRobotAccount(ctx, ID)
if err != nil {
- return nil, fmt.Errorf("error getting robot account: %w", err)
+ return nil, fmt.Errorf("failed to get robot account with ID %d: %w", ID, err)
}
// satellites should always have permission to satellite project by default
// to get state artifacts
projects = append(projects, "satellite")
robot.Permissions = harbor.GenRobotPerms(projects)
updated, err := harbor.UpdateRobotAccount(ctx, robot)
if err != nil {
- return nil, fmt.Errorf("error updating robot account: %w", err)
+ return nil, fmt.Errorf("failed to update robot account %d with projects %v: %w", ID, projects, err)
}
return updated, nil
}📝 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.
| // Update robot account | |
| func UpdateRobotProjects(ctx context.Context, projects []string, id string) (*robot.UpdateRobotOK, error) { | |
| ID, err := strconv.ParseInt(id, 10, 64) | |
| if err != nil { | |
| return nil, fmt.Errorf("error invalid ID: %w", err) | |
| } | |
| robot, err := harbor.GetRobotAccount(ctx, ID) | |
| if err != nil { | |
| return nil, fmt.Errorf("error getting robot account: %w", err) | |
| } | |
| // satellites should always have permission to satellite project by default | |
| // to get state artifacts | |
| projects = append(projects, "satellite") | |
| robot.Permissions = harbor.GenRobotPerms(projects) | |
| updated, err := harbor.UpdateRobotAccount(ctx, robot) | |
| if err != nil { | |
| return nil, fmt.Errorf("error updating robot account: %w", err) | |
| } | |
| return updated, nil | |
| } | |
| // Update robot account | |
| func UpdateRobotProjectsWithSatellite(ctx context.Context, projects []string, id string) (*robot.UpdateRobotOK, error) { | |
| if ctx == nil { | |
| return nil, fmt.Errorf("context cannot be nil") | |
| } | |
| if len(projects) == 0 { | |
| return nil, fmt.Errorf("projects list cannot be empty") | |
| } | |
| if id == "" { | |
| return nil, fmt.Errorf("robot ID cannot be empty") | |
| } | |
| ID, err := strconv.ParseInt(id, 10, 64) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid robot ID %q: %w", id, err) | |
| } | |
| robot, err := harbor.GetRobotAccount(ctx, ID) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get robot account with ID %d: %w", ID, err) | |
| } | |
| // satellites should always have permission to satellite project by default | |
| // to get state artifacts | |
| projects = append(projects, "satellite") | |
| robot.Permissions = harbor.GenRobotPerms(projects) | |
| updated, err := harbor.UpdateRobotAccount(ctx, robot) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to update robot account %d with projects %v: %w", ID, projects, err) | |
| } | |
| return updated, nil | |
| } |
| // Start a new transaction | ||
| tx, err := s.db.BeginTx(r.Context(), nil) | ||
| if err != nil { | ||
| log.Println(err) | ||
| HandleAppError(w, err) | ||
| return | ||
| } | ||
| // Create a new Queries object bound to the transaction | ||
| q := s.dbQueries.WithTx(tx) | ||
| // Ensure proper transaction handling with defer | ||
| defer func() { | ||
| if p := recover(); p != nil { | ||
| // If there's a panic, rollback the transaction | ||
| tx.Rollback() | ||
| } else if err != nil { | ||
| tx.Rollback() // Rollback transaction on error | ||
| } | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract transaction management into a helper function.
Transaction management code is duplicated across multiple handlers. This makes the code harder to maintain and increases the risk of inconsistent transaction handling.
Create a helper function:
func (s *Server) withTransaction(ctx context.Context, fn func(*database.Queries) error) error {
tx, err := s.db.BeginTx(ctx, nil)
if err != nil {
return err
}
q := s.dbQueries.WithTx(tx)
defer func() {
if p := recover(); p != nil {
tx.Rollback()
panic(p)
} else if err != nil {
tx.Rollback()
}
}()
if err = fn(q); err != nil {
return err
}
return tx.Commit()
}| // check if project satellite exists and if does not exist create project satellite | ||
| satExist, err := harbor.GetProject(r.Context(), "satellite") | ||
| if err != nil { | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: fmt.Sprintf("Error: Checking satellite project: %v", err), | ||
| Code: http.StatusBadGateway, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } | ||
| if !satExist { | ||
| _, err := harbor.CreateSatelliteProject(r.Context()) | ||
| if err != nil { | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: fmt.Sprintf("Error: creating satellite project: %v", err), | ||
| Code: http.StatusBadGateway, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract satellite project creation logic into a helper function.
This code block is duplicated in both groupsSyncHandler and registerSatelliteHandler.
Create a helper function:
func (s *Server) ensureSatelliteProjectExists(ctx context.Context) error {
satExist, err := harbor.GetProject(ctx, "satellite")
if err != nil {
return fmt.Errorf("checking satellite project: %w", err)
}
if !satExist {
if _, err := harbor.CreateSatelliteProject(ctx); err != nil {
return fmt.Errorf("creating satellite project: %w", err)
}
}
return nil
}| for _, groupName := range *req.Groups { | ||
| // check if groups are declared in replication | ||
| replications, err := harbor.ListReplication(r.Context(), harbor.ListParams{ | ||
| Q: fmt.Sprintf("name=%s", groupName), | ||
| }) | ||
| if len(replications) < 1 { | ||
| if err != nil { | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: fmt.Sprintf("Error: Group Name: %s, does not exist in replication, Please give a Valid Group Name", groupName), | ||
| Code: http.StatusBadRequest, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } | ||
| } | ||
| group, err := q.GetGroupByName(r.Context(), groupName) | ||
| if err != nil { | ||
| log.Println(err) | ||
| err := &AppError{ | ||
| Message: fmt.Sprintf("Error: Invalid Group Name: %v", groupName), | ||
| Code: http.StatusBadRequest, | ||
| } | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } | ||
| err = q.AddSatelliteToGroup(r.Context(), database.AddSatelliteToGroupParams{ | ||
| SatelliteID: satellite.ID, | ||
| GroupID: group.ID, | ||
| }) | ||
| if err != nil { | ||
| log.Println(err) | ||
| HandleAppError(w, err) | ||
| tx.Rollback() | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize group validation to reduce API calls.
The current implementation makes multiple API calls within a loop. Consider validating all groups upfront to improve performance.
func (s *Server) validateGroups(ctx context.Context, q *database.Queries, groupNames []string) error {
// Validate all groups exist in replication first
replicationFilter := strings.Join(groupNames, "|")
replications, err := harbor.ListReplication(ctx, harbor.ListParams{
Q: fmt.Sprintf("name=~(%s)", replicationFilter),
})
if err != nil {
return err
}
validReplications := make(map[string]bool)
for _, r := range replications {
validReplications[r.Name] = true
}
for _, name := range groupNames {
if !validReplications[name] {
return fmt.Errorf("group %s does not exist in replication", name)
}
}
return nil
}| log.Println("what the fuck") | ||
| log.Println(err) |
There was a problem hiding this comment.
Remove inappropriate debug log message.
The debug log message contains inappropriate language and should be removed or replaced with a professional message.
- log.Println("what the fuck")
+ log.Println("error decoding request body")📝 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.
| log.Println("what the fuck") | |
| log.Println(err) | |
| log.Println("error decoding request body") | |
| log.Println(err) |
Signed-off-by: bupd <[email protected]>
Fix: #41
Also Fixes: #36
Sample
config.gen.jsonCreated by Satellite{ "auth": { // Credentials for accessing the registry. "name": "robot$sat10", "registry": "https://registry.bupd.xyz", "secret": "w3BBguuGhmQAMc2hPYJplQnTbcBA8GHt" }, "bring_own_registry": false, "ground_control_url": "http://localhost:8080", "log_level": "error", "own_registry_adr": "127.0.0.1", "own_registry_port": "8585", "states": [ // contains array of state artifact urls to be followed. "https://registry.bupd.xyz/satellite/group10/state:latest" ], "url_or_file": "https://demo.goharbor.io/v2/myproject/album-server", // this should be removed "zotconfigpath": "./registry/config.json" }Current Database Diagram
Summary by CodeRabbit
Release Notes
New Features
registry_urlandprojects.Bug Fixes
Documentation
Chores