Support reading JSON data from STDIN #1905

hiyosi · 2020-10-10T02:07:44Z

Signed-off-by: Tomoya Usami tousami@zlab.co.jp

Pull Request check list

Commit conforms to CONTRIBUTING.md?
Proper tests/regressions included?
Documentation updated?

Affected functionality

Server CLI: entry create/update

Description of change

entry create/update commands accept JSON data from STDIN.

Which issue this PR fixes

#1864

hiyosi · 2020-10-10T02:09:04Z

parameter

$ kubectl exec -n spire spire-server-0 -- bin/spire-server entry create -spiffeID spiffe://example.org/web -parentID spiffe://example.org/my-server -selector k8s:ns:web -selector k8s:sa:web
Entry ID      : 15acd155-28e9-4a06-8263-ac0ad09c7e2c
SPIFFE ID     : spiffe://example.org/web
Parent ID     : spiffe://example.org/my-server
Revision      : 0
TTL           : default
Selector      : k8s:ns:web
Selector      : k8s:sa:web

file

$ kubectl exec -n spire spire-server-0 -- bin/spire-server entry create -data test-entry.json
Entry ID      : 4780c252-6635-47c9-a870-6628a557e292
SPIFFE ID     : spiffe://example.org/app
Parent ID     : spiffe://example.org/my-server
Revision      : 0
TTL           : default
Selector      : k8s:ns:app
Selector      : k8s:sa:app

stdin

cat test-registration.json | kubectl exec -i -n spire spire-server-0 -- bin/spire-server entry create -data -
Entry ID      : 79da91cf-ae13-4bb8-aae8-c92166f74642
SPIFFE ID     : spiffe://example.org/db
Parent ID     : spiffe://example.org/my-server
Revision      : 0
TTL           : default
Selector      : k8s:ns:db
Selector      : k8s:sa:db

update

cat update-registration.json | kubectl exec -i -n spire spire-server-0 -- bin/spire-server entry update -data -
Entry ID      : c7f606a5-4457-4234-9c6f-cb72eda9efc4
SPIFFE ID     : spiffe://example.org/db
Parent ID     : spiffe://example.org/my-server
Revision      : 1
TTL           : default
Selector      : k8s:ns:db
Selector      : k8s:sa:db
Admin         : true

blaggacao

From my side just some minor wording suggestions. Thank you for the work!

Otherwise LGTM

cmd/spire-server/cli/entry/create.go

@@ -233,7 +218,7 @@ func (CreateCLI) newConfig(args []string) (*CreateConfig, error) {
 	f.StringVar(&c.SpiffeID, "spiffeID", "", "The SPIFFE ID that this record represents")
 	f.IntVar(&c.TTL, "ttl", 0, "The lifetime, in seconds, for SVIDs issued based on this registration entry")

-	f.StringVar(&c.Path, "data", "", "Path to a file containing registration JSON (optional)")
+	f.StringVar(&c.Path, "data", "", "Path to a file containing registration JSON (optional). If set '-' as file path, read the JSON passed into stdin.")


cmd/spire-server/cli/entry/update.go

@@ -219,7 +204,7 @@ func (UpdateCLI) newConfig(args []string) (*UpdateConfig, error) {
 	f.StringVar(&c.SpiffeID, "spiffeID", "", "The SPIFFE ID that this record represents")
 	f.IntVar(&c.TTL, "ttl", 0, "The lifetime, in seconds, for SVIDs issued based on this registration entry")

-	f.StringVar(&c.Path, "data", "", "Path to a file containing registration JSON (optional)")
+	f.StringVar(&c.Path, "data", "", "Path to a file containing registration JSON (optional). If set '-' as file path, read the JSON passed into stdin.")


doc/spire_server.md

@@ -229,7 +229,7 @@ Creates registration entries.
 | Command          | Action                                                                 | Default        |
 |:-----------------|:-----------------------------------------------------------------------|:---------------|
 | `-admin`         | If set, the SPIFFE ID in this entry will be granted access to the Registration API | |
-| `-data`          | Path to a file containing registration data in JSON format (optional). |                |
+| `-data`          | Path to a file containing registration data in JSON format (optional). If set '-' as file path, read the JSON passed into stdin. |                |


doc/spire_server.md

@@ -248,7 +248,7 @@ Updates registration entries.
 | Command          | Action                                                                 | Default        |
 |:-----------------|:-----------------------------------------------------------------------|:---------------|
 | `-admin`         | If true, the SPIFFE ID in this entry will be granted access to the Registration API | |
-| `-data`          | Path to a file containing registration data in JSON format (optional). |                |
+| `-data`          | Path to a file containing registration data in JSON format (optional). If set '-' as file path, read the JSON passed into stdin. |                |


APTy · 2020-10-13T19:05:39Z

cmd/spire-server/cli/entry/util.go

@@ -91,6 +95,34 @@ func printEntry(e *common.RegistrationEntry) {
 	fmt.Println()
 }

+// ParseFile parses JSON represented RegistrationEntries
+// if path is "" or "*" read JSON from STDIN
+func ParseFile(in io.Reader, path string) ([]*common.RegistrationEntry, error) {


Personally I think this can be made to:

func ParseFile(path string) ([]*common.RegistrationEntry, error) { return parseFile(os.Stdin, path) } func parseFile(stdin io.Reader, path string) ([]*common.RegistrationEntry, error) { ... }

This provides a nice public API (the consumer doesn't need to pass os.Stdin, which is somewhat of an implementation detail), which also giving you the testability I believe you're looking for.

Thanks! Looks good!

6096a68

APTy · 2020-10-13T19:09:54Z

cmd/spire-server/cli/entry/util.go

@@ -91,6 +95,34 @@ func printEntry(e *common.RegistrationEntry) {
 	fmt.Println()
 }

+// ParseFile parses JSON represented RegistrationEntries
+// if path is "" or "*" read JSON from STDIN


Replace "*" with "-"

Also, I'm not confident we want to alias an empty path as stdin.. as it could mask user error.

Yes, the empty path doesn't work well.
The comment seems to have just been forgotten to remove.

6096a68

APTy · 2020-10-13T19:11:25Z

cmd/spire-server/cli/entry/util.go

+
+	var r io.Reader
+	if path == "-" {
+		r = in


this could be simplified to remove the else with something like:

r := stdin if path != "-" { f, err := os.Open(path) if err != nil { return nil, err } defer f.Close() r = f }

APTy

LGTM, but a few questions on the tests

APTy · 2020-10-15T18:53:55Z

cmd/spire-server/cli/entry/util.go

@@ -91,6 +95,36 @@ func printEntry(e *common.RegistrationEntry) {
 	fmt.Println()
 }

+// ParseFile parses JSON represented RegistrationEntries
+// if path is "-" read JSON from STDIN
+func ParseFile(path string) ([]*common.RegistrationEntry, error) {


nit: this actually doesn't need to be a public method, since it's not used outside of this package

Yeah, exactly.
587ac9c

APTy · 2020-10-15T19:06:44Z

cmd/spire-server/cli/entry/util_test.go

+	assert.NoError(t, err)
+	// Replace os.Stdin temporary
+	orgStdin := os.Stdin
+	os.Stdin = fakeStdin


I'd prefer not to override the os.Stdin package global in tests.

One option is to use the dependency injection and test the parseFile(io.Reader, filename) function and leave ParseFile(filename) without directly testing it.

Another option is to set var stdin = os.Stdin and then replace the package local stdin var, which is much less invasive than mutating os.Stdin entirely.

587ac9c
This commit changes the target and tests the parseFile(io.Reader, filename) function.

APTy · 2020-10-15T19:07:20Z

cmd/spire-server/cli/entry/util_test.go

@@ -45,3 +49,93 @@ func selectorToFlag(t *testing.T, selectors []*common.Selector) StringsFlag {

 	return resp
 }
+
+func TestRegisterParseFile(t *testing.T) {
+	p := path.Join(util.ProjectRoot(), "test/fixture/registration/good.json")


is there any bad json we should test against?

587ac9c
Invalid JSON is now also included in the test case.
Also refactored test functions.

APTy

LGTM!

evan2645 · 2020-10-20T18:51:22Z

Thank you for this feature @hiyosi!

We need to rebase the PR to get it merged in. Would you mind either rebasing/merging master or allowing us to do it? Thanks again!

Signed-off-by: Tomoya Usami <tousami@zlab.co.jp> fixup! Support reading JSON data from STDIN Signed-off-by: Tomoya Usami <tousami@zlab.co.jp> fixup! fixup! Support reading JSON data from STDIN Signed-off-by: Tomoya Usami <tousami@zlab.co.jp>

hiyosi · 2020-10-20T23:28:03Z

@APTy @evan2645 Done rebasing with master branch. Thanks!

blaggacao · 2020-10-21T15:18:11Z

@hiyosi Thank you very much for your effort! Supremely appreciated over here.

hiyosi force-pushed the entry_json_from_stdin branch from 138265c to 0646577 Compare October 10, 2020 02:08

hiyosi mentioned this pull request Oct 10, 2020

Accept json data from stdin #1864

Closed

hiyosi force-pushed the entry_json_from_stdin branch 2 times, most recently from 42bf8d7 to aa95332 Compare October 10, 2020 02:31

blaggacao reviewed Oct 10, 2020

View reviewed changes

hiyosi marked this pull request as ready for review October 10, 2020 04:13

hiyosi requested review from amartinezfayo, APTy, azdagron, evan2645 and mcpherrinm as code owners October 10, 2020 04:13

blaggacao mentioned this pull request Oct 13, 2020

SPIRE/SPIFFE based deployment for TLS Trow-Registry/trow#184

Open

amartinezfayo assigned APTy Oct 13, 2020

APTy reviewed Oct 13, 2020

View reviewed changes

hiyosi force-pushed the entry_json_from_stdin branch from 7cd898b to ce0929b Compare October 14, 2020 01:06

APTy previously approved these changes Oct 15, 2020

View reviewed changes

hiyosi dismissed APTy’s stale review via b9be999 October 15, 2020 23:41

hiyosi force-pushed the entry_json_from_stdin branch 4 times, most recently from 587ac9c to bd3d93e Compare October 19, 2020 23:22

APTy previously approved these changes Oct 20, 2020

View reviewed changes

hiyosi dismissed APTy’s stale review via 610fb24 October 20, 2020 23:00

hiyosi force-pushed the entry_json_from_stdin branch from bd3d93e to 610fb24 Compare October 20, 2020 23:00

Support reading JSON data from STDIN

6300e57

Signed-off-by: Tomoya Usami <tousami@zlab.co.jp> fixup! Support reading JSON data from STDIN Signed-off-by: Tomoya Usami <tousami@zlab.co.jp> fixup! fixup! Support reading JSON data from STDIN Signed-off-by: Tomoya Usami <tousami@zlab.co.jp>

hiyosi force-pushed the entry_json_from_stdin branch from 610fb24 to 6300e57 Compare October 20, 2020 23:00

azdagron approved these changes Oct 21, 2020

View reviewed changes

azdagron merged commit 8bf81b4 into spiffe:master Oct 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support reading JSON data from STDIN #1905

Support reading JSON data from STDIN #1905

hiyosi commented Oct 10, 2020

hiyosi commented Oct 10, 2020

blaggacao left a comment •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

APTy Oct 13, 2020

hiyosi Oct 13, 2020

APTy Oct 13, 2020

hiyosi Oct 13, 2020 •

edited

Loading

APTy Oct 13, 2020

hiyosi Oct 13, 2020

APTy left a comment

APTy Oct 15, 2020

hiyosi Oct 16, 2020

APTy Oct 15, 2020

hiyosi Oct 16, 2020

APTy Oct 15, 2020

hiyosi Oct 16, 2020

APTy left a comment

evan2645 commented Oct 20, 2020

hiyosi commented Oct 20, 2020

blaggacao commented Oct 21, 2020 •

edited

Loading

Support reading JSON data from STDIN #1905

Support reading JSON data from STDIN #1905

Conversation

hiyosi commented Oct 10, 2020

hiyosi commented Oct 10, 2020

blaggacao left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiyosi Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APTy 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

APTy left a comment

Choose a reason for hiding this comment

evan2645 commented Oct 20, 2020

hiyosi commented Oct 20, 2020

blaggacao commented Oct 21, 2020 • edited Loading

blaggacao left a comment •

edited

Loading

hiyosi Oct 13, 2020 •

edited

Loading

blaggacao commented Oct 21, 2020 •

edited

Loading