[#42] feat(server-common): Support cors filter #1238

qqqttt123 · 2023-12-22T06:13:04Z

What changes were proposed in this pull request?

I wrap the cors filter of Jetty as our system internal filter. Most default values respect the default values of the cors filter of Jetty. I only change the default value of AllowMethods.The origin default value is PUT,GET,POST,HEAD. Because we usually use DELETE method, too. I change it to PUT,GET,POST,HEAD,DELETE.

Why are the changes needed?

Fix: #42

Does this PR introduce any user-facing change?

yes, I have added the document.

How was this patch tested?

UT + manual test

var http = new XMLHttpRequest();
var url = 'http://localhost:8090/api/version';
http.onreadystatechange = (e) => {
    console.log(http.responseText)
}
http.open("GET", url);
http.send();

yuqi1129 · 2023-12-22T08:07:36Z

docs/security.md

+| `gravitino.auxService.iceberg-rest.allowedHeaders`       | A comma separated list of HTTP headers that are allowed to be specified when accessing the resources. Default value is X-Requested-With,Content-Type,Accept,Origin. If the value is a single *, this means that any headers will be accepted. | `X-Requested-With,Content-Type,Accept,Origin` | No        | 0.4.0         |
+| `gravitino.auxService.iceberg-rest.preflightMaxAge`      | The number of seconds that preflight requests can be cached by the client. Default value is 1800 seconds, or 30 minutes.                                                                                                                      | `1800`                                        | No        | 0.4.0         |
+| `gravitino.auxService.iceberg-rest.allowCredentials`     | A boolean indicating if the resource allows requests with credentials. Default value is true.                                                                                                                                                 | `true`                                        | No        | 0.4.0         |
+| `gravitino.auxService.iceberg-rest.exposedHeaders`       | A comma separated list of HTTP headers that are allowed to be exposed on the client. Default value is the empty list.                                                                                                                         | ``                                            | No        | 0.4.0         |


I noticed that Server configuration and Iceberg REST service's configuration are almost the same, can we merge them into one and say 'Iceberg rest are the same, the difference is that the prefix of configuration key is ....'

Maybe it's will be difficult for users.

yuqi1129 · 2023-12-22T08:08:56Z

docs/security.md

+| `gravitino.server.webserver.allowedTimingOrigins` | A comma separated list of origins that are allowed to time the resource. Default value is the empty string, meaning no origins.                                                                                                               | ``                                            | No       | 0.4.0         |
+| `gravitino.server.webserver.allowedMethods`       | a comma separated list of HTTP methods that are allowed to be used when accessing the resources. Default value is GET,POST,HEAD,DELETE.                                                                                                       | `GET,POST,HEAD,DELETE`                        | No       | 0.4.0         |
+| `gravitino.server.webserver.allowedHeaders`       | A comma separated list of HTTP headers that are allowed to be specified when accessing the resources. Default value is X-Requested-With,Content-Type,Accept,Origin. If the value is a single *, this means that any headers will be accepted. | `X-Requested-With,Content-Type,Accept,Origin` | No       | 0.4.0         |
+| `gravitino.server.webserver.preflightMaxAge`      | The number of seconds that preflight requests can be cached by the client. Default value is 1800 seconds, or 30 minutes.                                                                                                                      | `1800`                                        | No       | 0.4.0         |


preflight is pre + flight or perf + light? The meaning of preflight is?

pre + flight. This word is from the Jetty.

It would be better to change to "gravitino.server.webserver.preflightMaxAgeInSecs"

yuqi1129 · 2023-12-22T08:11:28Z

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServerConfig.java

+          .createWithDefault("X-Requested-With,Content-Type,Accept,Origin");
+
+  public static final ConfigEntry<Integer> PREFLIGHT_MAX_AGE =
+      new ConfigBuilder("preflightMaxAge")


preflightMaxAge -> preflightMaxAgeInSecs

jerryshao · 2023-12-22T10:08:09Z

docs/security.md

+|---------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------|----------|---------------|
+| `gravitino.server.webserver.enableCorsFilter`     | Enable cross origin resource share filter.                                                                                                                                                                                                    | false                                         | No       | 0.4.0         |
+| `gravitino.server.webserver.allowedOrigins`       | A comma separated list of origins that are allowed to access the resources. Default value is *, meaning all origins.                                                                                                                          | `*`                                           | No       | 0.4.0         |
+| `gravitino.server.webserver.allowedTimingOrigins` | A comma separated list of origins that are allowed to time the resource. Default value is the empty string, meaning no origins.                                                                                                               | ``                                            | No       | 0.4.0         |


What' the meaning of "time the resource"?

This should a term of HTTP. time the resource should mean that get metrics time from the server.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Timing-Allow-Origin

jerryshao · 2023-12-22T10:09:26Z

docs/security.md

+| `gravitino.server.webserver.allowedTimingOrigins` | A comma separated list of origins that are allowed to time the resource. Default value is the empty string, meaning no origins.                                                                                                               | ``                                            | No       | 0.4.0         |
+| `gravitino.server.webserver.allowedMethods`       | a comma separated list of HTTP methods that are allowed to be used when accessing the resources. Default value is GET,POST,HEAD,DELETE.                                                                                                       | `GET,POST,HEAD,DELETE`                        | No       | 0.4.0         |
+| `gravitino.server.webserver.allowedHeaders`       | A comma separated list of HTTP headers that are allowed to be specified when accessing the resources. Default value is X-Requested-With,Content-Type,Accept,Origin. If the value is a single *, this means that any headers will be accepted. | `X-Requested-With,Content-Type,Accept,Origin` | No       | 0.4.0         |
+| `gravitino.server.webserver.preflightMaxAge`      | The number of seconds that preflight requests can be cached by the client. Default value is 1800 seconds, or 30 minutes.                                                                                                                      | `1800`                                        | No       | 0.4.0         |


It would be better to change to "gravitino.server.webserver.preflightMaxAgeInSecs"

jerryshao · 2023-12-22T10:11:42Z

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServerConfig.java

+  public static final ConfigEntry<String> ALLOWED_HEADERS =
+      new ConfigBuilder("allowedHeaders")
+          .doc(
+              "A comma separated list of HTTP headers that are allowed to be specified when accessing the resources. Default value is X-Requested-With,Content-Type,Accept,Origin. If the value is a single *, this means that any headers will be accepted.")


The code line seems too long, is it ok for spotless check?

Yes, I used spotless to format the file.

I would suggest to limit the line length to 100 words.

jerryshao · 2023-12-22T10:12:11Z

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServerConfig.java

+  public static final ConfigEntry<String> ALLOWED_TIMING_ORIGINS =
+      new ConfigBuilder("allowedTimingOrigins")
+          .doc(
+              "A comma separated list of origins that are allowed to time the resource. Default value is the empty string, meaning no origins.")


means no origins.

jerryshao · 2023-12-22T10:12:53Z

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServerConfig.java

+      new ConfigBuilder("enableCorsFilter")
+          .doc("Enable cross origin resource share filter")
+          .version("0.4.0")
+          .booleanConf()


Why don't you have checkValue here?

jerryshao · 2023-12-22T12:00:05Z

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServerConfig.java

+  public static final ConfigEntry<String> ALLOWED_HEADERS =
+      new ConfigBuilder("allowedHeaders")
+          .doc(
+              "A comma separated list of HTTP headers that are allowed to be specified when accessing the resources. Default value is X-Requested-With,Content-Type,Accept,Origin. If the value is a single *, this means that any headers will be accepted.")


I would suggest to limit the line length to 100 words.

jerryshao · 2023-12-22T12:03:22Z

server-common/src/main/java/com/datastrato/gravitino/server/web/CorsFilterHelper.java

+    FilterHolder filterHolder = new FilterHolder();
+    filterHolder.setClassName(CrossOriginFilter.class.getName());
+    filterHolder.setInitParameter(
+        JettyServerConfig.ALLOWED_ORIGINS.getKey(), config.getAllowedOrigins());


Will this cors filter check the validity of the configuration? For example, if the configuration is not legal or have additional/trailing whitespace.

No, this cors filter check less things. Only check int type whether can be parsed to Integer.

jerryshao · 2023-12-22T12:06:21Z

server-common/src/main/java/com/datastrato/gravitino/server/web/CorsFilterHelper.java

+import org.eclipse.jetty.servlet.FilterHolder;
+import org.eclipse.jetty.servlets.CrossOriginFilter;
+
+class CorsFilterHelper {


I would suggest renaming this class to CorsFilterHolder

jerryshao · 2023-12-22T12:07:14Z

server-common/src/main/java/com/datastrato/gravitino/server/web/CorsFilterHelper.java

+
+  private CorsFilterHelper() {}
+
+  public static FilterHolder createCorsFilterHolder(JettyServerConfig config) {


And you can simplify the method name to create to remove duplication with class name.

jerryshao · 2023-12-22T12:25:09Z

server-common/src/main/java/com/datastrato/gravitino/server/web/JettyServerConfig.java

@@ -253,15 +259,17 @@ public final class JettyServerConfig {
  public static final ConfigEntry<String> EXPOSED_HEADERS =
      new ConfigBuilder("exposedHeaders")
          .doc(
-              "A comma separated list of HTTP headers that are allowed to be exposed on the client. Default value is the empty list")
+              "A comma separated list of HTTP headers that are allowed to be exposed on the client."


I think you miss a whitespace when you fold the sentence into two lines. Not just here, but also in the above code.

qqqttt123 requested review from FANNG1, jerryshao, xunliu, diqiu50, yuqi1129, mchades and Clearvive December 22, 2023 06:34

yuqi1129 reviewed Dec 22, 2023

View reviewed changes

jerryshao reviewed Dec 22, 2023

View reviewed changes

Heng Qin added 7 commits December 22, 2023 18:32

[ISSUE-42] feat(server-common): Support cors filter

28e04d7

fix

2ae1b37

fix

9e93700

fix

02d7027

fix

391dbc6

fix

fe608b5

fix

561312d

qqqttt123 force-pushed the ISSUE-42-2 branch from 8cbdea7 to 561312d Compare December 22, 2023 11:03

jerryshao changed the title ~~[ISSUE-42] feat(server-common): Support cors filter~~ [#42] feat(server-common): Support cors filter Dec 22, 2023

jerryshao reviewed Dec 22, 2023

View reviewed changes

fix

053d746

jerryshao reviewed Dec 22, 2023

View reviewed changes

Heng Qin added 2 commits December 22, 2023 20:26

fix

03712f3

fix

54f08c5

jerryshao approved these changes Dec 22, 2023

View reviewed changes

qqqttt123 merged commit fbbb7d6 into apache:main Dec 22, 2023

qqqttt123 deleted the ISSUE-42-2 branch December 22, 2023 12:54


		private CorsFilterHelper() {}

		public static FilterHolder createCorsFilterHolder(JettyServerConfig config) {

[#42] feat(server-common): Support cors filter #1238

[#42] feat(server-common): Support cors filter #1238

Uh oh!

Conversation

qqqttt123 commented Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qqqttt123 commented Dec 22, 2023 •

edited

Loading