Add --cacert flag to velero cli commands #2364
Conversation
mansam
force-pushed
the
add-cacert-flag-to-client
branch
from
7e21540
to
32be406
Compare
Adds a --cacert flag to the log and describe commands that takes a path to a PEM-encoded certificate bundle as an alternative to --insecure-skip-tls-verify for dealing with self-signed certificates. Signed-off-by: Sam Lucidi <slucidi@redhat.com>
mansam
force-pushed
the
add-cacert-flag-to-client
branch
from
b542474
to
212a79d
Compare
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
mansam
changed the title
carlisia
requested review from
skriss,
nrb,
carlisia and
ashish-amarnath
and removed request for
skriss
There was a problem hiding this comment.
If you think these two comments make sense in your installer PR #2368 (comment), then please name the file/path var here the same. Other then that, this lgtm!
|
Could we have documentation for the commands added, too? |
| caPool.AppendCertsFromPEM(caCert) | ||
| } | ||
| httpClient := new(http.Client) | ||
| httpClient.Transport = &http.Transport{ |
There was a problem hiding this comment.
I think we probably want to use the timeout passed to Stream on the transport, since the Go HTTP client doesn't have one by default and will just wait forever if it's not specified (https://golang.org/src/net/http/transport.go#L211).
There was a problem hiding this comment.
Seems like the right call. While fixing that I also set the rest of the values on the transport to match go's DefaultTransport. https://golang.org/src/net/http/transport.go#L42
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
mansam
force-pushed
the
add-cacert-flag-to-client
branch
from
ccb0374
to
5fd333c
Compare
|
Happy to add documentation for the new flags though I don't see a good place to do so. @nrb where would you recommend I add that? |
Hm yeah, that is a very good question. My preference is it should go under "Use", much like the page on "Run in any namespace". A small text on the reason why it's neeced/what it solves, what commands to use it with, and the documentation for how to use it (basically the documentation for the cmd itself, name of cmd + file name/path). @nrb what is your preference? |
|
🤔 Yeah, this question's always the tough one to answer. Given your team is mostly concerned with restic integration @mansam, I was thinking https://velero.io/docs/v1.3.1/restic/, but this is useful beyond restic. I think @carlisia's suggestion for an entry under the Use heading is good, too. Maybe a new page documenting using the entirety of the feature set end-to-end called "Custom Certificate Bundles"? If it's a brand new page, I'm ok with that documentation being in it's own PR. |
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
mansam
force-pushed
the
add-cacert-flag-to-client
branch
from
5fd333c
to
0329a4e
Compare
|
Yes, this lgtm! |
Fixes #2330
Adds a --cacert flag to the log and describe commands
that takes a path to a PEM-encoded certificate bundle
as an alternative to --insecure-skip-tls-verify for
dealing with self-signed certificates.
Signed-off-by: Sam Lucidi slucidi@redhat.com