Re: [PR] Add OPTION and HEAD to quarkus.http.cors.methods [polaris]
dimas-b commented on PR #3941: URL: https://github.com/apache/polaris/pull/3941#issuecomment-4013096971 Thanks for the fix, @nandorKollar ! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add OPTION and HEAD to quarkus.http.cors.methods [polaris]
dimas-b merged PR #3941: URL: https://github.com/apache/polaris/pull/3941 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add OPTION and HEAD to quarkus.http.cors.methods [polaris]
dimas-b commented on PR #3941: URL: https://github.com/apache/polaris/pull/3941#issuecomment-4013091477 This change looks backward-compatible to me and worth having in 1.4.0, so I'm going to merge now. Happy to reconsider if concerns are raised after merging. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add OPTION and HEAD to quarkus.http.cors.methods [polaris]
adutra commented on code in PR #3941: URL: https://github.com/apache/polaris/pull/3941#discussion_r2892422329 ## runtime/defaults/src/main/resources/application.properties: ## @@ -58,7 +58,7 @@ quarkus.http.body.handle-file-uploads=false quarkus.http.limits.max-body-size=10240K quarkus.http.cors.origins=http://localhost:8080 -quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT +quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT, HEAD, OPTIONS Review Comment: > I'm wondering why `quarkus.http.cors.*` has any non-default value in our application properties, when cors is turned off? Is it because if someone turns it on, then we think that these values are the safest option? No, imho these values were meant as (dummy) examples. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add OPTION and HEAD to quarkus.http.cors.methods [polaris]
dimas-b commented on code in PR #3941: URL: https://github.com/apache/polaris/pull/3941#discussion_r2892030935 ## runtime/defaults/src/main/resources/application.properties: ## @@ -58,7 +58,7 @@ quarkus.http.body.handle-file-uploads=false quarkus.http.limits.max-body-size=10240K quarkus.http.cors.origins=http://localhost:8080 -quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT +quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT, HEAD, OPTIONS Review Comment: `quarkus.http.cors.*` values were added for [Polaris UI](https://github.com/apache/polaris-tools/tree/main/console), IIRC. Yet, if we switch to Quarkus default (which have wider scope), it should not hurt UI, I think. I think it is sufficient to document only what we explicitly change (or instruct users to change) in Polaris compared to Quarkus defaults. Interested users can always go to Quarkus config docs (which are pretty easy to navigate in my experience). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add OPTION and HEAD to quarkus.http.cors.methods [polaris]
nandorKollar commented on code in PR #3941: URL: https://github.com/apache/polaris/pull/3941#discussion_r2891959644 ## runtime/defaults/src/main/resources/application.properties: ## @@ -58,7 +58,7 @@ quarkus.http.body.handle-file-uploads=false quarkus.http.limits.max-body-size=10240K quarkus.http.cors.origins=http://localhost:8080 -quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT +quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT, HEAD, OPTIONS Review Comment: It does, but only those, which have some non-default values no? If we don't set any default for `quarkus.http.cors.*`, just add `quarkus.http.cors.enabled=false`, then I think it is sufficient do document only `quarkus.http.cors.enabled` no? I'm wondering why `quarkus.http.cors.*` has any non-default value in our application properties, when cors is turned off? Is it because if someone turns it on, then we think that these values are the safest option? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add OPTION and HEAD to quarkus.http.cors.methods [polaris]
adutra commented on code in PR #3941: URL: https://github.com/apache/polaris/pull/3941#discussion_r2891934082 ## runtime/defaults/src/main/resources/application.properties: ## @@ -58,7 +58,7 @@ quarkus.http.body.handle-file-uploads=false quarkus.http.limits.max-body-size=10240K quarkus.http.cors.origins=http://localhost:8080 -quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT +quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT, HEAD, OPTIONS Review Comment: > I think we should also remove those from `configuring-polaris.md` too, as Quarkus defaults are used then. Well this section attempts to present common configs that most users would want to configure. I think CORS deserves to be there, wdyt? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add OPTION and HEAD to quarkus.http.cors.methods [polaris]
dimas-b commented on code in PR #3941: URL: https://github.com/apache/polaris/pull/3941#discussion_r2891930212 ## site/content/in-dev/unreleased/configuration/configuring-polaris.md: ## @@ -87,25 +87,25 @@ For a comprehensive reference of all Polaris configuration options, see the [Con Some often useful Quarkus configuration properties are listed below. Please refer to the [Quarkus documentation](https://quarkus.io/guides/) for more details. -| Configuration Property | Default Value | Description | -|--|-|-| -| `quarkus.log.level` | `INFO` | Define the root log level. | -| `quarkus.log.category."org.apache.polaris".level`| | Define the log level for a specific category. | -| `quarkus.default-locale` | System locale | Force the use of a specific locale, for instance `en_US`. | -| `quarkus.http.port` | `8181` | Define the HTTP port number. | -| `quarkus.http.auth.basic`| `false` | Enable the HTTP basic authentication. | -| `quarkus.http.limits.max-body-size` | `10240K` | Define the HTTP max body size limit. | -| `quarkus.http.cors.enabled` | `false` | Enable the HTTP CORS filter. Must be set to `true` for any other CORS property to take effect. | -| `quarkus.http.cors.origins` | | Define the HTTP CORS origins. | -| `quarkus.http.cors.methods` | `PATCH, POST, DELETE, GET, PUT` | Define the HTTP CORS covered methods. | -| `quarkus.http.cors.headers` | `*` | Define the HTTP CORS covered headers. | -| `quarkus.http.cors.exposed-headers` | `*` | Define the HTTP CORS covered exposed headers. | -| `quarkus.http.cors.access-control-max-age` | `PT10M` | Define the HTTP CORS access control max age. | -| `quarkus.http.cors.access-control-allow-credentials` | `true` | Define the HTTP CORS access control allow credentials flag. | -| `quarkus.management.enabled` | `true` | Enable the management server. | -| `quarkus.management.port`| `8182` | Define the port number of the Polaris management server. | -| `quarkus.management.root-path` | | Define the root path where `/metrics` and `/health` endpoints are based on. | -| `quarkus.otel.sdk.disabled` | `true` | Enable the OpenTelemetry layer. | +| Configuration Property | Default Value | Description | +|--||| +| `quarkus.log.level` | `INFO` | Define the root log level. | +| `quarkus.log.category."org.apache.polaris".level`| | Define the log level for a specific category. | +| `quarkus.default-locale` | System locale | Force the use of a specific locale, for instance `en_US`. | +| `quarkus.http.port` | `8181` | Define the HTTP port number. | +| `quar
Re: [PR] Add OPTION and HEAD to quarkus.http.cors.methods [polaris]
nandorKollar commented on code in PR #3941: URL: https://github.com/apache/polaris/pull/3941#discussion_r2891923492 ## runtime/defaults/src/main/resources/application.properties: ## @@ -58,7 +58,7 @@ quarkus.http.body.handle-file-uploads=false quarkus.http.limits.max-body-size=10240K quarkus.http.cors.origins=http://localhost:8080 -quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT +quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT, HEAD, OPTIONS Review Comment: Oh okay true, the doc also says that. Yes, in this case better to comment out comment out those configs. I think we should also remove those from `configuring-polaris.md` too, as Quarkus defaults are used then. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add OPTION and HEAD to quarkus.http.cors.methods [polaris]
adutra commented on code in PR #3941: URL: https://github.com/apache/polaris/pull/3941#discussion_r2891904874 ## runtime/defaults/src/main/resources/application.properties: ## @@ -58,7 +58,7 @@ quarkus.http.body.handle-file-uploads=false quarkus.http.limits.max-body-size=10240K quarkus.http.cors.origins=http://localhost:8080 -quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT +quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT, HEAD, OPTIONS Review Comment: They are disabled by default. The default value of `quarkus.http.cors.enabled` is `false`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add OPTION and HEAD to quarkus.http.cors.methods [polaris]
nandorKollar commented on code in PR #3941: URL: https://github.com/apache/polaris/pull/3941#discussion_r2891888780 ## runtime/defaults/src/main/resources/application.properties: ## @@ -58,7 +58,7 @@ quarkus.http.body.handle-file-uploads=false quarkus.http.limits.max-body-size=10240K quarkus.http.cors.origins=http://localhost:8080 -quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT +quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT, HEAD, OPTIONS Review Comment: Hmm, should we disable CORS filters? The rational would be that CORS on REST endpoints it isn't less critical, than let's say on a website? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add OPTION and HEAD to quarkus.http.cors.methods [polaris]
adutra commented on code in PR #3941: URL: https://github.com/apache/polaris/pull/3941#discussion_r2891205044 ## runtime/defaults/src/main/resources/application.properties: ## @@ -58,7 +58,7 @@ quarkus.http.body.handle-file-uploads=false quarkus.http.limits.max-body-size=10240K quarkus.http.cors.origins=http://localhost:8080 -quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT +quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT, HEAD, OPTIONS Review Comment: That is the default value in Quarkus, so I wonder if it's simpler to just comment this out? Or better yet: I think we should include `quarkus.http.cors.enabled` (it's probably an oversight), and comment out all the rest: ```properties quarkus.http.cors.enabled=false #quarkus.http.cors.origins=http://example.com:8080 #quarkus.http.cors.methods=PATCH, POST, DELETE, GET, PUT, OPTIONS, HEAD #quarkus.http.cors.headers=* #quarkus.http.cors.exposed-headers=* #quarkus.http.cors.access-control-max-age=PT10M #quarkus.http.cors.access-control-allow-credentials=true ``` WDYT? ## site/content/in-dev/unreleased/configuration/configuring-polaris.md: ## @@ -87,25 +87,25 @@ For a comprehensive reference of all Polaris configuration options, see the [Con Some often useful Quarkus configuration properties are listed below. Please refer to the [Quarkus documentation](https://quarkus.io/guides/) for more details. -| Configuration Property | Default Value | Description | -|--|-|-| -| `quarkus.log.level` | `INFO` | Define the root log level. | -| `quarkus.log.category."org.apache.polaris".level`| | Define the log level for a specific category. | -| `quarkus.default-locale` | System locale | Force the use of a specific locale, for instance `en_US`. | -| `quarkus.http.port` | `8181` | Define the HTTP port number. | -| `quarkus.http.auth.basic`| `false` | Enable the HTTP basic authentication. | -| `quarkus.http.limits.max-body-size` | `10240K` | Define the HTTP max body size limit. | -| `quarkus.http.cors.enabled` | `false` | Enable the HTTP CORS filter. Must be set to `true` for any other CORS property to take effect. | -| `quarkus.http.cors.origins` | | Define the HTTP CORS origins. | -| `quarkus.http.cors.methods` | `PATCH, POST, DELETE, GET, PUT` | Define the HTTP CORS covered methods. | -| `quarkus.http.cors.headers` | `*` | Define the HTTP CORS covered headers. | -| `quarkus.http.cors.exposed-headers` | `*` | Define the HTTP CORS covered exposed headers. | -| `quarkus.http.cors.access-control-max-age` | `PT10M` | Define the HTTP CORS access control max age. | -| `quarkus.http.cors.access-control-allow-credentials` | `true` | Define the HTTP CORS access control allow credentials flag. | -| `quarkus.management.enabled` | `true` | Enable the management server. | -| `quarkus.management.port`| `8182` | Define the port number of the Polaris management server. | -| `quarkus.management.root-path` | | Define the root path where `/metrics` and `/health` endpoints are based on. | -| `quarkus.otel.sdk.disabled` | `true` | Enable the OpenTelemetry layer. | +| Configuration Property | Default Value | Description | +|
