Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Headers can be read from stdin? #116

Open
jammerful opened this issue Sep 20, 2019 · 8 comments
Open

Headers can be read from stdin? #116

jammerful opened this issue Sep 20, 2019 · 8 comments

Comments

@jammerful
Copy link
Contributor

I have a use case where I'd the headers to be able to be read from stdin, in particular I'm passing credentials through the headers and I don't want the credentials to show up as command line parameters which exposes the credentials.

Would it be possible to allow the headers to be read from stdin like the request data? I could put in a PR if this is simple, would you accept it?

I was thinking of putting a switch like this for reading from stdin just like the request data: https://github.com/fullstorydev/grpcurl/blob/master/cmd/grpcurl/grpcurl.go#L489

@jhump
Copy link
Contributor

jhump commented Sep 21, 2019

I think that's reasonable. But it could conflict with reading the request body from stdin. I think maybe the logic should look like so:

  1. If only headers are read from stdin, then read stdin, line-by-line. Each line must be a header in name: value form. Blank lines are ignored as are lines that start with a # (I'm a sucker for allowing comments if/when it's easy to support!). It probably needs to trim, too -- though maybe only trim from the left (to avoid leading whitespace in the header name, but allow trailing whitespace in the value?).
  2. If only the request is read from stdin, then the current behavior should be preserved.
  3. If both are to be read from stdin, it should have the headers first, one per line, and then a blank line, and then the request body (much like an HTTP 1.1 request might look).

What do you think? I doubt I'll have time to implement this soon, but I'll accept a patch if you're up for this.

@jhump
Copy link
Contributor

jhump commented Sep 21, 2019

Actually, I just had a thought: what if instead we added a flag that allowed header values to be re-written via environment variable interpolation?

So instead of having to feed the headers in via stdin, you could do something like so:

grpcurl -expand-headers -H 'name: ${value}'

Note use of single quotes, so the shell won't expand ${value}. Instead, grpcurl could do that, by search for ${...} sequences and replacing them with the corresponding environment variable.

What do you think about that? I can actually see cases where both would be useful -- ability to interpolate env vars into the values and ability to pass via stdin... Anyhow, I think I'd take a patch that had either (or both!) of these approaches implemented.

@jammerful
Copy link
Contributor Author

Thanks for the second solution, I think that is much simpler and I'll put a PR in for that.

@jammerful
Copy link
Contributor Author

jammerful commented Sep 23, 2019

I'll have a PR opened shortly, already have the patch ready.

After putting in the PR, I would need a new release cut and build artifacts published. Is this possible?

jammerful added a commit to jammerful/grpcurl that referenced this issue Sep 24, 2019
See discussion of the feature here:
fullstorydev#116
jammerful added a commit to jammerful/grpcurl that referenced this issue Sep 24, 2019
See discussion of the feature here:
fullstorydev#116
@jammerful
Copy link
Contributor Author

jammerful commented Sep 26, 2019

@jhump Thanks for merging my PR, is there anyway you can cut a new release with accompanying binaries?
Also should we close this issue, or should we leave it open for also accepting headers from stdin?

@jhump
Copy link
Contributor

jhump commented Sep 27, 2019

@jammerful, we can leave this issue open as an enhancement request to also accept via stdin.

I should be able to do a release soon, early next week at the latest. I'm looking for other issues that I might be able to address, too, just so it's not such a small release.

@jammerful
Copy link
Contributor Author

@jhump Thanks, let me know if I can help at all.

@jhump
Copy link
Contributor

jhump commented Oct 1, 2019

@jammerful, FYI, a new v1.4.0 has been released that includes the change you landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants