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

Added headers to http credentials provider #139

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

pranavr12
Copy link
Contributor

@pranavr12 pranavr12 commented Aug 8, 2024

Added headers to be sent as part of the request in the http credentials provider.
Added a couple of tests to check if the header is working is properly

@cla-bot cla-bot bot added the cla-signed label Aug 8, 2024
@@ -65,14 +74,17 @@ public TestingTrinoAwsProxyServer.Builder filter(TestingTrinoAwsProxyServer.Buil
.addModule(new HttpCredentialsModule())
.addModule(binder -> bindIdentityType(binder, TestingIdentity.class))
.withProperty("credentials-provider.type", HTTP_CREDENTIALS_PROVIDER_IDENTIFIER)
.withProperty("credentials-provider.http.endpoint", httpCredentialsServer.getBaseUrl().toString());
.withProperty("credentials-provider.http.endpoint", httpEndpointUri)
.withProperty("credentials-provider.http.headers", "Authorization: auth, Content-Type: application/json");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have 2 extra test cases:

  • A header value containing a colon (this should work based on your implementation), like Authorization: api-key:my-key
  • A header value containing a comma (unsure how Airlift deals with this, we may need to escape the comma?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a test case for the first case. It works as expected.
With a comma, it becomes tricky since its a string. To support commas, we might have to take in the input as an actual array rather than a string. Should we address this ? Comma in a header value seems a bit off

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTTP spec separates headers with \r\n so I don't think there's anything too strange about a comma on a header - the authorization header for SigV4 requests contains commas, for instance.

We can't easily make the config a native array as it's read from a text file, and looking at the Airlift config builder I don't think it supports escaping the commas.

We could read the config as a string and do the parsing ourselves. HTTP allows for all ASCII characters to be part of a header, but \r or \n cannot be used (since they separate different headers).

We could do something like

Splitter.on(",").trimResults().omitEmptyStrings().splitToStream(headerConfig.replaceAll(",,", "\r")).map(item -> item.replace("\r", ","))

This way if we want to have a comma as part of a header value, we need to double it (foo,,bar for header foo,bar).

Copy link
Contributor Author

@pranavr12 pranavr12 Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another potential solution - How about having another config like http.header.delimiter and we use this custom delimiter for spitting the header string. This gives the flexibility of having various kinds of delimiters. Ofcourse, the delimiter shouldn't be used in the header name or value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do that, but it feels like we are passing on the problem to the end users. A suggestion like the one above would work with any headers users want to provide

Copy link
Contributor Author

@pranavr12 pranavr12 Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought - ,, Seems to be a bit unfriendly if ,, has to be used in the header value itself. Or, if the original value contains let's say value,,value, the configured value would be value,,,,value.
Instead, using a delimiter provides a flexibility of choosing something that they don't normally use in the headers. We can always have a default of ,. We can also use /r as the delimiter. WDYT ? I'm happy to change to your suggestion, though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed that we'll be going ahead with @vagaerg 's approach of using ,, to represent commas in the header. Usage note will be added in the readme

@pranavr12 pranavr12 force-pushed the http-cred-provider-header branch from f64991e to d653f85 Compare August 8, 2024 10:52
@pranavr12 pranavr12 force-pushed the http-cred-provider-header branch 6 times, most recently from 641ee2d to 162aae0 Compare August 12, 2024 11:05
README.md Outdated Show resolved Hide resolved
@pranavr12 pranavr12 force-pushed the http-cred-provider-header branch from 162aae0 to 7fa1f7b Compare August 12, 2024 12:44
@pranavr12 pranavr12 force-pushed the http-cred-provider-header branch from 7fa1f7b to 6d87b51 Compare August 12, 2024 13:54
@vagaerg vagaerg merged commit 231b974 into trinodb:main Aug 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants