diff --git a/cmd/spinifex/cmd/admin.go b/cmd/spinifex/cmd/admin.go index 79db352f6..8f77189cb 100644 --- a/cmd/spinifex/cmd/admin.go +++ b/cmd/spinifex/cmd/admin.go @@ -2799,7 +2799,11 @@ func finalizeNodeSetup(dataDir, certPath, adminAccessKey, adminSecretKey, region admin.CreateServiceDirectories(dataDir) if os.Getuid() == 0 { - admin.SetServiceOwnership() + if err := admin.SetServiceOwnership(); err != nil { + fmt.Fprintf(os.Stderr, "Warning: service ownership not fully applied: %v\n", err) + } else { + fmt.Println("✅ Service ownership set") + } } } diff --git a/spinifex/admin/admin.go b/spinifex/admin/admin.go index 2271ca3c2..78a49f157 100644 --- a/spinifex/admin/admin.go +++ b/spinifex/admin/admin.go @@ -7,6 +7,7 @@ import ( "crypto/x509/pkix" "encoding/base64" "encoding/pem" + "errors" "fmt" "io/fs" "log/slog" @@ -351,22 +352,20 @@ func FileExists(path string) bool { } // ChownRecursive changes ownership of path and its contents to username. -// Best-effort: errors are logged but do not halt the operation. -func ChownRecursive(path, username string) { +// Failing to resolve the user is an error, since it leaves path untouched. +// Per-entry walk failures below path stay best-effort and are only logged. +func ChownRecursive(path, username string) error { u, err := user.Lookup(username) if err != nil { - slog.Warn("ChownRecursive: user lookup failed, skipping", "user", username, "path", path, "err", err) - return + return fmt.Errorf("chown %s: user %q not found: %w", path, username, err) } uid, err := strconv.Atoi(u.Uid) if err != nil { - slog.Warn("ChownRecursive: invalid UID, skipping", "user", username, "uid", u.Uid, "err", err) - return + return fmt.Errorf("chown %s: invalid UID %q for user %q: %w", path, u.Uid, username, err) } gid, err := strconv.Atoi(u.Gid) if err != nil { - slog.Warn("ChownRecursive: invalid GID, skipping", "user", username, "gid", u.Gid, "err", err) - return + return fmt.Errorf("chown %s: invalid GID %q for user %q: %w", path, u.Gid, username, err) } // Use os.Root to scope filesystem operations and avoid symlink TOCTOU races. @@ -380,7 +379,7 @@ func ChownRecursive(path, username string) { if chownErr := os.Lchown(path, uid, gid); chownErr != nil { slog.Warn("chown failed", "path", path, "err", chownErr) } - return + return nil } defer root.Close() @@ -395,43 +394,61 @@ func ChownRecursive(path, username string) { } return nil }) + return nil +} + +// servicePaths maps each per-service data/config directory to the system user +// that must own it. Kept separate from SetServiceOwnership so the aggregation +// logic can be tested against a small map rather than these absolute paths. +var servicePaths = map[string]string{ + "/etc/spinifex/nats": "spinifex-nats", + "/var/lib/spinifex/nats": "spinifex-nats", + "/etc/spinifex/predastore": "spinifex-storage", + "/var/lib/spinifex/predastore": "spinifex-storage", + "/etc/spinifex/northstar": "spinifex-northstar", + "/var/lib/spinifex/northstar": "spinifex-northstar", + "/etc/spinifex/viperblock": "spinifex-viperblock", + "/var/lib/spinifex/spinifex": "spinifex-daemon", + "/var/lib/spinifex/viperblock": "spinifex-viperblock", + "/var/lib/spinifex/vpcd": "spinifex-vpcd", + "/var/lib/spinifex/awsgw": "spinifex-gw", +} + +// chownServicePaths applies ChownRecursive to each path in services that +// exists on disk, collecting every failure instead of stopping at the +// first so one missing service user doesn't mask problems with the rest. +func chownServicePaths(services map[string]string) error { + var errs []error + for path, u := range services { + if _, err := os.Stat(path); err != nil { + continue + } + if err := ChownRecursive(path, u); err != nil { + errs = append(errs, err) + } + } + return errors.Join(errs...) } // SetServiceOwnership sets per-service ownership on data/config directories -// and shared config files to root:spinifex with correct modes. -func SetServiceOwnership() { +// and shared config files to root:spinifex with correct modes. Returns an error +// naming every service path whose ownership could not be applied. +func SetServiceOwnership() error { grp, err := user.LookupGroup("spinifex") if err != nil { slog.Error("SetServiceOwnership: spinifex group not found, skipping all ownership changes", "err", err) fmt.Fprintln(os.Stderr, "WARNING: spinifex group not found — service ownership not set. Run setup.sh first or create the group manually.") - return + return fmt.Errorf("spinifex group not found: %w", err) } gid, err := strconv.Atoi(grp.Gid) if err != nil { slog.Error("SetServiceOwnership: invalid spinifex group GID", "gid", grp.Gid, "err", err) fmt.Fprintln(os.Stderr, "WARNING: invalid spinifex group GID — service ownership not set.") - return + return fmt.Errorf("invalid spinifex group GID %q: %w", grp.Gid, err) } // Per-service directory trees - for path, u := range map[string]string{ - "/etc/spinifex/nats": "spinifex-nats", - "/var/lib/spinifex/nats": "spinifex-nats", - "/etc/spinifex/predastore": "spinifex-storage", - "/var/lib/spinifex/predastore": "spinifex-storage", - "/etc/spinifex/northstar": "spinifex-northstar", - "/var/lib/spinifex/northstar": "spinifex-northstar", - "/etc/spinifex/viperblock": "spinifex-viperblock", - "/var/lib/spinifex/spinifex": "spinifex-daemon", - "/var/lib/spinifex/viperblock": "spinifex-viperblock", - "/var/lib/spinifex/vpcd": "spinifex-vpcd", - "/var/lib/spinifex/awsgw": "spinifex-gw", - } { - if _, err := os.Stat(path); err != nil { - continue - } - ChownRecursive(path, u) - } + ownershipErr := chownServicePaths(servicePaths) // Shared data directories — root:spinifex 0770 so daemon + admin CLI can write for _, dir := range []string{ @@ -474,6 +491,8 @@ func SetServiceOwnership() { slog.Warn("SetServiceOwnership: chmod failed", "path", path, "err", err) } } + + return ownershipErr } // UpdateAWSINIFile updates or creates an AWS INI file section with the given key-value pairs. @@ -915,9 +934,13 @@ func SetupAWSCredentials(accessKey, secretKey, region, certPath, bindIP string) return err } - // Fix ownership so the sudo invoking user can read the files + // Fix ownership so the sudo invoking user can read the files. This is + // cosmetic convenience, not a service-critical permission, so a failure + // here is logged and never propagated to fail credential setup. if os.Getuid() == 0 && sudoUser != "" { - ChownRecursive(awsDir, sudoUser) + if err := ChownRecursive(awsDir, sudoUser); err != nil { + slog.Warn("failed to fix AWS config ownership for sudo user", "user", sudoUser, "path", awsDir, "err", err) + } } fmt.Printf(" Profile: %s\n", profileName) diff --git a/spinifex/admin/admin_test.go b/spinifex/admin/admin_test.go index 3b1ec7ea3..2533904d2 100644 --- a/spinifex/admin/admin_test.go +++ b/spinifex/admin/admin_test.go @@ -990,17 +990,20 @@ secretkey = "{{.SecretKey}}" } func TestChownRecursive_InvalidUser(t *testing.T) { - // ChownRecursive should silently return on invalid username + // ChownRecursive must report a non-nil error naming the user when the + // user cannot be resolved, rather than silently doing nothing. tmpDir := t.TempDir() testFile := filepath.Join(tmpDir, "test.txt") os.WriteFile(testFile, []byte("test"), 0644) - // Should not panic or error with a non-existent user - ChownRecursive(tmpDir, "nonexistent-user-that-does-not-exist-12345") + const badUser = "nonexistent-user-that-does-not-exist-12345" + err := ChownRecursive(tmpDir, badUser) + require.Error(t, err) + assert.Contains(t, err.Error(), badUser) // File should still exist and be readable - _, err := os.ReadFile(testFile) - assert.NoError(t, err) + _, readErr := os.ReadFile(testFile) + assert.NoError(t, readErr) } func TestChownRecursive_CurrentUser(t *testing.T) { @@ -1016,7 +1019,7 @@ func TestChownRecursive_CurrentUser(t *testing.T) { t.Skip("USER env not set") } - ChownRecursive(tmpDir, currentUser) + assert.NoError(t, ChownRecursive(tmpDir, currentUser)) // Verify files are still accessible _, err := os.ReadFile(testFile) @@ -1024,12 +1027,48 @@ func TestChownRecursive_CurrentUser(t *testing.T) { } func TestChownRecursive_NonExistentPath(t *testing.T) { - // Should not panic on a path that doesn't exist + // Should not panic on a path that doesn't exist; OpenRoot fails and the + // fallback Lchown fails too, but both are logged, not returned — the + // user itself resolved fine, so this is not one of the abort cases. currentUser := os.Getenv("USER") if currentUser == "" { t.Skip("USER env not set") } - ChownRecursive("/tmp/nonexistent-path-12345", currentUser) + assert.NoError(t, ChownRecursive("/tmp/nonexistent-path-12345", currentUser)) +} + +func TestChownServicePaths_MissingUser(t *testing.T) { + // chownServicePaths is the map-walking core extracted from + // SetServiceOwnership so it can be exercised against a small map + // instead of the hardcoded /etc/spinifex and /var/lib/spinifex paths, + // which are not writable/creatable without root in CI. + tmpDir := t.TempDir() + require.NoError(t, os.MkdirAll(tmpDir, 0755)) + + const badUser = "nonexistent-user-that-does-not-exist-12345" + err := chownServicePaths(map[string]string{tmpDir: badUser}) + require.Error(t, err) + assert.Contains(t, err.Error(), badUser) + assert.Contains(t, err.Error(), tmpDir) +} + +func TestChownServicePaths_SkipsMissingPath(t *testing.T) { + // A path that doesn't exist on disk is never attempted, so it can't + // produce a "user not found" failure even for a bogus user. + err := chownServicePaths(map[string]string{ + "/nonexistent-path-12345": "nonexistent-user-that-does-not-exist-12345", + }) + assert.NoError(t, err) +} + +func TestSetServiceOwnership_RequiresRoot(t *testing.T) { + // SetServiceOwnership operates on hardcoded /etc/spinifex and + // /var/lib/spinifex paths and chowns to the spinifex group, both of + // which require root. The per-service failure-aggregation logic itself + // is covered without root via TestChownServicePaths_MissingUser above. + if os.Geteuid() != 0 { + t.Skip("SetServiceOwnership requires root") + } } // --- SetGPUPassthrough --- diff --git a/spinifex/handlers/ec2/image/service_impl.go b/spinifex/handlers/ec2/image/service_impl.go index 7c84d8f9e..0421a0282 100644 --- a/spinifex/handlers/ec2/image/service_impl.go +++ b/spinifex/handlers/ec2/image/service_impl.go @@ -135,6 +135,11 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri var images []*ec2.Image encryptedAtRest := s.clusterEncryptionEnabled() + // Counting prefixes seen against those dropped for an unreadable config.json + // makes a mismatch with len(images) visible at INFO, so an empty result is + // distinguishable from one where every config failed to load. + var prefixesExamined, unreadableConfigs int + // Iterate over CommonPrefixes to find ami-* directories for _, prefix := range result.CommonPrefixes { if prefix.Prefix == nil { @@ -148,6 +153,7 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri if !strings.HasPrefix(prefixStr, "ami-") { continue } + prefixesExamined++ // Early skip: if image-id filter is set, check the prefix (ami-xxx/) // against filter values before fetching config from S3. @@ -168,19 +174,25 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri }) if err != nil { if objectstore.IsNoSuchKeyError(err) { + // No config.json means this is a legitimate non-AMI directory, + // not a fault, so it stays at DEBUG and isn't counted below. slog.DebugContext(ctx, "Config file not found", "key", configKey) } else { - slog.DebugContext(ctx, "Failed to get config file", "key", configKey, "err", err) + unreadableConfigs++ + slog.WarnContext(ctx, "Failed to get config file", "key", configKey, "err", err) } continue } body, err := io.ReadAll(getResult.Body) - if err := getResult.Body.Close(); err != nil { - slog.DebugContext(ctx, "Failed to close config body", "key", configKey, "err", err) + // A close failure alone doesn't drop the prefix (read below may still + // succeed), so it's only warned about, not counted as unreadable. + if closeErr := getResult.Body.Close(); closeErr != nil { + slog.WarnContext(ctx, "Failed to close config body", "key", configKey, "err", closeErr) } if err != nil { - slog.DebugContext(ctx, "Failed to read config body", "key", configKey, "err", err) + unreadableConfigs++ + slog.WarnContext(ctx, "Failed to read config body", "key", configKey, "err", err) continue } @@ -191,7 +203,8 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri VolumeConfig viperblock.VolumeConfig `json:"VolumeConfig"` } if err := json.Unmarshal(viperblock.StateBody(body), &vbConfig); err != nil { - slog.DebugContext(ctx, "Failed to unmarshal config", "key", configKey, "err", err) + unreadableConfigs++ + slog.WarnContext(ctx, "Failed to unmarshal config", "key", configKey, "err", err) continue } @@ -315,7 +328,8 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri } } - slog.InfoContext(ctx, "DescribeImages completed", "count", len(images)) + slog.InfoContext(ctx, "DescribeImages completed", "count", len(images), + "prefixesExamined", prefixesExamined, "unreadableConfigs", unreadableConfigs) return &ec2.DescribeImagesOutput{ Images: images, diff --git a/spinifex/handlers/ec2/image/service_impl_test.go b/spinifex/handlers/ec2/image/service_impl_test.go index 0d7bdd8c7..dfbea07bc 100644 --- a/spinifex/handlers/ec2/image/service_impl_test.go +++ b/spinifex/handlers/ec2/image/service_impl_test.go @@ -3,7 +3,9 @@ package handlers_ec2_image import ( "context" "encoding/json" + "fmt" "io" + "log/slog" "net/http" "net/http/httptest" "strings" @@ -735,6 +737,90 @@ func TestDescribeImages_InvalidConfigJSON(t *testing.T) { assert.Empty(t, result.Images) } +// TestDescribeImages_ConfigFaultCounting covers mulga-7r6pm: a config.json that +// exists but can't be fetched/read/parsed must be promoted to WARN and counted +// as unreadable so it stays visible at INFO, while a genuinely absent +// config.json (a non-AMI directory) stays silent at DEBUG and uncounted, and a +// healthy prefix is unaffected either way. +func TestDescribeImages_ConfigFaultCounting(t *testing.T) { + tests := []struct { + name string + setup func(t *testing.T, store *objectstore.MemoryObjectStore) + wantImageCount int + wantUnreadable int + wantWarnLogged bool + }{ + { + name: "malformed config.json is counted as unreadable and does not vanish silently", + setup: func(t *testing.T, store *objectstore.MemoryObjectStore) { + _, err := store.PutObject(context.Background(), &awss3.PutObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String("ami-malformed1/config.json"), + Body: strings.NewReader("{not valid json"), + }) + require.NoError(t, err) + }, + wantImageCount: 0, + wantUnreadable: 1, + wantWarnLogged: true, + }, + { + name: "missing config.json (NoSuchKey) is not counted as a fault", + setup: func(t *testing.T, store *objectstore.MemoryObjectStore) { + // A placeholder object under the prefix makes ListObjectsV2 surface + // "ami-nokey1/" as a CommonPrefix with no config.json underneath. + _, err := store.PutObject(context.Background(), &awss3.PutObjectInput{ + Bucket: aws.String(testBucket), + Key: aws.String("ami-nokey1/placeholder.txt"), + Body: strings.NewReader("x"), + }) + require.NoError(t, err) + }, + wantImageCount: 0, + wantUnreadable: 0, + wantWarnLogged: false, + }, + { + name: "healthy prefix still yields its image", + setup: func(t *testing.T, store *objectstore.MemoryObjectStore) { + createTestAMIConfigWithOwner(t, store, "ami-healthy1", "healthy-ami", testAccountID) + }, + wantImageCount: 1, + wantUnreadable: 0, + wantWarnLogged: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf strings.Builder + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelInfo}))) + t.Cleanup(func() { slog.SetDefault(prev) }) + + svc, store := setupTestImageService(t) + tt.setup(t, store) + + out, err := svc.DescribeImages(context.Background(), &ec2.DescribeImagesInput{}, testAccountID) + require.NoError(t, err) + require.NotNil(t, out) + assert.Len(t, out.Images, tt.wantImageCount) + + logs := buf.String() + assert.Contains(t, logs, "prefixesExamined=1", "the single ami-* prefix must be counted as examined") + assert.Contains(t, logs, fmt.Sprintf("unreadableConfigs=%d", tt.wantUnreadable)) + + if tt.wantWarnLogged { + assert.Contains(t, logs, "level=WARN", + "a config fetch/read/parse fault must be visible at WARN without raising the daemon's default level") + } else { + assert.NotContains(t, logs, "level=WARN", + "a legitimate skip (missing config or healthy prefix) must not be logged as a fault") + } + }) + } +} + func TestDescribeImages_EmptyImageIDSkipped(t *testing.T) { svc, store := setupTestImageService(t)