Conversation
| import java.io.IOException; | ||
|
|
||
|
|
||
| public class CrossOriginFilter implements Filter { |
There was a problem hiding this comment.
any specific reason on why creating a new filter versus using CrossOriginFilter provided by Jetty? see http://www.eclipse.org/jetty/documentation/current/cross-origin-filter.html for details
There was a problem hiding this comment.
I can import, I just didn't want to add dependency to pom.xml. What is your opinion on that?
There was a problem hiding this comment.
I was actually validating to use
https://github.com/eclipse/jetty.project/blob/master/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/CrossOriginFilter.java
There was a problem hiding this comment.
since Drill project is already using jetty, it's probably fine to add this dependency (if not already present)
There was a problem hiding this comment.
I will update this PR accordingly.
|
@laurentgo is it ok now? If yes what should be next step? |
pom.xml
Outdated
| <version>4.0.27.Final</version> | ||
| </dependency> | ||
|
|
||
| <dependency> |
There was a problem hiding this comment.
it's probably not the right place to add this dependency. exec/java-exec/pom.xml seems a more appropriate place. Also, you should use the same version as other jetty artifacts. If I remember correctly 9.3.9 is Java8 only whereas Drill still supports Java7.
|
I think the place where the dependency is added should be modified before getting this review merged. Also, I'm a simple commentator, not a committer for the project, so you would have to ping one of those to get it reviewed and merged. |
| http: { | ||
| enabled: true, | ||
| ssl_enabled: false, | ||
| cors_enabled: true, |
There was a problem hiding this comment.
perhaps we should consider extending cors configuration with
http: {
cors: {
enabled: true,
allowedOrigins: ['*.mydomain.com', '*.someother.net'], --> configures Access-Control-Allow-Origin
allowedMethods: ['option', 'get', 'post', 'some-other'], --> configures Access-Control-Allow-Methods
allowedHeaders: ['some-allowed-header'], --> configures Access-Control-Expose-Headers
credentials: true | false -- > configures Access-Control-Allow-Credentials
}
}
I am probably missing few in the list but I find these options essential.
There was a problem hiding this comment.
I will extend that configuration.
|
Looks pretty good. I will make another pass once reviews are addressed. |
|
I have updated PR according to your and laurentgo ideas. @hnfgns: Can you check second round of review? |
|
+1 this looks good to me but can one of you guys take a look, test and commit? |
|
@hnfgns @adeneche @sudheeshkatkam any udpates? |
|
I am not familiar with CORS. One question: why is this enabled by default? Also, there is a discussion about not increasing the size of the jdbc-all jar (subject: drill-jdbc-all-1.7.0-SNAPSHOT.jar max size). Any way to avoid that change? |
| session_max_idle_secs: 3600 # Default value 1hr | ||
| session_max_idle_secs: 3600, # Default value 1hr | ||
| cors: { | ||
| enabled: true, |
There was a problem hiding this comment.
I would default cors.enabled to false and/or set the access-cotrol-allow-origin to null. Ideally, only the end user should be able to enable CORS for all sites.
Otherwise looks good to me.
reduced size of dependencies (reset maxsize)
Added CrossOriginFilter to WebServer based on option HTTP_ENABLE_CORS Fixed issues related to style Restricted headers, added run of filterChain Filter from org.eclipse.jetty.servlets Enabled configuration, jetty version 9.1.5, restrict filtered paths CORS by default disabled, reduced size of dependencies (reset maxsize) This closes apache#507
|
Thanks for adding this! Are there docs anywhere on how to configure this option? |
|
Look in drill-override-example.conf that has the example configuration. You'll need to add something similar to your drill-override.conf. |
6 participants
Uh oh!
There was an error while loading. Please reload this page.