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.

Sign up for GitHub

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

Jump to bottom

Allow a ConnectionFactory (eg SslConnectionFactory) to automatically add a Customizer #4815

Merged
merged 4 commits into from
Apr 29, 2020
Merged

Allow a ConnectionFactory (eg SslConnectionFactory) to automatically add a Customizer #4815

merged 4 commits into from
Apr 29, 2020

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Apr 25, 2020 •
edited
Loading

This PR for #4814 adds a Connector.Configuring interface to allow the SslConnectionFactory to ensure there is a SecureRequestCustomizer.

I have made this a draft as I'm a bit worried about modifying shared HttpConfiguration instances... not sure if it is a concern and if it is, what to do about it?

There is also a good optimisation of the request attributes for SSL (probably should be a different PR... maybe even back ported to 9.4).

@gregw gregw requested a review from sbordet April 25, 2020 09:34
@gregw
Copy link
Contributor Author

gregw commented Apr 25, 2020

@lachlan-roberts look at what I've done with Attributes in this PR. This is kind of what I'm thinking we need to do with Async dispatch as well.

@joakime
Copy link
Contributor

joakime commented Apr 25, 2020

What happens when you mix SecureRequestCustomizer and ForwardedRequestCustomizer in the same connector/configuration?
Eg: Who wins the scheme and isSecure setting? (or will one undo the other?)
Does order of those customizers matter?

@gregw
Copy link
Contributor Author

gregw commented Apr 27, 2020 •
edited
Loading

Spun out Attributes improvements to #4816. Edit: I'll remove them from this PR shortly.

@gregw
Copy link
Contributor Author

gregw commented Apr 27, 2020

@joakime I don't think it matters, but probably should be reviewed carefully. SecureRequestCustomer will setSecure if the connection is secure (eg SSL), whilst ForwardedRequestCustomizer will setSecure if the headers say it is secure. I don't think either will clear secure, so the only clash is if they both trigger, but then they both should have the same action, so I think that is OK (but kind of strange setup).

@gregw
Issue #4814 Configuring Connection Factory
57a02bd 
Redo of this PR without Attributes improvements (moved to #4816).
Add a ConnectionFactory.Configuring interface to all connectors to be configured during doStart.
I have some concern about shared HttpConfigurations.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw force-pushed the jetty-10.0.x-4814-ConfiguringConnectionFactory branch from 331e8da to 57a02bd Compare April 27, 2020 09:24
@gregw
Copy link
Contributor Author

gregw commented Apr 27, 2020

I have forced pushed this PR with only the ConnectionFactory.Configuring changes. The Attributes improvements are now all in #4488

I still have a bit of a concern how this approach might affect any shared HttpConfiguration instances?

@sbordet sbordet linked an issue Apr 28, 2020 that may be closed by this pull request
Allow a ConnectionFactory (eg SslConnectionFactory) to automatically add a Customizer #4814
Closed
sbordet
sbordet approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Looks mostly ok, but there is no test that calls setEnsureSecureRequestCustomizer(). The implementation is quite trivial though, so maybe there is no need to.
Please update the javadocs before merge.

jetty-server/src/main/java/org/eclipse/jetty/server/ConnectionFactory.java
@@ -125,4 +125,16 @@
*/
Detection detect(ByteBuffer buffer);
}

/**
* A ConnectionFactory that can configure the connector.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix the javadoc, as Configuring is not a ConnectionFactory.
Unless, only a ConnectionFactory can implement Configuring.
If something else can, then the javadoc needs to be fixed, something like: "Implementing this class allows to configure the connector (or its component tree) at connector startup".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make it extend ConnectionFactory like the other 'ing' inner interfaces

@sbordet sbordet self-requested a review April 28, 2020 13:45
sbordet
sbordet requested changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

I meant to request the javadocs changes and a clarification of who can implement Configuring.

@sbordet
Copy link
Contributor

sbordet commented Apr 28, 2020

@gregw this is a draft PR - still needs to be draft?

@gregw
Copy link
Contributor Author

gregw commented Apr 28, 2020

@sbordet It is draft as I wanted to get feedback on the approach with regards to adding a customizer to a potentially shared HttpConfiguration.

@sbordet
Copy link
Contributor

sbordet commented Apr 28, 2020

@gregw the HttpConfiguration may be shared among HTTP connection factories on the same connector, i.e. HTTP/1.1 and HTTP/2.
I think that is good.

However, the PR also handles the case where HTTP/1.1 and HTTP/2 have been configured with two different HttpConfigurations, for example you want different buffer sizes or such.

If you want to manually configure the SecureRequestCustomizer, you can too.

So I think we're good.

@gregw
updates from review
82e68bb 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
ade9fe4 
…4814-ConfiguringConnectionFactory
@gregw gregw marked this pull request as ready for review April 29, 2020 07:16
@gregw gregw requested a review from sbordet April 29, 2020 07:16
@gregw gregw merged commit 81c4663 into jetty-10.0.x Apr 29, 2020
@gregw gregw deleted the jetty-10.0.x-4814-ConfiguringConnectionFactory branch April 29, 2020 09:45
@joakime joakime changed the title Jetty 10.0.x 4814 configuring connection factory Allow a ConnectionFactory (eg SslConnectionFactory) to automatically add a Customizer Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow a ConnectionFactory (eg SslConnectionFactory) to automatically add a Customizer
3 participants
@gregw @joakime @sbordet