Merge pull request #29497 from Luap99/v5.8

[v5.8] Backport the quadlet replace fix

pkg/domain/infra/abi/quadlet.go

@@ -172,7 +172,11 @@ func (ic *ContainerEngine) QuadletInstall(ctx context.Context, pathsOrURLs []str
 				baseName := strings.TrimSuffix(filepath.Base(toInstall), filepath.Ext(toInstall))
 				assetFile = "." + baseName + ".app"
 			} else {
-				assetFile = "." + filepath.Base(toInstall) + ".asset"
+				if systemdquadlet.IsExtSupported(toInstall) {
+					assetFile = "." + filepath.Base(toInstall) + ".app"
+				} else {
+					assetFile = "." + filepath.Base(toInstall) + ".asset"
+				}
 			}
 			validateQuadletFile = true
 		}
@@ -335,45 +339,73 @@ func (ic *ContainerEngine) installQuadlet(_ context.Context, path, destName, ins
 		return "", fmt.Errorf("%q is not a supported Quadlet file type", filepath.Ext(finalPath))
 	}
 
-	var osFlags = os.O_CREATE | os.O_WRONLY
+	var destFile *os.File
+	var tempPath string
 
 	if !replace {
-		osFlags |= os.O_EXCL
-	}
-
-	file, err := os.OpenFile(finalPath, osFlags, 0644)
-	if err != nil {
-		if errors.Is(err, fs.ErrExist) && !replace {
-			return "", fmt.Errorf("a Quadlet with name %s already exists, refusing to overwrite", filepath.Base(finalPath))
+		var err error
+		// O_EXCL ensures we fail if the file already exists (avoids TOCTOU race)
+		destFile, err = os.OpenFile(finalPath, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0o644)
+		if err != nil {
+			if errors.Is(err, fs.ErrExist) {
+				return "", fmt.Errorf("a Quadlet with name %s already exists, refusing to overwrite", filepath.Base(finalPath))
+			}
+			return "", fmt.Errorf("unable to open file %s: %w", finalPath, err)
 		}
-		return "", fmt.Errorf("unable to open file %s: %w", filepath.Base(finalPath), err)
+	} else {
+		var err error
+		destFile, err = os.CreateTemp(filepath.Dir(finalPath), ".quadlet-install-*")
+		if err != nil {
+			return "", fmt.Errorf("unable to create temp file: %w", err)
+		}
+		tempPath = destFile.Name()
 	}
-	defer file.Close()
 
-	// Move the file in
+	defer func() {
+		if destFile != nil {
+			destFile.Close()
+		}
+		if tempPath != "" {
+			os.Remove(tempPath)
+		}
+	}()
+
 	srcFile, err := os.Open(path)
 	if err != nil {
 		return "", fmt.Errorf("unable to open file: %w", err)
 	}
 	defer srcFile.Close()
 
-	err = fileutils.ReflinkOrCopy(srcFile, file)
+	err = fileutils.ReflinkOrCopy(srcFile, destFile)
 	if err != nil {
 		return "", fmt.Errorf("unable to copy file from %s to %s: %w", path, finalPath, err)
 	}
 
-	// When we install files using this function, caller of this function can turn off `validateQuadletFile`
-	// when they are installing `non-quadlet` files.
+	// Close before rename to flush writes; nil out to prevent double-close in defer
+	if err := destFile.Close(); err != nil {
+		return "", fmt.Errorf("unable to close file: %w", err)
+	}
+	destFile = nil
+
+	if tempPath != "" {
+		if err := os.Chmod(tempPath, 0o644); err != nil {
+			return "", fmt.Errorf("unable to set permissions on temp file: %w", err)
+		}
+
+		if err := os.Rename(tempPath, finalPath); err != nil {
+			return "", fmt.Errorf("unable to rename temp file to %s: %w", finalPath, err)
+		}
+		tempPath = ""
+	}
+
 	if !isQuadletFile {
-		err := appendStringToFile(filepath.Join(installDir, assetFile), filepath.Base(filepath.Clean(path)))
+		err := appendLineToFile(filepath.Join(installDir, assetFile), filepath.Base(filepath.Clean(path)))
 		if err != nil {
 			return "", fmt.Errorf("error while writing non-quadlet filename: %w", err)
 		}
 	} else if strings.HasSuffix(assetFile, ".app") {
-		// For quadlet files that are part of an application (indicated by .app extension),
-		// also write the quadlet filename to the .app file for proper application tracking
 		quadletName := filepath.Base(finalPath)
-		err := appendStringToFile(filepath.Join(installDir, assetFile), quadletName)
+		err := appendLineToFile(filepath.Join(installDir, assetFile), quadletName)
 		if err != nil {
 			return "", fmt.Errorf("error while writing quadlet filename to app file: %w", err)
 		}
@@ -381,17 +413,28 @@ func (ic *ContainerEngine) installQuadlet(_ context.Context, path, destName, ins
 	return finalPath, nil
 }
 
-// appendStringToFile appends the given text to the specified file.
-// If the file does not exist, it will be created with 0644 permissions.
-func appendStringToFile(filePath, text string) error {
-	f, err := os.OpenFile(filePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644)
+// appendLineToFile appends the given text as a line to the specified file,
+// ensuring it does not already exist (idempotency).
+func appendLineToFile(path, text string) error {
+	content, err := os.ReadFile(path)
+	if err == nil {
+		for _, line := range strings.Split(string(content), "\n") {
+			if line == text {
+				return nil // Already exists, do nothing
+			}
+		}
+	}
+
+	f, err := os.OpenFile(path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644)
 	if err != nil {
 		return err
 	}
 	defer f.Close()
 
-	_, err = f.WriteString(text + "\n")
-	return err
+	if _, err := f.WriteString(text + "\n"); err != nil {
+		return err
+	}
+	return nil
 }
 
 // quadletSection represents a single quadlet extracted from a multi-quadlet file

test/system/253-podman-quadlet.bats

@@ -465,43 +465,59 @@ EOF
     assert $status -eq 0 "quadlet rm --ignore should succeed even for non-existent quadlets"
 }
 
-@test "quadlet install --replace" {
-    local install_dir=$(get_quadlet_install_dir)
-    # Create a test quadlet file
-    local quadlet_file=$PODMAN_TMPDIR/alpine-quadlet.container
-    local initial_exec='Exec=sh -c "echo STARTED CONTAINER; trap '\''exit'\'' SIGTERM; while :; do sleep 0.1; done"'
-    cat > $quadlet_file <<EOF
+@test "podman quadlet install --replace" {
+    # 1. Create a valid "Long" quadlet file with many environment variables
+    cat > "$PODMAN_TMPDIR/long.container" <<EOF
 [Container]
 Image=$IMAGE
-$initial_exec
+Exec=sh -c "echo STARTED; trap 'exit' SIGTERM; while :; do sleep 0.1; done"
 EOF
-    # Test quadlet install
-    run_podman quadlet install $quadlet_file
-    # Verify install output contains the quadlet name on a single line
-    assert "$output" =~ "alpine-quadlet.container" "install output should contain quadlet name"
+    for i in {1..10}; do echo "Environment=VAR$i=VAL$i" >> "$PODMAN_TMPDIR/long.container"; done
 
-    # Without replace should fail
-    run_podman 125 quadlet install $quadlet_file
+    # 2. Install the LONG file first
+    run_podman quadlet install "$PODMAN_TMPDIR/long.container"
+    is "$output" ".*long.container"
+
+    # 3. Without replace should fail
+    run_podman 125 quadlet install "$PODMAN_TMPDIR/long.container"
     assert "$output" =~ "refusing to overwrite" "reinstall without --replace must fail with the overwrite error message"
 
-    cat > $quadlet_file <<EOF
+    # 4. Overwrite the source file with valid "Short" content
+    cat > "$PODMAN_TMPDIR/long.container" <<EOF
 [Container]
-Image=$IMAGE
-Exec=sh -c "echo STARTED CONTAINER UPDATED; trap 'exit' SIGTERM; while :; do sleep 0.1; done"
+Image=alpine
 EOF
-    # With replace should pass and update quadlet
-    run_podman quadlet install --replace $quadlet_file
 
-    # Verify install output contains the quadlet name on a single line
-    assert "$output" =~ "alpine-quadlet.container" "install output should contain quadlet name"
+    # 5. Install the SAME file again with --replace
+    run_podman quadlet install --replace "$PODMAN_TMPDIR/long.container"
 
-    run_podman quadlet print alpine-quadlet.container
+    # --- VERIFICATION 1: CHECK FOR TRUNCATION ---
+    local install_dir=$(get_quadlet_install_dir)
+    run cat "$install_dir/long.container"
+    assert "$output" == "$(<$PODMAN_TMPDIR/long.container)" "File was correctly truncated/replaced atomically"
 
-    assert "$output" !~ "$initial_exec" "Printed content must not show the initial version"
-    assert "$output" == "$(<$quadlet_file)" "Printed content must match the updated file content"
+    # --- VERIFICATION 2: CHECK FOR DUPLICATES IN .APP FILE ---
 
-    # Clean up
-    run_podman quadlet rm alpine-quadlet.container
+    local app_file="$install_dir/.long.container.app"
+
+    # Check if the file exists
+    if [ ! -f "$app_file" ]; then
+        # If .app is missing, check if .asset was created instead (debugging IsExtSupported)
+        if [ -f "$install_dir/.long.container.asset" ]; then
+             die "Failed: Created .asset file instead of .app file. IsExtSupported check failed?"
+        fi
+        die "Failed: .app file not found at $app_file"
+    fi
+
+    # Check content of the .app file
+    run cat "$app_file"
+    # It should contain exactly one line: "long.container"
+    assert "$output" == "long.container" ".app file should contain the quadlet name"
+
+    # Ensure no duplicates (line count should be 1)
+    run wc -l < "$app_file"
+    assert "$output" -eq 1 "Should only be listed once in tracking files"
+
+    # Cleanup: Remove the installed quadlet
+    run_podman quadlet rm long.container
 }
-
-# vim: filetype=sh