You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "dimas-b (via GitHub)" <gi...@apache.org> on 2025/01/08 23:26:46 UTC
[PR] Remove testRequestHeaderTooLarge() [polaris]
dimas-b opened a new pull request, #630:
URL: https://github.com/apache/polaris/pull/630
This test is not normative to Polaris APIs.
Moreover, it does not test any of Polaris code, but instead depends on Dropwizard's behaviour.
While the intent of making sure that header sizes are restricted is wise, it does not look like validating that in Polaris CI is the best option. As the comment in the deleted test suggests actual runtime behaviour is heavily affected by the execution environment.
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2580917683
From the other thread:
> I personally do not see much value in a test that either validates the status code or does not validate anything because a connection close exception is inconclusive and does not guarantee that even the request was sent.
Connection close is broad but if we have very similar code elsewhere and the connection only closes when we set large headers, that's pretty strong evidence that the large headers caused that.
IMO there's value in keeping this test. It decreases the likelihood that we accidentally flip a switch or migrate to a framework that disables this protection.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b closed pull request #630: Remove testRequestHeaderTooLarge()
URL: https://github.com/apache/polaris/pull/630
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2580961759
I don't think the fact that the test could (but has never been observed to?) sometimes flake and give false positives is a compelling enough reason to remove it without a replacement.
> Moreover, it does not test any of Polaris code, but instead depends on Dropwizard's behaviour.
It tests the Polaris _application_.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2579317192
I'm a little wary of removing this test. Is there a scenario (with Quarkus?) where it fails?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2580600882
After [this](https://github.com/apache/polaris/pull/631#discussion_r1908953673) discussion, I think I'll keep the test, but make it check only the status code, without checking the response payload since some systems may not provide the payload.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2583351292
Test robustness was improved in #590... Closing this PR.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2580982818
> It tests the Polaris application.
True, but is it "Polaris" that specifies the behaviour of handling excessively large headers? In this particular case, it looks like confounding influence from infrastructural components of the "application" is so great as to make a deterministic test impractical.
That said, I'll try and debug whether the connection closed problems might be a client-side thing.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2580988756
Converted this PR to "draft" pending in-depth investigation.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2580421142
@andrew4699 : 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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2580813967
Ok, pivoting again (per discussion under #631). Let's remove both the "body" and "header" (this PR) size tests.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2580923589
I'll do some debugging...
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2580929348
> Are you removing this test because it will fail after the Quarkus refactor?
No. Even with DW the "header" test expects connection closed exceptions. It's this uncertainty of behaviour that makes the test not so valuable, IMHO. If the test gives a false positive result (by getting a connection closed exception for _another_ reason), it's worse than having no test, IMHO.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2580985995
Another suggestion: maybe in the spirit of moving forward quickly with #590, let's keep the tests and add a catch clause for connection closed exceptions _in both tests_. How does that sound?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2583012097
I believe the connection reset happens if/when the request is split across two (or more) TCP packets. The server responds having read the first packet and closes the connection. The client attempts to keep writing the rest of the request and gets a connection closed exception (specifically `SocketException: Broken pipe` under Dropwizard).
I'd argue that the client in use by test is not robust enough for this test case.
We probably need a different client impl. to gracefully handle reading the server's status line in this case even though the full request could not be written.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2580428856
@eric-maynard : "fails" is what sense? Do you mean Quarkus might not have restrictions on header sizes?... or that Quarkus default limits might be too large and cause issues in Polaris code?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2583373582
Out of curiosity I ran the same test with `HttpURLConnection` and it was able to obtain the status code in hundreds of requests, while the existing client failed with a "connection reset" after about 15 requests... but even the `HttpURLConnection` was not 100% reliable, so I went with `Awaitility` in #590.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove testRequestHeaderTooLarge() [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on PR #630:
URL: https://github.com/apache/polaris/pull/630#issuecomment-2580897245
> "fails" is what sense?
In the sense of the test literally failing. Are you removing this test because it will break after the Quarkus refactor?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org