[FAB-18196] Add Channel Participation CLI #1907

stephyee · 2020-09-21T18:35:18Z

Type of change

New feature

Description

Adds CLI commands for the list, join, and remove channel participation APIs.

Related issues

FAB-18196

C0rWin · 2020-10-17T00:42:52Z

@stephyee there is no unit tests in this PR, could you add some to make sure the new code proceeds as expected?

tock-ibm · 2020-10-18T06:19:33Z

cmd/participation/remove.go

+
+	if resp.StatusCode != http.StatusNoContent {
+		panic(fmt.Sprintf("Removal of channel %s failed with status %d", channelID, resp.StatusCode))
+	}


In case of a failed request that returns an error status code, the response contains an informative error message. It is better to print it as well. This relates to all commands. See
https://docs.google.com/document/d/1kYoicS_Z59gNvmuuB7VFZ6WsOL31C34dWMeOFQiIygA/edit?usp=sharing

Refactored the code. We now print the status and error generated by the server for all commands.

tock-ibm · 2020-10-18T06:36:46Z

cmd/participation/join.go

+	bodyBytes, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		panic(err)
+	}
+	resp.Body.Close()
+
+	err = printResponseAsJSON(bodyBytes, &channelInfo{}, os.Stdout)
+	if err != nil {
+		panic(err)
+	}


What about checking the status to detect a failed request, and printing the body if it is a failure?

Refactored. Done.

tock-ibm · 2020-10-18T06:37:16Z

cmd/participation/list.go

+	bodyBytes, err := getBodyBytes(client, url)
+	if err != nil {
+		panic(err)
+	}
+
+	err = printResponseAsJSON(bodyBytes, &channelList{}, os.Stdout)
+	if err != nil {
+		panic(err)
+	}


What about checking the status to detect a failed request, and printing the body if it is a failure?

Refactored. Done.

tock-ibm · 2020-10-18T06:37:48Z

cmd/participation/list.go

+	if bodyBytes == nil {
+		fmt.Printf("channel '%s' not found\n", channelID)
+		return
+	}
+
+	err = printResponseAsJSON(bodyBytes, &channelInfo{}, os.Stdout)
+	if err != nil {
+		panic(err)
+	}


What about checking the status to detect a failed request, and printing the body if it is a failure?

Refactored. Done.

tock-ibm · 2020-10-18T06:39:17Z

cmd/participation/main.go

+	if resp.StatusCode != http.StatusOK {
+		return nil, nil
+	}


In case of a failed request that returns an error status code, the response contains an informative error message. It is better to print it as well.

Refactored. Done.

tock-ibm · 2020-10-18T06:41:41Z

cmd/participation/list.go

+	if bodyBytes == nil {
+		fmt.Printf("channel '%s' not found\n", channelID)
+		return
+	}


There is a specific error code that captures this. I think it is better to follow these.

Refactored. Done.

jyellick

Looks like the function is here, but I think it could use some refactoring, and definitely use some tests. I've added some comments of my own in addition to the good set from Yoav.

jyellick · 2020-10-20T02:57:17Z

cmd/participation/main.go

+}
+
+func main() {
+	var osn, channelID, tlsDir, configBlockPath string


This is a fairly small CLI, so perhaps it's not worth the effort, but I think the testability could be increased considerably by:

Defining some sort of argument structure, containing the things that the flags may set.

Parsing arguments as a []string so that you can test that different command lines stay valid.

Using a more sophisticated CLI parsing library -- for instance, we use https://godoc.org/gopkg.in/alecthomas/kingpin.v2 in some places, and by specifying the right flag types, you get things like checking that a directory exists, or that a file exists and giving you an open stream to it. Common things that you won't need to write, or test, yourself.

jyellick · 2020-10-20T02:57:43Z

cmd/participation/join.go

+func join(osn, tlsDir, channelID, configBlockPath string) {
+	blockBytes, err := ioutil.ReadFile(configBlockPath)
+	if err != nil {
+		panic(err)


A general comment, why so many panics here? Why not simply return the error.

Refactored. Done.

jyellick · 2020-10-20T03:01:14Z

cmd/participation/join.go

+	}
+	resp.Body.Close()
+
+	err = printResponseAsJSON(bodyBytes, &channelInfo{}, os.Stdout)


In general, it's a little odd to have the console printing pushed out into the leaf of the calls. I'd generally expect for a response and error to be propagated back up to the top, where, uniform decisions can be made about how to report success and failure, the output format, etc.

Refactored. Done.

jyellick · 2020-10-20T03:06:00Z

cmd/participation/main.go

+
+	joinCmd := flag.NewFlagSet("join", flag.ExitOnError)
+	joinCmd.StringVar(&osn, "orderer", "", "Ordering service endpoint")
+	joinCmd.StringVar(&tlsDir, "tlsDir", "", "Path to directory containing TLS materials")


The contents of this directory should be validated before we proceed with the invocations. Going back to a previous comment, we should do all of our parsing and setup early and in a consolidated place.

jyellick · 2020-10-20T03:10:36Z

cmd/participation/main.go

+	return bodyBytes, nil
+}
+
+func printResponseAsJSON(bodyBytes []byte, response interface{}, out io.Writer) error {


It's not the end of the world, but we have a JSON string, and we're turning it back into a map structure, then turning it back into JSON. You could instead use vanilla indent https://golang.org/pkg/encoding/json/#Indent

Refactored. Done.

wlahti · 2020-10-23T15:42:08Z

I did a pretty extensive refactoring of the code here. Still need to address the comments around flags and directory validation as well as add unit tests. More to come shortly.

wlahti · 2020-10-23T15:43:37Z

cmd/participation/main.go

+func printResponse(resp *http.Response, out io.Writer) {
+	bodyBytes, err := readBodyBytes(resp.Body)
+	if err != nil {
+		log.Fatalf("failed to read http response body: %s", err)


Not sure this is the "correct" thing to do here but for now I figured if we can't read the response body something has gone very wrong.

denyeart · 2020-10-26T04:04:30Z

/azp run

azure-pipelines · 2020-10-26T04:04:41Z

Azure Pipelines successfully started running 1 pipeline(s).

wlahti · 2020-10-27T16:43:44Z

I've updated this CLI to use kingpin and added some unit tests around flag parsing.

I think more unit tests are probably need but I'm a little confused on how to actually implement them. I don't see an obvious example to base my tests on. Should I create an http endpoint with a fake channel participation API that sends back faked responses or is that beyond the scope of UT here?

mastersingh24

I'd really like to understand why we are doing this? I apologize if I missed this as part of the RFC, but we really do not need another standalone CLI.

The beauty of REST APIs is that there are tons of code generators for virtually any language which can generate a client from Swagger.

I don't think we should merge this.

Discussed with a few other maintainers

wlahti · 2020-10-29T20:23:20Z

Added a fairly comprehensive set of unit tests now, which also helped me notice that we weren't supporting the various TLS configurations. This CLI now supports TLS, mutual TLS, and TLS disabled. I also moved away from the TLS directory flag with expected file names (an artifact of the test code we used as a basis for the http client code) to a "--caFile" flag (when the server is using TLS) and "--clientCert" + "--clientKey" flags (when the server is using mutual TLS).

jyellick

Generally looking pretty good -- test coverage looks nice.

I do think with a (small) rework of the main, you could get rid of all the forking, and session reading from your test. And, since the Config struct is not a parameter anywhere outside of the main, it doesn't really need to be exported anymore (or exist).

The biggest thing though that I caught on this pass, is that as written, this CLI is incompatible with intermediate CAs. I see this as a must-fix, although it should be pretty easy to do. Assuming we go the route of allowing the user to concatenate PEMs into a single file, the golang x509 CertPool will happily parse it with no additional work. Then it's just a matter of wiring the cert pool through.

jyellick · 2020-10-30T03:25:21Z

cmd/osnadmin/main.go

+)
+
+// command line flags
+var (


All but one of these variables is only referenced inside of configFromFlags(). Only the app is accessed from main().

So, why not simply do the following:

func executeForArgs(args []string) (text string, exit int, err error) { app := kingping.New(...) ... cmd, err := app.Parse(args) // handle err // handle cmd using the above vars directly. // make sure you only return an error for argument processing errors // otherwise simply return the desired output text and code // if the text could be large, we could consider a reader instead of a string // but we're already using ioutil.ReadAll generally, so it's equivalent. } func main() { text, exit, err := executeForArgs(os.Args[1:]) if err != nil { kingpin.Fatalf("Error parsing arguments, %s, try --help", err) } fmt.Print(text) os.Exit(exit) }

Now, you've ditched all of your package level state, and you can test that command parsing works as expected by passing in a slice of args, rather than having to try to invoke main().

I know I pushed you down the "Config struct" route, but, since you've decided not to make the Config struct a parameter to your functions, its value as an exported entity doesn't really make sense. All it's really serving to do is to add some unnecessary indirection.

Excellent idea. Done.

jyellick · 2020-10-30T03:29:55Z

cmd/osnadmin/main.go

+var (
+	app        = kingpin.New("osnadmin", "Orderer Service Node (OSN) administration")
+	orderer    = app.Flag("orderer", "Endpoint of the OSN").String()
+	caFile     = app.Flag("caFile", "Path to the PEM-encoded orderer root CA certificate").String()


Why make this a string? Why not an ExistingFile()

Same goes for the other files here.

Still need to look into this...

jyellick · 2020-10-30T03:31:09Z

cmd/osnadmin/main.go

+var (
+	app        = kingpin.New("osnadmin", "Orderer Service Node (OSN) administration")
+	orderer    = app.Flag("orderer", "Endpoint of the OSN").String()
+	caFile     = app.Flag("caFile", "Path to the PEM-encoded orderer root CA certificate").String()


Help text should probably not say "the root CA", we either need to allow parsing a single pem with lots of certs (we do this some places, not all), or provide a directory of certs. Also, we need to support intermediate certs as well (which is mostly for free if you parse as a cert pool).

Still need to look into this...

jyellick · 2020-10-30T03:48:28Z

cmd/osnadmin/main_test.go

+				"--orderer", urlString,
+				"--caFile", ordererCACert,
+			}
+			sess := runCommand(args, 0)


If you convert the main to allow execution from a slice, then you could eliminate this 'runCommand', and instead pass in the slice, and test the results directly.

jyellick · 2020-10-30T03:52:49Z

internal/osnadmin/httpclient.go

+)
+
+func httpClient(tlsClientCert *tls.Certificate, tlsCACert *x509.Certificate) *http.Client {
+	clientCertPool := x509.NewCertPool()


I really think we need to be passing the cert pool in as the parameter, not the certificate, all the way from the very top. Then we can parse a multi-block pem file and allow users to concatenate CAs into it as needed.

Still need to look into this...

jyellick · 2020-10-30T03:56:29Z

cmd/osnadmin/main_test.go

+				ordererCACert = "not-the-ca-cert-youre-looking-for"
+			})
+
+			It("returns with exit code 1 and prints the error", func() {


You could probably eliminate most of these test cases by using ExistingFile() from kingpin.

Still need to look into this...

jyellick · 2020-10-30T03:57:22Z

cmd/osnadmin/main_test.go

+	return sess
+}
+
+func checkOutput(sess *gexec.Session, expectedStatus int, expectedOutput interface{}) {


This gets to turn into a vanilla Equals() too.

jyellick · 2020-10-30T03:58:28Z

cmd/osnadmin/main_test.go

+	Expect(err).NotTo(HaveOccurred())
+}
+
+func blockWithGroups(groups map[string]*cb.ConfigGroup, channelID string) *cb.Block {


It's unfortunate how much knowledge of the participation internal workings bleeds into this test, but I understand that this may be the best option.

Adds CLI commands for the list, join, and remove using the new channel participation API. FAB-18196 #done Signed-off-by: Will Lahti <wtlahti@us.ibm.com> Signed-off-by: Tiffany Harris <tiffany.harris@ibm.com>

jyellick

Looks good to me!

stephyee force-pushed the fab-18196 branch 3 times, most recently from 42c0dd7 to b97f04d Compare September 22, 2020 18:25

stephyee marked this pull request as ready for review September 28, 2020 13:13

stephyee requested a review from a team as a code owner September 28, 2020 13:13

C0rWin added the channel-participation-api label Oct 15, 2020

tock-ibm reviewed Oct 18, 2020

View reviewed changes

jyellick reviewed Oct 20, 2020

View reviewed changes

wlahti force-pushed the fab-18196 branch from b97f04d to 75e9aba Compare October 23, 2020 15:36

wlahti reviewed Oct 23, 2020

View reviewed changes

wlahti force-pushed the fab-18196 branch 2 times, most recently from 64e70e0 to c6cd398 Compare October 23, 2020 21:00

wlahti force-pushed the fab-18196 branch 7 times, most recently from 8b0bd3e to 42fa171 Compare October 27, 2020 16:39

wlahti force-pushed the fab-18196 branch from 42fa171 to ad40d82 Compare October 27, 2020 19:15

mastersingh24 previously requested changes Oct 28, 2020

View reviewed changes

wlahti force-pushed the fab-18196 branch 3 times, most recently from b2bab1b to 7da6842 Compare October 29, 2020 19:54

jyellick suggested changes Oct 30, 2020

View reviewed changes

wlahti force-pushed the fab-18196 branch 14 times, most recently from 2845604 to 0bbc9b4 Compare November 2, 2020 20:06

osnadmin CLI

0675142

Adds CLI commands for the list, join, and remove using the new channel participation API. FAB-18196 #done Signed-off-by: Will Lahti <wtlahti@us.ibm.com> Signed-off-by: Tiffany Harris <tiffany.harris@ibm.com>

wlahti force-pushed the fab-18196 branch from 0bbc9b4 to 0675142 Compare November 2, 2020 20:13

jyellick approved these changes Nov 2, 2020

View reviewed changes

jyellick merged commit ea81d21 into hyperledger:master Nov 2, 2020

[FAB-18196] Add Channel Participation CLI #1907

[FAB-18196] Add Channel Participation CLI #1907

Conversation

stephyee commented Sep 21, 2020

Type of change

Description

Related issues

C0rWin commented Oct 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyellick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlahti Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

wlahti commented Oct 23, 2020

Choose a reason for hiding this comment

denyeart commented Oct 26, 2020

azure-pipelines bot commented Oct 26, 2020

wlahti commented Oct 27, 2020

mastersingh24 left a comment

Choose a reason for hiding this comment

wlahti commented Oct 29, 2020

jyellick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyellick left a comment

Choose a reason for hiding this comment

wlahti Oct 23, 2020 •

edited

Loading