Support Self-Signed CA Cert and skip-cert-verify with Hub Client #210
Conversation
anujc25
force-pushed
the
support-self-signed-ca-with-hub-client
branch
from
7941050 to
2250ed3
Compare
anujc25
force-pushed
the
support-self-signed-ca-with-hub-client
branch
from
2250ed3 to
adc517b
Compare
config/certs_test.go
Outdated
| assert.Nil(t, err) | ||
| assert.Equal(t, cert1, ctx) | ||
|
|
||
| ctx, err = GetCert("https://test1") |
There was a problem hiding this comment.
nit: This was already existing, but can we change the ctx to something so that it is more opt?
There was a problem hiding this comment.
+1
ctx->cert throughout would have made more sense.
prkalle
left a comment
prkalle
left a comment
There was a problem hiding this comment.
LGTM. I have a minor nit comment. Thanks
vuil
left a comment
vuil
left a comment
There was a problem hiding this comment.
very nice!
Some nits and suggestion for additional test cases.
|
|
||
| // GetCert retrieves the cert configuration by host | ||
| func GetCert(host string) (*configtypes.Cert, error) { | ||
| if host == "" { |
There was a problem hiding this comment.
suggestion: update comment to say "by host or URI"
config/certs_test.go
Outdated
| assert.Nil(t, err) | ||
| assert.Equal(t, cert1, ctx) | ||
|
|
||
| ctx, err = GetCert("https://test1/fake") |
There was a problem hiding this comment.
suggestion: add 3 more cases for
ws://test1/fake and
wss://test1/fake to show this works for websocket URIs as well.
and
https://test1:12345/fake for custom port handling
| ctx, err = GetCert("https://test1/fake") | ||
| assert.Nil(t, err) | ||
| assert.Equal(t, cert1, ctx) | ||
|
|
There was a problem hiding this comment.
nit: suggest add another case of invalid uri
ctx, err = GetCert("https://tes t1/fak e") or something, should we choose to return a better error as suggested above.
| if host == "" { | ||
| return nil, errors.New("host is empty") | ||
| } | ||
| u, err := url.Parse(host) |
There was a problem hiding this comment.
an unparsable URI ends up getting the same error as a missing entry.
how about just return here,
with errors.New("invalid uri") or wrap the err?
|
another nit: |
anujc25
force-pushed
the
support-self-signed-ca-with-hub-client
branch
from
450d083 to
c4dede5
Compare
anujc25
force-pushed
the
support-self-signed-ca-with-hub-client
branch
from
c4dede5 to
c3c2e33
Compare
…date to GetCert API (vmware-tanzu#210) * Support self-signed CA Cert and skip-cert-verify with Hub Client * GetCert API now accepts URI along with hostname
…date to GetCert API (#210) * Support self-signed CA Cert and skip-cert-verify with Hub Client * GetCert API now accepts URI along with hostname
5 participants
What this PR does / why we need it
GetCertAPI to get the certificate data for the specified hub endpoint.Which issue(s) this PR fixes
Fixes #
Describe testing done for PR
Release note
Additional information
Special notes for your reviewer