Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce quarkus.http.header #21102

Merged
merged 2 commits into from
Nov 3, 2021
Merged

Introduce quarkus.http.header #21102

merged 2 commits into from
Nov 3, 2021

Conversation

gastaldi
Copy link
Contributor

This allows to explicitly set fixed headers on HTTP responses

@gastaldi gastaldi changed the title Introduce quarkus.http.headers Introduce quarkus.http.headers Oct 29, 2021
@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor

geoand commented Oct 30, 2021

How is this expected to behave when there is an exception?

How about when then header has already been set by the application?

@gastaldi
Copy link
Contributor Author

The idea here is to use it for fixed headers, specially those from https://wiki.owasp.org/index.php/OWASP_Secure_Headers_Project, which must be set regardless of the outcome.

Other cases should adopt a filter instead

@geoand
Copy link
Contributor

geoand commented Oct 30, 2021

But is that how this behaves?

Furthermore, have you tried it (and added tests) for all the various HTTP options (pure Vert.x, Reactive Routes, RESTEasy Classic and RESTEasy Reactive)?

Also, the expected usage and behavior should be very clearly documented

@gastaldi
Copy link
Contributor Author

gastaldi commented Oct 30, 2021

Furthermore, have you tried it (and added tests) for all the various HTTP options (pure Vert.x, Reactive Routes, RESTEasy Classic and RESTEasy Reactive)?

No, I assumed it would work since they all use vertx-http, but maybe it would be nice to have tests for them too

@gastaldi
Copy link
Contributor Author

I think another common use case would be to add headers for a specific URL pattern.

I'll change it to a draft meanwhile

@gastaldi gastaldi marked this pull request as draft October 30, 2021 12:37
@gastaldi gastaldi changed the title Introduce quarkus.http.headers Introduce quarkus.http.header Oct 31, 2021
@gastaldi
Copy link
Contributor Author

gastaldi commented Oct 31, 2021

Moving out of draft. I believe this should be enough to allow setting fixed headers in the application regardless on the requested URL.

I'll check about having tests for the other HTTP options as mentioned by @geoand in #21102 (comment) (probably in a separate PR)

@gastaldi gastaldi marked this pull request as ready for review October 31, 2021 23:44
httpRouteRouter.route().order(Integer.MIN_VALUE).handler(new Handler<RoutingContext>() {
@Override
public void handle(RoutingContext event) {
event.response().headers().addAll(headers);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the application could clear this map, which is not uncommon in the case of errors (e.g. if you are sending an error response you might clear this to make sure nothing is left over from the failed response).

I don't know how much this matters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the primary goal of this feature is to set HTTP Headers in the response without having to write additional code. I think in the use case you described this would be still valid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least, the application can check the headers and decide if they need to be removed or not (even is most of the time, we just call clear).

@quarkus-bot
Copy link

quarkus-bot bot commented Nov 1, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building ebb18f0

Status Name Step Failures Logs Raw logs
Native Tests - Windows - hibernate-validator Build Failures Logs Raw logs

Failures

⚙️ Native Tests - Windows - hibernate-validator #

- Failing: integration-tests/hibernate-validator 

📦 integration-tests/hibernate-validator

Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-hibernate-validator: Failed to build quarkus application

httpRouteRouter.route().order(Integer.MIN_VALUE).handler(new Handler<RoutingContext>() {
@Override
public void handle(RoutingContext event) {
event.response().headers().addAll(headers);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least, the application can check the headers and decide if they need to be removed or not (even is most of the time, we just call clear).

@gastaldi
Copy link
Contributor Author

gastaldi commented Nov 2, 2021

Do you guys think that we should support paths in this PR or this can be achieved in a separate PR?

eg.

quarkus.http.header.Pragma.value=no-cache
quarkus.http.header.Pragma.path="/foo/bar"

Would apply the Pragma header only when /foo/baris requested

@gastaldi
Copy link
Contributor Author

gastaldi commented Nov 2, 2021

I think I'll implement support for path and method(s) in the Header configuration in this PR, this would make it more flexible IMHO

@gastaldi gastaldi marked this pull request as draft November 2, 2021 00:56
@gastaldi gastaldi marked this pull request as ready for review November 2, 2021 01:48
@quarkus-bot

This comment has been minimized.

This allows to explicitly set fixed headers on HTTP responses

Support path and method for the header

Signed-off-by: George Gastaldi <[email protected]>
@gastaldi gastaldi added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 3, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Nov 3, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 5934870

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 17 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: integration-tests/vault-agroal 

📦 integration-tests/vault-agroal

io.quarkus.vault.AgroalVaultITCase. - More details - Source on GitHub

java.lang.RuntimeException: Error waiting for test resource future to finish.
	at io.quarkus.test.common.TestResourceManager.waitForAllFutures(TestResourceManager.java:151)
	at io.quarkus.test.common.TestResourceManager.start(TestResourceManager.java:136)

io.quarkus.vault.AgroalVaultKv1ITCase. - More details - Source on GitHub

java.lang.RuntimeException: Error waiting for test resource future to finish.
	at io.quarkus.test.common.TestResourceManager.waitForAllFutures(TestResourceManager.java:151)
	at io.quarkus.test.common.TestResourceManager.start(TestResourceManager.java:136)

io.quarkus.vault.VaultKv1ITCase. - More details - Source on GitHub

java.lang.RuntimeException: Error waiting for test resource future to finish.
	at io.quarkus.test.common.TestResourceManager.waitForAllFutures(TestResourceManager.java:151)
	at io.quarkus.test.common.TestResourceManager.start(TestResourceManager.java:136)

@quarkus-bot
Copy link

quarkus-bot bot commented Nov 3, 2021

Failing Jobs - Building 5934870

Status Name Step Failures Logs Raw logs
Native Tests - Windows - hibernate-validator Build Failures Logs Raw logs

Failures

⚙️ Native Tests - Windows - hibernate-validator #

- Failing: integration-tests/hibernate-validator 

📦 integration-tests/hibernate-validator

Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-hibernate-validator: Failed to build quarkus application

@gastaldi gastaldi merged commit 96025ee into quarkusio:main Nov 3, 2021
@quarkus-bot quarkus-bot bot added this to the 2.5 - main milestone Nov 3, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 3, 2021
@gastaldi gastaldi deleted the http branch November 3, 2021 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants