fix(kubernetes): support repository ports in containerd certs path - #2915
fix(kubernetes): support repository ports in containerd certs path#2915Elaugaste (elaugaste) wants to merge 4 commits into
Conversation
Support repository ports in containerd configuration path for registries requiring port. Replaces trailing underscore followed by digits with colon. Assisted-By: Gemini <noreply@google.com> Signed-off-by: Elaugaste <elaugaste@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the Kubernetes cluster template to properly support repository ports within the containerd certificates directory. By implementing a regex replacement for secret keys, the system now correctly maps registry hostnames containing ports to the expected directory structure, ensuring compatibility with custom registry configurations. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTwo Helm templates are updated to consistently handle the underscore-to-colon naming convention for containerd registry certificates. The Secret template now derives config keys by stripping endpoint schemes, creates alternate lookup variants with colon-to-underscore conversion, and applies regex rewriting to the mirror server URL. The cluster template applies the inverse regex transformation to convert trailing Changescontainerd registry certificate path coordination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request updates the Kubernetes app template in packages/apps/kubernetes/templates/cluster.yaml to correctly format containerd registry host paths. It uses regexReplaceAll to convert trailing underscores followed by port numbers in secret keys (e.g., _5000) into colons (e.g., :5000) for the containerd directory structure. There are no review comments, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
… config lookup Trims scheme from endpoint URL and supports underscores when looking up configuration in registries.config. This allows private registries with ports to be authenticated correctly. Assisted-By: Gemini <noreply@google.com> Signed-off-by: Elaugaste <elaugaste@gmail.com>
589ee9c to
39666c6
Compare
…RL in containerd config When generating containerd hosts.toml configuration from registry maps, replace trailing underscores before port numbers with colons. This ensures the correct registry URL is set in the server parameter. Assisted-By: Gemini <noreply@google.com> Signed-off-by: Elaugaste <elaugaste@gmail.com>
39666c6 to
b4f0904
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/platform/templates/containerd-registry-secret.yaml (1)
16-18: ⚡ Quick winConfig lookup supports both colon and underscore separators.
The endpoint config resolution strips the scheme and tries both
host:portandhost_portlookup keys, providing useful flexibility for users. The colon format is preferred (tried first), which aligns with the values.yaml examples.However, this flexibility is not documented. Consider adding a comment in values.yaml explaining that
.registries.configkeys can use either format, thoughhost:port(e.g.,"10.0.0.1:8082") is the canonical form.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/platform/templates/containerd-registry-secret.yaml` around lines 16 - 18, The containerd-registry-secret.yaml template supports looking up registry configuration using both colon-separated (host:port) and underscore-separated (host_port) endpoint formats via the $.Values.registries.config lookups, but this flexibility is not documented. Add a comment or documentation entry in values.yaml for the .registries.config section explaining that configuration keys can use either colon or underscore format as separators, with the colon format (host:port, e.g., "10.0.0.1:8082") being the preferred canonical form. This clarifies the lookup behavior demonstrated in the template file's use of endpointWithoutScheme and endpointWithUnderscore variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/platform/templates/containerd-registry-secret.yaml`:
- Around line 16-18: The containerd-registry-secret.yaml template supports
looking up registry configuration using both colon-separated (host:port) and
underscore-separated (host_port) endpoint formats via the
$.Values.registries.config lookups, but this flexibility is not documented. Add
a comment or documentation entry in values.yaml for the .registries.config
section explaining that configuration keys can use either colon or underscore
format as separators, with the colon format (host:port, e.g., "10.0.0.1:8082")
being the preferred canonical form. This clarifies the lookup behavior
demonstrated in the template file's use of endpointWithoutScheme and
endpointWithUnderscore variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 349a5a80-426e-4cdd-b8d4-6c5bdbb155fe
📒 Files selected for processing (1)
packages/core/platform/templates/containerd-registry-secret.yaml
03278c7 to
3cb7733
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/kubernetes/templates/cluster.yaml`:
- Around line 485-486: The printf commands that configure containerd registry
authentication lack proper escaping for special characters in the $registry,
$username, and $password variables. If these values contain double quotes or
backslashes, the resulting TOML configuration will be malformed. Create a shell
function to properly escape TOML string values (escaping double quotes and
backslashes), then apply this function to escape $registry, $username, and
$password before embedding them into the printf commands for both the
io.containerd.cri.v1.images and io.containerd.grpc.v1.cri registry
configurations.
- Around line 481-482: The tr -d '[:space:]' command on the lines extracting
username and password values is removing all whitespace characters, including
spaces within valid credential strings. Replace the tr command with an approach
that only strips leading and trailing whitespace while preserving internal
spaces in the credentials. Use awk's built-in gsub function or sed to trim only
the edges of the extracted values, not all whitespace throughout them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5275b9bc-7472-4c04-a82e-211e4f4c049f
📒 Files selected for processing (1)
packages/apps/kubernetes/templates/cluster.yaml
| username=$(awk -F'"' '/username =/ {print $2}' "$hosts_file" | tr -d '[:space:]') | ||
| password=$(awk -F'"' '/password =/ {print $2}' "$hosts_file" | tr -d '[:space:]') |
There was a problem hiding this comment.
tr -d '[:space:]' removes ALL whitespace, including spaces within credentials.
The tr -d '[:space:]' command strips all whitespace characters, including those within the username or password strings. If a valid credential contains a space (e.g., password "my pass word"), it will be corrupted to "mypassword".
The awk command already extracts the value between double quotes, so internal whitespace should be preserved. The tr command appears intended to remove leading/trailing whitespace or newlines, but it's too aggressive.
🔧 Proposed fix to strip only leading/trailing whitespace
- username=$(awk -F'"' '/username =/ {print $2}' "$hosts_file" | tr -d '[:space:]')
- password=$(awk -F'"' '/password =/ {print $2}' "$hosts_file" | tr -d '[:space:]')
+ username=$(awk -F'"' '/username =/ {print $2}' "$hosts_file" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')
+ password=$(awk -F'"' '/password =/ {print $2}' "$hosts_file" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')Or use awk's built-in trimming:
- username=$(awk -F'"' '/username =/ {print $2}' "$hosts_file" | tr -d '[:space:]')
- password=$(awk -F'"' '/password =/ {print $2}' "$hosts_file" | tr -d '[:space:]')
+ username=$(awk -F'"' '/username =/ {gsub(/^[[:space:]]+|[[:space:]]+$/, "", $2); print $2}' "$hosts_file")
+ password=$(awk -F'"' '/password =/ {gsub(/^[[:space:]]+|[[:space:]]+$/, "", $2); print $2}' "$hosts_file")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| username=$(awk -F'"' '/username =/ {print $2}' "$hosts_file" | tr -d '[:space:]') | |
| password=$(awk -F'"' '/password =/ {print $2}' "$hosts_file" | tr -d '[:space:]') | |
| username=$(awk -F'"' '/username =/ {gsub(/^[[:space:]]+|[[:space:]]+$/, "", $2); print $2}' "$hosts_file") | |
| password=$(awk -F'"' '/password =/ {gsub(/^[[:space:]]+|[[:space:]]+$/, "", $2); print $2}' "$hosts_file") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/apps/kubernetes/templates/cluster.yaml` around lines 481 - 482, The
tr -d '[:space:]' command on the lines extracting username and password values
is removing all whitespace characters, including spaces within valid credential
strings. Replace the tr command with an approach that only strips leading and
trailing whitespace while preserving internal spaces in the credentials. Use
awk's built-in gsub function or sed to trim only the edges of the extracted
values, not all whitespace throughout them.
| printf "\n[plugins.\"io.containerd.cri.v1.images\".registry.configs.\"%s\".auth]\n username = \"%s\"\n password = \"%s\"\n" "$registry" "$username" "$password" >> /etc/containerd/config.toml | ||
| printf "\n[plugins.\"io.containerd.grpc.v1.cri\".registry.configs.\"%s\".auth]\n username = \"%s\"\n password = \"%s\"\n" "$registry" "$username" "$password" >> /etc/containerd/config.toml |
There was a problem hiding this comment.
Printf lacks escaping for special characters in TOML values.
The printf commands embed $registry, $username, and $password directly into TOML configuration strings without escaping. If these values contain double quotes, backslashes, or other special characters, the resulting TOML will be malformed.
For example:
- Registry
my"registry:5000→[plugins."io.containerd.cri.v1.images".registry.configs."my"registry:5000".auth](invalid TOML section) - Password
pass"word→password = "pass"word"(invalid TOML value)
This could also be an injection vector if credentials are sourced from untrusted input.
🛡️ Proposed fix to escape special characters
Add a shell function to escape TOML string values before the loop:
- |
+ # Escape double quotes and backslashes for TOML string values
+ toml_escape() {
+ printf '%s' "$1" | sed 's/\\/\\\\/g; s/"/\\"/g'
+ }
for hosts_file in /etc/containerd/certs.d/*/hosts.toml; do
[ -f "$hosts_file" ] || continue
registry=$(basename "$(dirname "$hosts_file")")
- username=$(awk -F'"' '/username =/ {print $2}' "$hosts_file" | tr -d '[:space:]')
- password=$(awk -F'"' '/password =/ {print $2}' "$hosts_file" | tr -d '[:space:]')
+ username=$(awk -F'"' '/username =/ {gsub(/^[[:space:]]+|[[:space:]]+$/, "", $2); print $2}' "$hosts_file")
+ password=$(awk -F'"' '/password =/ {gsub(/^[[:space:]]+|[[:space:]]+$/, "", $2); print $2}' "$hosts_file")
if [ -n "$username" ] && [ -n "$password" ]; then
echo "Configuring auth for $registry in config.toml"
- printf "\n[plugins.\"io.containerd.cri.v1.images\".registry.configs.\"%s\".auth]\n username = \"%s\"\n password = \"%s\"\n" "$registry" "$username" "$password" >> /etc/containerd/config.toml
- printf "\n[plugins.\"io.containerd.grpc.v1.cri\".registry.configs.\"%s\".auth]\n username = \"%s\"\n password = \"%s\"\n" "$registry" "$username" "$password" >> /etc/containerd/config.toml
+ printf "\n[plugins.\"io.containerd.cri.v1.images\".registry.configs.\"%s\".auth]\n username = \"%s\"\n password = \"%s\"\n" "$(toml_escape "$registry")" "$(toml_escape "$username")" "$(toml_escape "$password")" >> /etc/containerd/config.toml
+ printf "\n[plugins.\"io.containerd.grpc.v1.cri\".registry.configs.\"%s\".auth]\n username = \"%s\"\n password = \"%s\"\n" "$(toml_escape "$registry")" "$(toml_escape "$username")" "$(toml_escape "$password")" >> /etc/containerd/config.toml
fi
done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/apps/kubernetes/templates/cluster.yaml` around lines 485 - 486, The
printf commands that configure containerd registry authentication lack proper
escaping for special characters in the $registry, $username, and $password
variables. If these values contain double quotes or backslashes, the resulting
TOML configuration will be malformed. Create a shell function to properly escape
TOML string values (escaping double quotes and backslashes), then apply this
function to escape $registry, $username, and $password before embedding them
into the printf commands for both the io.containerd.cri.v1.images and
io.containerd.grpc.v1.cri registry configurations.
…g.toml Adds a bash script to preKubeadmCommands to parse username and password from hosts.toml configurations on the node, and append them directly to containerd's config.toml. This enables proper registry authentication for containerd 2.x and 1.x. Assisted-By: Gemini <noreply@google.com> Signed-off-by: Elaugaste <elaugaste@gmail.com>
3cb7733 to
aa92a0e
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the port-handling design is sound, but the credential-extraction script corrupts valid passwords and can emit malformed config.toml that breaks containerd, and the platform template changes ship without test coverage.
Business context: registries served on a non-443 port (e.g. registry.example.com:5000) currently mount their containerd mirror config under a _<port> directory instead of :<port>, so containerd never finds it. This PR rewrites _<port>→:<port> for both the certs.d path and the mirror server URL, and extracts mirror credentials into config.toml where containerd actually reads them.
Blockers
B1: tr -d '[:space:]' corrupts any credential containing whitespace
File: packages/apps/kubernetes/templates/cluster.yaml:481-482
Issue: usernames/passwords are piped through tr -d '[:space:]', which deletes every whitespace character, not just leading/trailing.
Evidence: reproduced — a password alice smith is extracted as alicesmith. awk -F'"' '{print $2}' already returns exactly the quoted field and $(...) already strips the trailing newline, so the tr is both harmful and unnecessary.
Impact: silent auth failures against any registry whose credential contains a space, and the credential looks "almost right" which makes it very hard to debug.
Fix: drop | tr -d '[:space:]' from both lines.
B2: credentials are written into config.toml without TOML escaping
File: packages/apps/kubernetes/templates/cluster.yaml:485-486
Issue: printf "... password = \"%s\" ..." inserts the raw value. A " or \ in the value produces invalid TOML.
Evidence: reproduced — a password p@ss" word\x yields password = "p@ss\" (the embedded " also makes awk -F'"' truncate the field at the quote), and a TOML parser rejects the result. containerd then fails to parse its own config.toml and node bring-up breaks — this takes down the whole node, not just one registry.
Fix: TOML-escape $registry, $username, $password (at minimum \→\\ then "→\") before the printf.
B3: the platform containerd-registry-secret.yaml changes have no test coverage
File: packages/core/platform/templates/containerd-registry-secret.yaml:12,16-18
Issue: three of the four commits change this template (server-URL _→: rewrite, scheme-stripping + underscore-fallback config lookup), but there is no helm-unittest suite for it — neither pre-existing nor added here.
Evidence: a search over packages/core/platform/tests/ finds no reference to the secret template, regexReplaceAll, or server =. The two trickiest behavioral changes in the PR are entirely unverified. (The certs.d path line in cluster.yaml relies on lookup, which returns empty under helm-unittest, so that one is legitimately not unit-testable — but the secret template uses .Values and is fully testable.)
Fix: add fixtures rendering the secret for a port'd registry (registry.example.com_5000 → server = "https://registry.example.com:5000"), a portless registry (unchanged), and config lookup hitting both colon- and underscore-keyed entries.
Non-blocking follow-ups
- Multiple auth endpoints under one registry collapse (
cluster.yaml:481-482): if ahosts.tomlhas more than one[host."…".auth]block,username=$(awk …)captures all matches as a multi-line string andtrconcatenates them — reproduced: two endpointsalice/bob→alicebob. Scope the awk to the first match ({print $2; exit}) or enforce single-auth-per-registry. - The underscore-vs-colon convention is undocumented: a user must key
registries.mirrorswith_<port>(forced by the Secret-key constraint, since data keys must match[-._a-zA-Z0-9]+) but keyregistries.configwith:<port>(per thevalues.yamlexample). Themirrorsexample shows only portless registries, so an operator adding a port'd private registry has no way to discover the_<port>requirement. Add a port'd registry to the example and state the convention.
What's correct
regexReplaceAll "_([0-9]+)$"is well-scoped — it touches only the trailing_<digits>and preserves internal underscores (verified againstdocker.io,my_registry.example.com_5000,host_5000_8080).- The endpoint-config lookup
or (colon-form) (underscore-form)actually fixes a latent mismatch: the documented config key"10.0.0.1:8082"never matched the scheme-prefixed endpoint before, so auth config was silently ignored. Not a regression. - All kubernetes-app and platform unit tests pass locally; the new test uses the correct
documentIndex.
| username=$(awk -F'"' '/username =/ {print $2}' "$hosts_file" | tr -d '[:space:]') | ||
| password=$(awk -F'"' '/password =/ {print $2}' "$hosts_file" | tr -d '[:space:]') |
There was a problem hiding this comment.
tr -d '[:space:]' deletes every whitespace, not just leading/trailing. Reproduced: a password alice smith is extracted as alicesmith → silent auth failure that looks "almost right". The tr is also unnecessary — awk -F'"' '{print $2}' already returns exactly the quoted field and $(...) strips the trailing newline. Drop | tr -d '[:space:]' from both lines.
Edge case on the same lines: if a hosts.toml has more than one [host."…".auth] block, awk emits multiple usernames and they collapse into one string (alice/bob → alicebob). Scope to the first match ({print $2; exit}) or enforce single-auth-per-registry.
| printf "\n[plugins.\"io.containerd.cri.v1.images\".registry.configs.\"%s\".auth]\n username = \"%s\"\n password = \"%s\"\n" "$registry" "$username" "$password" >> /etc/containerd/config.toml | ||
| printf "\n[plugins.\"io.containerd.grpc.v1.cri\".registry.configs.\"%s\".auth]\n username = \"%s\"\n password = \"%s\"\n" "$registry" "$username" "$password" >> /etc/containerd/config.toml |
There was a problem hiding this comment.
No TOML escaping when embedding credentials. Reproduced: a password p@ss" word\x yields password = "p@ss\" (the embedded " also makes awk -F'"' truncate the field), which a TOML parser rejects. containerd then fails to parse its own config.toml and the whole node fails to come up — not just that one registry. TOML-escape $registry, $username, $password (\→\\, then "→\") before the printf.
|
Closing this one: the code it patches no longer exists. #2931 replaced the kubeadm/Ubuntu worker bootstrap with Talos via CABPT, which took The need behind it is still real. Per-tenant registry mirrors for in-guest pulls are a Phase 2 follow-up, noted in |
What this PR does
This PR adds support for repository ports in the containerd configuration path (
KubeadmConfigTemplateand/etc/containerd/certs.d/).It replaces the last occurrence of
_followed by digits (the port number) and.tomlwith a colon:and the port number.For example, a secret key
fontanero.hoztnode.net_5050.tomlwill now correctly map to:/etc/containerd/certs.d/fontanero.hoztnode.net:5050/hosts.tomlThe regex ensures that only the last
_followed by digits is replaced, leaving other underscores in the domain untouched.Screenshots
N/A (no UI changes)
Release note
Summary by CodeRabbit
hosts.tomlmounting to use the correct directory naming, so containerd reliably reads the intended configuration.authsettings are applied consistently across supported containerd registry plugins.