feat(registry): adding initial support for containerd registry mirror configurations by yajith · Pull Request #120 · portainer/kubesolo

yajith · 2026-03-21T05:14:19Z

No description provided.

Copilot

Pull request overview

Adds initial containerd registry mirror support to KubeSolo by wiring a new --registry-mirror flag through startup, generating hosts.toml entries under containerd’s certs.d, and documenting the behavior/ownership model.

Changes:

Introduces --registry-mirror (StringMap) and plumbs mirror config through types.Embedded into the containerd runtime service.
Writes per-registry /etc/containerd/certs.d/<upstream>/hosts.toml with a “managed by kubesolo” marker and override_path handling for path-based mirrors.
Sets containerd registry.config_path to /etc/containerd/certs.d, updates install.sh to pass mirror flags, and adds docs + unit tests.

Reviewed changes

Copilot reviewed 11 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`types/types.go`	Adds `RegistryMirrors` to embedded config passed into runtime components.
`types/const.go`	Defines `DefaultContainerdCertsDir` used for containerd registry config.
`cmd/kubesolo/main.go`	Plumbs flag value into `types.Embedded` for containerd service consumption.
`internal/config/flags/flags.go`	Adds `--registry-mirror` flag (and env var) as a string map.
`pkg/runtime/containerd/service.go`	Stores registry mirror map on the containerd service struct.
`pkg/runtime/containerd/executor.go`	Writes mirror files before generating/starting containerd.
`pkg/runtime/containerd/config.go`	Sets containerd registry `config_path` to the certs directory.
`pkg/runtime/containerd/registry.go`	Implements hosts.toml generation + managed-file ownership logic + writing.
`pkg/runtime/containerd/registry_test.go`	Unit tests for TOML generation and managed-marker detection.
`install.sh`	Accepts `--registry-mirror` and appends it to generated service args.
`docs/configuration/registry-mirrors.md`	Documents usage, generated config, and manual-ownership model.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pkg/runtime/containerd/registry.go

install.sh

Copilot

Pull request overview

Copilot reviewed 12 out of 15 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-22T05:59:43Z

pkg/runtime/containerd/registry.go

+// certs.d. It rejects empty values, path separators, and path traversal sequences that
+// could allow writes outside the certs.d directory.
+func isValidUpstream(upstream string) bool {
+	return upstream != "" && !strings.Contains(upstream, "/") && !strings.Contains(upstream, "..")


isValidUpstream still allows special directory names like "." which makes filepath.Join(types.DefaultContainerdCertsDir, upstream) resolve to the certs.d root and could cause kubesolo to write /etc/containerd/certs.d/hosts.toml instead of a per-registry subdir. Tighten validation to reject "." (and ideally any value where filepath.Clean(upstream) differs or results in "."/".."), and add a test case for it.

Suggested change

return upstream != "" && !strings.Contains(upstream, "/") && !strings.Contains(upstream, "..")

if upstream == "" {

return false

}

// Reject obvious traversal patterns early.

if strings.Contains(upstream, "..") {

return false

}

// Ensure the value is a single, clean path element that is not "." or "..".

cleaned := filepath.Clean(upstream)

if cleaned != upstream {

return false

}

if cleaned == "." || cleaned == ".." {

return false

}

// Disallow any path separators to ensure we only ever create one subdirectory

// under the certs.d root.

if strings.Contains(upstream, "/") || strings.Contains(upstream, string(os.PathSeparator)) {

return false

}

return true

Copilot · 2026-03-22T05:59:44Z

install.sh

+    --registry-mirror=*)
+      val="${arg#*=}"
+      case "$val" in
+        *[\;\&\|\`\$\<\>]*)
+          handle_error "--registry-mirror: mirror URL contains shell metacharacters: $val"
+          ;;
+      esac
+      REGISTRY_MIRRORS_ARGS="$REGISTRY_MIRRORS_ARGS --registry-mirror=$val"
+      ;;


The --registry-mirror value is appended into CMD_ARGS and later executed via eval (foreground/daemon modes). The current metacharacter filter does not block quotes, whitespace, or newlines, so a crafted argument could break out of the intended command and execute arbitrary shell code. Prefer avoiding eval entirely (build an argv array and exec it), or at minimum apply robust escaping/quoting and explicitly reject control characters/whitespace/quotes in val.

stevensbkang

Please take a look at comments

stevensbkang · 2026-03-22T19:57:06Z

pkg/runtime/containerd/executor.go

+	if err := s.writeRegistryMirrorFiles(s.registryMirrors); err != nil {
+		log.Error().Str("component", "containerd").Msgf("failed to write registry mirror files: %v", err)
+		s.terminate()
+		return err
+	}
+


Is it necessary to terminate the program if writing registry mirror files fails?

stevensbkang · 2026-03-22T20:10:34Z

pkg/runtime/containerd/service.go

 	sandboxImageFile              string
 	localPathProvisionerImageFile string
 	isPortainerEdge               bool
+	registryMirrors               map[string]string


Should we name registryMirrors to registries instead? I think it can be used for normal private registries as well, not just mirrors!

stevensbkang · 2026-03-22T20:21:21Z

pkg/runtime/containerd/registry.go

+		u, err := url.Parse(mirrorURL)
+		if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
+			log.Warn().Str("component", "containerd").
+				Str("upstream", upstream).
+				Str("mirror", mirrorURL).
+				Msg("skipping registry mirror: invalid URL, unsupported scheme (must be http or https), or missing host")
+			continue
+		}
+		if u.User != nil {
+			log.Warn().Str("component", "containerd").
+				Str("upstream", upstream).
+				Msg("skipping registry mirror: credentials in mirror URL are not supported — manage the hosts.toml file manually")
+			continue
+		}


How about relocating these checks into the isValidUpstream function?

stevensbkang · 2026-03-22T20:25:36Z

pkg/runtime/containerd/registry.go

+// certs.d. It rejects empty values, path separators, and path traversal sequences that
+// could allow writes outside the certs.d directory.
+func isValidUpstream(upstream string) bool {
+	return upstream != "" && !strings.Contains(upstream, "/") && !strings.Contains(upstream, "..")


If this line is to check if upstream string is safe to use as a directory name, how about using filepath.IsLocal(upstream)?

yajith added 4 commits March 20, 2026 23:55

adding support for registry configuration

d8cf619

update description and added unit tests

31b89f8

added a docs section

a421688

address user modifying hosts.toml scenario

bd27c7e

yajith requested a review from Copilot March 21, 2026 05:29

Copilot started reviewing on behalf of yajith March 21, 2026 05:29 View session

Copilot AI reviewed Mar 21, 2026

View reviewed changes

addressing copilot comments

10fc178

yajith requested a review from Copilot March 22, 2026 05:56

Copilot started reviewing on behalf of yajith March 22, 2026 05:56 View session

Copilot AI reviewed Mar 22, 2026

View reviewed changes

stevensbkang reviewed Mar 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(registry): adding initial support for containerd registry mirror configurations#120

feat(registry): adding initial support for containerd registry mirror configurations#120
yajith wants to merge 5 commits intodevelopfrom
feat/registry-mirror-config

yajith commented Mar 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 22, 2026

Uh oh!

Copilot AI Mar 22, 2026

Uh oh!

stevensbkang left a comment

Uh oh!

stevensbkang Mar 22, 2026

Uh oh!

stevensbkang Mar 22, 2026

Uh oh!

stevensbkang Mar 22, 2026

Uh oh!

stevensbkang Mar 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-	return upstream != "" && !strings.Contains(upstream, "/") && !strings.Contains(upstream, "..")
+	if upstream == "" {
+		return false
+	}
+	// Reject obvious traversal patterns early.
+	if strings.Contains(upstream, "..") {
+		return false
+	}
+	// Ensure the value is a single, clean path element that is not "." or "..".
+	cleaned := filepath.Clean(upstream)
+	if cleaned != upstream {
+		return false
+	}
+	if cleaned == "." || cleaned == ".." {
+		return false
+	}
+	// Disallow any path separators to ensure we only ever create one subdirectory
+	// under the certs.d root.
+	if strings.Contains(upstream, "/") || strings.Contains(upstream, string(os.PathSeparator)) {
+		return false
+	}
+	return true

Conversation

yajith commented Mar 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

stevensbkang left a comment

Choose a reason for hiding this comment

Uh oh!

stevensbkang Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

stevensbkang Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

stevensbkang Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

stevensbkang Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants