Zhijie Shen commented on YARN-2277:

bq.  I have provided a minimal CORS filter that will give us an idea if this is 
the direction to go. Based on the direction of this patch, the scope has 
widened to create a general CrossOriginFilter for use within all Hadoop REST 
APIs. Probably, we will want to split the different pieces us across JIRAs, 
umbrella, Filter and FilterInitializer, additional configuration, and 
individual REST servers. This way we can focus on the end goal of getting Tez 
UI done in a timely manner without forgetting completeness of CORS support.
[~jeagles], thanks for your contribution!

+1 for making the minimal CORS filter. Another concern is that if we upgrade 
jetty sometime in the future, we can reuse the cross-origin filter provided by 
it, rebase this on top of it. One additional suggestion is that we can start 
the CORS filter even in smaller scope: the timeline server only, which means we 
should move the filter/filter initializer to this sub module. Once it is made 
robust enough and proved to be reliable, we can promote it to 
hadoop-yarn-common or even hadoop-common. How do you think?

Bellow are some detailed comments for the patch:

1. The prefix changes to "yarn.timeline-service.http.cross-origin"?
+  public static final String PREFIX = "hadoop.http.filter.cross.origin.";

2. ALLOWED_ORIGINS -> "allowed-origins"? Not to make the config name too long.
+  // Filter configuration
+  public static final String ALLOWED_ORIGINS = 

3. Should most of the methods in CrossOriginFilter be private?

4. Is it better to make it configurable as well? Why allowedMethods doesn't 
have PUT? In the doc: 
https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS, it seems 
that the headers can go beyond the following set.
+  void initializeAllowedMethods(FilterConfig filterConfig) {
+    allowedMethods.add("GET");
+    allowedMethods.add("POST");
+    allowedMethods.add("HEAD");
+    LOG.info("Allowed Methods: " + getAllowedMethodsHeader());
+  }
+  void initializeAllowedHeaders(FilterConfig filterConfig) {
+    allowedHeaders.add("X-Requested-With");
+    allowedHeaders.add("Content-Type");
+    allowedHeaders.add("Accept");
+    allowedHeaders.add("Origin");
+    LOG.info("Allowed Headers: " + getAllowedHeadersHeader());
+  }

5. Should we include Access-Control-Max-Age?

6. Is it better to invoke doCrossFilter after chain.doFilter, in case devs are 
going to do something special in servlet with the res object directly?
+  @Override
+  public void doFilter(ServletRequest req, ServletResponse res, FilterChain 
+    throws IOException, ServletException {
+    doCrossFilter((HttpServletRequest) req, (HttpServletResponse) res);
+    chain.doFilter(req, res);
+  }

> Add Cross-Origin support to the ATS REST API
> --------------------------------------------
>                 Key: YARN-2277
>                 URL: https://issues.apache.org/jira/browse/YARN-2277
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Jonathan Eagles
>            Assignee: Jonathan Eagles
>         Attachments: YARN-2277-CORS.patch, YARN-2277-JSONP.patch, 
> YARN-2277-v2.patch, YARN-2277-v3.patch, YARN-2277-v3.patch, 
> YARN-2277-v4.patch, YARN-2277-v5.patch, YARN-2277-v6.patch
> As the Application Timeline Server is not provided with built-in UI, it may 
> make sense to enable JSONP or CORS Rest API capabilities to allow for remote 
> UI to access the data directly via javascript without cross side server 
> browser blocks coming into play.
> Example client may be like
> http://api.jquery.com/jQuery.getJSON/ 
> This can alleviate the need to create a local proxy cache.

This message was sent by Atlassian JIRA

Reply via email to