From 4190ab4979ce3e74b8a9133484cf3da70d855f2d Mon Sep 17 00:00:00 2001 From: vg006 Date: Tue, 11 Aug 2026 23:09:35 +0530 Subject: [PATCH 1/3] refac: Change int64 to uint64 type in OAS Signed-off-by: vg006 --- internal/groundcontrol/server/helpers.go | 4 ++++ internal/groundcontrol/server/satellite_handlers.go | 6 +++--- internal/groundcontrol/server/server.gen.go | 6 +++--- pkg/groundcontrol/client.gen.go | 6 +++--- spec/ground-control/openapi.yaml | 3 +++ 5 files changed, 16 insertions(+), 9 deletions(-) diff --git a/internal/groundcontrol/server/helpers.go b/internal/groundcontrol/server/helpers.go index 6ea5b6e5..22d66b40 100644 --- a/internal/groundcontrol/server/helpers.go +++ b/internal/groundcontrol/server/helpers.go @@ -363,6 +363,10 @@ func toNullInt64(n int64) sql.NullInt64 { return sql.NullInt64{Int64: n, Valid: true} } +func toNullUInt64(n uint64) sql.NullInt64 { + return sql.NullInt64{Int64: int64(n), Valid: true} +} + func toNullInt32(n int32) sql.NullInt32 { return sql.NullInt32{Int32: n, Valid: true} } diff --git a/internal/groundcontrol/server/satellite_handlers.go b/internal/groundcontrol/server/satellite_handlers.go index cc0edde1..32606aa4 100644 --- a/internal/groundcontrol/server/satellite_handlers.go +++ b/internal/groundcontrol/server/satellite_handlers.go @@ -671,9 +671,9 @@ func (s *Server) SyncSatellite(w http.ResponseWriter, r *http.Request) { LatestStateDigest: toNullString(req.LatestStateDigest), LatestConfigDigest: toNullString(req.LatestConfigDigest), CpuPercent: toNullString(fmt.Sprintf("%.2f", req.CPUPercent)), - MemoryUsedBytes: toNullInt64(req.MemoryUsedBytes), - StorageUsedBytes: toNullInt64(req.StorageUsedBytes), - LastSyncDurationMs: toNullInt64(req.LastSyncDurationMs), + MemoryUsedBytes: toNullUInt64(req.MemoryUsedBytes), + StorageUsedBytes: toNullUInt64(req.StorageUsedBytes), + LastSyncDurationMs: toNullUInt64(req.LastSyncDurationMs), ImageCount: toNullInt32(req.ImageCount), ReportedAt: req.RequestCreatedTime, ArtifactIds: artifactIDs, diff --git a/internal/groundcontrol/server/server.gen.go b/internal/groundcontrol/server/server.gen.go index 0b2ce2e9..0f6355a9 100644 --- a/internal/groundcontrol/server/server.gen.go +++ b/internal/groundcontrol/server/server.gen.go @@ -416,14 +416,14 @@ type SatelliteStatusRequest struct { CachedImages []CachedImageReport `json:"cached_images,omitempty,omitzero"` CPUPercent float64 `json:"cpu_percent,omitempty,omitzero"` ImageCount int32 `json:"image_count,omitempty,omitzero"` - LastSyncDurationMs int64 `json:"last_sync_duration_ms,omitempty,omitzero"` + LastSyncDurationMs uint64 `json:"last_sync_duration_ms,omitempty,omitzero"` LatestConfigDigest string `json:"latest_config_digest,omitempty,omitzero"` LatestStateDigest string `json:"latest_state_digest,omitempty,omitzero"` - MemoryUsedBytes int64 `json:"memory_used_bytes,omitempty,omitzero"` + MemoryUsedBytes uint64 `json:"memory_used_bytes,omitempty,omitzero"` Name string `json:"name,omitempty,omitzero"` RequestCreatedTime time.Time `json:"request_created_time,omitempty,omitzero"` StateReportInterval string `json:"state_report_interval,omitempty,omitzero"` - StorageUsedBytes int64 `json:"storage_used_bytes,omitempty,omitzero"` + StorageUsedBytes uint64 `json:"storage_used_bytes,omitempty,omitzero"` } // SatelliteStatusResponse defines model for SatelliteStatusResponse. diff --git a/pkg/groundcontrol/client.gen.go b/pkg/groundcontrol/client.gen.go index 64b3edc7..47b7f89b 100644 --- a/pkg/groundcontrol/client.gen.go +++ b/pkg/groundcontrol/client.gen.go @@ -420,14 +420,14 @@ type SatelliteStatusRequest struct { CachedImages []CachedImageReport `json:"cached_images,omitempty,omitzero"` CPUPercent float64 `json:"cpu_percent,omitempty,omitzero"` ImageCount int32 `json:"image_count,omitempty,omitzero"` - LastSyncDurationMs int64 `json:"last_sync_duration_ms,omitempty,omitzero"` + LastSyncDurationMs uint64 `json:"last_sync_duration_ms,omitempty,omitzero"` LatestConfigDigest string `json:"latest_config_digest,omitempty,omitzero"` LatestStateDigest string `json:"latest_state_digest,omitempty,omitzero"` - MemoryUsedBytes int64 `json:"memory_used_bytes,omitempty,omitzero"` + MemoryUsedBytes uint64 `json:"memory_used_bytes,omitempty,omitzero"` Name string `json:"name,omitempty,omitzero"` RequestCreatedTime time.Time `json:"request_created_time,omitempty,omitzero"` StateReportInterval string `json:"state_report_interval,omitempty,omitzero"` - StorageUsedBytes int64 `json:"storage_used_bytes,omitempty,omitzero"` + StorageUsedBytes uint64 `json:"storage_used_bytes,omitempty,omitzero"` } // SatelliteStatusResponse defines model for SatelliteStatusResponse. diff --git a/spec/ground-control/openapi.yaml b/spec/ground-control/openapi.yaml index db0689dd..fce0b6b4 100644 --- a/spec/ground-control/openapi.yaml +++ b/spec/ground-control/openapi.yaml @@ -2319,6 +2319,7 @@ components: type: integer format: int64 x-go-name: LastSyncDurationMs + x-go-type: uint64 latest_config_digest: type: string x-go-name: LatestConfigDigest @@ -2330,6 +2331,7 @@ components: format: int64 minimum: 0 x-go-name: MemoryUsedBytes + x-go-type: uint64 name: type: string x-go-name: Name @@ -2345,6 +2347,7 @@ components: format: int64 minimum: 0 x-go-name: StorageUsedBytes + x-go-type: uint64 GroupSyncRequest: type: object title: GroupSyncRequest contains a group state artifact synchronized from From 3cb03c73bee8ec7acba3c62a9036d08dd1135637 Mon Sep 17 00:00:00 2001 From: vg006 Date: Tue, 11 Aug 2026 23:51:02 +0530 Subject: [PATCH 2/3] refac: Update satellite to use generated GC Signed-off-by: vg006 --- internal/satellite/state/catalog.go | 20 ++--- internal/satellite/state/helpers.go | 21 +++++ .../satellite/state/registration_process.go | 73 +++++++---------- .../state/registration_process_test.go | 82 ++++++++++++++++++- internal/satellite/state/report.go | 27 +++--- internal/satellite/state/report_test.go | 7 +- internal/satellite/state/reporting_process.go | 78 +++++++++--------- .../satellite/state/reporting_process_test.go | 72 +++++++++++++++- .../satellite/state/spiffe_registration.go | 51 ++++++------ 9 files changed, 286 insertions(+), 145 deletions(-) diff --git a/internal/satellite/state/catalog.go b/internal/satellite/state/catalog.go index b5d3dc23..fd8037e0 100644 --- a/internal/satellite/state/catalog.go +++ b/internal/satellite/state/catalog.go @@ -13,13 +13,9 @@ import ( v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/container-registry/harbor-satellite/internal/logger" + "github.com/container-registry/harbor-satellite/pkg/groundcontrol" ) -type CachedImage struct { - Reference string `json:"reference"` - SizeBytes int64 `json:"size_bytes"` -} - type catalogResponse struct { Repositories []string `json:"repositories"` } @@ -28,7 +24,7 @@ type tagsResponse struct { Tags []string `json:"tags"` } -func collectCachedImages(ctx context.Context, registryHost string, insecure bool) ([]CachedImage, error) { +func collectCachedImages(ctx context.Context, registryHost string, insecure bool) ([]groundcontrol.CachedImageReport, error) { log := logger.FromContext(ctx) client := &http.Client{Timeout: 30 * time.Second} @@ -37,7 +33,7 @@ func collectCachedImages(ctx context.Context, registryHost string, insecure bool return nil, fmt.Errorf("fetch catalog: %w", err) } - var images []CachedImage + var images []groundcontrol.CachedImageReport for _, repo := range repos { tags, err := fetchTags(ctx, client, registryHost, repo, insecure) if err != nil { @@ -56,13 +52,13 @@ func collectCachedImages(ctx context.Context, registryHost string, insecure bool } if images == nil { - return []CachedImage{}, nil + return []groundcontrol.CachedImageReport{}, nil } return images, nil } -func collectImageInfo(ref string, ctxOpt crane.Option, insecure bool) (CachedImage, error) { +func collectImageInfo(ref string, ctxOpt crane.Option, insecure bool) (groundcontrol.CachedImageReport, error) { opts := []crane.Option{ctxOpt} if insecure { opts = append(opts, crane.Insecure) @@ -70,17 +66,17 @@ func collectImageInfo(ref string, ctxOpt crane.Option, insecure bool) (CachedIma raw, err := crane.Manifest(ref, opts...) if err != nil { - return CachedImage{}, fmt.Errorf("get manifest for %s: %w", ref, err) + return groundcontrol.CachedImageReport{}, fmt.Errorf("get manifest for %s: %w", ref, err) } size, err := computeManifestSize(raw) if err != nil { - return CachedImage{}, fmt.Errorf("compute size for %s: %w", ref, err) + return groundcontrol.CachedImageReport{}, fmt.Errorf("compute size for %s: %w", ref, err) } digest := fmt.Sprintf("sha256:%x", sha256.Sum256(raw)) - return CachedImage{ + return groundcontrol.CachedImageReport{ Reference: ref + "@" + digest, SizeBytes: size, }, nil diff --git a/internal/satellite/state/helpers.go b/internal/satellite/state/helpers.go index 5bc0e681..0dfcd8c8 100644 --- a/internal/satellite/state/helpers.go +++ b/internal/satellite/state/helpers.go @@ -2,9 +2,11 @@ package state import ( "fmt" + "strings" "github.com/container-registry/harbor-satellite/internal/utils" "github.com/container-registry/harbor-satellite/pkg/config" + "github.com/container-registry/harbor-satellite/pkg/groundcontrol" "github.com/rs/zerolog" ) @@ -21,3 +23,22 @@ func getStateFetcherForInputWithTLS(input, username, password string, useInsecur return NewURLStateFetcherWithTLS(input, username, password, useInsecure, tlsCfg), nil } + +func stateConfigFromResponse(response groundcontrol.StateConfigResponse) config.StateConfig { + return config.StateConfig{ + RegistryCredentials: config.RegistryCredentials{ + URL: config.URL(response.Auth.URL), + Username: response.Auth.Username, + Password: response.Auth.Password, + }, + StateURL: response.State, + } +} + +func responseError(operation, status string, response *groundcontrol.AppError) error { + return fmt.Errorf("%s: %s: code=%d message=%q", operation, status, response.Code, response.Message) +} + +func unknownResponseError(operation, status string, body []byte) error { + return fmt.Errorf("%s: unexpected response status=%q body=%q", operation, status, strings.TrimSpace(string(body))) +} diff --git a/internal/satellite/state/registration_process.go b/internal/satellite/state/registration_process.go index 43656f0e..7ce2dea8 100644 --- a/internal/satellite/state/registration_process.go +++ b/internal/satellite/state/registration_process.go @@ -1,10 +1,8 @@ package state import ( - "bytes" "context" "crypto/tls" - "encoding/json" "fmt" "net/http" "strings" @@ -14,14 +12,10 @@ import ( "github.com/container-registry/harbor-satellite/internal/logger" satTLS "github.com/container-registry/harbor-satellite/internal/satellite/tls" "github.com/container-registry/harbor-satellite/pkg/config" + "github.com/container-registry/harbor-satellite/pkg/groundcontrol" "github.com/rs/zerolog" ) -const ( - ZeroTouchRegistrationRoute = "satellites/ztr" - ZeroTouchRegistrationEventName = "zero-touch-registration-event" -) - type ZtrProcess struct { // Name is the name of the process name string @@ -63,7 +57,6 @@ func (z *ZtrProcess) Execute(ctx context.Context) error { // Register the satellite stateConfig, err := registerSatellite( gcURL, - ZeroTouchRegistrationRoute, z.cm.GetToken(), z.cm.GetTLSConfig(), z.cm.UseUnsecure(), @@ -199,43 +192,41 @@ func sanitizeAuditReason(err error, token string) string { return s } -func registerSatellite(groundControlURL, path, token string, tlsCfg config.TLSConfig, useUnsecure bool, ctx context.Context) (config.StateConfig, error) { - ztrURL := fmt.Sprintf("%s/%s", groundControlURL, path) - body, err := json.Marshal(map[string]string{"token": token}) - if err != nil { - return config.StateConfig{}, fmt.Errorf("failed to encode request: %w", err) - } - - client, err := createHTTPClient(tlsCfg, useUnsecure) +func registerSatellite(groundControlURL, token string, tlsCfg config.TLSConfig, useUnsecure bool, ctx context.Context) (config.StateConfig, error) { + httpClient, err := createHTTPClient(tlsCfg, useUnsecure) if err != nil { return config.StateConfig{}, fmt.Errorf("failed to create HTTP client: %w", err) } - req, err := http.NewRequestWithContext(ctx, http.MethodPost, ztrURL, bytes.NewReader(body)) - if err != nil { - return config.StateConfig{}, fmt.Errorf("failed to create request: %w", err) - } - req.Header.Set("Content-Type", "application/json") - response, err := client.Do(req) + client, err := groundcontrol.NewClientWithResponses( + groundControlURL, + groundcontrol.WithHTTPClient(httpClient), + ) if err != nil { - return config.StateConfig{}, fmt.Errorf("failed to send request: %w", err) + return config.StateConfig{}, fmt.Errorf("failed to create Ground Control client: %w", err) } - defer func() { - if err := response.Body.Close(); err != nil { - logger.FromContext(ctx).Warn().Err(err).Msg("error closing response body") - } - }() - if response.StatusCode != http.StatusOK { - return config.StateConfig{}, fmt.Errorf("failed to register satellite: %s", response.Status) + response, err := client.ZtrWithResponse(ctx, groundcontrol.ZTRRequest{Token: token}) + if err != nil { + return config.StateConfig{}, fmt.Errorf("failed to send registration request: %w", err) } - var authResponse config.StateConfig - if err := json.NewDecoder(response.Body).Decode(&authResponse); err != nil { - return config.StateConfig{}, fmt.Errorf("failed to decode response: %w", err) + switch { + case response.JSON200 != nil: + return stateConfigFromResponse(*response.JSON200), nil + case response.JSON400 != nil: + return config.StateConfig{}, responseError("failed to register satellite", response.Status(), response.JSON400) + case response.JSON401 != nil: + return config.StateConfig{}, responseError("failed to register satellite", response.Status(), response.JSON401) + case response.JSON422 != nil: + return config.StateConfig{}, responseError("failed to register satellite", response.Status(), response.JSON422) + case response.JSON429 != nil: + return config.StateConfig{}, responseError("failed to register satellite", response.Status(), response.JSON429) + case response.JSON500 != nil: + return config.StateConfig{}, responseError("failed to register satellite", response.Status(), response.JSON500) + default: + return config.StateConfig{}, unknownResponseError("failed to register satellite", response.Status(), response.Body) } - - return authResponse, nil } func createHTTPClient(tlsCfg config.TLSConfig, useUnsecure bool) (*http.Client, error) { @@ -247,23 +238,21 @@ func createHTTPClient(tlsCfg config.TLSConfig, useUnsecure bool) (*http.Client, if useUnsecure { transport.TLSClientConfig = &tls.Config{ - MinVersion: tls.VersionTLS12, + MinVersion: tls.VersionTLS12, + InsecureSkipVerify: true, //nolint:gosec // Explicitly enabled by use_unsecure. } - transport.TLSClientConfig.InsecureSkipVerify = useUnsecure } else if tlsCfg.CertFile != "" || tlsCfg.CAFile != "" { - cfg := &satTLS.Config{ + loadedTLSConfig, err := satTLS.LoadClientTLSConfig(&satTLS.Config{ CertFile: tlsCfg.CertFile, KeyFile: tlsCfg.KeyFile, CAFile: tlsCfg.CAFile, SkipVerify: tlsCfg.SkipVerify, MinVersion: tls.VersionTLS12, - } - - tlsConfig, err := satTLS.LoadClientTLSConfig(cfg) + }) if err != nil { return nil, fmt.Errorf("load TLS config: %w", err) } - transport.TLSClientConfig = tlsConfig + transport.TLSClientConfig = loadedTLSConfig } return &http.Client{ diff --git a/internal/satellite/state/registration_process_test.go b/internal/satellite/state/registration_process_test.go index 9bb3009e..e5ed17fb 100644 --- a/internal/satellite/state/registration_process_test.go +++ b/internal/satellite/state/registration_process_test.go @@ -6,10 +6,12 @@ import ( "errors" "net/http" "net/http/httptest" + "path/filepath" "strings" "testing" "github.com/container-registry/harbor-satellite/pkg/config" + "github.com/container-registry/harbor-satellite/pkg/groundcontrol" "github.com/stretchr/testify/require" ) @@ -40,18 +42,90 @@ func TestRegisterSatellitePostsTokenInJSONBody(t *testing.T) { require.Equal(t, "/satellites/ztr", r.URL.Path) require.Equal(t, "application/json", r.Header.Get("Content-Type")) - var body map[string]string + var body groundcontrol.ZTRRequest require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) - require.Equal(t, token, body["token"]) + require.Equal(t, token, body.Token) w.Header().Set("Content-Type", "application/json") - _, err := w.Write([]byte(`{}`)) + _, err := w.Write([]byte(`{"auth":{"url":"registry.test","username":"robot","password":"secret"},"state":"registry.test/state:latest"}`)) require.NoError(t, err) })) defer server.Close() - _, err := registerSatellite(server.URL, ZeroTouchRegistrationRoute, token, config.TLSConfig{}, false, context.Background()) + stateConfig, err := registerSatellite(server.URL, token, config.TLSConfig{}, true, context.Background()) require.NoError(t, err) + require.Equal(t, config.URL("registry.test"), stateConfig.RegistryCredentials.URL) + require.Equal(t, "robot", stateConfig.RegistryCredentials.Username) + require.Equal(t, "secret", stateConfig.RegistryCredentials.Password) + require.Equal(t, "registry.test/state:latest", stateConfig.StateURL) +} + +func TestRegisterSatelliteReturnsTypedError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, err := w.Write([]byte(`{"code":40101,"message":"registration token expired"}`)) + require.NoError(t, err) + })) + defer server.Close() + + _, err := registerSatellite(server.URL, "expired-token", config.TLSConfig{}, true, context.Background()) + + require.ErrorContains(t, err, "401 Unauthorized") + require.ErrorContains(t, err, "code=40101") + require.ErrorContains(t, err, `message="registration token expired"`) +} + +func TestRegisterSatellitePreservesUnknownResponseContext(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTeapot) + _, err := w.Write([]byte("unexpected proxy response")) + require.NoError(t, err) + })) + defer server.Close() + + _, err := registerSatellite(server.URL, "token", config.TLSConfig{}, true, context.Background()) + + require.ErrorContains(t, err, "418 I'm a teapot") + require.ErrorContains(t, err, "unexpected proxy response") +} + +func TestZtrProcessDoesNotPersistInvalidRegistrationData(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, err := w.Write([]byte(`{"auth":{"url":"new-registry","username":"new-user"},"state":"new-state"}`)) + require.NoError(t, err) + })) + defer server.Close() + + existingState := config.StateConfig{ + RegistryCredentials: config.RegistryCredentials{ + URL: "existing-registry", + Username: "existing-user", + Password: "existing-password", + }, + StateURL: "existing-state", + } + cfg := &config.Config{ + StateConfig: existingState, + AppConfig: config.AppConfig{ + GroundControlURL: config.URL(server.URL), + UseUnsecure: true, + }, + ZotConfigRaw: json.RawMessage(`{}`), + } + dir := t.TempDir() + cm, err := config.NewConfigManager( + filepath.Join(dir, "config.json"), + filepath.Join(dir, "prev.json"), + "token", server.URL, false, cfg, + ) + require.NoError(t, err) + + err = NewZtrProcess(cm).Execute(testContext()) + + require.ErrorContains(t, err, "invalid state auth config") + require.Equal(t, existingState, cm.GetStateConfig()) } func TestSanitizeAuditReason_TokenAppearsMultipleTimes(t *testing.T) { diff --git a/internal/satellite/state/report.go b/internal/satellite/state/report.go index a850eeb1..741d269a 100644 --- a/internal/satellite/state/report.go +++ b/internal/satellite/state/report.go @@ -10,27 +10,20 @@ import ( "github.com/container-registry/harbor-satellite/internal/logger" "github.com/container-registry/harbor-satellite/pkg/config" + "github.com/container-registry/harbor-satellite/pkg/groundcontrol" "github.com/shirou/gopsutil/v3/cpu" "github.com/shirou/gopsutil/v3/disk" "github.com/shirou/gopsutil/v3/mem" ) -type StatusReportParams struct { - Name string `json:"name"` - Activity string `json:"activity"` - StateReportInterval string `json:"state_report_interval"` - LatestStateDigest string `json:"latest_state_digest"` - LatestConfigDigest string `json:"latest_config_digest"` - MemoryUsedBytes uint64 `json:"memory_used_bytes"` - StorageUsedBytes uint64 `json:"storage_used_bytes"` - CPUPercent float64 `json:"cpu_percent"` - RequestCreatedTime time.Time `json:"request_created_time"` - LastSyncDurationMs int64 `json:"last_sync_duration_ms"` - ImageCount int `json:"image_count"` - CachedImages []CachedImage `json:"cached_images,omitempty"` -} - -func collectStatusReportParams(ctx context.Context, heartbeatInterval time.Duration, req *StatusReportParams, cfg config.MetricsConfig, registryURL string, insecure bool) { +func collectStatusReportParams( + ctx context.Context, + heartbeatInterval time.Duration, + req *groundcontrol.SatelliteStatusRequest, + cfg config.MetricsConfig, + registryURL string, + insecure bool, +) { log := logger.FromContext(ctx) if cfg.CollectCPU { @@ -49,7 +42,7 @@ func collectStatusReportParams(ctx context.Context, heartbeatInterval time.Durat log.Warn().Err(err).Msg("Failed to collect cached images") } else { req.CachedImages = cached - req.ImageCount = len(cached) + req.ImageCount = int32(len(cached)) } } } diff --git a/internal/satellite/state/report_test.go b/internal/satellite/state/report_test.go index f02be978..275b06e7 100644 --- a/internal/satellite/state/report_test.go +++ b/internal/satellite/state/report_test.go @@ -5,23 +5,24 @@ import ( "time" "github.com/container-registry/harbor-satellite/pkg/config" + "github.com/container-registry/harbor-satellite/pkg/groundcontrol" "github.com/stretchr/testify/require" ) func TestCollectStatusReportParams_EmptyRegistryURL(t *testing.T) { ctx := testContext() - req := &StatusReportParams{} + req := &groundcontrol.SatelliteStatusRequest{} cfg := config.MetricsConfig{} collectStatusReportParams(ctx, 30*time.Second, req, cfg, "", false) require.Nil(t, req.CachedImages) - require.Equal(t, 0, req.ImageCount) + require.Equal(t, int32(0), req.ImageCount) } func TestCollectStatusReportParams_UnreachableRegistry(t *testing.T) { ctx := testContext() - req := &StatusReportParams{} + req := &groundcontrol.SatelliteStatusRequest{} cfg := config.MetricsConfig{} collectStatusReportParams(ctx, 30*time.Second, req, cfg, "127.0.0.1:1", true) diff --git a/internal/satellite/state/reporting_process.go b/internal/satellite/state/reporting_process.go index 63f562e3..6f72b3f9 100644 --- a/internal/satellite/state/reporting_process.go +++ b/internal/satellite/state/reporting_process.go @@ -1,9 +1,7 @@ package state import ( - "bytes" "context" - "encoding/json" "fmt" "net/http" "strings" @@ -15,10 +13,9 @@ import ( "github.com/container-registry/harbor-satellite/internal/spiffe" "github.com/container-registry/harbor-satellite/internal/utils" "github.com/container-registry/harbor-satellite/pkg/config" + "github.com/container-registry/harbor-satellite/pkg/groundcontrol" ) -const StatusReportRoute = "satellites/sync" - type StatusReportingProcess struct { name string isRunning bool @@ -85,7 +82,7 @@ func (s *StatusReportingProcess) Execute(ctx context.Context) error { metricsCfg := s.cm.GetMetricsConfig() - req := &StatusReportParams{ + req := &groundcontrol.SatelliteStatusRequest{ Name: satelliteName, StateReportInterval: heartbeatExpr, RequestCreatedTime: time.Now().UTC(), @@ -140,62 +137,67 @@ func formatCRIActivity(results []runtime.CRIConfigResult) string { return "cri_fallback_configured: " + strings.Join(parts, ", ") } -func (s *StatusReportingProcess) sendStatusReport(ctx context.Context, groundControlURL string, req *StatusReportParams) error { - body, err := json.Marshal(req) - if err != nil { - return fmt.Errorf("marshal status report: %w", err) - } - - syncURL := fmt.Sprintf("%s/%s", groundControlURL, StatusReportRoute) - - var client *http.Client +func (s *StatusReportingProcess) sendStatusReport( + ctx context.Context, + groundControlURL string, + req *groundcontrol.SatelliteStatusRequest, +) error { + var httpClient *http.Client + var err error + clientOptions := make([]groundcontrol.ClientOption, 0, 2) if s.spiffeClient != nil { if err := s.spiffeClient.Connect(ctx); err != nil { return fmt.Errorf("connect to SPIRE agent: %w", err) } - client, err = s.spiffeClient.CreateHTTPClient() + httpClient, err = s.spiffeClient.CreateHTTPClient() if err != nil { return fmt.Errorf("create SPIFFE HTTP client: %w", err) } } else { - client, err = createHTTPClient(s.cm.GetTLSConfig(), s.cm.UseUnsecure()) + if !s.cm.UseUnsecure() && !strings.HasPrefix(groundControlURL, "https://") { + return fmt.Errorf("insecure connection: Ground Control URL %q must use HTTPS when use_unsecure is false", groundControlURL) + } + + httpClient, err = createHTTPClient(s.cm.GetTLSConfig(), s.cm.UseUnsecure()) if err != nil { return fmt.Errorf("create HTTP client: %w", err) } - } - httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, syncURL, bytes.NewReader(body)) - if err != nil { - return fmt.Errorf("create request: %w", err) - } - httpReq.Header.Set("Content-Type", "application/json") - - if s.spiffeClient == nil { - if !s.cm.UseUnsecure() && !strings.HasPrefix(syncURL, "https://") { - return fmt.Errorf("insecure connection: sync URL %q must use HTTPS when use_unsecure is false", syncURL) - } username := s.cm.GetSourceRegistryUsername() password := s.cm.GetSourceRegistryPassword() if username != "" && password != "" { - httpReq.SetBasicAuth(username, password) + clientOptions = append(clientOptions, groundcontrol.WithRequestEditorFn( + func(_ context.Context, request *http.Request) error { + request.SetBasicAuth(username, password) + return nil + }, + )) } } - resp, err := client.Do(httpReq) + clientOptions = append(clientOptions, groundcontrol.WithHTTPClient(httpClient)) + client, err := groundcontrol.NewClientWithResponses(groundControlURL, clientOptions...) if err != nil { - return fmt.Errorf("send request: %w", err) + return fmt.Errorf("create Ground Control client: %w", err) } - defer func() { - if err := resp.Body.Close(); err != nil { - logger.FromContext(ctx).Warn().Err(err).Msg("error closing response body") - } - }() - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("status report failed: %s", resp.Status) + response, err := client.SyncSatelliteWithResponse(ctx, *req) + if err != nil { + return fmt.Errorf("send status report: %w", err) } - return nil + switch { + case response.StatusCode() == 200: + return nil + case response.JSON400 != nil: + return responseError("status report failed", response.Status(), response.JSON400) + case response.JSON403 != nil: + return responseError("status report failed", response.Status(), response.JSON403) + case response.JSON500 != nil: + return responseError("status report failed", response.Status(), response.JSON500) + default: + return unknownResponseError("status report failed", response.Status(), response.Body) + } } func (s *StatusReportingProcess) Name() string { diff --git a/internal/satellite/state/reporting_process_test.go b/internal/satellite/state/reporting_process_test.go index 838eb0e1..935e0c7c 100644 --- a/internal/satellite/state/reporting_process_test.go +++ b/internal/satellite/state/reporting_process_test.go @@ -10,6 +10,7 @@ import ( runtime "github.com/container-registry/harbor-satellite/internal/satellite/container_runtime" "github.com/container-registry/harbor-satellite/pkg/config" + "github.com/container-registry/harbor-satellite/pkg/groundcontrol" "github.com/stretchr/testify/require" ) @@ -147,7 +148,7 @@ func TestExecute_CRIReporting(t *testing.T) { } t.Run("successful send clears CRI results", func(t *testing.T) { - var received StatusReportParams + var received groundcontrol.SatelliteStatusRequest srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { require.NoError(t, json.NewDecoder(r.Body).Decode(&received)) w.WriteHeader(http.StatusOK) @@ -194,7 +195,7 @@ func TestExecute_CRIReporting(t *testing.T) { var callCount int var lastActivity string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - var req StatusReportParams + var req groundcontrol.SatelliteStatusRequest require.NoError(t, json.NewDecoder(r.Body).Decode(&req)) callCount++ lastActivity = req.Activity @@ -220,7 +221,7 @@ func TestExecute_CRIReporting(t *testing.T) { shouldFail := true var lastActivity string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - var req StatusReportParams + var req groundcontrol.SatelliteStatusRequest require.NoError(t, json.NewDecoder(r.Body).Decode(&req)) lastActivity = req.Activity if shouldFail { @@ -256,3 +257,68 @@ func TestExecute_CRIReporting(t *testing.T) { p.mu.Unlock() }) } + +func TestSendStatusReportUsesBasicAuthEditor(t *testing.T) { + const ( + username = "robot$satellite-test" + password = "robot-secret" + ) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, request *http.Request) { + gotUsername, gotPassword, ok := request.BasicAuth() + require.True(t, ok) + require.Equal(t, username, gotUsername) + require.Equal(t, password, gotPassword) + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + cm := newReportingTestCM(t, server.URL) + cm.With(config.SetStateConfig(config.StateConfig{ + RegistryCredentials: config.RegistryCredentials{ + Username: username, + Password: password, + }, + StateURL: cm.GetStateURL(), + })) + process := &StatusReportingProcess{name: "test", mu: &sync.Mutex{}, cm: cm} + + err := process.sendStatusReport(testContext(), server.URL, &groundcontrol.SatelliteStatusRequest{Name: "test-sat"}) + + require.NoError(t, err) +} + +func TestSendStatusReportReturnsTypedError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusForbidden) + _, err := w.Write([]byte(`{"code":40301,"message":"satellite is not authorized"}`)) + require.NoError(t, err) + })) + defer server.Close() + + cm := newReportingTestCM(t, server.URL) + process := &StatusReportingProcess{name: "test", mu: &sync.Mutex{}, cm: cm} + + err := process.sendStatusReport(testContext(), server.URL, &groundcontrol.SatelliteStatusRequest{Name: "test-sat"}) + + require.ErrorContains(t, err, "403 Forbidden") + require.ErrorContains(t, err, "code=40301") + require.ErrorContains(t, err, "satellite is not authorized") +} + +func TestSendStatusReportPreservesUnknownResponseContext(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadGateway) + _, err := w.Write([]byte("upstream unavailable")) + require.NoError(t, err) + })) + defer server.Close() + + cm := newReportingTestCM(t, server.URL) + process := &StatusReportingProcess{name: "test", mu: &sync.Mutex{}, cm: cm} + + err := process.sendStatusReport(testContext(), server.URL, &groundcontrol.SatelliteStatusRequest{Name: "test-sat"}) + + require.ErrorContains(t, err, "502 Bad Gateway") + require.ErrorContains(t, err, "upstream unavailable") +} diff --git a/internal/satellite/state/spiffe_registration.go b/internal/satellite/state/spiffe_registration.go index 54523cca..3b509d72 100644 --- a/internal/satellite/state/spiffe_registration.go +++ b/internal/satellite/state/spiffe_registration.go @@ -2,20 +2,17 @@ package state import ( "context" - "encoding/json" "errors" "fmt" - "net/http" "sync" "github.com/container-registry/harbor-satellite/internal/logger" "github.com/container-registry/harbor-satellite/internal/spiffe" "github.com/container-registry/harbor-satellite/pkg/config" + "github.com/container-registry/harbor-satellite/pkg/groundcontrol" "github.com/rs/zerolog" ) -const SPIFFEZeroTouchRegistrationRoute = "satellites/spiffe-ztr" - type SpiffeZtrProcess struct { name string isRunning bool @@ -102,39 +99,41 @@ func (s *SpiffeZtrProcess) Execute(ctx context.Context) error { func (s *SpiffeZtrProcess) registerWithSPIFFE(ctx context.Context, log *zerolog.Logger) (config.StateConfig, error) { gcURL := s.cm.ResolveGroundControlURL() - ztrURL := fmt.Sprintf("%s/%s", gcURL, SPIFFEZeroTouchRegistrationRoute) httpClient, err := s.spiffeClient.CreateHTTPClient() if err != nil { return config.StateConfig{}, fmt.Errorf("create SPIFFE HTTP client: %w", err) } - req, err := http.NewRequestWithContext(ctx, http.MethodGet, ztrURL, nil) - if err != nil { - return config.StateConfig{}, fmt.Errorf("create request: %w", err) - } + log.Debug().Str("url", gcURL).Msg("Sending SPIFFE-authenticated ZTR request") - log.Debug().Str("url", ztrURL).Msg("Sending SPIFFE-authenticated ZTR request") - resp, err := httpClient.Do(req) + client, err := groundcontrol.NewClientWithResponses( + gcURL, + groundcontrol.WithHTTPClient(httpClient), + ) if err != nil { - return config.StateConfig{}, fmt.Errorf("send request: %w", err) - } - defer func() { - if err := resp.Body.Close(); err != nil { - log.Warn().Err(err).Msg("error closing response body") - } - }() - - if resp.StatusCode != http.StatusOK { - return config.StateConfig{}, fmt.Errorf("registration failed: %s", resp.Status) + return config.StateConfig{}, fmt.Errorf("create Ground Control client: %w", err) } - var stateConfig config.StateConfig - if err := json.NewDecoder(resp.Body).Decode(&stateConfig); err != nil { - return config.StateConfig{}, fmt.Errorf("decode response: %w", err) + response, err := client.SpiffeZtrWithResponse(ctx) + if err != nil { + return config.StateConfig{}, fmt.Errorf("send SPIFFE registration request: %w", err) + } + + switch { + case response.JSON200 != nil: + return stateConfigFromResponse(*response.JSON200), nil + case response.JSON400 != nil: + return config.StateConfig{}, responseError("SPIFFE registration failed", response.Status(), response.JSON400) + case response.JSON401 != nil: + return config.StateConfig{}, responseError("SPIFFE registration failed", response.Status(), response.JSON401) + case response.JSON429 != nil: + return config.StateConfig{}, responseError("SPIFFE registration failed", response.Status(), response.JSON429) + case response.JSON500 != nil: + return config.StateConfig{}, responseError("SPIFFE registration failed", response.Status(), response.JSON500) + default: + return config.StateConfig{}, unknownResponseError("SPIFFE registration failed", response.Status(), response.Body) } - - return stateConfig, nil } func (s *SpiffeZtrProcess) CanExecute(log *zerolog.Logger) (bool, string) { From 70679ef5b8a1701ca14d3a241e20f3dbe70169f5 Mon Sep 17 00:00:00 2001 From: vg006 Date: Wed, 12 Aug 2026 21:10:12 +0530 Subject: [PATCH 3/3] fix: Resolve review comments Signed-off-by: vg006 --- .../server/cached_images_test.go | 28 +++++++++++++++++++ internal/groundcontrol/server/helpers.go | 4 --- .../server/satellite_handlers.go | 10 +++++++ .../satellite/state/registration_process.go | 2 +- internal/satellite/state/report_test.go | 2 +- internal/satellite/state/reporting_process.go | 8 +++--- .../satellite/state/reporting_process_test.go | 20 +++++++++++++ 7 files changed, 64 insertions(+), 10 deletions(-) diff --git a/internal/groundcontrol/server/cached_images_test.go b/internal/groundcontrol/server/cached_images_test.go index 87ce0850..46d84b6f 100644 --- a/internal/groundcontrol/server/cached_images_test.go +++ b/internal/groundcontrol/server/cached_images_test.go @@ -5,6 +5,7 @@ import ( "database/sql" "encoding/json" "fmt" + "math" "net/http" "net/http/httptest" "testing" @@ -264,6 +265,33 @@ func TestSyncHandler_InvalidBody(t *testing.T) { require.Equal(t, http.StatusBadRequest, rr.Code) } +func TestSyncHandler_RejectsMetricsAboveMaxInt64(t *testing.T) { + tests := []struct { + name string + req SatelliteStatusRequest + }{ + {name: "memory used bytes", req: SatelliteStatusRequest{MemoryUsedBytes: math.MaxInt64 + 1}}, + {name: "storage used bytes", req: SatelliteStatusRequest{StorageUsedBytes: math.MaxInt64 + 1}}, + {name: "last sync duration", req: SatelliteStatusRequest{LastSyncDurationMs: math.MaxInt64 + 1}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server, mock := newMockServer(t) + body := mustMarshalJSON(t, tt.req) + req := httptest.NewRequest(http.MethodPost, "/satellites/sync", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + + rr := httptest.NewRecorder() + server.SyncSatellite(rr, req) + + require.Equal(t, http.StatusBadRequest, rr.Code) + require.Contains(t, rr.Body.String(), "status metrics exceed the maximum supported value") + require.NoError(t, mock.ExpectationsWereMet()) + }) + } +} + func TestSyncHandler_InvalidHeartbeatInterval(t *testing.T) { server, mock := newMockServer(t) diff --git a/internal/groundcontrol/server/helpers.go b/internal/groundcontrol/server/helpers.go index 22d66b40..4339df11 100644 --- a/internal/groundcontrol/server/helpers.go +++ b/internal/groundcontrol/server/helpers.go @@ -359,10 +359,6 @@ func toNullString(s string) sql.NullString { return sql.NullString{String: s, Valid: s != ""} } -func toNullInt64(n int64) sql.NullInt64 { - return sql.NullInt64{Int64: n, Valid: true} -} - func toNullUInt64(n uint64) sql.NullInt64 { return sql.NullInt64{Int64: int64(n), Valid: true} } diff --git a/internal/groundcontrol/server/satellite_handlers.go b/internal/groundcontrol/server/satellite_handlers.go index 32606aa4..c40fb420 100644 --- a/internal/groundcontrol/server/satellite_handlers.go +++ b/internal/groundcontrol/server/satellite_handlers.go @@ -4,6 +4,7 @@ import ( "database/sql" "fmt" "log" + "math" "net/http" "strconv" "time" @@ -606,6 +607,15 @@ func (s *Server) SyncSatellite(w http.ResponseWriter, r *http.Request) { HandleAppError(w, err) return } + if req.MemoryUsedBytes > math.MaxInt64 || + req.StorageUsedBytes > math.MaxInt64 || + req.LastSyncDurationMs > math.MaxInt64 { + HandleAppError(w, &AppError{ + Message: "status metrics exceed the maximum supported value", + Code: http.StatusBadRequest, + }) + return + } // Check SPIFFE identity first for dual auth var satelliteName string diff --git a/internal/satellite/state/registration_process.go b/internal/satellite/state/registration_process.go index 7ce2dea8..aa0b88fe 100644 --- a/internal/satellite/state/registration_process.go +++ b/internal/satellite/state/registration_process.go @@ -241,7 +241,7 @@ func createHTTPClient(tlsCfg config.TLSConfig, useUnsecure bool) (*http.Client, MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, //nolint:gosec // Explicitly enabled by use_unsecure. } - } else if tlsCfg.CertFile != "" || tlsCfg.CAFile != "" { + } else if tlsCfg.CertFile != "" || tlsCfg.CAFile != "" || tlsCfg.SkipVerify { loadedTLSConfig, err := satTLS.LoadClientTLSConfig(&satTLS.Config{ CertFile: tlsCfg.CertFile, KeyFile: tlsCfg.KeyFile, diff --git a/internal/satellite/state/report_test.go b/internal/satellite/state/report_test.go index 275b06e7..80fdd670 100644 --- a/internal/satellite/state/report_test.go +++ b/internal/satellite/state/report_test.go @@ -29,7 +29,7 @@ func TestCollectStatusReportParams_UnreachableRegistry(t *testing.T) { // Should gracefully handle the error - no cached images, image count stays 0 require.Nil(t, req.CachedImages) - require.Equal(t, 0, req.ImageCount) + require.Equal(t, int32(0), req.ImageCount) } func TestExtractSatelliteNameFromURL(t *testing.T) { diff --git a/internal/satellite/state/reporting_process.go b/internal/satellite/state/reporting_process.go index 6f72b3f9..5920ad6b 100644 --- a/internal/satellite/state/reporting_process.go +++ b/internal/satellite/state/reporting_process.go @@ -145,6 +145,10 @@ func (s *StatusReportingProcess) sendStatusReport( var httpClient *http.Client var err error clientOptions := make([]groundcontrol.ClientOption, 0, 2) + if !s.cm.UseUnsecure() && !strings.HasPrefix(groundControlURL, "https://") { + return fmt.Errorf("insecure connection: Ground Control URL %q must use HTTPS when use_unsecure is false", groundControlURL) + } + if s.spiffeClient != nil { if err := s.spiffeClient.Connect(ctx); err != nil { return fmt.Errorf("connect to SPIRE agent: %w", err) @@ -154,10 +158,6 @@ func (s *StatusReportingProcess) sendStatusReport( return fmt.Errorf("create SPIFFE HTTP client: %w", err) } } else { - if !s.cm.UseUnsecure() && !strings.HasPrefix(groundControlURL, "https://") { - return fmt.Errorf("insecure connection: Ground Control URL %q must use HTTPS when use_unsecure is false", groundControlURL) - } - httpClient, err = createHTTPClient(s.cm.GetTLSConfig(), s.cm.UseUnsecure()) if err != nil { return fmt.Errorf("create HTTP client: %w", err) diff --git a/internal/satellite/state/reporting_process_test.go b/internal/satellite/state/reporting_process_test.go index 935e0c7c..d5ff9e8d 100644 --- a/internal/satellite/state/reporting_process_test.go +++ b/internal/satellite/state/reporting_process_test.go @@ -9,6 +9,7 @@ import ( "testing" runtime "github.com/container-registry/harbor-satellite/internal/satellite/container_runtime" + "github.com/container-registry/harbor-satellite/internal/spiffe" "github.com/container-registry/harbor-satellite/pkg/config" "github.com/container-registry/harbor-satellite/pkg/groundcontrol" "github.com/stretchr/testify/require" @@ -287,6 +288,25 @@ func TestSendStatusReportUsesBasicAuthEditor(t *testing.T) { require.NoError(t, err) } +func TestSendStatusReportRejectsInsecureURLBeforeSPIFFESetup(t *testing.T) { + cm := newReportingTestCM(t, "http://ground-control.test") + cm.With(config.SetUseUnsecure(false)) + process := &StatusReportingProcess{ + name: "test", + mu: &sync.Mutex{}, + cm: cm, + spiffeClient: &spiffe.Client{}, + } + + err := process.sendStatusReport( + testContext(), + "http://ground-control.test", + &groundcontrol.SatelliteStatusRequest{Name: "test-sat"}, + ) + + require.ErrorContains(t, err, "must use HTTPS when use_unsecure is false") +} + func TestSendStatusReportReturnsTypedError(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json")