Add Expand Headers Feature #117

jammerful · 2019-09-24T03:01:33Z

See discussion of the feature here:
#116

See discussion of the feature here: fullstorydev#116

jammerful · 2019-09-24T03:10:30Z

@jhump As discussed here is the PR. Note I've made a few decisions regarding the functionality:

If no corresponding environmental variable is found, the header is sent as is. This is instead of an error being thrown causing grpcurl to terminate.
I've allowed header keys also to be set using environmental variables.

Open to changing the above.

jammerful · 2019-09-24T03:12:12Z

grpcurl.go

@@ -161,6 +163,37 @@ func MetadataFromHeaders(headers []string) metadata.MD {
 	return md
 }

+/* Expands environmental variables contained in the header string


Note I went with this comment format, as golint was complaining.

Please stick with //, for consistency in this file.

To appease go lint, you need the comment to start with the named of the doc'ed thing, in this case "Expand Headers":

// ExpandHeaders expands environment variable references contained in the string. // etc...

jhump · 2019-09-25T12:10:04Z

cmd/grpcurl/grpcurl.go

@@ -313,6 +319,13 @@ func main() {
 		return cc
 	}

+	var headers []string
+	if *expandHeaders {
+		headers = grpcurl.ExpandHeaders(addlHeaders)


Also need to expand rpcHeaders and reflHeaders.

jhump · 2019-09-25T12:11:19Z

grpcurl.go

@@ -161,6 +163,37 @@ func MetadataFromHeaders(headers []string) metadata.MD {
 	return md
 }

+/* Expands environmental variables contained in the header string


Please stick with //, for consistency in this file.

To appease go lint, you need the comment to start with the named of the doc'ed thing, in this case "Expand Headers":

// ExpandHeaders expands environment variable references contained in the string. // etc...

jhump · 2019-09-25T12:12:44Z

grpcurl.go

+	expandedHeaders := make([]string, len(headers))
+	for idx, header := range headers {
+		if header != "" {
+			regex := regexp.MustCompile(`\${\w+}`)


Definitely don't compile regex's in the loop. I know it's a simple and fast tool, so performance at this step may not usually be a concern. But we can at least hoist this out and only compile it once (make it an unexported package var) in case anyone ends up using this API in an unexpected way in the future.

Done. Sorry for oversight.

jhump · 2019-09-25T12:15:32Z

grpcurl.go

+					envVarValue := os.Getenv(result[2 : len(result)-1])
+					replacementValue := envVarValue
+					// If no corresponding env var is found, leave the header as is.
+					if len(envVarValue) == 0 {


I think this will be confusing. Based on how other tools generally work with env vars, I would expect either an error that my input is bad or missing env vars to interpolate the empty string.

I'm a bit partial to the former since it is more likely catch bugs.

Although this does bring up an interesting point: if you want to use a literal ${ in the value, this scheme is too simple and would result in an error. I think we need a way to escape the $. For example, $$ or \$ meaning literal dollar sign, not the start of an env var reference.

Escaping can probably be left as a TODO since, by default, grpcurl does not try to do any variable expansion. So you can either do variable expansion or use literal dollar signs, which is likely sufficient for now.

Agreed, on the confusing behavior. Fixed.

jammerful · 2019-09-25T21:48:49Z

Thanks for the great feedback, I will update shortly.

Expand Headers should throw an error for unset environmental variables. Also address additional PR feedback.

jammerful · 2019-09-26T01:03:50Z

cmd/grpcurl/grpcurl.go

@@ -313,6 +320,22 @@ func main() {
 		return cc
 	}

+	if *expandHeaders {
+		var err error
+		addlHeaders, err = grpcurl.ExpandHeaders(addlHeaders)


Let me know if you wanted this DRY'd, was on the fence.

This seems fine.

jhump

This is looking pretty good. I left several style nits that should hopefully be easy to address. After they are taken care of, I'll merge.

jhump · 2019-09-26T02:09:28Z

cmd/grpcurl/grpcurl.go

+	rpcHeaders    multiString
+	reflHeaders   multiString
+	expandHeaders = flags.Bool("expand-headers", false, prettify(`
+		If set any environmental variables contained contained in any additional, rpc,


I think some of the wording in here is a little awkward. How about something like this instead:

If set, headers may use '${NAME}' syntax to reference environment variables. These will be expanded to the actual environment variable value before sending to the server. For example, if there is an environment variable defined like FOO=bar, then a header of 'key: ${FOO}' would expand to 'key: bar'. This applies to -H, -rpc-header, and -reflect-header options. No other expansion/escaping is performed. This can be used to supply credentials/secrets without having to put them in command-line arguments.

jhump · 2019-09-26T02:10:06Z

cmd/grpcurl/grpcurl.go

+		var err error
+		addlHeaders, err = grpcurl.ExpandHeaders(addlHeaders)
+		if err != nil {
+			fail(err, "Failed to expand additional headers, missing environmental variable")


nit: "environment variable" instead of "environmental variable"

Actually, the error returned from grpcurl.ExpandHeaders should suffice so that you don't need the "missing environment variable" bit -- it just makes the error that gets printed unnecessarily longer.

jhump · 2019-09-26T02:10:40Z

grpcurl.go

+var envVarRegex = regexp.MustCompile(`\${\w+}`)
+
+// ExpandHeaders expands environmental variables contained in the header string.
+// If no corresponding environmental variable is found an error is thrown.


nit: "environment" instead of "environmental". Also "an error is returned" (not thrown).

jhump · 2019-09-26T02:13:02Z

grpcurl.go

+					envVarName := result[2 : len(result)-1]
+					envVarValue := os.Getenv(envVarName)
+					replacementValue := envVarValue
+					if len(envVarValue) == 0 {


I think this is best tested using os.LookupEnv instead of os.GetEnv. That way, an explicitly blank environment variable is allowed -- but an absent variable will result in an error:

envVarValue, ok := os.LookupEnv(envVarName) if !ok { return nil, fmt.Errorf("no variable %q is present in environment", envVarName) }

jhump · 2019-09-26T02:14:03Z

grpcurl_test.go

+	}
+	for _, expandedHeader := range outHeaders {
+		if _, ok := expectedHeaders[expandedHeader]; !ok {
+			t.Errorf("The ExpandHeaders function has generated an unexpected header. Recieved unexpected header %s", expandedHeader)


nit: "Received" not "Recieved"

jhump · 2019-09-26T02:16:00Z

grpcurl.go

+	for idx, header := range headers {
+		if header != "" {
+			results := envVarRegex.FindAllString(header, -1)
+			if results != nil {


It is almost always best to check len(results) > 0 instead of testing against nil (nil and empty slices should be treated the same).

jhump · 2019-09-26T02:17:28Z

grpcurl.go

+func ExpandHeaders(headers []string) ([]string, error) {
+	expandedHeaders := make([]string, len(headers))
+	for idx, header := range headers {
+		if header != "" {


Instead of deepening the logic with indentation under an if block, how about invert the condition:

if header == "" { continue }

Similar for below:

if len(results) == 0 { expandedHeaders[idx] = headers[idx] continue }

jhump · 2019-09-26T02:18:29Z

grpcurl.go

+			if results != nil {
+				expandedHeader := header
+				for _, result := range results {
+					envVarName := result[2 : len(result)-1]


Maybe a comment to clarify that these indices are to cut out the '${' in the beginning and the '}' at the end.

jhump · 2019-09-26T02:19:03Z

cmd/grpcurl/grpcurl.go

@@ -313,6 +320,22 @@ func main() {
 		return cc
 	}

+	if *expandHeaders {
+		var err error
+		addlHeaders, err = grpcurl.ExpandHeaders(addlHeaders)


This seems fine.

jhump · 2019-09-26T02:20:40Z

grpcurl.go

+					envVarValue := os.Getenv(envVarName)
+					replacementValue := envVarValue
+					if len(envVarValue) == 0 {
+						return nil, fmt.Errorf("environmental variable '%s' is not set", envVarName)


It would be nice to reference the actual bad input. Maybe instead this:

return nil, fmt.Errorf("header %q refers to missing environment variable %q", header, envVarName)

Also fix all other PR feedback.

Add Expand Headers Feature

21ea8b8

See discussion of the feature here: fullstorydev#116

jammerful commented Sep 24, 2019

View reviewed changes

jhump reviewed Sep 25, 2019

View reviewed changes

Make Expand Headers Generate Expected Errors

45b5ae0

Expand Headers should throw an error for unset environmental variables. Also address additional PR feedback.

jammerful commented Sep 26, 2019

View reviewed changes

jhump reviewed Sep 26, 2019

View reviewed changes

Fix -expand-headers Help String

9dad8ec

Also fix all other PR feedback.

jammerful requested a review from jhump September 26, 2019 19:25

jhump approved these changes Sep 26, 2019

View reviewed changes

jhump merged commit 9248ea0 into fullstorydev:master Sep 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Expand Headers Feature #117

Add Expand Headers Feature #117

jammerful commented Sep 24, 2019

jammerful commented Sep 24, 2019

jammerful Sep 24, 2019

jhump Sep 25, 2019

jammerful Sep 26, 2019

jhump Sep 25, 2019

jammerful Sep 26, 2019

jhump Sep 25, 2019

jhump Sep 25, 2019

jammerful Sep 26, 2019

jhump Sep 25, 2019

jhump Sep 25, 2019

jammerful Sep 26, 2019

jammerful commented Sep 25, 2019

jammerful Sep 26, 2019

jhump Sep 26, 2019

jhump left a comment

jhump Sep 26, 2019

jhump Sep 26, 2019

jhump Sep 26, 2019

jhump Sep 26, 2019

jhump Sep 26, 2019

jhump Sep 26, 2019

jhump Sep 26, 2019

jhump Sep 26, 2019

jhump Sep 26, 2019

jhump Sep 26, 2019

jhump Sep 26, 2019

Add Expand Headers Feature #117

Add Expand Headers Feature #117

Conversation

jammerful commented Sep 24, 2019

jammerful commented Sep 24, 2019

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

jammerful commented Sep 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhump 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