fix(registry): deprecated skip-tls and add use-http and skip-tls-verify #898
Conversation
|
Hi @jpower432. 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 Once the patch is verified, the new status will be reflected by the 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
bot
added
the
needs-ok-to-test
|
cc @dinhxuanvu |
Codecov Report
@@ Coverage Diff @@
## master #898 +/- ##
==========================================
- Coverage 52.18% 52.16% -0.03%
==========================================
Files 103 103
Lines 9094 9108 +14
==========================================
+ Hits 4746 4751 +5
- Misses 3442 3451 +9
Partials 906 906
Continue to review full report at Codecov.
|
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dinhxuanvu, jpower432 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@joelanford Would you mind taking a look? Thanks |
cmd/opm/alpha/bundle/unpack.go
Outdated
| if skipTLS { | ||
| // Set useHTTP when use deprecated skipTlS | ||
| // for functional parity with existing | ||
| useHTTP = true | ||
| } |
There was a problem hiding this comment.
What happens if someone uses --skip-tls=true --use-http=false? Should we error out in that case?
Maybe even better, should we just make skip-tls mutually exclusive from both skip-tls-verify and use-http?
There was a problem hiding this comment.
Also, it seems like --skip-tls-verify=false --use-http=true is an invalid combination, right?
There was a problem hiding this comment.
Agree with doing the flag combination checks. My intent here was to make it simpler to rip out skip-tls eventually because it is being deprecated here, but I see how doing that might introduce some unexpected changes since it may not always equate to use-http in every case. I will make that change.
There was a problem hiding this comment.
Not sure if it's possible, but given this code is repeated in so many places, it might be worth taking a look at writing a helper function somewhere in the cmd tree (perhaps cmd/internal/util or something?) so it can be reused everywhere.
The skip-tls flag currently uses HTTP instead of the expected behavior of skipping TLS cert validated with HTTPS registries. The new flags seperate untrusted HTTPS from HTTP registries and behave as expected Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
jpower432
force-pushed
the
bug/skip-tls-http
branch
from
926e014 to
590f334
Compare
…mmand Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
|
@joelanford Ready for the final review. |
|
/lgtm |
4 participants
The skip-tls flag currently uses HTTP instead of the expected behavior of skipping
TLS cert validated with HTTPS registries. The new flags seperate untrusted HTTPS from
HTTP registries and behave as expected
Signed-off-by: Jennifer Power barnabei.jennifer@gmail.com
Description of the change:
This PR deprecated the
skip-tlsflag and create two new flags,skip-tls-verifyanduse-http. The new flags disable TLS validation, use plain HTTP, respectively.Motivation for the change:
The motivation for the changed is resolve the confusion due to the
skip-tlsflag description and to allow the use of self-signed or untrusted HTTPS when usingopmReviewer Checklist
/docsRelated to #872