[#1190] feat(server-common): Support custom filters #1191

qqqttt123 · 2023-12-18T10:05:45Z

What changes were proposed in this pull request?

Add the support of custom filters referring to the Spark.

Why are the changes needed?

Some filters can be configured by users, the needs of users are different. Some filters are implemented by other libs.
For example, Jetty has implemented the CORS filter, so we supported custom filters to satisfy the needs of users.

Fix: #1190

Does this PR introduce any user-facing change?

Yes, I have added the document.

How was this patch tested?

test by the browser console
I test Jetty's cors filter.

Configuration

gravitino.server.webserver.customFilters=org.eclipse.jetty.servlets.CrossOriginFilter
gravitino.server.webserver.org.eclipse.jetty.servlets.CrossOriginFilter.param.allowedOrigins=*

Browser console code

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

qqqttt123 · 2023-12-18T10:07:04Z

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

@@ -245,8 +255,11 @@ private JettyServerConfig(Map<String, String> configs) {
            Sets.newHashSet(internalConfig.get(ENABLE_CIPHER_ALGORITHMS).split(SPLITTER)));
    this.enableClientAuth = internalConfig.get(ENABLE_CLIENT_AUTH);
    this.trustStorePath = internalConfig.get(SSL_TRUST_STORE_PATH);
-    this.trustStorePasword = internalConfig.get(SSL_TRUST_STORE_PASSWORD);
+    this.trustStorePassword = internalConfig.get(SSL_TRUST_STORE_PASSWORD);


An unrelated typo. Just fix by the way.

qqqttt123 · 2023-12-18T10:07:10Z

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

@@ -330,14 +343,18 @@ public String getTrustStorePath() {
    return trustStorePath;
  }

-  public String getTrustStorePasword() {
-    return trustStorePasword;
+  public String getTrustStorePassword() {


An unrelated typo. Just fix by the way.

docs/security.md

yuqi1129 · 2023-12-19T13:00:39Z

docs/security.md

+### Iceberg REST service's configuration
+| Configuration item                                  | Description                                                        | Default value | Required  | Since version |
+|-----------------------------------------------------|--------------------------------------------------------------------|---------------|-----------|---------------|
+| `gravitino.auxService.iceberg-rest.customFilters`   | Comma separated list of filter class names to apply to the APIs.   | ``            | Yes       | 0.4.0         |


Why is the default value `` and it's a required value?

Require value means that the value can't be null. It's ok to use `` as the value. It means that we don't use any filter.

I have the same question, Required means user must fillup this field or the program couldn't startup as I understand.

Required means that this field can't be null.

I have the same question, Required means user must fillup this field or the program couldn't startup as I understand.

According to your standard, gravitino.auxService.iceberg-rest.catalog-backend isn't a required field. But you choose it as a required field.

Following the previous document specification, we use "(none)" to indicate no default value.

JDBC catalog:

Hive catalog:

OK, I see other documents follow this standard. I will change this.

#1214 I raised another pr to fix the comments in other places.
If this config option has default value, this config option is not required.

yuqi1129 · 2023-12-19T13:01:34Z

docs/security.md

+|-----------------------------------------------------|--------------------------------------------------------------------|---------------|-----------|---------------|
+| `gravitino.auxService.iceberg-rest.customFilters`   | Comma separated list of filter class names to apply to the APIs.   | ``            | Yes       | 0.4.0         |
+The filter should be a standard javax servlet Filter.
+Filter parameters can also be specified in the configuration, by setting config entries of the form `gravitino.auxService.iceberg-rest.<class name of filter>.param.<param name>=<value>`


The key seems to be too long, can we shorten it?

Refer to Spark's way. I have no better idea.

yuqi1129 · 2023-12-19T13:06:16Z

docs/security.md

+| Configuration item                          | Description                                                      | Default value | Required  | Since version |
+|---------------------------------------------|------------------------------------------------------------------|---------------|-----------|---------------|
+| `gravitino.server.webserver.customFilters`  | Comma separated list of filter class names to apply to the APIs. | ``            | Yes       | 0.4.0         |
+The filter should be a standard javax servlet Filter.


Please add an empty line here.

OK, will fix.

FANNG1 · 2023-12-20T01:26:49Z

docs/security.md

@@ -258,3 +258,51 @@ If you want to use the command `curl`, you can follow the commands:
 openssl x509 -inform der -in $JAVA_HOME/localhost.crt -out certificate.pem
 curl -v -X GET --cacert ./certificate.pem -H "Accept: application/vnd.gravitino.v1+json" -H "Content-Type: application/json" https://localhost:8433/api/version
 ```
+
+## Custom filters


I prefer to move doc to GravitinoServer Configuration because Custom filters is general and not limited to safety

I refer to Spark. https://spark.apache.org/docs/latest/security.html

I would suggest to move this part to server configurations, not here. Since user can configure other filters like redirect, it is not purely a security related feature.

FANNG1 · 2023-12-20T01:33:01Z

docs/security.md

+
+```shell
+wget https://repo1.maven.org/maven2/org/eclipse/jetty/jetty-servlets/9.4.51.v20230217/jetty-servlets-9.4.51.v20230217.jar 
+cp jetty-servlets-9.4.51.v20230217.jar <gravitno home>/libs


gravitno home -> gravitino home

OK, I will fix.

This reverts commit 55184a6.

…ty.md` (#1214) ### What changes were proposed in this pull request? Modify the required value for the document `security.md`. I modify some document mistakes by the way. ### Why are the changes needed? According the review #1191 (comment) , modify the required value. If a config option has a default value, this config option is not required. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No need. --------- Co-authored-by: Heng Qin <[email protected]>

…ty.md` (#1223) ### What changes were proposed in this pull request? Modify the required value for the document `security.md`. I modify some document mistakes by the way. ### Why are the changes needed? According the review #1191 (comment) , modify the required value. If a config option has a default value, this config option is not required. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No need. Co-authored-by: qqqttt123 <[email protected]> Co-authored-by: Heng Qin <[email protected]>

jerryshao · 2023-12-21T06:44:22Z

docs/security.md

+
+```shell
+wget https://repo1.maven.org/maven2/org/eclipse/jetty/jetty-servlets/9.4.51.v20230217/jetty-servlets-9.4.51.v20230217.jar 
+cp jetty-servlets-9.4.51.v20230217.jar <gravitino home>/libs


Can you please change to <GRAVITINO_HOME>/libs

OK. will fix.

jerryshao · 2023-12-21T06:47:15Z

Can you please add some UTs, at least to test the correctness of the configurations?

jerryshao · 2023-12-21T08:15:10Z

Also, I think we should support cors filter by our own, rather than letting users to configure it. So you should have a new PR to implement cors filter.

qqqttt123 · 2023-12-21T08:26:41Z

Also, I think we should support cors filter by our own, rather than letting users to configure it. So you should have a new PR to implement cors filter.

Jetty have implemented CORS filters: org.eclipse.jetty.servlets.CrossOriginFilter . Why do we reuse it? You can see my test.

jerryshao · 2023-12-21T08:42:51Z

You should at least wrap the jetty's implementation and provide gravitino configurations. Cors support is very important for all the web based apps, like Livy, Zeppelin, I don't see them letting users configure a filter to achieve this.

a) Users don't know where's the right filter they want, and how to enable this, unless they read the code of that filter. Here jetty for example, how did users configure cors without knowing Jetty?
b) Spark is not a web based app, so don't take Spark as an example for everything.

qqqttt123 · 2023-12-21T10:13:06Z

You should at least wrap the jetty's implementation and provide gravitino configurations. Cors support is very important for all the web based apps, like Livy, Zeppelin, I don't see them letting users configure a filter to achieve this.

a) Users don't know where's the right filter they want, and how to enable this, unless they read the code of that filter. Here jetty for example, how did users configure cors without knowing Jetty? b) Spark is not a web based app, so don't take Spark as an example for everything.

Ok, I will prefer wrapping the CORS filter of Jetty.

qqqttt123 · 2023-12-21T11:31:57Z

Can you please add some UTs, at least to test the correctness of the configurations?

Fixed.

qqqttt123 marked this pull request as draft December 18, 2023 10:05

qqqttt123 changed the title ~~[ISSUE-1190]feat(server-common): Support custom filter~~ [ISSUE-1190]feat(server-common): Support custom filters Dec 18, 2023

qqqttt123 commented Dec 18, 2023

View reviewed changes

docs/security.md Outdated Show resolved Hide resolved

Heng Qin added 2 commits December 18, 2023 18:11

[ISSUE-1190]feat(server-common): Support custom filter

bc09888

fix

18faf4d

qqqttt123 self-assigned this Dec 18, 2023

Heng Qin added 2 commits December 18, 2023 20:57

Merge remote-tracking branch 'upstream/main' into ISSUE-42

b17e332

fix

d5272f5

qqqttt123 force-pushed the ISSUE-42 branch from 64ed801 to d5272f5 Compare December 19, 2023 07:35

qqqttt123 changed the title ~~[ISSUE-1190]feat(server-common): Support custom filters~~ [ISSUE-1190] feat(server-common): Support custom filters Dec 19, 2023

qqqttt123 marked this pull request as ready for review December 19, 2023 08:05

qqqttt123 requested review from jerryshao, FANNG1, diqiu50, yuqi1129, mchades and Clearvive December 19, 2023 08:05

yuqi1129 reviewed Dec 19, 2023

View reviewed changes

FANNG1 reviewed Dec 20, 2023

View reviewed changes

[MINOR] fix(docs): Modify the required value for the document

55184a6

qqqttt123 mentioned this pull request Dec 20, 2023

[MINOR] fix(docs): Modify the required value for the document security.md #1214

Merged

Heng Qin added 3 commits December 20, 2023 11:32

fix

239e55d

Revert "[MINOR] fix(docs): Modify the required value for the document"

9a9c107

This reverts commit 55184a6.

fixed

831feee

github-actions bot mentioned this pull request Dec 20, 2023

[MINOR] fix(docs): Modify the required value for the document security.md #1223

Merged

jerryshao changed the title ~~[ISSUE-1190] feat(server-common): Support custom filters~~ [#1190] feat(server-common): Support custom filters Dec 21, 2023

jerryshao reviewed Dec 21, 2023

View reviewed changes

fix

d72c39d

jerryshao approved these changes Dec 21, 2023

View reviewed changes

jerryshao merged commit 18feb15 into apache:main Dec 21, 2023
11 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1190] feat(server-common): Support custom filters #1191

[#1190] feat(server-common): Support custom filters #1191

qqqttt123 commented Dec 18, 2023 •

edited

Loading

qqqttt123 Dec 18, 2023

qqqttt123 Dec 18, 2023

yuqi1129 Dec 19, 2023

qqqttt123 Dec 19, 2023

FANNG1 Dec 20, 2023

qqqttt123 Dec 20, 2023

qqqttt123 Dec 20, 2023

mchades Dec 20, 2023

qqqttt123 Dec 20, 2023

qqqttt123 Dec 20, 2023

qqqttt123 Dec 21, 2023

yuqi1129 Dec 19, 2023

qqqttt123 Dec 19, 2023

yuqi1129 Dec 19, 2023

qqqttt123 Dec 19, 2023

qqqttt123 Dec 20, 2023

FANNG1 Dec 20, 2023

qqqttt123 Dec 20, 2023

jerryshao Dec 21, 2023

qqqttt123 Dec 21, 2023

FANNG1 Dec 20, 2023

qqqttt123 Dec 20, 2023

qqqttt123 Dec 20, 2023

jerryshao Dec 21, 2023

qqqttt123 Dec 21, 2023

qqqttt123 Dec 21, 2023

jerryshao commented Dec 21, 2023

jerryshao commented Dec 21, 2023 •

edited

Loading

qqqttt123 commented Dec 21, 2023

jerryshao commented Dec 21, 2023 •

edited

Loading

qqqttt123 commented Dec 21, 2023

qqqttt123 commented Dec 21, 2023

[#1190] feat(server-common): Support custom filters #1191

[#1190] feat(server-common): Support custom filters #1191

Conversation

qqqttt123 commented Dec 18, 2023 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Dec 21, 2023

jerryshao commented Dec 21, 2023 • edited Loading

qqqttt123 commented Dec 21, 2023

jerryshao commented Dec 21, 2023 • edited Loading

qqqttt123 commented Dec 21, 2023

qqqttt123 commented Dec 21, 2023

qqqttt123 commented Dec 18, 2023 •

edited

Loading

jerryshao commented Dec 21, 2023 •

edited

Loading

jerryshao commented Dec 21, 2023 •

edited

Loading