2804 addon omit flags import

[vvkrishnakanth9]

Fixes #2804

Motivation

Support omitting optional addon images during offline OCI import.

Modifications

• Added repeatable --omit support to addon import.
• Added omittedImages manifest configuration and image pruning logic.
• Preserved shared images required by other selected implementations.
• Added unit, CLI, E2E and manifest validation coverage.

Verification

• manual validation and auto workflows verified.
• Offline import with and without --omit: verified

[krotz-dieter]

I'll systematically review this comprehensive PR that adds support for omitting container images during addon imports.

Let me review the critical files from the diff. I'll focus on logic, error handling, and potential bugs:

Now let me analyze the critical logic in detail:

Code Review Analysis

✅ Positive Observations

1. Well-designed architecture: The three-phase approach (extract → plan → import) is sound and separates concerns effectively.
2. Comprehensive test coverage: New unit tests (imageprune.module.unit.tests.ps1, addons_omittedimages_test.go) provide good regression protection.
3. Manifest snapshot strategy: The snapshot of installed manifest before config-layer overwrite (regression C-1) is a clever solution to preserve omit declarations from older artifacts.
4. Safe defaults: Images are only pruned when NO selected implementation needs them—conservative approach.

⚠️ Potential Issues Found

1. PowerShell: Import.ps1 - Missing null check on $planEntry.Addon

File: addons/Import.ps1, lines ~760, ~850

The code uses $addon = $planEntry.Addon and references $addon.name later without checking if $planEntry.Addon is null:

$addon = $planEntry.Addon
# ... later ...
Write-Log "[Prune] Not importing '$tarToSkip' for '$addonKey'" -Console

While the plan construction should prevent this, a defensive check would be safer. Minor risk.

2. Go: importcmd.go - Missing type assertion safety

File: cmd/k2s/cmd/addons/importcmd/importcmd.go, line ~195

The parseOmitOptions function is robust, but appendOmitParam trusts that strings.Join() will always produce the right format. The function is safe but could benefit from validation of the joined string format before passing to PowerShell.

3. PowerShell: imageprune.module.psm1 - Potential directory traversal

File: addons/imageprune.module.psm1, lines ~186-200

In Get-OmitFlagsForAddon, relative paths from manifests are resolved using Join-Path $BaseDirectory $relativePath. If a manifest contains a malicious path like ../../../../../../windows/system32/evil.yaml, this could read unintended files. However:

• The paths come from addon manifests (controlled by maintainers)
• Tests validate that paths exist and declare images
• This is a LOW RISK issue in practice but worth noting

4. PowerShell: Import.ps1 - Error handling in tarExtraction

File: addons/Import.ps1, lines ~757-780

When extracting images from tar, if tar returns an error code but $LASTEXITCODE isn't checked properly before proceeding to image import, this could silently fail. Looking at the code:

$extractedTars = Get-ChildItem -Path $tempImagesDir -Filter '*.tar' -File

If extraction fails but the directory exists with 0 files, it continues silently. The code does check exit codes later but the flow could be clearer.

5. Go: Missing validation for duplicate omit flags

File: cmd/k2s/cmd/addons/importcmd/importcmd.go, lines ~199-212

The parseOmitOptions doesn't deduplicate repeated flags:

Expect(cmd.Flags().Set(omitFlagName, "omitKeycloak")).To(Succeed())
Expect(cmd.Flags().Set(omitFlagName, "omitKeycloak")).To(Succeed())  // duplicate?

The code should deduplicate or warn about duplicates. Low impact since the pruning logic handles duplicates gracefully, but semantically wrong.

6. PowerShell: imageprune.module.psm1 - Multi-document YAML parsing

File: addons/imageprune.module.psm1 (references Go code in addons.go)

The matching fix in internal/core/addons/addons.go uses yaml.NewDecoder to handle multi-document files. PowerShell doesn't have native multi-document YAML support. Verify that Get-FromYamlFile in the callers handles this correctly (not visible in diff).

7. Test File: UTF-8 BOM issue

File: addons/imageprune.module.unit.tests.ps1, line 1

The file starts with # SPDX-FileCopyrightText: © 2026 - the © indicates a UTF-8 BOM encoding issue. This could cause PowerShell parsing issues on some systems. Same issue appears in:

• cmd/k2s/cmd/addons/importcmd/importcmd_test.go, line 1
• test/e2e/addons/security/exportimport/security_export_import_test.go, line 1

SEVERITY: MEDIUM - These encoding issues can cause script failures.

8. PowerShell: Missing cleanup on error

File: addons/Import.ps1, line ~373

The $tempLayerDir is created but if the import fails after that point, cleanup isn't guaranteed. The code doesn't have a try-finally block wrapping Phase 3. However, this might be handled by the caller's error handling.

9. Manifest schema validation - anyOf requirement

File: addons/addon.manifest.schema.json, lines ~300-314

The schema requires either fromFiles OR explicit:

"anyOf": [
    { "required": ["fromFiles"] },
    { "required": ["explicit"] }
]

But allows additionalProperties: false. The test (addons_omittedimages_test.go:82) validates "every omit* flag must declare omittedImages". This is good, but the schema doesn't enforce that the omittedImages object itself is non-empty when present. A manifest could declare omittedImages: {} and pass schema validation but fail at runtime.

10. Go: Missing error context in appendOmitParam

File: cmd/k2s/cmd/addons/importcmd/importcmd.go, lines ~214-227

The function silently skips empty options. While this is correct behavior, there's no logging/output. Users won't know if they passed a blank --omit "" and it was discarded.

✅ No Major Security Issues

• No injection vulnerabilities detected
• No credential leaks
• Proper escaping with EscapeWithSingleQuotes for PowerShell parameters
• File path validation happens at test time

✅ Breaking Changes Assessment

• The --omit flag is new and additive - no breaking changes
• Existing imports without --omit work identically (backward compatible)
• PowerShell signature changes (adding -Omit parameter) are handled with default empty array

Files Reviewed

• ✅ addons/Export.ps1
• ✅ addons/Import.ps1
• ✅ addons/README.md
• ✅ addons/addon.manifest.schema.json
• ✅ addons/imageprune.module.psm1
• ✅ addons/imageprune.module.unit.tests.ps1
• ✅ addons/ingress/addon.manifest.yaml
• ✅ addons/logging/addon.manifest.yaml
• ✅ addons/monitoring/addon.manifest.yaml
• ✅ addons/oci.module.psm1
• ✅ addons/security/addon.manifest.yaml
• ✅ cmd/k2s/cmd/addons/generic/generic.go
• ✅ cmd/k2s/cmd/addons/importcmd/importcmd.go
• ✅ cmd/k2s/cmd/addons/importcmd/importcmd_test.go
• ✅ docs/user-guide/addons.md
• ✅ internal/core/addons/addons.go
• ✅ internal/core/addons/addons_additionalimagesfiles_test.go
• ✅ internal/core/addons/addons_omittedimages_test.go
• ✅ test/e2e/addons/export_import_all_test.go
• ✅ test/e2e/addons/exportimport/helpers.go
• ✅ test/e2e/addons/security/exportimport/security_export_import_test.go

Summary

Overall Assessment: Solid feature implementation with good design and test coverage.

Critical Issues: 1 (UTF-8 BOM encoding in 3 files)

Medium Issues: None confirmed

Minor Issues: 3-4 (edge cases, null checks, duplicate handling)

Recommendation: Address the UTF-8 BOM encoding issues before merge. The encoding errors in .ps1 and .go files at line 1 will cause failures in strict PowerShell/Go environments.

[krotz-dieter]

I'll review this PR systematically across all changed files. This is a substantial feature adding --omit support for skipping container images during addon import.

Let me systematically review the key changes:

Now let me verify specific technical issues in the code:

Now let me review the critical parts in detail:

ISSUE #1: Module Export Completeness

In addons/imageprune.module.psm1, line 532-533, the exported functions list appears incomplete. The module defines many functions like ConvertTo-OmitToken, Get-MatchingOmitToken, Get-OmitFlagsFromImplementation, Get-OmitFlagsForAddon, Get-OmittedImagesForFlag, New-AddonImagePrunePlan, Write-AddonImagePrunePlan, but I need to verify these are actually called correctly from Import.ps1.

Looking at Import.ps1 line 702 onward:

$planFlags = @(Get-OmitFlagsForAddon ...

and line 730:

$prunePlan = New-AddonImagePrunePlan -Entries $importPlan -Omit $Omit
Write-AddonImagePrunePlan -Plan $prunePlan

These functions ARE exported. ✓

ISSUE #2: oci.module.psm1 Missing Export?

Looking at the diff, line 510-511 adds two new functions to the export list. However, imageprune.module.psm1 depends on ConvertTo-ImageTarFileName (line 352 in imageprune tests). Checking line 36 of imageprune.module.psm1:

Import-Module $infraModule, $addonsModule, $ociModule

The ociModule is imported, so ConvertTo-ImageTarFileName should be available. ✓

ISSUE #3: Multi-Document YAML Parsing

In internal/core/addons/addons.go, lines 406-428, the code was changed from yaml.Unmarshal to yaml.NewDecoder for multi-document support. Looking at the logic:

decoder := yaml.NewDecoder(bytes.NewReader(data))
for {
    var document interface{}
    err := decoder.Decode(&document)
    if err == io.EOF {
        break
    }
    if err != nil {
        return nil, err
    }
    images = append(images, extractImagesFromYAMLContent(document)...)
}

This looks correct - it properly handles multi-document YAML files and accumulates images from all documents.✓

ISSUE #4: Go Code - Missing Import?

In cmd/k2s/cmd/addons/importcmd/importcmd.go, the code uses lo.SomeBy on line 73 (in generic.go). Let me check if this is imported... Looking at the provided diff, I don't see the import section of these Go files, so I cannot verify if samber/lo is imported. However, this is common pattern and likely already in the codebase.

POTENTIAL ISSUE #5: Error Handling in Import.ps1

Looking at Import.ps1 around line 705-710:

$planFlags = @(Get-OmitFlagsForAddon `
        -ArtifactManifestPath $configManifestPath `
        -InstalledManifestPath $installedManifestSnapshot `
        -ImplementationName $addon.implementation)

There's no error handling if Get-OmitFlagsForAddon fails. However, looking at imageprune.module.psm1 line 168-189, the function catches errors:

try {
    $manifest = Get-FromYamlFile -Path $source.Path
}
catch {
    Write-Log "[Prune] Could not parse '$($source.Path)': $($_.Exception.Message)"
    continue
}

So it's fail-safe and doesn't throw. ✓

POTENTIAL ISSUE #6: Manifest Snapshot Path Issue

In Import.ps1 around line 393-401, the snapshot code has a potential issue:

$installedManifestSnapshot = $null
$installedManifestCandidates = @(Join-Path $destinationPath 'addon.manifest.yaml')
if ($folderParts.Count -gt 1) {
    # space-separated addon names keep their manifest in the parent folder
    $installedManifestCandidates += (Join-Path (Split-Path -Path $destinationPath -Parent) 'addon.manifest.yaml')
}

The variable $folderParts is used but I need to check if it's defined. Looking at the context around line 381-390:

$addonKey = $addon.name
if (-not [string]::IsNullOrWhiteSpace($addon.implementation) -and $addon.implementation -ne $addon.name) {
    $addonKey = "$($addon.name)/$($addon.implementation)"
}
$tempLayerDirName = 'layer-temp-' + ($addonKey -replace '[\\/\s]', '-')

I don't see $folderParts being defined in the provided diff. This could be a bug! Let me search for where it should come from...

Looking earlier in the original code (before the diff), around where $implementationPath is set, there should be parsing of addon name/implementation. The line that uses $folderParts appears to assume it exists from earlier addon name splitting.

CRITICAL ISSUE FOUND: In addons/Import.ps1 line 399, the code references $folderParts.Count but I cannot see where $folderParts is defined in the provided diff context. This variable needs to come from earlier in the Import.ps1 script. This is likely a pre-existing variable, but the change adds a new use of it that could fail if undefined.

Let me verify this is actually being used correctly:

Looking at line 380 in the diff context, the code that builds $addonKey doesn't show where $folderParts comes from. This needs to be checked against the full Import.ps1 file to ensure it's properly defined before line 399.

ISSUE #7: Go Test Coverage

In cmd/k2s/cmd/addons/importcmd/importcmd_test.go, the tests verify the omit flag is properly parsed and forwarded. However, I should check if escaping is correct. Looking at line 200:

return append(params, " -Omit "+strings.Join(quoted, ","))

The function uses utils.EscapeWithSingleQuotes(option) which should be safe. ✓

---

Summary of Findings

✓ ACCEPTABLE:

• Module export lists are complete and correct
• Multi-document YAML parsing fix is correct
• Error handling in imageprune module is proper
• Test coverage for new functionality is comprehensive
• Manifest snapshot approach is solid for backward compatibility

⚠ NEEDS VERIFICATION:

• The $folderParts variable usage in Import.ps1 line 399 - this variable must be defined earlier in the script (not visible in diff context). Need to verify it exists in the full file.

✓ NO SECURITY ISSUES:

• No credential leaks
• Proper input validation in Go code
• Safe string escaping for PowerShell commands

---

Files Reviewed

• ✅ addons/Export.ps1
• ✅ addons/Import.ps1
• ✅ addons/README.md
• ✅ addons/addon.manifest.schema.json
• ✅ addons/imageprune.module.psm1
• ✅ addons/imageprune.module.unit.tests.ps1
• ✅ addons/ingress/addon.manifest.yaml
• ✅ addons/logging/addon.manifest.yaml
• ✅ addons/monitoring/addon.manifest.yaml
• ✅ addons/oci.module.psm1
• ✅ addons/security/addon.manifest.yaml
• ✅ cmd/k2s/cmd/addons/generic/generic.go
• ✅ cmd/k2s/cmd/addons/importcmd/importcmd.go
• ✅ cmd/k2s/cmd/addons/importcmd/importcmd_test.go
• ✅ docs/user-guide/addons.md
• ✅ internal/core/addons/addons.go
• ✅ internal/core/addons/addons_additionalimagesfiles_test.go
• ✅ internal/core/addons/addons_omittedimages_test.go
• ✅ test/e2e/addons/export_import_all_test.go
• ✅ test/e2e/addons/exportimport/helpers.go
• ✅ test/e2e/addons/security/exportimport/security_export_import_test.go