You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by PythonicNinja <gi...@git.apache.org> on 2016/05/21 18:49:25 UTC
[GitHub] drill pull request: DRILL-4690: CORS in REST API
GitHub user PythonicNinja opened a pull request:
https://github.com/apache/drill/pull/507
DRILL-4690: CORS in REST API
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/PythonicNinja/drill DRILL-4690
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/507.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #507
----
commit 73d659ae23e455464a3530fd18b2eff3ba192a30
Author: Wojciech Nowak <ma...@pythonic.ninja>
Date: 2016-05-21T16:00:37Z
DRILL-4690: initial support for CORS
commit 74c608092a874cbfc918acfbef7ac4361a1f8d6f
Author: Wojciech Nowak <ma...@pythonic.ninja>
Date: 2016-05-21T18:48:23Z
DRILL-4690: added CrossOriginFilter to WebServer based on option HTTP_ENABLE_CORS
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #507: DRILL-4690: CORS in REST API
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/507
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #507: DRILL-4690: CORS in REST API
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r66303719
--- Diff: exec/jdbc-all/pom.xml ---
@@ -441,7 +441,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>20000000</maxsize>
+ <maxsize>21000000</maxsize>
--- End diff --
@PythonicNinja can you make a pass at adding the exclusions as well to this. The idea is to exclude any dependencies not directly needed for the CORS functionality.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #507: DRILL-4690: CORS in REST API
Posted by chriddyp <gi...@git.apache.org>.
Github user chriddyp commented on the issue:
https://github.com/apache/drill/pull/507
Thanks for adding this! Are there docs anywhere on how to configure this option?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on the pull request:
https://github.com/apache/drill/pull/507#issuecomment-221669367
Looks pretty good. I will make another pass once reviews are addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64603060
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java ---
@@ -159,6 +163,14 @@ public void start() throws Exception {
servletContextHandler.setSessionHandler(createSessionHandler(servletContextHandler.getSecurityHandler()));
}
+ if (config.getBoolean(ExecConstants.HTTP_ENABLE_CORS)) {
+ FilterHolder cors = servletContextHandler.addFilter(CrossOriginFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
+ cors.setInitParameter(CrossOriginFilter.ALLOWED_ORIGINS_PARAM, "*");
--- End diff --
Not so much experience regarding CORS, but should we expect admins to need to restrict this to a specific set of origins? I believe so, but I hope people more experienced regarding web security to comment on that. My take is that we should probably make it configurable too. Default to * is probably although not as restrictive as the same-origin policy used when filter is disabled.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by PythonicNinja <gi...@git.apache.org>.
Github user PythonicNinja commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64476006
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CrossOriginFilter.java ---
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.server.rest;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.FilterChain;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+
+public class CrossOriginFilter implements Filter {
--- End diff --
I will update this PR accordingly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #507: DRILL-4690: CORS in REST API
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on the issue:
https://github.com/apache/drill/pull/507
Look in drill-override-example.conf that has the example configuration. You'll need to add something similar to your drill-override.conf.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64472615
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CrossOriginFilter.java ---
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.server.rest;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.FilterChain;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+
+public class CrossOriginFilter implements Filter {
--- End diff --
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64627653
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java ---
@@ -159,6 +163,14 @@ public void start() throws Exception {
servletContextHandler.setSessionHandler(createSessionHandler(servletContextHandler.getSecurityHandler()));
}
+ if (config.getBoolean(ExecConstants.HTTP_ENABLE_CORS)) {
+ FilterHolder cors = servletContextHandler.addFilter(CrossOriginFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
+ cors.setInitParameter(CrossOriginFilter.ALLOWED_ORIGINS_PARAM, "*");
--- End diff --
+1 that may pose a security risk. We should extend the configuration with allowed-origins. Use of * is too relaxed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #507: DRILL-4690: CORS in REST API
Posted by PythonicNinja <gi...@git.apache.org>.
Github user PythonicNinja commented on the issue:
https://github.com/apache/drill/pull/507
@hnfgns @adeneche @sudheeshkatkam any udpates?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by PythonicNinja <gi...@git.apache.org>.
Github user PythonicNinja commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64630482
--- Diff: exec/jdbc-all/pom.xml ---
@@ -441,7 +441,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>20000000</maxsize>
+ <maxsize>21000000</maxsize>
--- End diff --
Once I have added that:
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-servlets</artifactId>
<version>9.3.9.v20160517</version>
size grew, I will change version to match with jetty version.
Maybe we should add exclusions for that dependency.
@hnfgns Can you suggest what to add in exclusions list?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by PythonicNinja <gi...@git.apache.org>.
Github user PythonicNinja commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64473041
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CrossOriginFilter.java ---
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.server.rest;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.FilterChain;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+
+public class CrossOriginFilter implements Filter {
--- End diff --
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] drill issue #507: DRILL-4690: CORS in REST API
Posted by Jacques Nadeau <ja...@dremio.com>.
FYI, since the JDBC driver doesn't include the webservice, the extra jar
should be able to be excluded with no ill effects.
--
Jacques Nadeau
CTO and Co-Founder, Dremio
On Wed, Jun 8, 2016 at 11:25 AM, Chunhui Shi <cs...@maprtech.com> wrote:
> I think to avoid that size increase is to revert back to the previous
> change which implemented the CORS manually and did not introduce this extra
> jar file. If our goal is to eat only one egg we don't need to buy a hen.
>
> On Mon, Jun 6, 2016 at 2:36 PM, sudheeshkatkam <gi...@git.apache.org> wrote:
>
> > Github user sudheeshkatkam commented on the issue:
> >
> > https://github.com/apache/drill/pull/507
> >
> > 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?
> >
> >
> > ---
> > If your project is set up for it, you can reply to this email and have
> your
> > reply appear on GitHub as well. If your project does not have this
> feature
> > enabled and wishes so, or if the feature is enabled but not working,
> please
> > contact infrastructure at infrastructure@apache.org or file a JIRA
> ticket
> > with INFRA.
> > ---
> >
>
Re: [GitHub] drill issue #507: DRILL-4690: CORS in REST API
Posted by Chunhui Shi <cs...@maprtech.com>.
I think to avoid that size increase is to revert back to the previous
change which implemented the CORS manually and did not introduce this extra
jar file. If our goal is to eat only one egg we don't need to buy a hen.
On Mon, Jun 6, 2016 at 2:36 PM, sudheeshkatkam <gi...@git.apache.org> wrote:
> Github user sudheeshkatkam commented on the issue:
>
> https://github.com/apache/drill/pull/507
>
> 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?
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
[GitHub] drill issue #507: DRILL-4690: CORS in REST API
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the issue:
https://github.com/apache/drill/pull/507
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #507: DRILL-4690: CORS in REST API
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on the issue:
https://github.com/apache/drill/pull/507
+1 this looks good to me but can one of you guys take a look, test and commit?
@adeneche @sudheeshkatkam
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on the pull request:
https://github.com/apache/drill/pull/507#issuecomment-221628280
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by PythonicNinja <gi...@git.apache.org>.
Github user PythonicNinja commented on the pull request:
https://github.com/apache/drill/pull/507#issuecomment-221565297
@laurentgo is it ok now? If yes what should be next step?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64632355
--- Diff: exec/jdbc-all/pom.xml ---
@@ -441,7 +441,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>20000000</maxsize>
+ <maxsize>21000000</maxsize>
--- End diff --
I would not worry about this too much at this time.
Laurent made a good point above. We should match the jetty-servlets version with our jetty-server version. So you might want to use 9.1.5.v20140505 instead.
[9.1.5.v20140505](http://mvnrepository.com/artifact/org.eclipse.jetty/jetty-servlets/9.1.5.v20140505)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #507: DRILL-4690: CORS in REST API
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r65994192
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -111,7 +111,14 @@ drill.exec: {
enabled: true,
ssl_enabled: false,
port: 8047
- session_max_idle_secs: 3600 # Default value 1hr
+ session_max_idle_secs: 3600, # Default value 1hr
+ cors: {
+ enabled: true,
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by PythonicNinja <gi...@git.apache.org>.
Github user PythonicNinja commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64472934
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CrossOriginFilter.java ---
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.server.rest;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.FilterChain;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+
+public class CrossOriginFilter implements Filter {
--- End diff --
I can import, I just didn't want to add dependency to pom.xml. What is your opinion on that?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64602050
--- Diff: pom.xml ---
@@ -530,6 +530,12 @@
</dependency>
<dependency>
+ <groupId>org.eclipse.jetty</groupId>
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64475479
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/CrossOriginFilter.java ---
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.server.rest;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.FilterChain;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+
+public class CrossOriginFilter implements Filter {
--- End diff --
since Drill project is already using jetty, it's probably fine to add this dependency (if not already present)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64627240
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java ---
@@ -159,6 +163,14 @@ public void start() throws Exception {
servletContextHandler.setSessionHandler(createSessionHandler(servletContextHandler.getSecurityHandler()));
}
+ if (config.getBoolean(ExecConstants.HTTP_ENABLE_CORS)) {
+ FilterHolder cors = servletContextHandler.addFilter(CrossOriginFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
--- End diff --
Please apply cors on API endpoints only. The static path should be excluded.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64629211
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -110,6 +110,7 @@ drill.exec: {
http: {
enabled: true,
ssl_enabled: false,
+ cors_enabled: true,
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64629460
--- Diff: exec/jdbc-all/pom.xml ---
@@ -441,7 +441,7 @@
This is likely due to you adding new dependencies to a java-exec and not updating the excludes in this module. This is important as it minimizes the size of the dependency of Drill application users.
</message>
- <maxsize>20000000</maxsize>
+ <maxsize>21000000</maxsize>
--- End diff --
Is this really needed? Looks as though you tried to add a new dependency but failed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64602614
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java ---
@@ -159,6 +163,14 @@ public void start() throws Exception {
servletContextHandler.setSessionHandler(createSessionHandler(servletContextHandler.getSecurityHandler()));
}
+ if (config.getBoolean(ExecConstants.HTTP_ENABLE_CORS)) {
+ FilterHolder cors = servletContextHandler.addFilter(CrossOriginFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
+ cors.setInitParameter(CrossOriginFilter.ALLOWED_ORIGINS_PARAM, "*");
+ cors.setInitParameter(CrossOriginFilter.ACCESS_CONTROL_ALLOW_ORIGIN_HEADER, "*");
--- End diff --
if it doesn't end with _PARAM, it probably means it's not a param but a constant used for something else (likely to set a HTTP header in the response). It's probably harmless, but better be removed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by PythonicNinja <gi...@git.apache.org>.
Github user PythonicNinja commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64629803
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -110,6 +110,7 @@ drill.exec: {
http: {
enabled: true,
ssl_enabled: false,
+ cors_enabled: true,
--- End diff --
I will extend that configuration.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by PythonicNinja <gi...@git.apache.org>.
Github user PythonicNinja commented on the pull request:
https://github.com/apache/drill/pull/507#issuecomment-222073117
I have updated PR according to your and laurentgo ideas. @hnfgns: Can you check second round of review?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4690: CORS in REST API
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/507#discussion_r64603706
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java ---
@@ -159,6 +163,14 @@ public void start() throws Exception {
servletContextHandler.setSessionHandler(createSessionHandler(servletContextHandler.getSecurityHandler()));
}
+ if (config.getBoolean(ExecConstants.HTTP_ENABLE_CORS)) {
+ FilterHolder cors = servletContextHandler.addFilter(CrossOriginFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
--- End diff --
Not so much experience regarding CORS, but do we want to apply it to any resource, or just the REST API, or maybe a subset of those?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---