Skip to content

feat: add transparent proxy-cache satellite mode#355

Open
bupd wants to merge 12 commits intocontainer-registry:mainfrom
bupd:transparent-proxy-cache
Open

feat: add transparent proxy-cache satellite mode#355
bupd wants to merge 12 commits intocontainer-registry:mainfrom
bupd:transparent-proxy-cache

Conversation

@bupd
Copy link
Copy Markdown
Member

@bupd bupd commented Mar 17, 2026

Summary

  • Add a new proxy-cache satellite operating mode where Zot uses its built-in extensions.sync for on-demand image caching from upstream Harbor, instead of pre-caching a defined list of images via groups
  • Ground Control creates a wildcard robot account (namespace: "*") with pull access to all current and future Harbor projects for proxy-cache satellites
  • Satellite configures Zot with on-demand sync + configurable polling interval after ZTR completes, with Docker v2s2 compatibility and digest preservation enabled

Changes

Ground Control

  • DB migration (016_satellite_mode.sql): adds mode column to satellites table (normal | proxy-cache)
  • Robot accounts: new ProxyCacheRobotAccountTemplate with wildcard namespace: "*" pull permission and no expiry (Duration: -1)
  • Registration handler: accepts mode field, branches on proxy-cache (skips groups, state artifacts, uses wildcard robot)
  • ZTR handlers: both token and SPIFFE handlers skip group state assembly and state artifact creation for proxy-cache satellites, include satellite name in response
  • Group guards: rejects adding proxy-cache satellites to groups

Satellite

  • Config model: adds proxy_cache_mode, sync_poll_interval to AppConfig, name to StateConfig
  • Zot sync config builder (internal/registry/sync_config.go): builds Zot config with extensions.sync (on-demand + polling, preserveDigest: true, prefix: "**", http.compat: ["docker2s2"])
  • ProxyCacheSetupProcess: one-shot scheduler process that runs after ZTR, writes sync credentials file and hot-reloads Zot config with sync extension
  • Satellite orchestration: in proxy-cache mode, runs ZTR + proxy-cache setup + heartbeat (no state replication)
  • Reporting: resolves satellite name from config field before falling back to URL parsing
  • ZTR validation: relaxed to allow empty StateURL in proxy-cache mode

Summary by CodeRabbit

  • New Features

    • Added proxy-cache mode for satellites, enabling on-demand registry caching without group assignments.
    • Introduced configuration options for sync polling interval (default: 6 hours) and proxy-cache mode enablement.
    • Added support for sync credentials file to manage upstream registry authentication in proxy-cache deployments.
  • Configuration

    • Extended configuration schema to support satellite mode selection and registry fallback sync settings.

bupd added 12 commits March 17, 2026 03:58
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
…rsing

Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 17, 2026

📝 Walkthrough

Walkthrough

This PR introduces proxy-cache mode support for satellites across the codebase. Changes include adding a Mode field to satellite configuration, implementing a new Zot sync configuration builder for proxy-cache setup, introducing a ProxyCacheSetupProcess scheduler, and updating handlers and state management to conditionally execute logic based on satellite mode.

Changes

Cohort / File(s) Summary
Satellite Constructor & Core Config
cmd/main.go, pkg/config/config.go, pkg/config/constants.go, pkg/config/getters.go, pkg/config/modifiers.go, pkg/config/paths.go
Added ProxyCacheMode and SyncPollInterval config fields, extended PathConfig with SyncCredentialsFile, added corresponding getters/setters, and updated NewSatellite call to pass additional sync-related file paths.
Database Schema & Migration
ground-control/sql/schema/016_satellite_mode.sql, ground-control/sql/queries/satellites.sql
Added new mode column to satellites table with default value 'normal' and updated CreateSatellite query to accept and persist mode parameter.
Ground-Control Database Generated Code
ground-control/internal/database/models.go, ground-control/internal/database/satellites.sql.go
Added Mode field to Satellite struct, introduced CreateSatelliteParams type, updated CreateSatellite method signature to accept struct parameter, and updated all satellite query methods (GetSatellite, GetSatelliteByName, ListSatellites) to return Mode.
Ground-Control Database Version Updates
ground-control/internal/database/artifacts.sql.go, ground-control/internal/database/configs.sql.go, ground-control/internal/database/db.go, ground-control/internal/database/groups.sql.go, ground-control/internal/database/login_attempts.sql.go, ground-control/internal/database/robot_accounts.sql.go, ground-control/internal/database/satellite_configs.sql.go, ground-control/internal/database/satellite_groups.sql.go, ground-control/internal/database/satellite_status.sql.go, ground-control/internal/database/satellite_token.sql.go, ground-control/internal/database/sessions.sql.go, ground-control/internal/database/users.sql.go
Updated sqlc-generated file headers from v1.28.0 to v1.30.0 (metadata only, no functional changes).
Ground-Control Handlers
ground-control/internal/server/satellite_handlers.go, ground-control/internal/server/spire_handlers.go
Added Mode field to RegisterSatelliteParams, implemented proxy-cache mode validation and conditional control flow (skipping group assignment, using CreateProxyCacheRobotAcc, skipping state artifacts). Updated CreateSatellite calls to pass CreateSatelliteParams.
Ground-Control Test Updates
ground-control/internal/server/cached_images_test.go, ground-control/internal/server/satellite_listing_test.go
Updated mock SQL rows to include new 'mode' column in satellites table fixtures.
Proxy-Cache Robot Account Support
ground-control/reg/harbor/robot.go, ground-control/internal/utils/helper.go
Added SatelliteModeNormal and SatelliteModeProxyCache constants, introduced ProxyCacheRobotAccountTemplate factory function with GenProxyCacheRobotPerms, and added CreateProxyCacheRobotAcc helper.
Satellite State Management
internal/satellite/satellite.go, internal/state/proxy_cache_setup.go, internal/state/registration_process.go, internal/state/reporting_process.go, internal/state/spiffe_registration.go
Extended Satellite struct with syncCredentialsFile and zotTempConfigPath fields, added ProxyCacheSetupProcess scheduler for proxy-cache initialization, updated registration and reporting processes to handle proxy-cache mode (relaxed StateURL requirement, updated credential validation), and introduced conditional Run logic to branch between proxy-cache and normal modes.
Proxy-Cache Zot Configuration Utilities
internal/registry/sync_config.go
Added new module with data models for Zot sync configuration (SyncRegistryConfig, SyncExtensionConfig, etc.) and core functions: BuildProxyCacheZotConfig to construct proxy-cache Zot configs and WriteSyncCredentialsFile to persist registry credentials.
Dependency Updates
ground-control/go.mod, config.example.json
Bumped testify to v1.11.1, added local replace directive for harbor-satellite, expanded indirect dependencies, and added proxy_cache_mode and sync_poll_interval example config options.

Sequence Diagram(s)

The changes are extensive but disparate across configuration, database, handlers, and state management layers. While proxy-cache mode introduces conditional logic throughout the system, the primary flows (satellite registration, setup scheduling, credential handling) are relatively straightforward self-contained operations. No single cohesive multi-component sequence would significantly benefit from visualization.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Suggested labels

enhancement, component:satellite, component:ground-control, golang

Suggested reviewers

  • Vad1mo
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 41.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: add transparent proxy-cache satellite mode' clearly and specifically summarizes the main feature being introduced in the changeset.
Description check ✅ Passed The description provides comprehensive sections on Summary, Changes (organized by Ground Control and Satellite), addressing all major aspects of the feature and changes in detail.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found across 37 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="ground-control/internal/server/spire_handlers.go">

<violation number="1" location="ground-control/internal/server/spire_handlers.go:284">
P1: This hard-coded `normal` mode prevents SPIFFE registrations from creating `proxy-cache` satellites.</violation>
</file>

<file name="internal/satellite/satellite.go">

<violation number="1" location="internal/satellite/satellite.go:87">
P2: Proxy-cache setup can be delayed by a full registration interval after ZTR succeeds. If the initial setup run happens before credentials are written, sync stays disabled until the next registration tick.</violation>
</file>

<file name="cmd/main.go">

<violation number="1" location="cmd/main.go:309">
P1: This new proxy-cache wiring never applies the rebuilt Zot config to the running registry. `ProxyCacheSetupProcess` only updates `ConfigManager` in memory, so Zot keeps its original config and proxy-cache mode will not enable `extensions.sync`.</violation>
</file>

<file name="ground-control/go.mod">

<violation number="1" location="ground-control/go.mod:7">
P1: Don't commit a local `replace` for the parent module; it bypasses the pinned version and breaks builds that don't include the repo root as `../`.</violation>
</file>

<file name="internal/registry/sync_config.go">

<violation number="1" location="internal/registry/sync_config.go:62">
P2: Appending `docker2s2` here is safer than replacing `http.compat`, otherwise proxy-cache mode can remove existing HTTP compatibility settings from the base Zot config.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

satellite, err = txQueries.CreateSatellite(r.Context(), req.SatelliteName)
satellite, err = txQueries.CreateSatellite(r.Context(), database.CreateSatelliteParams{
Name: req.SatelliteName,
Mode: "normal",
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: This hard-coded normal mode prevents SPIFFE registrations from creating proxy-cache satellites.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ground-control/internal/server/spire_handlers.go, line 284:

<comment>This hard-coded `normal` mode prevents SPIFFE registrations from creating `proxy-cache` satellites.</comment>

<file context>
@@ -278,7 +279,10 @@ func (s *Server) registerSatelliteWithSPIFFEHandler(w http.ResponseWriter, r *ht
-		satellite, err = txQueries.CreateSatellite(r.Context(), req.SatelliteName)
+		satellite, err = txQueries.CreateSatellite(r.Context(), database.CreateSatelliteParams{
+			Name: req.SatelliteName,
+			Mode: "normal",
+		})
 		if err != nil {
</file context>
Fix with Cubic

})

s := satellite.NewSatellite(cm, criResults, pathConfig.StateFile)
s := satellite.NewSatellite(cm, criResults, pathConfig.StateFile, pathConfig.SyncCredentialsFile, pathConfig.ZotTempConfig)
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: This new proxy-cache wiring never applies the rebuilt Zot config to the running registry. ProxyCacheSetupProcess only updates ConfigManager in memory, so Zot keeps its original config and proxy-cache mode will not enable extensions.sync.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/main.go, line 309:

<comment>This new proxy-cache wiring never applies the rebuilt Zot config to the running registry. `ProxyCacheSetupProcess` only updates `ConfigManager` in memory, so Zot keeps its original config and proxy-cache mode will not enable `extensions.sync`.</comment>

<file context>
@@ -306,7 +306,7 @@ func run(opts SatelliteOptions, pathConfig *config.PathConfig, shutdownTimeout s
 	})
 
-	s := satellite.NewSatellite(cm, criResults, pathConfig.StateFile)
+	s := satellite.NewSatellite(cm, criResults, pathConfig.StateFile, pathConfig.SyncCredentialsFile, pathConfig.ZotTempConfig)
 	err = s.Run(ctx)
 	if err != nil {
</file context>
Fix with Cubic


toolchain go1.24.3

replace github.com/container-registry/harbor-satellite => ../
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Don't commit a local replace for the parent module; it bypasses the pinned version and breaks builds that don't include the repo root as ../.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ground-control/go.mod, line 7:

<comment>Don't commit a local `replace` for the parent module; it bypasses the pinned version and breaks builds that don't include the repo root as `../`.</comment>

<file context>
@@ -4,6 +4,8 @@ go 1.24.1
 
 toolchain go1.24.3
 
+replace github.com/container-registry/harbor-satellite => ../
+
 require (
</file context>
Fix with Cubic

s.syncCredentialsFile,
s.zotTempConfigPath,
)
setupScheduler, err := scheduler.NewSchedulerWithInterval(
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Proxy-cache setup can be delayed by a full registration interval after ZTR succeeds. If the initial setup run happens before credentials are written, sync stays disabled until the next registration tick.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/satellite/satellite.go, line 87:

<comment>Proxy-cache setup can be delayed by a full registration interval after ZTR succeeds. If the initial setup run happens before credentials are written, sync stays disabled until the next registration tick.</comment>

<file context>
@@ -67,20 +76,42 @@ func (s *Satellite) Run(ctx context.Context) error {
+			s.syncCredentialsFile,
+			s.zotTempConfigPath,
+		)
+		setupScheduler, err := scheduler.NewSchedulerWithInterval(
+			s.cm.GetRegistrationInterval(),
+			setupProcess,
</file context>
Fix with Cubic

if !ok {
httpSection = map[string]any{}
}
httpSection["compat"] = []string{"docker2s2"}
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Appending docker2s2 here is safer than replacing http.compat, otherwise proxy-cache mode can remove existing HTTP compatibility settings from the base Zot config.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/registry/sync_config.go, line 62:

<comment>Appending `docker2s2` here is safer than replacing `http.compat`, otherwise proxy-cache mode can remove existing HTTP compatibility settings from the base Zot config.</comment>

<file context>
@@ -0,0 +1,156 @@
+	if !ok {
+		httpSection = map[string]any{}
+	}
+	httpSection["compat"] = []string{"docker2s2"}
+	zotCfg["http"] = httpSection
+
</file context>
Fix with Cubic

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (8)
ground-control/internal/server/satellite_listing_test.go (1)

22-24: Assert the new mode field explicitly in responses.

These changes update mocked DB rows for mode, but the tests still don’t validate mode in handler output. Add assertions (and ideally one proxy-cache case) so regressions in mode serialization are caught.

Also applies to: 148-149, 200-202

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ground-control/internal/server/satellite_listing_test.go` around lines 22 -
24, The test populates the mocked DB rows with a new "mode" column but never
asserts that the handler's response includes/serializes that field; update the
test that builds `rows` and the corresponding response assertions to explicitly
check the returned satellite's Mode value (e.g., verify Mode == "normal" for the
first row and Mode == "proxy-cache" for at least one case), and add an
additional mocked row and assertion for a "proxy-cache" mode to cover that
branch; locate the test where `rows` is created and where the handler response
is unmarshaled/checked and add assertions against the response struct/JSON's
Mode field to prevent regressions.
ground-control/internal/server/spire_handlers.go (1)

282-285: Use shared mode constant instead of a string literal.

Mode: "normal" can drift from the canonical value used elsewhere. Prefer harbor.SatelliteModeNormal for consistency.

Suggested change
 	satellite, err = txQueries.CreateSatellite(r.Context(), database.CreateSatelliteParams{
 		Name: req.SatelliteName,
-		Mode: "normal",
+		Mode: harbor.SatelliteModeNormal,
 	})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ground-control/internal/server/spire_handlers.go` around lines 282 - 285, The
CreateSatellite call sets Mode using a string literal ("normal") which can
drift; update the CreateSatellite invocation in spire_handlers.go to use the
canonical constant harbor.SatelliteModeNormal for the Mode field, and ensure the
harbor package is imported if not already present so the compiled code
references the shared constant instead of the literal.
internal/state/proxy_cache_setup.go (2)

96-100: Consider using RLock for read-only Name() accessor.

The Name() method only reads p.name which is set once at construction and never modified. Using RLock() instead of Lock() would be more semantically correct and allow concurrent reads.

♻️ Proposed change
 func (p *ProxyCacheSetupProcess) Name() string {
-	p.mu.Lock()
-	defer p.mu.Unlock()
+	p.mu.RLock()
+	defer p.mu.RUnlock()
 	return p.name
 }

Note: This requires changing mu from *sync.Mutex to *sync.RWMutex.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/state/proxy_cache_setup.go` around lines 96 - 100, The Name()
accessor currently uses p.mu.Lock()/Unlock() although it only reads p.name;
change the mutex type from *sync.Mutex to *sync.RWMutex on the struct, update
any construction/initialization to allocate a &sync.RWMutex{}, and replace the
exclusive lock in ProxyCacheSetupProcess.Name() with p.mu.RLock()/RUnlock() so
concurrent readers are allowed (ensure any writers still use
p.mu.Lock()/Unlock()).

56-59: Silent failure on missing credentials could mask configuration issues.

When credentials are missing after ZTR reports completion (IsZTRDone() == true), logging at Error level but returning nil allows the scheduler to keep retrying silently. This is appropriate for the retry loop, but consider whether this state (ZTR done but credentials empty) represents an actual error condition that should eventually fail rather than retry indefinitely.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/state/proxy_cache_setup.go` around lines 56 - 59, The code currently
logs an error and returns nil when upstreamURL/username/password are empty,
which lets the scheduler retry indefinitely; change the behavior to return a
non-nil error when IsZTRDone() is true so the failure surfaces instead of
silently retrying. Specifically, in the block that checks upstreamURL, username,
and password (and where IsZTRDone() is consulted), replace the bare return nil
with returning an error (e.g., fmt.Errorf("missing upstream registry credentials
after ZTR completion: upstreamURL=%q username=%q", upstreamURL, username)) and
keep or enhance the log.Error().Msg to include the same context; ensure the
function signature supports returning an error so callers will stop retrying
when ZTR is done.
ground-control/internal/server/satellite_handlers.go (2)

165-175: Mode resolution logic can be simplified.

The mode has already been validated at lines 91-97 to be either empty, "normal", or "proxy-cache". The resolution logic here can be simplified since req.Mode is already known to be valid.

♻️ Simplified mode resolution
-	// Resolve satellite mode (default to "normal")
-	mode := "normal"
-	if req.Mode == "proxy-cache" {
-		mode = "proxy-cache"
-	}
+	// Resolve satellite mode (default to "normal")
+	mode := req.Mode
+	if mode == "" {
+		mode = harbor.SatelliteModeNormal
+	}

This also uses the constant instead of the string literal for consistency.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ground-control/internal/server/satellite_handlers.go` around lines 165 - 175,
Replace the redundant mode resolution with a concise defaulting of req.Mode
(since it was already validated); e.g., set mode := req.Mode and if mode == ""
set it to the existing canonical "normal" constant (use the project's defined
constant instead of the string literal), then pass mode into
CreateSatellite(r.Context(), database.CreateSatelliteParams{ Name: req.Name,
Mode: mode }). This removes the if req.Mode == "proxy-cache" branch and ensures
consistency by using the declared constant for the "normal" mode.

523-579: Placeholder URL inconsistency between proxy-cache and SKIP_HARBOR_HEALTH_CHECK paths.

When SKIP_HARBOR_HEALTH_CHECK=true and mode is normal, result.StateURL is set to "placeholder://spiffe-testing/" + satellite.Name (line 576). However, for proxy-cache mode, result.StateURL remains empty (never set). This is likely intentional since proxy-cache doesn't use state URLs, but the code paths could be clearer.

Additionally, the harborURL fallback at lines 524-526 uses a hardcoded placeholder that could mask configuration issues in production.

Consider logging a warning when using the placeholder Harbor URL:

 	harborURL := os.Getenv("HARBOR_URL")
 	if harborURL == "" {
+		log.Printf("Warning: HARBOR_URL not set, using placeholder")
 		harborURL = "http://placeholder-registry:5000"
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ground-control/internal/server/satellite_handlers.go` around lines 523 - 579,
Detect and log when the Harbor fallback is used (harborURL variable) by emitting
a warning via log.Printf so the hardcoded "http://placeholder-registry:5000"
isn’t silently masking config issues; also make the result.StateURL behavior
explicit and consistent: when satellite.Mode == harbor.SatelliteModeProxyCache
either set result.StateURL to the same placeholder state
("placeholder://spiffe-testing/"+satellite.Name) or explicitly set it to an
empty string with a clear log entry so callers of result.StateURL know the
intended behavior; update the code paths around satellite.Mode,
SKIP_HARBOR_HEALTH_CHECK, AssembleSatelliteState and
CreateOrUpdateSatStateArtifact/fetchSatelliteConfig to reflect this change and
include the warning when the fallback harborURL is chosen.
internal/registry/sync_config.go (2)

80-81: Magic values for retry configuration.

MaxRetries: 3 and RetryDelay: "5m" are hardcoded. Consider extracting these as constants (similar to defaultSyncPollInterval) or adding them to ProxyCacheParams for configurability.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/registry/sync_config.go` around lines 80 - 81, The retry settings
MaxRetries: 3 and RetryDelay: "5m" are hardcoded; extract them into named
constants (e.g., defaultSyncMaxRetries, defaultSyncRetryDelay) or add fields to
ProxyCacheParams and use those fields when constructing the retry config in
sync_config.go; update any references to defaultSyncPollInterval style naming
and ensure the new constants/struct fields are documented/initialized with the
previous defaults so behavior is unchanged unless configured.

62-62: Overwrites existing compat settings.

This line replaces any existing compat array. If the base config already has compatibility settings configured, they will be lost.

Consider appending "docker2s2" if it's not already present:

♻️ Suggested fix to preserve existing compat settings
-	httpSection["compat"] = []string{"docker2s2"}
+	existingCompat, _ := httpSection["compat"].([]any)
+	hasDocker2s2 := false
+	for _, c := range existingCompat {
+		if c == "docker2s2" {
+			hasDocker2s2 = true
+			break
+		}
+	}
+	if !hasDocker2s2 {
+		httpSection["compat"] = append(existingCompat, "docker2s2")
+	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/registry/sync_config.go` at line 62, The current assignment
httpSection["compat"] = []string{"docker2s2"} overwrites any existing compat
entries; instead, retrieve the existing httpSection["compat"] value (e.g., in
sync_config.go where httpSection is built), if it exists and is a slice/array
append "docker2s2" only when not already present, otherwise create a new slice
with "docker2s2"; ensure you preserve the original type and handle possible
interface{}->[]string conversion safely so you don't lose previously configured
compatibility values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ground-control/go.mod`:
- Around line 131-135: The go.mod pins several vulnerable modules; update the
affected module versions to their patched releases: bump
github.com/containerd/containerd from v1.7.26 to v1.7.29+ (replace reference to
github.com/containerd/containerd v1.7.26), bump
github.com/containerd/containerd/v2 from v2.0.3 to v2.0.7+, upgrade
go.opentelemetry.io/otel/sdk from v1.37.0 to v1.40.0+, and upgrade
helm.sh/helm/v3 from v3.17.1 to v3.17.3+; after changing these module lines run
go get / go mod tidy and rerun your vulnerability scanner to verify fixes.

In `@ground-control/internal/database/satellite_status.sql.go`:
- Line 3: The SQL query parameters lack explicit integer casting, causing sqlc
to generate untyped interface{} parameters for DeleteOldSatelliteStatus and
DeleteOrphanedArtifacts; update the source SQL queries to cast the retention day
parameters to ::integer (e.g., change the parameter placeholders to ... WHERE
created_at < now() - $1::integer * interval '1 day' or similar), then re-run
sqlc to regenerate the Go code so the DeleteOldSatelliteStatus and
DeleteOrphanedArtifacts function signatures use typed int parameters instead of
interface{}.

In `@ground-control/sql/queries/satellites.sql`:
- Around line 2-3: Add a CHECK constraint on the satellites table to enforce
allowed mode values so invalid values cannot be inserted; in the migration that
adds the mode column (ALTER TABLE satellites ADD COLUMN mode TEXT NOT NULL
DEFAULT 'normal'), also add ALTER TABLE satellites ADD CONSTRAINT
check_mode_values CHECK (mode IN ('normal','proxy-cache')) and include a
matching down/rollback step to DROP CONSTRAINT check_mode_values; ensure any
existing data conforms to the allowed set before applying the migration.

In `@ground-control/sql/schema/016_satellite_mode.sql`:
- Line 3: Add a DB-level CHECK constraint on the satellites.mode column to
restrict values to the canonical allowed set (e.g., replace the current ALTER
TABLE satellites ADD COLUMN mode TEXT NOT NULL DEFAULT 'normal' with a migration
that defines mode as TEXT NOT NULL DEFAULT 'normal' CHECK (mode IN
('normal','standby','maintenance','decommissioned')) or similar), give the
constraint an explicit name (e.g., satellites_mode_check), and ensure the
default value is one of the allowed values; update the migration statement in
016_satellite_mode.sql accordingly so the schema enforces allowed modes.

In `@internal/state/proxy_cache_setup.go`:
- Around line 16-24: Remove the unused zotTempConfigPath field from the
ProxyCacheSetupProcess struct and eliminate its corresponding parameter from the
constructor (e.g., NewProxyCacheSetupProcess or whichever function initializes
ProxyCacheSetupProcess); update any call sites that pass that parameter to stop
providing it and adjust the constructor signature and assignments to no longer
reference zotTempConfigPath so the struct and constructor only keep used fields
(name, isRunning, done, mu, cm, syncCredentialsFile).

In `@internal/state/reporting_process.go`:
- Around line 23-29: When assigning or appending new CRI results to the state's
pendingCRI slice (where pendingCRI is updated), reset the reporting latch by
setting criReported = false so newly queued results will be sent; update the
code that modifies pendingCRI (referencing the pendingCRI and criReported fields
on the reporting_process struct) to set criReported = false immediately after
updating pendingCRI.

---

Nitpick comments:
In `@ground-control/internal/server/satellite_handlers.go`:
- Around line 165-175: Replace the redundant mode resolution with a concise
defaulting of req.Mode (since it was already validated); e.g., set mode :=
req.Mode and if mode == "" set it to the existing canonical "normal" constant
(use the project's defined constant instead of the string literal), then pass
mode into CreateSatellite(r.Context(), database.CreateSatelliteParams{ Name:
req.Name, Mode: mode }). This removes the if req.Mode == "proxy-cache" branch
and ensures consistency by using the declared constant for the "normal" mode.
- Around line 523-579: Detect and log when the Harbor fallback is used
(harborURL variable) by emitting a warning via log.Printf so the hardcoded
"http://placeholder-registry:5000" isn’t silently masking config issues; also
make the result.StateURL behavior explicit and consistent: when satellite.Mode
== harbor.SatelliteModeProxyCache either set result.StateURL to the same
placeholder state ("placeholder://spiffe-testing/"+satellite.Name) or explicitly
set it to an empty string with a clear log entry so callers of result.StateURL
know the intended behavior; update the code paths around satellite.Mode,
SKIP_HARBOR_HEALTH_CHECK, AssembleSatelliteState and
CreateOrUpdateSatStateArtifact/fetchSatelliteConfig to reflect this change and
include the warning when the fallback harborURL is chosen.

In `@ground-control/internal/server/satellite_listing_test.go`:
- Around line 22-24: The test populates the mocked DB rows with a new "mode"
column but never asserts that the handler's response includes/serializes that
field; update the test that builds `rows` and the corresponding response
assertions to explicitly check the returned satellite's Mode value (e.g., verify
Mode == "normal" for the first row and Mode == "proxy-cache" for at least one
case), and add an additional mocked row and assertion for a "proxy-cache" mode
to cover that branch; locate the test where `rows` is created and where the
handler response is unmarshaled/checked and add assertions against the response
struct/JSON's Mode field to prevent regressions.

In `@ground-control/internal/server/spire_handlers.go`:
- Around line 282-285: The CreateSatellite call sets Mode using a string literal
("normal") which can drift; update the CreateSatellite invocation in
spire_handlers.go to use the canonical constant harbor.SatelliteModeNormal for
the Mode field, and ensure the harbor package is imported if not already present
so the compiled code references the shared constant instead of the literal.

In `@internal/registry/sync_config.go`:
- Around line 80-81: The retry settings MaxRetries: 3 and RetryDelay: "5m" are
hardcoded; extract them into named constants (e.g., defaultSyncMaxRetries,
defaultSyncRetryDelay) or add fields to ProxyCacheParams and use those fields
when constructing the retry config in sync_config.go; update any references to
defaultSyncPollInterval style naming and ensure the new constants/struct fields
are documented/initialized with the previous defaults so behavior is unchanged
unless configured.
- Line 62: The current assignment httpSection["compat"] = []string{"docker2s2"}
overwrites any existing compat entries; instead, retrieve the existing
httpSection["compat"] value (e.g., in sync_config.go where httpSection is
built), if it exists and is a slice/array append "docker2s2" only when not
already present, otherwise create a new slice with "docker2s2"; ensure you
preserve the original type and handle possible interface{}->[]string conversion
safely so you don't lose previously configured compatibility values.

In `@internal/state/proxy_cache_setup.go`:
- Around line 96-100: The Name() accessor currently uses p.mu.Lock()/Unlock()
although it only reads p.name; change the mutex type from *sync.Mutex to
*sync.RWMutex on the struct, update any construction/initialization to allocate
a &sync.RWMutex{}, and replace the exclusive lock in
ProxyCacheSetupProcess.Name() with p.mu.RLock()/RUnlock() so concurrent readers
are allowed (ensure any writers still use p.mu.Lock()/Unlock()).
- Around line 56-59: The code currently logs an error and returns nil when
upstreamURL/username/password are empty, which lets the scheduler retry
indefinitely; change the behavior to return a non-nil error when IsZTRDone() is
true so the failure surfaces instead of silently retrying. Specifically, in the
block that checks upstreamURL, username, and password (and where IsZTRDone() is
consulted), replace the bare return nil with returning an error (e.g.,
fmt.Errorf("missing upstream registry credentials after ZTR completion:
upstreamURL=%q username=%q", upstreamURL, username)) and keep or enhance the
log.Error().Msg to include the same context; ensure the function signature
supports returning an error so callers will stop retrying when ZTR is done.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: dcb877a0-d8df-4a8f-af63-c640ad54ccd6

📥 Commits

Reviewing files that changed from the base of the PR and between f85832a and 669c817.

⛔ Files ignored due to path filters (1)
  • ground-control/go.sum is excluded by !**/*.sum
📒 Files selected for processing (36)
  • cmd/main.go
  • config.example.json
  • ground-control/go.mod
  • ground-control/internal/database/artifacts.sql.go
  • ground-control/internal/database/configs.sql.go
  • ground-control/internal/database/db.go
  • ground-control/internal/database/groups.sql.go
  • ground-control/internal/database/login_attempts.sql.go
  • ground-control/internal/database/models.go
  • ground-control/internal/database/robot_accounts.sql.go
  • ground-control/internal/database/satellite_configs.sql.go
  • ground-control/internal/database/satellite_groups.sql.go
  • ground-control/internal/database/satellite_status.sql.go
  • ground-control/internal/database/satellite_token.sql.go
  • ground-control/internal/database/satellites.sql.go
  • ground-control/internal/database/sessions.sql.go
  • ground-control/internal/database/users.sql.go
  • ground-control/internal/server/cached_images_test.go
  • ground-control/internal/server/satellite_handlers.go
  • ground-control/internal/server/satellite_listing_test.go
  • ground-control/internal/server/spire_handlers.go
  • ground-control/internal/utils/helper.go
  • ground-control/reg/harbor/robot.go
  • ground-control/sql/queries/satellites.sql
  • ground-control/sql/schema/016_satellite_mode.sql
  • internal/registry/sync_config.go
  • internal/satellite/satellite.go
  • internal/state/proxy_cache_setup.go
  • internal/state/registration_process.go
  • internal/state/reporting_process.go
  • internal/state/spiffe_registration.go
  • pkg/config/config.go
  • pkg/config/constants.go
  • pkg/config/getters.go
  • pkg/config/modifiers.go
  • pkg/config/paths.go

Comment on lines +131 to +135
github.com/containerd/cgroups/v3 v3.0.5 // indirect
github.com/containerd/containerd v1.7.26 // indirect
github.com/containerd/containerd/api v1.8.0 // indirect
github.com/containerd/containerd/v2 v2.0.3 // indirect
github.com/containerd/continuity v0.4.5 // indirect
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the ground-control/go.mod file around the specified lines
fd "go.mod" | grep ground-control

Repository: container-registry/harbor-satellite

Length of output: 100


🏁 Script executed:

# List all files in the repository to locate ground-control/go.mod
git ls-files | grep -E "(ground-control|go\.mod)" | head -20

Repository: container-registry/harbor-satellite

Length of output: 842


🏁 Script executed:

# Check the structure of the repo
find . -type f -name "go.mod" | head -10

Repository: container-registry/harbor-satellite

Length of output: 111


🏁 Script executed:

# Read the ground-control/go.mod file with line numbers to verify the specific lines
wc -l ground-control/go.mod

Repository: container-registry/harbor-satellite

Length of output: 104


🏁 Script executed:

# Read the relevant sections around the flagged lines
sed -n '131,135p' ground-control/go.mod

Repository: container-registry/harbor-satellite

Length of output: 351


🏁 Script executed:

# Check lines 424-425 and 452-453
sed -n '424,425p;452,453p' ground-control/go.mod

Repository: container-registry/harbor-satellite

Length of output: 254


🌐 Web query:

containerd v1.7.26 CVE vulnerability advisories

💡 Result:

containerd v1.7.26 is covered by these upstream security advisories (i.e., v1.7.26 is affected):

  • CVE-2024-40635 / GHSA-265r-hfxg-fhmg – Integer overflow in User ID handling can result in a container running as UID 0 (root) when User is set to a UID:GID larger than max 32-bit signed int. Fixed in containerd 1.7.27. [1]
  • CVE-2024-25621 / GHSA-pwhc-rpq9-4c8w – Overly broad default permissions on several containerd directories (e.g., under /var/lib/containerd and /run/containerd/...) can enable local privilege escalation. Affected versions include 0.1.0 through 1.7.28 (so includes 1.7.26). Fixed in 1.7.29. [2][3]
  • CVE-2025-64329 / GHSA-m6hq-p25p-ffr2 – containerd CRI Attach goroutine leak allows host memory exhaustion (DoS) via repeated attach calls. Fixed in 1.7.29. [4]

Recommended remediation for v1.7.26: upgrade to containerd v1.7.29 or later to address all of the above in the 1.7 line. [2][4]

References:
[1] GHSA-265r-hfxg-fhmg
[2] https://nvd.nist.gov/vuln/detail/CVE-2024-25621
[3] GHSA-pwhc-rpq9-4c8w
[4] GHSA-m6hq-p25p-ffr2


🌐 Web query:

containerd v2.0.3 HIGH severity vulnerability

💡 Result:

For containerd v2.0.3, the main HIGH-severity issues you should check/patch are:

  1. CVE-2024-25621 — local privilege escalation via overly-permissive directories (CVSS 7.3 HIGH)
  • Affected: includes 2.0.3 (affected range: 2.0.0-beta.0 through 2.0.6) [2]
  • What: containerd creates key directories with overly broad permissions (e.g., /var/lib/containerd 0711 vs 0700; CRI dirs too open), enabling local host users to access data and potentially escalate privileges (including via Kubernetes local volumes) [1][2]
  • Fix: upgrade to containerd 2.0.7+ (or 2.1.5 / 2.2.0 / 1.7.29) [1][2]
  • Mitigation (if you can’t upgrade immediately): chmod 700 on the affected directories, or run containerd rootless [1][2]
  1. CVE-2025-47291 — usernamespaced pods not placed under Kubernetes cgroup hierarchy → node DoS (NVD: CVSS 7.5 HIGH)
  • Affected: 2.0.1 ≤ version < 2.0.5 (so 2.0.3 is affected) [3]
  • Impact: may allow denial of service of the Kubernetes node due to limits not being honored [3]
  • Fix: upgrade to 2.0.5+ (or 2.1.0+) [3]
  • Workaround: temporarily disable usernamespaced pods [3]

Sources: GitHub containerd advisory for CVE-2024-25621 [1], Ubuntu CVE record (CVSS 7.3 HIGH and affected versions) [2], and NVD entry for CVE-2025-47291 (affected versions, impact, fixes, and CVSS 7.5 HIGH) [3].


🌐 Web query:

go.opentelemetry.io/otel/sdk v1.37.0 CVE vulnerability

💡 Result:

go.opentelemetry.io/otel/sdk@v1.37.0 is affected by CVE-2026-24051 (GitHub Advisory GHSA-9h8m-3fm2-qjrq): an arbitrary code execution risk on macOS/Darwin due to PATH hijacking. The SDK’s resource detection (sdk/resource/host_id.go) executes the ioreg command via the system PATH; if a local attacker can influence PATH, they may get a malicious ioreg executed in the app’s context. Affected versions: >= 1.21.0, < 1.40.0 (so 1.37.0 is vulnerable). Fixed in: 1.40.0. [1][2][3]

Remediation: upgrade to go.opentelemetry.io/otel/sdk@v1.40.0 or newer. [1][2]

Sources
[1] GitHub Advisory Database (GHSA-9h8m-3fm2-qjrq / CVE-2026-24051) (github.com)
[2] NVD CVE-2026-24051 detail (nvd.nist.gov)
[3] GitLab Advisory Database entry (advisories.gitlab.com)

Citations:


🌐 Web query:

helm.sh/helm/v3 v3.17.1 vulnerability advisory

💡 Result:

Helm v3.17.1 (module helm.sh/helm/v3) is affected by:

  • CVE-2025-32387 / GHSA-5xqw-8hwv-wg92 (Moderate, CVSS 3.1: 6.5) – A specially crafted values.schema.json (JSON Schema) inside a Helm chart can trigger uncontrolled recursion leading to a stack overflow, causing a crash / DoS. Affected versions: all < 3.17.3 (so 3.17.1 is vulnerable). Fix: upgrade to Helm 3.17.3 (or later). Workaround: avoid/inspect charts containing very large schemas with deeply nested references. [1][2]

Sources:
[1] GitHub Security Advisory (helm/helm) GHSA-5xqw-8hwv-wg92
[2] NVD entry for CVE-2025-32387


Multiple critical and high-severity vulnerabilities in pinned dependencies require immediate patching.

This update includes versions flagged by public security advisories:

Patched versions are available for all affected modules. Update dependencies and re-run vulnerability scanning before release.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ground-control/go.mod` around lines 131 - 135, The go.mod pins several
vulnerable modules; update the affected module versions to their patched
releases: bump github.com/containerd/containerd from v1.7.26 to v1.7.29+
(replace reference to github.com/containerd/containerd v1.7.26), bump
github.com/containerd/containerd/v2 from v2.0.3 to v2.0.7+, upgrade
go.opentelemetry.io/otel/sdk from v1.37.0 to v1.40.0+, and upgrade
helm.sh/helm/v3 from v3.17.1 to v3.17.3+; after changing these module lines run
go get / go mod tidy and rerun your vulnerability scanner to verify fixes.

// Code generated by sqlc. DO NOT EDIT.
// versions:
// sqlc v1.28.0
// sqlc v1.30.0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Generated signatures still using interface{}:"
rg -n --type=go 'func \(q \*Queries\) (DeleteOldSatelliteStatus|DeleteOrphanedArtifacts)\(ctx context\.Context, .*interface\{\}\)' ground-control/internal/database

echo
echo "Source SQL definitions to tighten:"
fd 'satellite_status\.sql$'
fd 'artifacts\.sql$'
rg -n -C2 'DeleteOldSatelliteStatus|DeleteOrphanedArtifacts|INTERVAL '\''1 day'\'' \*' .

Repository: container-registry/harbor-satellite

Length of output: 4625


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check the source SQL files more carefully
echo "=== Source SQL: satellite_status.sql ==="
cat -n ground-control/sql/queries/satellite_status.sql | sed -n '45,52p'

echo
echo "=== Source SQL: artifacts.sql ==="
cat -n ground-control/sql/queries/artifacts.sql | sed -n '8,20p'

echo
echo "=== All interface{} usages in Go files ==="
rg -n 'interface\{\}' --type=go ground-control/internal/database/ | head -20

echo
echo "=== Check for 'any' type usage in generated files ==="
rg -n '\bany\b' --type=go ground-control/internal/database/ | head -10

Repository: container-registry/harbor-satellite

Length of output: 1484


🏁 Script executed:

#!/bin/bash
# Verify sqlc version and check if it supports typed parameters with :: casting
echo "=== sqlc.yaml or config check ==="
find . -name "sqlc.yaml" -o -name "sqlc.yml" -o -name "sqlc.json" | xargs cat 2>/dev/null || echo "No sqlc config found at root"

echo
echo "=== Go version and module info ==="
head -20 ground-control/go.mod

Repository: container-registry/harbor-satellite

Length of output: 971


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check sqlc documentation in code or examples
echo "=== Check sqlc version and capabilities ==="
rg -i 'sqlc.*v[0-9]' ground-control/go.* || echo "sqlc not in go.mod deps"

echo
echo "=== Check if there are other SQL query files with type casting ==="
rg -n '::' ground-control/sql/queries/ ground-control/sql/schema/

echo
echo "=== Look at the artifacts.sql file fully to see the refs casting ==="
cat -n ground-control/sql/queries/artifacts.sql

Repository: container-registry/harbor-satellite

Length of output: 1485


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check what the actual generated code looks like for refs parameter
echo "=== Check generated code for refs parameter in artifacts.sql.go ==="
rg -n -A3 'refs' ground-control/internal/database/artifacts.sql.go | head -20

echo
echo "=== Full artifacts.sql.go to see all function signatures ==="
wc -l ground-control/internal/database/artifacts.sql.go
head -50 ground-control/internal/database/artifacts.sql.go

Repository: container-registry/harbor-satellite

Length of output: 1940


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check what types are used elsewhere in the schema
echo "=== Check integer type usage in schema ==="
rg -i 'integer|bigint|smallint' ground-control/sql/schema/ | head -10

echo
echo "=== Check the satellite_status table schema for created_at type ==="
rg -n 'created_at' ground-control/sql/schema/ -A1 -B1

echo
echo "=== See if there are existing examples of casting in queries ==="
rg 'INTERVAL.*\*.*::' ground-control/sql/

Repository: container-registry/harbor-satellite

Length of output: 3839


🌐 Web query:

sqlc v1.30.0 typed parameters with PostgreSQL type casting syntax

💡 Result:

In sqlc v1.30.0, the supported way to “type” a query parameter for PostgreSQL is to cast the parameter expression in SQL, using either Postgres’ ::type syntax or CAST(... AS type).

Named parameters (recommended)

Use sqlc.arg(name) (or the @name shorthand) and cast it:

-- name: GetByID :one
SELECT *
FROM items
WHERE id = sqlc.arg(id)::uuid;

-- shorthand (PostgreSQL)
WHERE id = `@id`::uuid;

This pattern is explicitly shown in sqlc’s “Naming parameters” docs (including ::bool, ::text, and the @param shorthand). (docs.sqlc.dev)

Nullable parameters

Use sqlc.narg(name) and cast the same way:

WHERE (`@limit`::int IS NULL OR some_col = `@limit`::int);
-- or
LIMIT sqlc.narg(limit)::int
OFFSET sqlc.narg(offset)::int;

sqlc.narg is the nullable variant of sqlc.arg. (docs.sqlc.dev)

Notes

  • The @param shorthand is documented as not supported in MySQL (fine for PostgreSQL). (docs.sqlc.dev)
  • sqlc does not require a special “typed parameter” syntax beyond standard SQL casting; just cast the parameter expression as above. (docs.sqlc.dev)

Citations:


Add type casting to retention day parameters in SQL queries.

The generated signatures for DeleteOldSatelliteStatus (line 21) and DeleteOrphanedArtifacts (line 39) still use interface{} because the SQL parameters lack type casting. Cast them to ::integer in the source SQL and regenerate with sqlc to emit typed parameters.

Required SQL changes

In ground-control/sql/queries/satellite_status.sql:

-WHERE created_at < NOW() - INTERVAL '1 day' * $1;
+WHERE created_at < NOW() - INTERVAL '1 day' * $1::integer;

In ground-control/sql/queries/artifacts.sql:

-AND created_at < NOW() - INTERVAL '1 day' * `@retention_days`;
+AND created_at < NOW() - INTERVAL '1 day' * `@retention_days`::integer;

After regenerating, both function signatures should have typed int parameters instead of interface{}, satisfying the coding guideline to avoid untyped parameters.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ground-control/internal/database/satellite_status.sql.go` at line 3, The SQL
query parameters lack explicit integer casting, causing sqlc to generate untyped
interface{} parameters for DeleteOldSatelliteStatus and DeleteOrphanedArtifacts;
update the source SQL queries to cast the retention day parameters to ::integer
(e.g., change the parameter placeholders to ... WHERE created_at < now() -
$1::integer * interval '1 day' or similar), then re-run sqlc to regenerate the
Go code so the DeleteOldSatelliteStatus and DeleteOrphanedArtifacts function
signatures use typed int parameters instead of interface{}.

Comment on lines +2 to +3
INSERT INTO satellites (name, mode, created_at, updated_at)
VALUES ($1, $2, NOW(), NOW())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify schema/migrations enforce satellites.mode integrity
rg -n --type sql -C2 'CREATE TABLE\s+satellites|ALTER TABLE\s+satellites|mode|CHECK|NOT NULL|DEFAULT'

Repository: container-registry/harbor-satellite

Length of output: 11538


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search for any CHECK constraints on mode column
echo "=== Searching for CHECK constraints on mode ==="
rg -n --type sql 'mode.*CHECK|CHECK.*mode' 

echo ""
echo "=== Looking at full schema for mode column definition ==="
cat -n ground-control/sql/schema/016_satellite_mode.sql

echo ""
echo "=== Searching for mode value usage in code ==="
rg -n 'mode\s*[=:].*["\x27]' --type-list | head -20

Repository: container-registry/harbor-satellite

Length of output: 126


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Full content of migration file ==="
cat -n ground-control/sql/schema/016_satellite_mode.sql

echo ""
echo "=== Searching for mode value assignments in code ==="
rg -n "mode\s*[:=]" --type go --type py -A2 -B2 | head -40

echo ""
echo "=== Searching for 'proxy-cache' or 'normal' mode values ==="
rg -n "'normal'|'proxy-cache'|\"normal\"|\"proxy-cache\"" | head -20

Repository: container-registry/harbor-satellite

Length of output: 5364


Add CHECK constraint to enforce valid mode values at the database layer.

The mode column currently has NOT NULL DEFAULT 'normal', but lacks a constraint restricting values to the allowed set (normal, proxy-cache). Since this parameter is inserted directly ($2), enforce the constraint in the schema to prevent invalid modes from being persisted.

Migration file excerpt:
ALTER TABLE satellites ADD COLUMN mode TEXT NOT NULL DEFAULT 'normal';

Add:

ALTER TABLE satellites ADD CONSTRAINT check_mode_values CHECK (mode IN ('normal', 'proxy-cache'));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ground-control/sql/queries/satellites.sql` around lines 2 - 3, Add a CHECK
constraint on the satellites table to enforce allowed mode values so invalid
values cannot be inserted; in the migration that adds the mode column (ALTER
TABLE satellites ADD COLUMN mode TEXT NOT NULL DEFAULT 'normal'), also add ALTER
TABLE satellites ADD CONSTRAINT check_mode_values CHECK (mode IN
('normal','proxy-cache')) and include a matching down/rollback step to DROP
CONSTRAINT check_mode_values; ensure any existing data conforms to the allowed
set before applying the migration.

@@ -0,0 +1,6 @@
-- +goose Up

ALTER TABLE satellites ADD COLUMN mode TEXT NOT NULL DEFAULT 'normal';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add a DB CHECK constraint for allowed satellite modes.

TEXT + default is not enough to protect integrity; invalid mode strings can still be inserted outside handler validation. Constrain values at schema level.

Suggested migration adjustment
-ALTER TABLE satellites ADD COLUMN mode TEXT NOT NULL DEFAULT 'normal';
+ALTER TABLE satellites
+  ADD COLUMN mode TEXT NOT NULL DEFAULT 'normal',
+  ADD CONSTRAINT satellites_mode_check CHECK (mode IN ('normal', 'proxy-cache'));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ground-control/sql/schema/016_satellite_mode.sql` at line 3, Add a DB-level
CHECK constraint on the satellites.mode column to restrict values to the
canonical allowed set (e.g., replace the current ALTER TABLE satellites ADD
COLUMN mode TEXT NOT NULL DEFAULT 'normal' with a migration that defines mode as
TEXT NOT NULL DEFAULT 'normal' CHECK (mode IN
('normal','standby','maintenance','decommissioned')) or similar), give the
constraint an explicit name (e.g., satellites_mode_check), and ensure the
default value is one of the allowed values; update the migration statement in
016_satellite_mode.sql accordingly so the schema enforces allowed modes.

Comment on lines +16 to +24
type ProxyCacheSetupProcess struct {
name string
isRunning bool
done bool
mu *sync.Mutex
cm *config.ConfigManager
syncCredentialsFile string
zotTempConfigPath string
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if zotTempConfigPath is used anywhere in the proxy_cache_setup.go file
rg -n 'zotTempConfigPath' internal/state/proxy_cache_setup.go

Repository: container-registry/harbor-satellite

Length of output: 287


🏁 Script executed:

cat -n internal/state/proxy_cache_setup.go

Repository: container-registry/harbor-satellite

Length of output: 4639


🏁 Script executed:

rg 'zotTempConfigPath' --type go -B2 -A2

Repository: container-registry/harbor-satellite

Length of output: 2523


Remove unused zotTempConfigPath field.

The zotTempConfigPath field is stored in the struct but never referenced in any method. The Zot config is applied via cm.With(config.SetZotConfigRaw(...)) which triggers hot-reload through the config manager, not by writing to a temp file.

🔧 If the field is not needed
 type ProxyCacheSetupProcess struct {
 	name                string
 	isRunning           bool
 	done                bool
 	mu                  *sync.Mutex
 	cm                  *config.ConfigManager
 	syncCredentialsFile string
-	zotTempConfigPath   string
 }

And update the constructor to remove the parameter.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/state/proxy_cache_setup.go` around lines 16 - 24, Remove the unused
zotTempConfigPath field from the ProxyCacheSetupProcess struct and eliminate its
corresponding parameter from the constructor (e.g., NewProxyCacheSetupProcess or
whichever function initializes ProxyCacheSetupProcess); update any call sites
that pass that parameter to stop providing it and adjust the constructor
signature and assignments to no longer reference zotTempConfigPath so the struct
and constructor only keep used fields (name, isRunning, done, mu, cm,
syncCredentialsFile).

Comment on lines +23 to +29
name string
isRunning bool
mu *sync.Mutex
cm *config.ConfigManager
spiffeClient *spiffe.Client
pendingCRI []runtime.CRIConfigResult
criReported bool
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Reset CRI reporting latch when new pending results are set.

Line 58 updates pendingCRI, but criReported can remain true after a prior successful send, so newly queued CRI results may never be reported.

Proposed fix
 func (s *StatusReportingProcess) SetPendingCRIResults(results []runtime.CRIConfigResult) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 	s.pendingCRI = results
+	s.criReported = false
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/state/reporting_process.go` around lines 23 - 29, When assigning or
appending new CRI results to the state's pendingCRI slice (where pendingCRI is
updated), reset the reporting latch by setting criReported = false so newly
queued results will be sent; update the code that modifies pendingCRI
(referencing the pendingCRI and criReported fields on the reporting_process
struct) to set criReported = false immediately after updating pendingCRI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant