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

SOLR-16859 Missing Proxy support for Http2SolrClient #1779

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

stillalex
Copy link
Member

@stillalex stillalex commented Jul 12, 2023

https://issues.apache.org/jira/browse/SOLR-16859

Description

Please provide a short description of the changes you're making with this pull request.

Solution

Please provide a short description of the approach taken to implement your solution.

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

@sonatype-lift
Copy link

sonatype-lift bot commented Jul 12, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

@epugh
Copy link
Contributor

epugh commented Jul 13, 2023

Is this the type of integration test that would be easier to implement via .bats test??

@stillalex
Copy link
Member Author

@epugh would you mind adding a bit more detail? how would a bats test help

@epugh
Copy link
Contributor

epugh commented Jul 13, 2023

I noticed that we have a test_ssl.bats test that exercises Solr. I wonder if extending those tests would help validate this fix?

@stillalex stillalex force-pushed the SOLR-16859-Http2SolrClient-proxy branch from eef363f to 851e74e Compare August 10, 2023 21:06
@stillalex
Copy link
Member Author

stillalex commented Aug 10, 2023

I noticed that we have a test_ssl.bats test that exercises Solr. I wonder if extending those tests would help validate this fix?

no, we'd need a proxy server for testing ...unless there is some command line proxy server that is easy to integrate into a bats test.

I manually installed a proxy (toxiproxy) on my local to smoke test this and was able to check http1 and http2 modes. @epugh would you say this is sufficient to push the code in? if there are bugs in more complex setups we can fix them as they come (as opposed to not having this feature in). I would hate to leave this PR open for much longer.

Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

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

This looks great to me! Someday I hope we have some way of testing out ALL the various ways of running Solr, though I can imagine that the tooling to support that might be overwhelming too ;-). Merge it!

@stillalex
Copy link
Member Author

minor update. test is failing when the SSL context is enabled, which I did not know it can happen. I spent a bit of time on this and I can not figure out how to get the Solr SocketProxy to talk to the Solr node via https - in theory it has a flag, but I could not get it working, I must be missing some key info here.
I am a bit undecided on what to do with https support. on one hand this will work for plain connections, on the other I can't get the test to setup https so I can't verify https works or not. will sit on this PR a bit more, maybe I can find a way to set this up correctly.

@stillalex stillalex marked this pull request as draft August 11, 2023 20:38
@stillalex stillalex force-pushed the SOLR-16859-Http2SolrClient-proxy branch from 851e74e to 1a5ff4a Compare August 11, 2023 20:45
@stillalex stillalex force-pushed the SOLR-16859-Http2SolrClient-proxy branch from 1a5ff4a to b6a6438 Compare August 16, 2023 17:38
@stillalex stillalex marked this pull request as ready for review August 16, 2023 17:40
@stillalex
Copy link
Member Author

re. SSL testing. it is very difficult to setup the exiting proxy server for this test, I think it does not actually support SSL. I've sent an email to dev list to ask for more help. @risdenk spent some time on this, thanks Kevin! I don't think it's worth while to sit on this much longer. if/when there is feedback that needs more fixes, we can address then, as needed.
Thanks @epugh, will merge this PR very soon.

@stillalex stillalex merged commit 21740ac into apache:main Aug 16, 2023
3 checks passed
@stillalex stillalex deleted the SOLR-16859-Http2SolrClient-proxy branch August 16, 2023 18:06
stillalex added a commit that referenced this pull request Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants