You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/05/04 18:11:32 UTC
[GitHub] [trafficserver] randall opened a new pull request, #8818: Expose setting some HTTP/2 tunables via sni.yaml
randall opened a new pull request, #8818:
URL: https://github.com/apache/trafficserver/pull/8818
proxy.config.http2.default_buffer_water_mark can now be set on a
per-domain basis.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on a diff in pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
randall commented on code in PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#discussion_r869634201
##########
iocore/net/I_NetVConnection.h:
##########
@@ -187,6 +187,8 @@ struct NetVCOptions {
uint32_t packet_tos;
uint32_t packet_notsent_lowat;
+ uint32_t http2_buffer_water_mark;
Review Comment:
That sounds reasonable.
And yes, I remember connection coalescing. I work for a CDN that tends to keep SANs for certs separated by SNI. ;) How this works today, is perfect for our use case. But I understand others probably do not work in the same way.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall merged pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
randall merged PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#discussion_r877668134
##########
proxy/http2/Http2ClientSession.cc:
##########
@@ -116,9 +117,17 @@ Http2ClientSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB
this->_read_buffer_reader = reader ? reader : this->read_buffer->alloc_reader();
// This block size is the buffer size that we pass to SSLWriteBuffer
- auto buffer_block_size_index = iobuffer_size_to_index(Http2::write_buffer_block_size, MAX_BUFFER_SIZE_INDEX);
- this->write_buffer = new_MIOBuffer(buffer_block_size_index);
- this->write_buffer->water_mark = Http2::buffer_water_mark;
+ auto buffer_block_size_index = iobuffer_size_to_index(Http2::write_buffer_block_size, MAX_BUFFER_SIZE_INDEX);
+ this->write_buffer = new_MIOBuffer(buffer_block_size_index);
+
+ uint32_t buffer_water_mark;
+ SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(this->_vc);
Review Comment:
Sorry for bugging you on this, but the point of TLSSomethingSupport is to remove the dependency for SSLNetVC. Including P_SSLNetVConnection.h brings a lot of dependencies unnecessarily (I haven't completely removed the dependency so there's no difference at the moment though).
You could include "TLSSNISupport.h" instead, and access the member like:
```
TLSSNISupport *snis = dynamic_cast<TLSSNISupport *>(this->_vc);
if (snis && snis->hints_from_sni....
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#discussion_r877668134
##########
proxy/http2/Http2ClientSession.cc:
##########
@@ -116,9 +117,17 @@ Http2ClientSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB
this->_read_buffer_reader = reader ? reader : this->read_buffer->alloc_reader();
// This block size is the buffer size that we pass to SSLWriteBuffer
- auto buffer_block_size_index = iobuffer_size_to_index(Http2::write_buffer_block_size, MAX_BUFFER_SIZE_INDEX);
- this->write_buffer = new_MIOBuffer(buffer_block_size_index);
- this->write_buffer->water_mark = Http2::buffer_water_mark;
+ auto buffer_block_size_index = iobuffer_size_to_index(Http2::write_buffer_block_size, MAX_BUFFER_SIZE_INDEX);
+ this->write_buffer = new_MIOBuffer(buffer_block_size_index);
+
+ uint32_t buffer_water_mark;
+ SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(this->_vc);
Review Comment:
Sorry for bugging you on this, but the point of TLSSomethingSupport is to remove dependencies for SSLNetVC. Including P_SSLNetVConnection.h brings a lot of dependencies unnecessarily (I haven't completely removed the dependency so there's no difference at the moment though).
You could include "TLSSNISupport.h" instead, and access the member like:
```
TLSSNISupport *snis = dynamic_cast<TLSSNISupport *>(this->_vc);
if (snis && snis->hints_from_sni....
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#issuecomment-1143025953
I approved this PR, but someone else need to approve it to merge.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
randall commented on PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#issuecomment-1143697699
For back porting to 9.2.x: https://github.com/apache/trafficserver/pull/8875
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on a diff in pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
randall commented on code in PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#discussion_r877504719
##########
iocore/net/I_NetVConnection.h:
##########
@@ -187,6 +188,8 @@ struct NetVCOptions {
uint32_t packet_tos;
uint32_t packet_notsent_lowat;
+ TLSSNISupport::HintsFromSNI hintsFromSNI;
Review Comment:
ah. Now I see the relationship here. Corrected this.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on a diff in pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
randall commented on code in PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#discussion_r876543276
##########
proxy/http2/Http2ClientSession.cc:
##########
@@ -116,9 +116,16 @@ Http2ClientSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB
this->_read_buffer_reader = reader ? reader : this->read_buffer->alloc_reader();
// This block size is the buffer size that we pass to SSLWriteBuffer
- auto buffer_block_size_index = iobuffer_size_to_index(Http2::write_buffer_block_size, MAX_BUFFER_SIZE_INDEX);
- this->write_buffer = new_MIOBuffer(buffer_block_size_index);
- this->write_buffer->water_mark = Http2::buffer_water_mark;
+ auto buffer_block_size_index = iobuffer_size_to_index(Http2::write_buffer_block_size, MAX_BUFFER_SIZE_INDEX);
+ this->write_buffer = new_MIOBuffer(buffer_block_size_index);
+
+ uint32_t buffer_water_mark;
+ if (this->_vc->options.http2_buffer_water_mark != 0) {
Review Comment:
Updated to use `std::optional`. I think it's clearer now
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#discussion_r876568253
##########
iocore/net/I_NetVConnection.h:
##########
@@ -187,6 +188,8 @@ struct NetVCOptions {
uint32_t packet_tos;
uint32_t packet_notsent_lowat;
+ TLSSNISupport::HintsFromSNI hintsFromSNI;
Review Comment:
Do you still need this? Since you added `hints_from_sni` to `SNISupport`, you should be able to access the member like `vc->hints_from_sni`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on a diff in pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
randall commented on code in PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#discussion_r886077890
##########
proxy/http2/Http2ClientSession.cc:
##########
@@ -116,9 +117,17 @@ Http2ClientSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB
this->_read_buffer_reader = reader ? reader : this->read_buffer->alloc_reader();
// This block size is the buffer size that we pass to SSLWriteBuffer
- auto buffer_block_size_index = iobuffer_size_to_index(Http2::write_buffer_block_size, MAX_BUFFER_SIZE_INDEX);
- this->write_buffer = new_MIOBuffer(buffer_block_size_index);
- this->write_buffer->water_mark = Http2::buffer_water_mark;
+ auto buffer_block_size_index = iobuffer_size_to_index(Http2::write_buffer_block_size, MAX_BUFFER_SIZE_INDEX);
+ this->write_buffer = new_MIOBuffer(buffer_block_size_index);
+
+ uint32_t buffer_water_mark;
+ SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(this->_vc);
Review Comment:
I think I finally understand what you're getting at here. Take another look, please.
I'm sorry for being slow here. I thought it was weird there was no other casts to SSLNetVC.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#discussion_r869342728
##########
iocore/net/P_SNIActionPerformer.h:
##########
@@ -98,6 +98,30 @@ class ControlH2 : public ActionItem
bool enable_h2 = false;
};
+class HTTP2BufferWaterMark : public ActionItem
+{
+public:
+ HTTP2BufferWaterMark(int value) : value(value) {}
+ ~HTTP2BufferWaterMark() override {}
+
+ int
+ SNIAction(TLSSNISupport *snis, const Context &ctx) const override
+ {
+ if (!value) {
Review Comment:
For non-bools, I suggest explicit compares:
```
if (value == 0) {
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on a diff in pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
randall commented on code in PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#discussion_r869642293
##########
proxy/http2/Http2ClientSession.cc:
##########
@@ -116,9 +116,16 @@ Http2ClientSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB
this->_read_buffer_reader = reader ? reader : this->read_buffer->alloc_reader();
// This block size is the buffer size that we pass to SSLWriteBuffer
- auto buffer_block_size_index = iobuffer_size_to_index(Http2::write_buffer_block_size, MAX_BUFFER_SIZE_INDEX);
- this->write_buffer = new_MIOBuffer(buffer_block_size_index);
- this->write_buffer->water_mark = Http2::buffer_water_mark;
+ auto buffer_block_size_index = iobuffer_size_to_index(Http2::write_buffer_block_size, MAX_BUFFER_SIZE_INDEX);
+ this->write_buffer = new_MIOBuffer(buffer_block_size_index);
+
+ uint32_t buffer_water_mark;
+ if (this->_vc->options.http2_buffer_water_mark != 0) {
Review Comment:
That's a good callout. I might've posted this PR in mid transition from the PR this required.
The desire is to only apply the change if the override(via `sni.yaml`)has been explicitly been set. Or if it's not explicitly set , to use the default from `records.config`.
Maybe I should use `std::optional` here as well instead of trying to track/remember sentinel values.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#discussion_r869565055
##########
proxy/http2/Http2ClientSession.cc:
##########
@@ -116,9 +116,16 @@ Http2ClientSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOB
this->_read_buffer_reader = reader ? reader : this->read_buffer->alloc_reader();
// This block size is the buffer size that we pass to SSLWriteBuffer
- auto buffer_block_size_index = iobuffer_size_to_index(Http2::write_buffer_block_size, MAX_BUFFER_SIZE_INDEX);
- this->write_buffer = new_MIOBuffer(buffer_block_size_index);
- this->write_buffer->water_mark = Http2::buffer_water_mark;
+ auto buffer_block_size_index = iobuffer_size_to_index(Http2::write_buffer_block_size, MAX_BUFFER_SIZE_INDEX);
+ this->write_buffer = new_MIOBuffer(buffer_block_size_index);
+
+ uint32_t buffer_water_mark;
+ if (this->_vc->options.http2_buffer_water_mark != 0) {
Review Comment:
Can you help me think through this: is this OK if the value is the default of `-1`? Or is `_vc->options.http2_buffer_water_mark` guaranteed to not be `-1` here? I might be missing something...
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#discussion_r866459276
##########
iocore/net/I_NetVConnection.h:
##########
@@ -187,6 +187,8 @@ struct NetVCOptions {
uint32_t packet_tos;
uint32_t packet_notsent_lowat;
+ uint32_t http2_buffer_water_mark;
Review Comment:
I'm ok with exposing a setting tunable via sni.yaml and I'm not going to block this PR because we can change the internal implementation later without breaking compatibility, but I don't like to have this option here because it's for a specific application protocol (not for generic connections).
I'd put it like:
```
class TLSSNISupport {
...
struct HintsFromSNI { // NEW
uint32_t http2_buffer_water_mark;
} hints_from_sni;
};
```
It'd still have an application specific setting in one of TLS stuff, but it's better than contaminating NetVCOptions, IMO. And it also suggests the value is just a hint, and you can ignore it if appropriate (remember connection coalescing?).
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org