Update c/image for sysregistriesv2 changes and automatic docker:// insecure configuration #1930

mtrmac · 2018-12-03T22:23:15Z

This updates c/image to include containers/image#468 , buildah to include containers/buildah#1214, and updates the callers.

In addition to the straightforward API change, it also rips out the existing code manually handling the list of insecure registries; this should now be handled by the docker:// transport.

See the individual commit messages for details.

The way types.SystemContext.SystemRegistriesConfPath is set continues to be fairly irregular, only at the places that immediately need it; this really feels like something that should be set in one (and no more than one) shared helper, and types.SystemContext values should be passed along either the call stack or the data structures (Runtime?); this PR only makes a localized transition, admits to it via added FIXMEs, and does not have the ambition to rework the SystemContext use at all.

Warning: I haven’t tested this manually one bit.

This is a prerequisite to merging the blob-info-caching branch.

vrothberg · 2018-12-04T13:33:06Z

/approve

openshift-ci-robot · 2018-12-04T13:33:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vrothberg]

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

vrothberg · 2018-12-04T13:36:23Z

/test images

giuseppe · 2018-12-04T14:16:01Z

pkg/registries/registries.go

previously the REGISTRIES_CONFIG_PATH environment variable was honored also for the rootless case.

It is probably enough to change the order of the checks

Oops, thanks for catching this. Should be fixed now.

vrothberg

Just minor nits. Thanks for the nice PR! Especially for breaking the PR up into easier to digest pieces.

vrothberg · 2018-12-04T14:08:27Z

docs/podman-login.1.md

s/used,/used./

If set to false + "," + then

Same for docs/podman-push.1.md

Thanks, updated all 5 man pages with --tls-verify similarly.

vrothberg · 2018-12-04T14:23:07Z

libpod/image/image.go

An idea for future work, orthogonal to this PR: maybe a (OptinalBoolFalse).IsFalse() method would be nice. Same for IsTrue().

(Or, maybe, at a higher level, something roughly like TLSVerificationDisabled, to also clarify he double negatives in SkipVerify…. And, independently, in podman, a shared helper for the !c.BoolT("tls-verify") to make sure there can’t be simple typos… — or, ultimately, a CLI rework that does not rely on the "tis-verify" string.

Luckily, this particular line was completely removed in one of the next commits, in favor of sysregistriesv2 doing the right thing, and no check of an optional boolean is the best kind of check of an optional boolean :) )

rh-atomic-bot · 2018-12-04T20:08:30Z

☔ The latest upstream changes (presumably #1932) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2018-12-05T21:03:35Z

/lgtm

rhatdan · 2018-12-05T21:04:12Z

Nice work @mtrmac and @vrothberg

rh-atomic-bot · 2018-12-05T22:55:40Z

☔ The latest upstream changes (presumably #1924) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2018-12-06T08:22:14Z

@mtrmac Needs a rebase.

rh-atomic-bot · 2018-12-06T15:02:36Z

☔ The latest upstream changes (presumably #1946) made this pull request unmergeable. Please resolve the merge conflicts.

mtrmac · 2018-12-06T17:57:43Z

Rebased, tests are passing.

rh-atomic-bot · 2018-12-06T20:09:08Z

☔ The latest upstream changes (presumably #1905) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2018-12-06T21:22:15Z

@mtrmac Sadly we got a conflict.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

This updates buildah for the sysregistriesv2 changes. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... instead of unnecessarily adapting it for the DockerInsecureSkipTLSVerify type change. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Unrelated to the rest of the PR. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Following SystemContext.DockerInsecureSkipTLSVerify, make the DockerRegistryOne also an OptionalBool, and update callers. Explicitly document that --tls-verify=true and --tls-verify unset have different behavior in those commands where the behavior changed (or where it hasn't changed but the documentation needed updating). Also make the --tls-verify man page sections a tiny bit more consistent throughout. This is a minimal fix, without changing the existing "--tls-verify=true" paths nor existing manual insecure registry lookups. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

DockerRegistryOptions.DockerInsecureSkipTLSVerify as an types.OptionalBool can now represent that value, so forceSecure is redundant. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

The newly introduced SystemRegistriesConfPath somewhat decreases duplication, but more importantly will allow future callers to set just a types.SystemContext.SystemRegistriesConfPath and not call GetRegistries / GetInsecureRegistries at all. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Instead, just set SystemRegistriesConfPath and let the transport do it. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

DockerRegistryOptions.DockerInsecureSkipTLSVerify as an types.OptionalBool can now represent that value, so forceSecure is redundant. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Instead, just set SystemRegistriesConfPath and let the transport do it. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

i.e. actually reflect the environment variable and/or rootless mode instead of always using the default path. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Instead, just set SystemRegistriesConfPath and let the transport do it. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac · 2018-12-06T22:36:33Z

Rebased again; non-Linux builds are broken, so absolutely untested – not even that it compiles.

mtrmac · 2018-12-06T23:18:35Z

Tests are passing. PTAL

mheon · 2018-12-06T23:24:53Z

LGTM

mheon · 2018-12-06T23:25:01Z

/lgtm

openshift-ci-robot requested review from giuseppe and umohnani8 December 3, 2018 22:23

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 3, 2018

mtrmac force-pushed the sysregistriesv2 branch 3 times, most recently from 1022f43 to 9ef5816 Compare December 4, 2018 13:28

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2018

mtrmac mentioned this pull request Dec 4, 2018

Vendor c/image + Buildah after merging blob-info-caching in both #1857

Merged

giuseppe reviewed Dec 4, 2018

View reviewed changes

mtrmac force-pushed the sysregistriesv2 branch from 9ef5816 to aff7bbf Compare December 4, 2018 14:24

vrothberg reviewed Dec 4, 2018

View reviewed changes

vrothberg mentioned this pull request Dec 4, 2018

WIP: move to sysregistriesv2 #709

Closed

mtrmac force-pushed the sysregistriesv2 branch from aff7bbf to 37e0665 Compare December 4, 2018 15:22

mtrmac force-pushed the sysregistriesv2 branch from 37e0665 to 2129d1b Compare December 4, 2018 21:01

mtrmac mentioned this pull request Dec 5, 2018

Vendor in latest containers/storage #1946

Merged

openshift-ci-robot assigned rhatdan Dec 5, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2018

mtrmac force-pushed the sysregistriesv2 branch from 2129d1b to ceccbbf Compare December 6, 2018 15:00

openshift-ci-robot added size/XL and removed lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 6, 2018

mtrmac force-pushed the sysregistriesv2 branch 2 times, most recently from 042ff83 to 0794d52 Compare December 6, 2018 17:07

mtrmac added 12 commits December 6, 2018 23:31

Update containers/image to 63a1cbdc5e6537056695cf0d627c0a33b334df53

33fcb35

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Vendor buildah after merging containers/buildah#1214

d3be6b8

This updates buildah for the sysregistriesv2 changes. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Remove some unused data structures and code

6a0afce

... instead of unnecessarily adapting it for the DockerInsecureSkipTLSVerify type change. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Fix build on non-Linux

b70f8b3

Unrelated to the rest of the PR. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Remove the forceSecure parameter of Image.PushImageTo*

7407d66

DockerRegistryOptions.DockerInsecureSkipTLSVerify as an types.OptionalBool can now represent that value, so forceSecure is redundant. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Remove manual handling of insecure registries in PushImageToReference

1b893be

Instead, just set SystemRegistriesConfPath and let the transport do it. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Remove the forceSecure parameter on the pull call stack

93e14b6

DockerRegistryOptions.DockerInsecureSkipTLSVerify as an types.OptionalBool can now represent that value, so forceSecure is redundant. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Remove manual handling of insecure registries in doPullImage

e885311

Instead, just set SystemRegistriesConfPath and let the transport do it. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Fix reporting the registries.conf path on error

99d9626

i.e. actually reflect the environment variable and/or rootless mode instead of always using the default path. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Remove manual handling of insecure registries in (podman search)

489164f

Instead, just set SystemRegistriesConfPath and let the transport do it. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac force-pushed the sysregistriesv2 branch from 0794d52 to 489164f Compare December 6, 2018 22:36

openshift-ci-robot assigned mheon Dec 6, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2018

openshift-merge-robot merged commit a387c72 into containers:master Dec 6, 2018

mtrmac deleted the sysregistriesv2 branch December 6, 2018 23:31

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 27, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023

Update c/image for sysregistriesv2 changes and automatic docker:// insecure configuration #1930

Update c/image for sysregistriesv2 changes and automatic docker:// insecure configuration #1930

Uh oh!

Conversation

mtrmac commented Dec 3, 2018

Uh oh!

vrothberg commented Dec 4, 2018

Uh oh!

openshift-ci-robot commented Dec 4, 2018

Uh oh!

vrothberg commented Dec 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rh-atomic-bot commented Dec 4, 2018

Uh oh!

rhatdan commented Dec 5, 2018

Uh oh!

rhatdan commented Dec 5, 2018

Uh oh!

rh-atomic-bot commented Dec 5, 2018

Uh oh!

rhatdan commented Dec 6, 2018

Uh oh!

rh-atomic-bot commented Dec 6, 2018

Uh oh!

mtrmac commented Dec 6, 2018

Uh oh!

rh-atomic-bot commented Dec 6, 2018

Uh oh!

rhatdan commented Dec 6, 2018

Uh oh!

mtrmac commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Dec 6, 2018

Uh oh!

mheon commented Dec 6, 2018

Uh oh!

mheon commented Dec 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mtrmac commented Dec 6, 2018 •

edited

Loading