Skip to content

tink-agent-setup fixes#71

Open
jacobbmay wants to merge 4 commits into
tinkerbell:mainfrom
jacobbmay:fix/tink-agent-registry-login
Open

tink-agent-setup fixes#71
jacobbmay wants to merge 4 commits into
tinkerbell:mainfrom
jacobbmay:fix/tink-agent-registry-login

Conversation

@jacobbmay

Copy link
Copy Markdown

Description

  • If registry credentials are provided, the tink-agent-setup script now performs a nerdctl login so the tink-agent image can be pulled from a registry requiring auth
  • Allow configuring both http and https insecure registries instead of only http
    • Implementation is backwards compatible for actual http registries so an insecure http registry configured like example.your.domain would still be configured as an http registry, but explicitly setting https on the registry like https://example.your.domain will now configure it as an https registry that skips TLS verification

Fixes:
Currently tink-agent-setup doesn't authenticate with credentials provided so it doesn't work with registries requiring auth.

Also noticed that the current handling of insecure registries being passed in assumes they are all http registries but also sets skip_verify which is only relevant to https registries. nerdctl seems to see the tls related setting and attempts to use https first when pulling an image, but not when authenticating. So after adding the login command, authentication still wasn't working since I am testing with an https registry configured as insecure.

How Has This Been Tested?

Built with uv run ./build.py and then tested with QEMU using the following command:

uv run ./build.py qemu-test \
    --tink-worker-image baz.local/tink-agent:v0 \
    --tink-docker-registry baz.local \
    --tink-registry-username testuser \
    --tink-registry-password testpass-shell-validation \
    --tink-insecure-registries "foo.local,http://bar.local:5000,https://baz.local" \
    --qemu-append "systemd.journald.forward_to_console=1"

Validated that all 3 insecure registries were configured correctly to ensure both http and https insecure registries are handled as expected:

root@captainos:~# cat /etc/containerd/certs.d/bar.local\:5000/hosts.toml 
server = "http://bar.local:5000"

[host."http://bar.local:5000"]
  capabilities = ["pull", "resolve", "push"]
root@captainos:~# cat /etc/containerd/certs.d/baz.local/hosts.toml 
server = "https://baz.local"

[host."https://baz.local"]
  capabilities = ["pull", "resolve", "push"]
  skip_verify = true
root@captainos:~# cat /etc/containerd/certs.d/foo.local/hosts.toml 
server = "http://foo.local"

[host."http://foo.local"]
  capabilities = ["pull", "resolve", "push"]

Also verified that the login to baz.local was attempted using https (expected auth failure since the registry doesn't exist. Just validating it is using the correct scheme):

Jul 01 19:14:18 captainos tink-agent-setup[275]: {"time":"2026-07-01T19:14:18Z","level":"info","msg":"logging into registry","host":"baz.local"}
Jul 01 19:14:18 captainos tink-agent-setup[437]: time="2026-07-01T19:14:18Z" level=error msg="failed to call tryLoginWithRegHost" error="failed to call rh.Client.Do: Get \"https://baz.local/v2/\": dial tcp: lookup baz.local on 10.0.2.3:53: no such host" i=0

Next to validate auth to an http registry still uses the correct scheme, I tested with this command:

uv run ./build.py qemu-test \
    --tink-worker-image foo.local/tink-agent:v0 \
    --tink-docker-registry foo.local \
    --tink-registry-username testuser \
    --tink-registry-password testpass-shell-validation \
    --tink-insecure-registries "foo.local,http://bar.local:5000,https://baz.local" \
    --qemu-append "systemd.journald.forward_to_console=1"

And verified in journalctl the login related logs show the correct scheme for foo.local (again, expected failure but wanted to validated the scheme used for login):

Jul 01 19:21:57 captainos tink-agent-setup[277]: {"time":"2026-07-01T19:21:57Z","level":"info","msg":"logging into registry","host":"foo.local"}
Jul 01 19:21:57 captainos tink-agent-setup[439]: time="2026-07-01T19:21:57Z" level=error msg="failed to call tryLoginWithRegHost" error="failed to call rh.Client.Do: Get \"http://foo.local/v2/\": dial tcp: lookup foo.local on 10.0.2.3:53: no such host" i=0

How are existing users impacted? What migration steps/scripts do we need?

The tink-agent image can now be pulled from a registry requiring authentication and insecure TLS registries can be configured correctly.

Existing users configuring insecure http registries are unaffected since this change keeps the existing behavior of insecure registries not specifying a scheme defaulting to http.

Existing users configuring insecure https registries that don't require authentication will need to add the https:// scheme to the beginning of their insecure registry hostnames since this is no longer relying on nerdctl auto attempting https for image pulling.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

jacobbmay added 2 commits July 1, 2026 15:48
Signed-off-by: jacobbmay <134300709+jacobbmay@users.noreply.github.com>
…stries

Signed-off-by: jacobbmay <134300709+jacobbmay@users.noreply.github.com>
@jacobbmay
jacobbmay force-pushed the fix/tink-agent-registry-login branch from 1c6dbe9 to 453644f Compare July 1, 2026 19:48
@jacobweinstock

Copy link
Copy Markdown
Member

Hey @jacobbmay. This is great, thanks for contributing it. I manually reviewed it and pointed Copilot at it locally. Here's the result. Registries with ports is the biggest change. The others should be small and quick. Also, each commit need a DCO sign off, please. Let me know any questions or concerns. Thanks!

Review

Nice fix overall. The direction is right: adding nerdctl login for auth-protected registries, supporting HTTPS-with-skip-verify insecure registries, and making tink-agent-setup.service self-healing are all real improvements. I do have one blocking bug around ported registries, plus some robustness/nit feedback.


🔴 Blocking: ported registries write hosts.toml to a directory containerd never reads

In configure_insecure_registries, the port is stripped from host before the directory and server line are built, but kept only in the [host."…"] key:

if [[ "$host" == *:* ]]; then
    port="${host##*:}"
    host="${host%:*}"          # port removed from $host
...
mkdir -p "${certs_dir}/${host}"    # -> certs.d/bar.local   (no port)
server = "http://${host}"           # -> http://bar.local    (no port)
[host."http://${host}:${port}"]     # -> http://bar.local:5000 (port kept)

Tracing the final code for http://bar.local:5000 produces:

DIR=certs.d/bar.local   server=http://bar.local   hostkey=http://bar.local:5000

Per the containerd hosts.md docs, for a registry with a port containerd searches these directories in order and nothing else:

bar.local_5000_
bar.local:5000
_default

It never looks in bar.local. So for an image like bar.local:5000/tink-agent, the generated hosts.toml is ignored, containerd falls back to default behavior (HTTPS to bar.local:5000), and both the pull and nerdctl login bar.local:5000 fail for an insecure/HTTP-ported registry — exactly the case the "set registry ports where needed" commit was meant to support.

This also contradicts the PR's own "How Has This Been Tested" output, which shows certs.d/bar.local:5000/hosts.toml with server = "http://bar.local:5000" (port present in both the dir and server). That output predates the final commit, so the ported case looks like it wasn't re-tested after the last change.

Fix: keep the port in the directory name and the server URL whenever it's present. Simplest is to preserve the original registry token (with port) for the directory/server and only derive scheme/port separately — e.g. name the dir ${host}:${port} (or the filesystem-preferred ${host}_${port}_) and emit server = "${scheme}://${host}:${port}". Keep all three consistent.


🟡 Robustness

  1. login_host fallback can be wrong for image-only refs. local login_host="${DOCKER_REGISTRY:-${TINK_IMAGE%%/*}}" yields a bogus host (e.g. tink-agent:latest) if TINK_IMAGE has no registry component. Edge case, but a guard would help.
  2. Auth against an HTTPS-insecure registry requires it to also be in insecure_registries. registry_login logs into DOCKER_REGISTRY, but skip-verify is only configured if that same host is listed in insecure_registries with an https:// prefix. This is implied by the migration notes but easy to get wrong operationally — worth calling out explicitly in the README, or auto-adding the login host to the insecure set when credentials + insecure TLS are in play.

🟢 Minor / nits

  1. ExecStartPost path: prefer /usr/bin/systemctl over /bin/systemctl (works today only via Debian's usr-merge symlink). A one-line comment noting it's deliberately re-triggering a dependent unit that systemd won't auto-retry would help future readers.
  2. IPv6 hosts break the [[ "$host" == *:* ]] port detection (e.g. [::1]:5000). Likely out of scope, but the parsing silently mishandles it.
  3. server line inconsistency (non-ported case): you emit server = "http://foo.local" but [host."http://foo.local:80"]. Harmless (host entry is tried first), but inconsistent — unifying the explicit :80/:443 reads cleaner.
  4. Partial creds silently ignored: if only username or only password is set, registry_login returns without logging why. A debug/warn line would aid troubleshooting.

Verdict

Requesting changes for the port-handling bug — it makes ported insecure registries silently non-functional and regresses the behavior shown in the PR's own test output. Fix the directory/server port handling, re-test with a host:port insecure registry (both HTTP and HTTPS, plus a nerdctl login against it), and the rest is polish. Everything else looks good. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants