auto update: fix usage of --authfile

[vrothberg]

The --authfile flag has been ignored. Fix that and add a test to make
sure we won't regress another time. Requires a new --tls-verify flag
to actually test the code.

Also bump c/common since common/pull/1538 is required to correctly check
for updates. Note that I had to use the go-mod-edit-replace trick on
c/common as c/buildah would otherwise be moved back to 1.30.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2218315
Signed-off-by: Valentin Rothberg vrothberg@redhat.com

Does this PR introduce a user-facing change?

Fix a bug podman-auto-update to correctly use authentication files when contacting container registries.

[vrothberg]

Definitely a 4.6 candidate but requires a new .1 release c/common before.

[vrothberg]

@edsantiago PTAL

[edsantiago]

I'm seeing timeouts. Continuing to investigate.

[vrothberg]

I'm seeing timeouts. Continuing to investigate.

Thank you! I addressed the comments and will push once I have green light from you.

[edsantiago]

Nothing more that I can find, except, the CI failures look unexpected. I will assume that you understand those and your next push will resolve those failures. So, please go ahead!

[vrothberg]

Nothing more that I can find, except, the CI failures look unexpected. I will assume that you understand those and your next push will resolve those failures. So, please go ahead!

Thanks a lot for your help!

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [edsantiago,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[edsantiago]

Why is the error message different?

[vrothberg]

Why is the error message different?

The first one fails with an TLS error, the second one with an auth one. It passed before on my F38 machine but I assume it's a golang thing.

[edsantiago]

I mean, why is the TLS error different? Your tests passed on my f38. In CI, the TLS error is different. (At least on debian, and ISTR another arch also but the errors are gone). I would like to understand why the TLS errors are different.

[vrothberg]

I mean, why is the TLS error different? Your tests passed on my f38. In CI, the TLS error is different. (At least on debian, and ISTR another arch also but the errors are gone). I would like to understand why the TLS errors are different.

I strongly suspect it's golang.

[umohnani8]

changes LGTM

[Luap99]

Also bump c/common since common/pull/1538 is required to correctly check
for updates. Note that I had to use the go-mod-edit-replace trick on
c/common as c/buildah would otherwise be moved back to 1.30.

This is fine for now but I think we should bump c/common for buildah then vendor buildah here to resolve this properly. These replaces are annoying to deal with in the long run.

LGTM

[edsantiago]

@vrothberg see #19106 for my attempt at refactoring the registry code. I've tried a few slightly-different approaches, last week and today, and am happiest with this.

If you agree that this is a worthwhile approach, please feel free to steal it, or to remove your new tests and just put a "TBD" placeholder (so we don't have to undo all that unnecessary systemd-helper clunkiness). If #19106 fails CI or is otherwise not a desirable approach, no problem.

[vrothberg]

@edsantiago I am cool with merging #19106 as is and rebase this PR on top 👍 OR, I rebase this PR directly on top of it.

[vrothberg]

Rebased on top of #19106 and it works like charm (on my machine). Thanks a lot, @edsantiago!

[vrothberg]

@Luap99 @edsantiago PTanotherL

I am cool to merge as it.

[Luap99]

LGTM but I let @edsantiago to have the final say for the tests

[edsantiago]

eeeeek, please be careful with my registry-refactor, it is going to cause flakes because wait_for_port is currently semi-broken: it seems to be doing "wait for port to be bound", not "wait for someone to actually be listening on the port", and those are not the same thing under podman -p. I was not able to solve this yesterday. A simple sleep 2 after wait_for_port can mitigate the flakes until I get around to fixing it. Which can't be today, sorry.

[vrothberg]

Thank you, @edsantiago! We can wait with merging this PR until the issue is fixed. If 4.6.0 gets out before, then the PR will make it into 4.6.1 :)

[edsantiago]

Seeing your new diffs was a strong motivator for me to get the registry refactor done! New diffs in #19106, CI in progress:

   [registry helper, run_podman ... registry ...]
    cid="$output"

    # wait_for_port isn't enough: that just checks that podman has mapped the port...
    wait_for_port 127.0.0.1 ${PODMAN_LOGIN_REGISTRY_PORT}
    # ...so we look in container logs for confirmation that registry is running.
    _PODMAN_TEST_OPTS="${PODMAN_LOGIN_ARGS}" wait_for_output "listening on .::.:5000" $cid

[vrothberg]

Apologies in case my comments encouraged you to work, @edsantiago. I will remain silent until tomorrow :)

[vrothberg]

Rebased since #19106 just merged. Good to go ✔️

[edsantiago]

/lgtm
/hold

[vrothberg]

/hold cancel

[vrothberg]

Still needs a backport. We can cut a new c/common once containers/common#1505 is in.