The `--skip-tls` flag shouldn't use the http protocol #872

omertuc · 2021-11-26T14:54:03Z

Description of the change:
This commit makes it so that the https scheme is still used even when
the --skip-tls flag is specified

Motivation for the change:
When using the operator-sdk run bundle's --skip-tls flag which is
described as:

skip authentication of image registry TLS certificate when pulling a bundle image in-cluster

It tries to access the image registry (given in the <bundle_image>
positional argument) using HTTP rather than HTTPS.

This behavior is unexpected and fails when the image registry only
speaks the HTTPS protocol.

The commit (a16399f) which seems to have introduced
this behavior doesn't mention this behavior anywhere, so I'm assuming
it's unintentional and therefore a bug that needs fixing - Let me know
if you think otherwise.

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

openshift-ci · 2021-11-26T14:54:12Z

Hi @omertuc. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-11-26T14:54:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: omertuc
To complete the pull request process, please assign dinhxuanvu after the PR has been reviewed.
You can assign the PR to them by writing /assign @dinhxuanvu in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

codecov · 2021-11-26T15:00:53Z

Codecov Report

Merging #872 (0ef24ff) into master (f478fdd) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #872      +/-   ##
==========================================
+ Coverage   52.06%   52.07%   +0.01%     
==========================================
  Files         103      103              
  Lines        9063     9063              
==========================================
+ Hits         4719     4720       +1     
+ Misses       3439     3438       -1     
  Partials      905      905

Impacted Files	Coverage Δ
pkg/registry/query.go	`60.93% <0.00%> (+0.52%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f478fdd...0ef24ff. Read the comment docs.

When using the `operator-sdk run bundle`'s `--skip-tls` flag which is described as: > skip authentication of image registry TLS certificate when pulling a bundle image in-cluster It tries to access the image registry (given in the <bundle_image> positional argument) using HTTP rather than HTTPS. This behavior is unexpected and fails when the image registry only speaks the HTTPS protocol. This commit fixes it so that the `https` scheme is still used even when the `--skip-tls` flag is specified The commit (a16399f) which seems to have introduced this behavior doesn't mention this behavior anywhere, so I'm assuming it's unintentional and therefore a bug that needs fixing. Signed-off-by: Omer Tuchfeld <otuchfel@redhat.com>

perdasilva · 2021-11-29T18:23:40Z

pkg/image/containerdregistry/resolver.go

 		)),
 		docker.WithClient(client),
 	}
-	if insecure {


Question: Have you checked your changes still work with http endpoints?
I'm not sure what these regopts do exactly. Should we maybe still keep this but only add if if the url scheme is http?
@njhale any thoughts here?

I have not checked that, good point. Didn't think about that.

But if I do think of it, why would someone pass --skip-tls when using HTTP in the first place? And also, how does one even use http registries? e.g. How would you tell docker to use http to communicate with quay.io/foo/bar:latest? It uses https by default. Searching online didn't lead me to a conclusive answer, prefixing with http:// i.e. http://quay.io/foo/bar:latest is illegal

Here's how it's done:
https://docs.docker.com/registry/insecure/#deploy-a-plain-http-registry

Basically a config file lists all registries where docker should try to communicate with HTTPS, if it gets a bad cert, ignore any errors, if there's no HTTPS on that server at all then it fallbacks to HTTP.

Doesn't seem like you can specify the protocol in the image address itself in any way I could find

Also this of-course applies to the Docker CLI, not sure about the behavior for the docker library used here

I agree with you here. --skip-tls and http are not related at all. I just don't know if these regopts we are removing here are needed for something specific (e.g. pipeline). We may still need to add a --use-http options or something like that. So, I'll set it to ok-to-test but I'd like @njhale (who I think originally added this) to ok.

Original author is here a16399f

We can't change/remove current --skip-tls functionality, but we can

update the flag's description to relay it's current functionality

deprecate the flag

add new flags --skip-tls-verify and --use-http to enable their respective namesake's functionality

Assuming @omertuc does not update this PR soon, @dinhxuanvu @jpower432 can one of you take on the changes in a separate PR?

Ah didn't realize your bullets were concrete change suggestions.

Yeah please take it on yourselves as I don't feel confident enough in this code base to make those changes

Sounds good, thanks for all you've done so far!

I can make these changes.

perdasilva · 2021-11-29T23:20:14Z

/ok-to-test

estroz · 2021-12-17T19:06:00Z

This is something I'd definitely like to see fixed. Thanks for your submission @omertuc!

openshift-ci · 2022-01-21T02:16:58Z

@omertuc: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dinhxuanvu · 2022-01-21T02:36:35Z

This PR is replaced by #898

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 26, 2021

openshift-ci bot requested review from exdx and kevinrizza November 26, 2021 14:54

omertuc force-pushed the skiptlspr branch from 3f60c53 to 0ef24ff Compare November 26, 2021 16:45

perdasilva reviewed Nov 29, 2021

View reviewed changes

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 29, 2021

jpower432 mentioned this pull request Dec 17, 2021

[BUG] oc-mirror will only use HTTPS with TLS validation turned off in some operations and not others. openshift/oc-mirror#209

Closed

jpower432 mentioned this pull request Jan 14, 2022

fix(registry): deprecated skip-tls and add use-http and skip-tls-verify #898

Merged

5 tasks

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 21, 2022

dinhxuanvu closed this Jan 21, 2022

The --skip-tls flag shouldn't use the http protocol #872

The --skip-tls flag shouldn't use the http protocol #872

Uh oh!

Conversation

omertuc commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Nov 26, 2021

Uh oh!

openshift-ci bot commented Nov 26, 2021

Uh oh!

codecov bot commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

perdasilva Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

omertuc Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omertuc Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perdasilva Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

omertuc Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

estroz Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

omertuc Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

estroz Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

jpower432 Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

perdasilva commented Nov 29, 2021

Uh oh!

estroz commented Dec 17, 2021

Uh oh!

openshift-ci bot commented Jan 21, 2022

Uh oh!

dinhxuanvu commented Jan 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

The `--skip-tls` flag shouldn't use the http protocol #872

The `--skip-tls` flag shouldn't use the http protocol #872

omertuc commented Nov 26, 2021 •

edited

Loading

codecov bot commented Nov 26, 2021 •

edited

Loading

omertuc Nov 29, 2021 •

edited

Loading

omertuc Nov 29, 2021 •

edited

Loading

estroz Dec 17, 2021 •

edited

Loading