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

GrizzlyHttpServerTest - testing stability with HTTP, HTTPS and HTTP/2 #4740

Merged
merged 1 commit into from
Sep 16, 2021
Merged

GrizzlyHttpServerTest - testing stability with HTTP, HTTPS and HTTP/2 #4740

merged 1 commit into from
Sep 16, 2021

Conversation

dmatej
Copy link
Contributor

@dmatej dmatej commented Mar 4, 2021

  • parallel execution on random free ports
  • disabled for jdk8
  • possibility to use three different clients
    • jersey-client - but it doesn't support HTTP/2
    • jdk11+ HttpClient - but it is not available in older JDK versions
    • jetty-client - but version compatible with jdk8 uses different classes than version compatible with jdk11
  • each test creates a configured server, then creates several clients
    which are sending GET requests for given time.
  • all responses must be HTTP 200
  • after the test then number of processed requests is written to the STDOUT
  • if there will be just one non compliant response, all clients are stopped and the test fails.
  • originally created to reproduce the issue Grizzly HTTP2 Session/Stream - Memory Leak grizzly#2125 of the Grizzly project, which used Jersey and Grizzly together.

- parallel execution on random free ports
- disabled for jdk8
- possibility to use three different clients
  - jersey-client - but it doesn't support HTTP/2
  - jdk11+ HttpClient - but it is not available in older JDK versions
  - jetty-client - but version compatible with jdk8 uses different classes
      than version compatible with jdk11
- each test creates a configured server, then creates several clients
  which are sending GET requests for given time.
- all responses must be HTTP 200
- after the test then number of processed requests is written to the STDOUT
- if there will be just one non compliant response, all clients are stopped
  and the test fails.
- originally created to reproduce the issue #2125 of the Grizzly project,
  which used Jersey and Grizzly together.

Signed-off-by: David Matějček <dmatej@seznam.cz>
@jansupol
Copy link
Contributor

BouncyCastle requires CQ, but it has a proprietary BouncyCastle license. Do we need the BouncyCastle?

@dmatej
Copy link
Contributor Author

dmatej commented Apr 28, 2021

BouncyCastle requires CQ, but it has a proprietary BouncyCastle license. Do we need the BouncyCastle?

License should not be a problem: https://www.bouncycastle.org/fr/licence.html

@dmatej
Copy link
Contributor Author

dmatej commented Apr 28, 2021

But maybe I can try to simplify it, basically I reused my older code.
Also newest JDK updates have some issues with keystores as they removed some ciphers, so it would be safer if I would review my code once more.

@dmatej
Copy link
Contributor Author

dmatej commented Apr 28, 2021

Aaargh, I deleted wrong repository by mistake ... so I have to create a new PR ... I think there is no other way to fix it.

@dmatej
Copy link
Contributor Author

dmatej commented May 2, 2021

So, current state:

  1. Please tell me if I should create new duplicit PR, because I deleted the original fork by mistake. Now I created new fork, based directly on the Eclipse's instead of Payara's fork. I am not sure if Github can merge it even when the repo vanished.
  2. I need some navigation for that CQ, I "googled" just this link, but I cannot find anything like that.
  3. Alternative for the BouncyCastle exists, but it is proprietary too - it is the keytool implementation under sun.security.x509.* package, which requires additional instructions for the compiler. I expect this package will be completely replaced in future JDK releases, also I'm not sure if it is present in all current JDK distributions with the same api (but maybe is). But I would expect that the certificate management will significantly change in comming years, so ... from my point of view replacing BC dependency with some "future standard" is easier than having to mess with the compiler settings and all changes one by one (I still remember issues with ALPN).
  4. BC is just a test dependency, so it doesn't affect dependencies of the Jersey artifacts.

@arjantijms
Copy link
Contributor

BouncyCastle requires CQ, but it has a proprietary BouncyCastle license. Do we need the BouncyCastle?

To create a self-signed cert, I think it's one of the only viable options. I typically use it too in tests like e.g. this:

private static X509Certificate createSelfSignedCertificate(KeyPair keys) {
        try {
            Provider provider = new BouncyCastleProvider();
            Security.addProvider(provider);
            return new JcaX509CertificateConverter()
                    .setProvider(provider)
                    .getCertificate(
                            new X509v3CertificateBuilder(
                                    new X500Name("CN=lfoo, OU=bar, O=kaz, L=zak, ST=lak, C=UK"),
                                    ONE,
                                    Date.from(now()),
                                    Date.from(now().plus(1, DAYS)),
                                    new X500Name("CN=lfoo, OU=bar, O=kaz, L=zak, ST=lak, C=UK"),
                                    SubjectPublicKeyInfo.getInstance(keys.getPublic().getEncoded()))
                    .build(
                            new JcaContentSignerBuilder("SHA256WithRSA")
                                .setProvider(provider)
                                .build(keys.getPrivate())));
        } catch (CertificateException | OperatorCreationException e) {
            throw new IllegalStateException(e);
        }
    }

As it's only used for testing, it would concern a "works with" CQ, which is far more relaxed.

@arjantijms
Copy link
Contributor

A small ping here. @jansupol @dmatej any progress?

@dmatej
Copy link
Contributor Author

dmatej commented Aug 23, 2021

A small ping here. @jansupol @dmatej any progress?

I expected you will create the request to approve using BouncyCastle in test dependencies, I don't have rights to do that ;)
Even your example uses the BouncyCastleProvider - there are no reliable alternatives, because the sun.* implementation in JDK which can do that too changes API between JDK versions. BC not.

@senivam
Copy link
Contributor

senivam commented Sep 7, 2021

CQ#: 23715 RESOLVED
CQ#: 23714 RESOLVED

Copy link
Contributor

@senivam senivam left a comment

Choose a reason for hiding this comment

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

CQs are RESOLVED, LGTM

@dmatej
Copy link
Contributor Author

dmatej commented Sep 8, 2021

Seems there is some bug on GitHub - I just rebased the branch to current eclipse/3.x branch, but now GitHub says that the repository was deleted, which is not true. There were no conflicts, I verified even that the version is still the same. Is it still possible to merge it, or should I create a new PR, same as this one?
I don't have write access here, I would try that otherwise.
https://github.com/dmatej/jersey/tree/http2-test-3.x

@senivam
Copy link
Contributor

senivam commented Sep 8, 2021

@dmatej as from my point of view (at GH) the PR still exists and can be merged (w/o conflicts), so no reason to change anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants