diff --git a/cmd/cli/cmd/llm/benchmark/dashboard/handlers.go b/cmd/cli/cmd/llm/benchmark/dashboard/handlers.go index 1f5cd31e..44cf5c15 100644 --- a/cmd/cli/cmd/llm/benchmark/dashboard/handlers.go +++ b/cmd/cli/cmd/llm/benchmark/dashboard/handlers.go @@ -42,13 +42,17 @@ type Server struct { } // NewServer creates a new HTTP server instance. -func NewServer(dataDir string) *Server { +func NewServer(dataDir string) (*Server, error) { + baseAbs, err := filepath.Abs(dataDir) + if err != nil { + return nil, err + } s := &Server{ - dataDir: dataDir, + dataDir: baseAbs, mux: http.NewServeMux(), } s.registerRoutes() - return s + return s, nil } // registerRoutes sets up all HTTP routes. @@ -101,12 +105,6 @@ func (s *Server) handleExperimentRoutes(w http.ResponseWriter, r *http.Request) experimentName := parts[0] - // Validate experiment name to prevent path traversal - if !isValidName(experimentName) { - s.writeError(w, http.StatusBadRequest, "Invalid experiment name") - return - } - if len(parts) == 1 { // GET /api/experiments/{name} s.handleGetExperiment(w, r, experimentName) @@ -177,9 +175,12 @@ func (s *Server) handleGetResult(w http.ResponseWriter, r *http.Request, experim return } - // Validate inputs to prevent path traversal - if !isValidName(experimentName) || !isValidName(fileName) { - s.writeError(w, http.StatusBadRequest, "Invalid experiment name or filename") + if strings.Contains(experimentName, "/") || strings.Contains(experimentName, "\\") || strings.Contains(experimentName, "..") { + s.writeError(w, http.StatusBadRequest, "Invalid experiment name") + return + } + if strings.Contains(fileName, "/") || strings.Contains(fileName, "\\") || strings.Contains(fileName, "..") { + s.writeError(w, http.StatusBadRequest, "Invalid result filename") return } @@ -194,10 +195,17 @@ func (s *Server) handleGetResult(w http.ResponseWriter, r *http.Request, experim return } - filePath := filepath.Join(s.dataDir, experimentName, fileName) + // Local safety check: validate path is within dataDir + // This provides explicit validation that static analysis tools can recognize + expDir, ok := isSafeRelativePath(s.dataDir, experimentName) + if !ok { + s.writeError(w, http.StatusForbidden, "Access denied") + return + } - // Security check: ensure the path is within dataDir - if !s.isPathSafe(filePath) { + // Validate fileName is safe and construct the final file path + filePath, ok := isSafeRelativePath(expDir, fileName) + if !ok { s.writeError(w, http.StatusForbidden, "Access denied") return } @@ -230,10 +238,12 @@ func (s *Server) handleGetImage(w http.ResponseWriter, r *http.Request, experime s.writeError(w, http.StatusMethodNotAllowed, "Method not allowed") return } - - // Validate inputs to prevent path traversal - if !isValidName(experimentName) || !isValidName(fileName) { - s.writeError(w, http.StatusBadRequest, "Invalid experiment name or filename") + if strings.Contains(experimentName, "/") || strings.Contains(experimentName, "\\") || strings.Contains(experimentName, "..") { + s.writeError(w, http.StatusBadRequest, "Invalid experiment name") + return + } + if strings.Contains(fileName, "/") || strings.Contains(fileName, "\\") || strings.Contains(fileName, "..") { + s.writeError(w, http.StatusBadRequest, "Invalid result filename") return } @@ -247,10 +257,17 @@ func (s *Server) handleGetImage(w http.ResponseWriter, r *http.Request, experime return } - filePath := filepath.Join(s.dataDir, experimentName, fileName) + // Local safety check: validate path is within dataDir + // This provides explicit validation that static analysis tools can recognize + expDir, ok := isSafeRelativePath(s.dataDir, experimentName) + if !ok { + s.writeError(w, http.StatusForbidden, "Access denied") + return + } - // Security check - if !s.isPathSafe(filePath) { + // Validate fileName is safe and construct the final file path + filePath, ok := isSafeRelativePath(expDir, fileName) + if !ok { s.writeError(w, http.StatusForbidden, "Access denied") return } @@ -401,18 +418,35 @@ func (s *Server) listExperiments() ([]Experiment, error) { // getExperiment returns details of a specific experiment. func (s *Server) getExperiment(name string) (*Experiment, error) { - // Validate name to prevent path traversal - if !isValidName(name) { + if strings.Contains(name, "/") || strings.Contains(name, "\\") || strings.Contains(name, "..") { return nil, os.ErrNotExist } - - expDir := filepath.Join(s.dataDir, name) - - // Security check: ensure the path is within dataDir - if !s.isPathSafe(expDir) { + // Resolve symlinks for both the base directory and the experiment directory + // before checking containment, so a symlink under s.dataDir cannot escape it. + baseDir, err := filepath.EvalSymlinks(s.dataDir) + if err != nil { + return nil, err + } + baseDir, err = filepath.Abs(baseDir) + if err != nil { + return nil, err + } + expDirJoined := filepath.Join(s.dataDir, name) + expDir, err := filepath.EvalSymlinks(expDirJoined) + if err != nil { + return nil, err + } + expDir, err = filepath.Abs(expDir) + if err != nil { + return nil, err + } + relPath, err := filepath.Rel(baseDir, expDir) + if err != nil { + return nil, err + } + if relPath == ".." || strings.HasPrefix(relPath, ".."+string(os.PathSeparator)) { return nil, os.ErrNotExist } - info, err := os.Stat(expDir) if err != nil { return nil, err @@ -460,15 +494,14 @@ func (s *Server) getExperiment(name string) (*Experiment, error) { // getResultSummaries returns summaries of all result files in an experiment. func (s *Server) getResultSummaries(experimentName string) ([]ResultSummary, error) { - // Validate name to prevent path traversal - if !isValidName(experimentName) { + if strings.Contains(experimentName, "/") || strings.Contains(experimentName, "\\") || strings.Contains(experimentName, "..") { return nil, os.ErrNotExist } - expDir := filepath.Join(s.dataDir, experimentName) - - // Security check: ensure the path is within dataDir - if !s.isPathSafe(expDir) { + // Local safety check: validate path is within dataDir + // This provides explicit validation that static analysis tools can recognize + expDir, ok := isSafeRelativePath(s.dataDir, experimentName) + if !ok { return nil, os.ErrNotExist } @@ -551,64 +584,58 @@ func isValidName(name string) bool { return true } -// isPathSafe checks if the given path is within the data directory. -// It resolves symlinks before checking containment to prevent symlink-based path traversal attacks. -func (s *Server) isPathSafe(path string) bool { - // Clean the path to resolve .. and . segments - cleanPath := filepath.Clean(path) - - // Get absolute paths - absPath, err := filepath.Abs(cleanPath) - if err != nil { - return false +// isSafeRelativePath validates that subpath is safe relative to baseDir and returns the absolute path. +// It checks that the joined and normalized path is still within baseDir. +// This provides a local, explicit safety check that static analysis tools can recognize. +func isSafeRelativePath(absBaseDir, subpath string) (string, bool) { + // First validate the subpath contains only safe characters + if !isValidName(subpath) { + return "", false } - absDataDir, err := filepath.Abs(s.dataDir) + + // Join and clean the path + joined := filepath.Join(absBaseDir, subpath) + absPath, err := filepath.Abs(joined) if err != nil { - return false + return "", false } - // Resolve the data directory (must exist) - resolvedDataDir, err := filepath.EvalSymlinks(absDataDir) + // Resolve symlinks for both base dir and target path + // This prevents symlink-based path traversal attacks + evalBaseDir, err := filepath.EvalSymlinks(absBaseDir) if err != nil { - return false + return "", false } - dataDirPrefix := resolvedDataDir + string(filepath.Separator) - // Check if the path exists - _, err = os.Stat(absPath) - if err == nil { - // Path exists - fully resolve it and check containment - resolvedPath, err := filepath.EvalSymlinks(absPath) + // For the target path, we need to handle the case where it may not exist yet + // Try to evaluate symlinks, but if the path doesn't exist, we'll validate + // against the parent directory + evalPath, err := filepath.EvalSymlinks(absPath) + if err != nil { + // Path doesn't exist - validate against base dir + // Use filepath.Rel to ensure the path is relative to base dir + relPath, err := filepath.Rel(absBaseDir, absPath) if err != nil { - return false + return "", false } - return strings.HasPrefix(resolvedPath+string(filepath.Separator), dataDirPrefix) - } - - // Path doesn't exist - check that the nearest existing ancestor is within dataDir - // We can only verify the safety of existing directories; non-existent portions - // will be validated when actually created - current := filepath.Dir(absPath) - for { - // Check if current directory exists - _, err := os.Stat(current) - if err == nil { - // Found an existing ancestor - resolve it and verify it's within dataDir - resolvedCurrent, err := filepath.EvalSymlinks(current) - if err != nil { - return false - } - return strings.HasPrefix(resolvedCurrent+string(filepath.Separator), dataDirPrefix) + // Check if relative path escapes base dir + if strings.HasPrefix(relPath, "..") || relPath == ".." { + return "", false } + return absPath, true + } - // Current doesn't exist, go to parent - parent := filepath.Dir(current) - if parent == current { - // Reached root without finding any existing directory - return false - } - current = parent + // Path exists - verify it's within the resolved base dir + relPath, err := filepath.Rel(evalBaseDir, evalPath) + if err != nil { + return "", false + } + // Check if relative path escapes base dir + if strings.HasPrefix(relPath, "..") || relPath == ".." { + return "", false } + + return evalPath, true } // writeJSON writes a JSON response. diff --git a/cmd/cli/cmd/llm/benchmark/dashboard/handlers_test.go b/cmd/cli/cmd/llm/benchmark/dashboard/handlers_test.go index 2e7a705d..9c462fb1 100644 --- a/cmd/cli/cmd/llm/benchmark/dashboard/handlers_test.go +++ b/cmd/cli/cmd/llm/benchmark/dashboard/handlers_test.go @@ -22,22 +22,57 @@ import ( "testing" ) -// TestIsPathSafe_SymlinkTraversal tests that symlinks pointing outside the data directory -// are properly rejected, preventing symlink-based path traversal attacks. -func TestIsPathSafe_SymlinkTraversal(t *testing.T) { - // Create a temporary directory structure: - // /tmp/xxx/data/ (data directory) - // /tmp/xxx/outside/ (directory outside data) - // /tmp/xxx/outside/secret.txt (sensitive file) - // /tmp/xxx/data/link -> /tmp/xxx/outside/secret.txt (symlink to outside) +// TestIsSafeRelativePath_ValidNames tests that valid names are accepted. +func TestIsSafeRelativePath_ValidNames(t *testing.T) { + tempDir := t.TempDir() + dataDir := filepath.Join(tempDir, "data") + if err := os.MkdirAll(dataDir, 0755); err != nil { + t.Fatalf("failed to create data dir: %v", err) + } + + // Create server to get absolute dataDir + server, err := NewServer(dataDir) + if err != nil { + t.Fatalf("Failed to create server: %v", err) + } + + tests := []struct { + name string + subpath string + wantOk bool + }{ + {"simple name", "experiment1", true}, + {"name with hyphen", "my-experiment", true}, + {"name with underscore", "my_experiment", true}, + {"name with dots", "experiment.v1.0", true}, + {"alphanumeric", "exp123", true}, + {"empty string", "", false}, + {"single dot", ".", false}, + {"double dot", "..", false}, + {"path traversal", "../outside", false}, + {"absolute path", "/etc/passwd", false}, + {"path separator", "subdir/file", false}, + {"backslash", "subdir\\file", false}, + {"parent reference", "exp/../file", false}, + {"special chars", "exp@#$%", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, ok := isSafeRelativePath(server.dataDir, tt.subpath) + if ok != tt.wantOk { + t.Errorf("isSafeRelativePath(%q) = %v, want %v", tt.subpath, ok, tt.wantOk) + } + }) + } +} +// TestIsSafeRelativePath_PathTraversal tests that path traversal attempts are rejected. +func TestIsSafeRelativePath_PathTraversal(t *testing.T) { tempDir := t.TempDir() dataDir := filepath.Join(tempDir, "data") outsideDir := filepath.Join(tempDir, "outside") - secretFile := filepath.Join(outsideDir, "secret.txt") - symlinkPath := filepath.Join(dataDir, "link") - // Create directories if err := os.MkdirAll(dataDir, 0755); err != nil { t.Fatalf("failed to create data dir: %v", err) } @@ -45,61 +80,37 @@ func TestIsPathSafe_SymlinkTraversal(t *testing.T) { t.Fatalf("failed to create outside dir: %v", err) } - // Create a secret file outside the data directory - if err := os.WriteFile(secretFile, []byte("sensitive data"), 0644); err != nil { - t.Fatalf("failed to create secret file: %v", err) - } - - // Create a symlink inside dataDir pointing to the file outside - if err := os.Symlink(secretFile, symlinkPath); err != nil { - t.Fatalf("failed to create symlink: %v", err) - } - - server := NewServer(dataDir) - - // Test 1: The symlink itself should be detected as unsafe - // because it resolves to a path outside the data directory - if server.isPathSafe(symlinkPath) { - t.Errorf("isPathSafe(%q) = true, want false - symlink pointing outside data dir should be rejected", symlinkPath) + server, err := NewServer(dataDir) + if err != nil { + t.Fatalf("Failed to create server: %v", err) } - // Test 2: A normal file inside the data directory should be safe - normalFile := filepath.Join(dataDir, "normal.txt") - if err := os.WriteFile(normalFile, []byte("normal data"), 0644); err != nil { - t.Fatalf("failed to create normal file: %v", err) - } - if !server.isPathSafe(normalFile) { - t.Errorf("isPathSafe(%q) = false, want true - normal file inside data dir should be safe", normalFile) + // Create a secret file outside data directory + secretFile := filepath.Join(outsideDir, "secret.txt") + if err := os.WriteFile(secretFile, []byte("sensitive"), 0644); err != nil { + t.Fatalf("failed to create secret file: %v", err) } - // Test 3: A symlink pointing to another file inside dataDir should be safe - insideTarget := filepath.Join(dataDir, "target.txt") - if err := os.WriteFile(insideTarget, []byte("target data"), 0644); err != nil { - t.Fatalf("failed to create inside target file: %v", err) - } - insideSymlink := filepath.Join(dataDir, "inside_link") - if err := os.Symlink(insideTarget, insideSymlink); err != nil { - t.Fatalf("failed to create inside symlink: %v", err) - } - if !server.isPathSafe(insideSymlink) { - t.Errorf("isPathSafe(%q) = false, want true - symlink pointing inside data dir should be safe", insideSymlink) + // Path traversal attempts should all be rejected + traversalAttempts := []string{ + ".." + string(filepath.Separator) + "outside", + "..", + "../outside/secret.txt", } - // Test 4: Path traversal using .. should still be rejected - traversalPath := filepath.Join(dataDir, "..", "outside", "secret.txt") - if server.isPathSafe(traversalPath) { - t.Errorf("isPathSafe(%q) = true, want false - path traversal should be rejected", traversalPath) + for _, attempt := range traversalAttempts { + _, ok := isSafeRelativePath(server.dataDir, attempt) + if ok { + t.Errorf("isSafeRelativePath(%q) = true, want false - path traversal should be rejected", attempt) + } } - // Test 5: Absolute path outside data dir should be rejected - if server.isPathSafe(secretFile) { - t.Errorf("isPathSafe(%q) = true, want false - absolute path outside data dir should be rejected", secretFile) - } + // Note: "...." is actually a valid filename (just dots), not a path traversal + // filepath.Join(dataDir, "....") would create a file named "...." inside dataDir } -// TestIsPathSafe_NonExistentPath tests that non-existent paths are handled correctly -// by checking their parent directory's safety. -func TestIsPathSafe_NonExistentPath(t *testing.T) { +// TestIsSafeRelativePath_SymlinkTraversal tests that symlink-based path traversal is rejected. +func TestIsSafeRelativePath_SymlinkTraversal(t *testing.T) { tempDir := t.TempDir() dataDir := filepath.Join(tempDir, "data") outsideDir := filepath.Join(tempDir, "outside") @@ -111,120 +122,63 @@ func TestIsPathSafe_NonExistentPath(t *testing.T) { t.Fatalf("failed to create outside dir: %v", err) } - server := NewServer(dataDir) - - // Test: Non-existent file inside data dir should be safe (parent is safe) - nonExistentInside := filepath.Join(dataDir, "newdir", "newfile.txt") - if !server.isPathSafe(nonExistentInside) { - t.Errorf("isPathSafe(%q) = false, want true - non-existent path inside data dir should be safe", nonExistentInside) + server, err := NewServer(dataDir) + if err != nil { + t.Fatalf("Failed to create server: %v", err) } - // Test: Non-existent file outside data dir should be rejected - nonExistentOutside := filepath.Join(outsideDir, "newfile.txt") - if server.isPathSafe(nonExistentOutside) { - t.Errorf("isPathSafe(%q) = true, want false - non-existent path outside data dir should be rejected", nonExistentOutside) + // Create a secret file outside data directory + secretFile := filepath.Join(outsideDir, "secret.txt") + if err := os.WriteFile(secretFile, []byte("sensitive"), 0644); err != nil { + t.Fatalf("failed to create secret file: %v", err) } -} -// TestIsPathSafe_SymlinkToDirectory tests symlinks that point to directories. -func TestIsPathSafe_SymlinkToDirectory(t *testing.T) { - tempDir := t.TempDir() - dataDir := filepath.Join(tempDir, "data") - outsideDir := filepath.Join(tempDir, "outside") - insideSubdir := filepath.Join(dataDir, "subdir") - - if err := os.MkdirAll(insideSubdir, 0755); err != nil { - t.Fatalf("failed to create inside subdir: %v", err) - } - if err := os.MkdirAll(outsideDir, 0755); err != nil { - t.Fatalf("failed to create outside dir: %v", err) + // Create a symlink inside dataDir that points outside + symlinkPath := filepath.Join(dataDir, "evil-symlink") + if err := os.Symlink(outsideDir, symlinkPath); err != nil { + t.Fatalf("failed to create symlink: %v", err) } - server := NewServer(dataDir) - - // Create a symlink inside data pointing to outside directory - symlinkToOutside := filepath.Join(dataDir, "link_to_outside") - if err := os.Symlink(outsideDir, symlinkToOutside); err != nil { - t.Fatalf("failed to create symlink to outside dir: %v", err) + // Attempt to access files through symlink should be rejected + _, ok := isSafeRelativePath(server.dataDir, "evil-symlink") + if ok { + t.Errorf("isSafeRelativePath(%q) = true, want false - symlink to outside dir should be rejected", "evil-symlink") } - // This should be rejected - if server.isPathSafe(symlinkToOutside) { - t.Errorf("isPathSafe(%q) = true, want false - symlink to outside directory should be rejected", symlinkToOutside) + // Attempt to access secret file through symlink should be rejected + _, ok = isSafeRelativePath(server.dataDir, "evil-symlink/secret.txt") + if ok { + t.Errorf("isSafeRelativePath(%q) = true, want false - symlink path traversal should be rejected", "evil-symlink/secret.txt") } - // Create a symlink inside data pointing to another directory inside data - symlinkToInside := filepath.Join(dataDir, "link_to_inside") - if err := os.Symlink(insideSubdir, symlinkToInside); err != nil { - t.Fatalf("failed to create symlink to inside dir: %v", err) + // Create a valid experiment directory with a symlink inside + expDir := filepath.Join(dataDir, "exp1") + if err := os.MkdirAll(expDir, 0755); err != nil { + t.Fatalf("failed to create experiment dir: %v", err) } - // This should be allowed - if !server.isPathSafe(symlinkToInside) { - t.Errorf("isPathSafe(%q) = false, want true - symlink to inside directory should be safe", symlinkToInside) + // Valid experiment directory should be accepted + _, ok = isSafeRelativePath(server.dataDir, "exp1") + if !ok { + t.Errorf("isSafeRelativePath(%q) = false, want true - valid experiment dir should be accepted", "exp1") } } -// TestIsPathSafe_MultiLevelSymlink tests nested/chained symlinks. -// Creates: data/link1 -> data/link2 -> outside/secret.txt -func TestIsPathSafe_MultiLevelSymlink(t *testing.T) { +// TestNewServer_AbsolutePath tests that NewServer converts dataDir to absolute path. +func TestNewServer_AbsolutePath(t *testing.T) { tempDir := t.TempDir() dataDir := filepath.Join(tempDir, "data") - outsideDir := filepath.Join(tempDir, "outside") - secretFile := filepath.Join(outsideDir, "secret.txt") - if err := os.MkdirAll(dataDir, 0755); err != nil { t.Fatalf("failed to create data dir: %v", err) } - if err := os.MkdirAll(outsideDir, 0755); err != nil { - t.Fatalf("failed to create outside dir: %v", err) - } - if err := os.WriteFile(secretFile, []byte("sensitive"), 0644); err != nil { - t.Fatalf("failed to create secret file: %v", err) - } - - server := NewServer(dataDir) - - // Create a chain of symlinks: link2 -> outside, link1 -> link2 - link2 := filepath.Join(dataDir, "link2") - link1 := filepath.Join(dataDir, "link1") - - if err := os.Symlink(secretFile, link2); err != nil { - t.Fatalf("failed to create link2: %v", err) - } - if err := os.Symlink(link2, link1); err != nil { - t.Fatalf("failed to create link1: %v", err) - } - - // Both symlinks should be rejected as they ultimately resolve to outside - if server.isPathSafe(link2) { - t.Errorf("isPathSafe(%q) = true, want false - symlink chain to outside should be rejected", link2) - } - if server.isPathSafe(link1) { - t.Errorf("isPathSafe(%q) = true, want false - nested symlink to outside should be rejected", link1) - } - // Test safe chain: data/link4 -> data/link3 -> data/realfile.txt - realFile := filepath.Join(dataDir, "realfile.txt") - if err := os.WriteFile(realFile, []byte("safe content"), 0644); err != nil { - t.Fatalf("failed to create real file: %v", err) + server, err := NewServer(dataDir) + if err != nil { + t.Fatalf("Failed to create server: %v", err) } - link3 := filepath.Join(dataDir, "link3") - link4 := filepath.Join(dataDir, "link4") - - if err := os.Symlink(realFile, link3); err != nil { - t.Fatalf("failed to create link3: %v", err) - } - if err := os.Symlink(link3, link4); err != nil { - t.Fatalf("failed to create link4: %v", err) - } - - // Safe chain should be allowed - if !server.isPathSafe(link3) { - t.Errorf("isPathSafe(%q) = false, want true - symlink to inside file should be safe", link3) - } - if !server.isPathSafe(link4) { - t.Errorf("isPathSafe(%q) = false, want true - nested symlink staying inside should be safe", link4) + // Server's dataDir should be absolute + if !filepath.IsAbs(server.dataDir) { + t.Errorf("server.dataDir = %q, want absolute path", server.dataDir) } } diff --git a/cmd/cli/cmd/llm/benchmark/dashboard/main.go b/cmd/cli/cmd/llm/benchmark/dashboard/main.go index 1591c180..761e9d7c 100644 --- a/cmd/cli/cmd/llm/benchmark/dashboard/main.go +++ b/cmd/cli/cmd/llm/benchmark/dashboard/main.go @@ -46,7 +46,10 @@ func main() { } // Create server - server := NewServer(dataDir) + server, err := NewServer(dataDir) + if err != nil { + log.Fatalf("Failed to create server: %v", err) + } // Setup HTTP server addr := fmt.Sprintf(":%d", port) diff --git a/pkg/reconciler/roleinstance/core/instance_core.go b/pkg/reconciler/roleinstance/core/instance_core.go index 55d2ea15..a0fe5492 100644 --- a/pkg/reconciler/roleinstance/core/instance_core.go +++ b/pkg/reconciler/roleinstance/core/instance_core.go @@ -23,6 +23,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/klog/v2" kubecontroller "k8s.io/kubernetes/pkg/controller" workloadsv1alpha2 "sigs.k8s.io/rbgs/api/workloads/v1alpha2" @@ -197,7 +198,14 @@ func newComponentPodGroup(component *workloadsv1alpha2.RoleInstanceComponent, po desiredReplicas := *component.Size var existIDs, toDeleteIDs, toScaleIDs = sets.New[int32](), sets.New[int32](), sets.New[int32]() for _, pod := range pods { - existIDs.Insert(instanceutil.GetPodComponentID(pod)) + componentID := instanceutil.GetPodComponentID(pod) + // Skip pods with invalid component IDs (-1 indicates parse failure) + // This prevents malformed labels/names from causing ID collisions + if componentID == -1 { + klog.Warningf("Pod %s has invalid component ID (label or name parsing failed), skipping", pod.Name) + continue + } + existIDs.Insert(componentID) } for id := range existIDs { if id >= desiredReplicas { @@ -205,11 +213,12 @@ func newComponentPodGroup(component *workloadsv1alpha2.RoleInstanceComponent, po } } var toDeletePods []*v1.Pod - if toDeleteIDs.Len() > 0 { - for _, pod := range pods { - if toDeleteIDs.Has(instanceutil.GetPodComponentID(pod)) { - toDeletePods = append(toDeletePods, pod) - } + for _, pod := range pods { + componentID := instanceutil.GetPodComponentID(pod) + // Delete pods with invalid component IDs (malformed labels/names) + // Also delete pods with IDs exceeding desired replicas + if componentID == -1 || toDeleteIDs.Has(componentID) { + toDeletePods = append(toDeletePods, pod) } } for i := 0; i < int(desiredReplicas); i++ { diff --git a/pkg/reconciler/roleinstance/utils/instance_utils.go b/pkg/reconciler/roleinstance/utils/instance_utils.go index 6633da0e..fb629371 100644 --- a/pkg/reconciler/roleinstance/utils/instance_utils.go +++ b/pkg/reconciler/roleinstance/utils/instance_utils.go @@ -168,15 +168,27 @@ func GetPodComponentName(pod *v1.Pod) string { return list[len(list)-2] } +// GetPodComponentID extracts the component ID from pod labels or name. +// Returns -1 if parsing fails, as -1 is not a valid replica index (replicas start from 0). +// This prevents parse errors from masquerading as valid component ID 0. func GetPodComponentID(pod *v1.Pod) int32 { componentId := pod.Labels[constants.ComponentIDLabelKey] if len(componentId) != 0 { - id, _ := strconv.Atoi(componentId) + id, err := strconv.ParseInt(componentId, 10, 32) + if err != nil { + return -1 + } return int32(id) } list := strings.Split(pod.Name, "-") + if len(list) == 0 { + return -1 + } componentId = list[len(list)-1] - id, _ := strconv.Atoi(componentId) + id, err := strconv.ParseInt(componentId, 10, 32) + if err != nil { + return -1 + } return int32(id) }