SDI-2730: Add TLS capability to snap-cli #6
Conversation
candysmurf
force-pushed
the
ssl
branch
10 times, most recently
from
e76a351 to
b35cfe0
Compare
README.md
Outdated
| #### Sample Use Cases | ||
|
|
||
| Here is a list of good and bad command use cases. |
There was a problem hiding this comment.
@candysmurf, could we change this section. My proposition is:
- renaming that to
### Usageor### Examples - change the table below to be more clear which options are provided by snapteld, which of them by snaptel.
- start with successful scenario
- add notice 1 that only GRPC plugins are supported and what happens when non-grpc plugin is tried to be loaded, what error msg will be returned; the goal is to provide to users only such information which will be enough to point them why they receive the error, but no more
- add notice 2 that HTTPS is required + what happens when snapteld was started without HTTPS.
- add notice 3 that there is no trusted CA and what error will be returned. Also, you can mention there about possibility of using "--insecure" flag which is dedicated only for testing purpose (please highlight that usage of this flag is not recommended).
That's all. I think it is quite what you have at the moment, but the order is different and sections should help users find information which they are looking for, so try to keep clearance.
README.md
Outdated
| ``` | ||
|
|
||
| > :collision: Urgh! The server was not started using HTTPs |
There was a problem hiding this comment.
Great idea to explain error messages. However please remove icons - they're unnecessary.
Please use straightforward messages:
What happened: the server was not started using HTTPs.
README.md
Outdated
| ``` | ||
|
|
||
| > :white_check_mark: using this setting to start `snapteld` for a seured GRPC plugin communication. |
There was a problem hiding this comment.
Please include a message highlighting that snapteld started successfully.
Please avoid using icons, they're unnecessary.
README.md
Outdated
| ``` | ||
|
|
||
| > :collision: Urgh! HTTPS does not have a trusted CA. There is no way to specify a CA using a flag for HTTPS currently. Putting the trusted CA in your OS trust store in production. Using --insecure flag for your testing convenience. |
There was a problem hiding this comment.
Please avoid using icons, they're unnecessary. Please review further places in this PR.
snaptel/common.go
Outdated
| @@ -142,6 +149,10 @@ func getErrorDetail(err error, ctx *cli.Context) error { | |||
| case *tasks.UpdateTaskStateUnauthorized: | |||
| return newUsageError(fmt.Sprintf("%v", err.(*tasks.UpdateTaskStateUnauthorized).Payload.Message), ctx) | |||
| default: | |||
| // this is a hack | |||
| if strings.Contains(err.Error(), "tls: oversized record") || strings.Contains(err.Error(), "malformed HTTP response") { | |||
There was a problem hiding this comment.
Such hacks should be avoided. Can't this error situation be generalized?
There was a problem hiding this comment.
@marcintao, a good question. I'm not sure where this error comes from. If you know the place, please let me know. I'll see if we can classify it.
There was a problem hiding this comment.
@marcintao, if it's from third party lib, if we can catch the error somewhere, we may still be able to classify it. Not knowing where it comes from, it's hard.
README.md
Outdated
| ``` | ||
|
|
||
| ##### Error One |
There was a problem hiding this comment.
It looks much better! Thank You. I am just thinking about the "error" section title and how to improve it. What do you think about adding sth like below:
Notice that only GRPC plugins are supported. There is also a requirement to use trusted CA and providing both plugin-cert and plugin-key. Below common error messages are presented that you might receive if one of those requirements are not fulfilled.
Case 1: Missing plugin key
Case 2: Using utrasted CA
Case 3: Trying to set TLS GRPC communication for non-GRPC plugin
Let me know what do you think about that. Thank You.
There was a problem hiding this comment.
@IzabellaRaulin, I love your version. Thanks for your suggestions. I'll make these changes.
|
@IzabellaRaulin: incorporated feedback. Both of you, please review. thanks. |
IzabellaRaulin
changed the title
main.go
Outdated
| @@ -66,13 +73,30 @@ func beforeAction(ctx *cli.Context) error { | |||
| glog.Fatal(err) | |||
| } | |||
|
|
|||
| c := client.NewHTTPClientWithConfig(nil, &client.TransportConfig{Host: u.Host, BasePath: snaptel.FlAPIVer.Value, Schemes: []string{u.Scheme}}) | |||
| tlcOpts := tlsClientOptions{insecureSkipVerify: ctx.Bool("insecure")} | |||
There was a problem hiding this comment.
Please rename TLS-related variables to follow pattern: tlsOpts, tlsClient.
There was a problem hiding this comment.
@marcintao, would you please let me know which variables you like me to change?
There was a problem hiding this comment.
That's tlcOpts and tlcClient, as far as I can grep.
|
Hello @candysmurf, I noticed that PR #8 has been already merged and that is the reason why your PR has a conflict in file snaptel/plugin.go. Are you ok to resolve it? If you have any problem, please contact to @mkleina. |
…ing a plugin error
Summary of Changes
Test Done