Bug 1866437: skip TLS option for pulling indexes #402
Conversation
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ankitathomas, ecordell 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-robot
added
the
approved
|
/retest Please review the full test history for this PR and help us cut down flakes. |
15 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
ankitathomas
changed the title
openshift-ci-robot
added
the
bugzilla/severity-low
|
@ankitathomas: This pull request references Bugzilla bug 1866437, which is valid. 3 validation(s) were run on this bug
In response to this:
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-robot
added
the
bugzilla/valid-bug
|
Is it worth adding a one line log output when running opm commands with the I suppose their intent is already known, based on them including the flag in their command, but maybe its worth reinforcing that this is insecure and for development purposes only? |
| CaFile string | ||
| SkipTLS bool |
There was a problem hiding this comment.
This has got me thinking: do we want to set a custom CA and TLS settings per-request, or for all requests an indexer makes? I don't think we need to change this here, but worth remember for future refactoring.
| } | ||
| } | ||
|
|
||
| func (r *ContainerCommandRunner) argsForCmd(cmd string, args ...string) []string { |
There was a problem hiding this comment.
an alternative approach would be to have a CommandBuilder abstraction to incrementally build out a command and execute it.
| func (r *ContainerCommandRunner) argsForCmd(cmd string, args ...string) []string { | ||
| cmdArgs := []string{cmd} | ||
| switch r.containerTool { | ||
| case PodmanTool: |
There was a problem hiding this comment.
Don't we also need to do this for docker?
There was a problem hiding this comment.
The docker daemon itself maintains a list of insecure remote registries it can talk to, so we'd have to re-run the daemon with a new list of registries if we wanted to support those. Insecure registries on localhost are fine though, we don't need to do anything special with the client for that.
|
/retest |
ankitathomas
force-pushed
the
skiptls
branch
2 times, most recently
from
3163afe to
3e11895
Compare
|
/lgtm |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
@ankitathomas: All pull requests linked via external trackers have merged: operator-framework/operator-registry#402. Bugzilla bug 1866437 has been moved to the MODIFIED state. In response to this:
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. |
Description of the change:
Extending skipTLS option to allow opm to fetch index images from insecure registries with containerd
Motivation for the change:
When using an insecure registry in a dev environment, opm passes the skipTLS option only for pulling of bundles from the registry. Any processing of pre-existing index images requires using docker as a pull tool, since it allows pulling from insecure registries as a default. This PR passes the skipTLS option to pulling of the index images as well, removing the need for having docker as a requirement for working with insecure registries.
Reviewer Checklist
/docs