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/07/14 20:58:12 UTC
[GitHub] [trafficserver] bneradt opened a new pull request, #8963: Http2 to origin
bneradt opened a new pull request, #8963:
URL: https://github.com/apache/trafficserver/pull/8963
This implements the HTTP/2 to origin feature.
---
This is a rebase on top of 10-Dev of the original HTTP/2 to origin branch from @shinrich: #8447. I'm currently marking it as a draft because the background fill feature has to be fixed. @duke8253 and I will be working on that. I suspect that the tunnel needs a call to reenable somewhere in those circumstances.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r941692658
##########
proxy/http/HttpSM.cc:
##########
@@ -974,19 +973,34 @@ HttpSM::state_watch_for_client_abort(int event, void *data)
*/
case VC_EVENT_EOS: {
// We got an early EOS. If the tunnal has cache writer, don't kill it for background fill.
- NetVConnection *netvc = ua_txn->get_netvc();
- if (ua_txn->allow_half_open() || tunnel.has_consumer_besides_client()) {
- if (netvc) {
- netvc->do_io_shutdown(IO_SHUTDOWN_READ);
+ if (!terminate_sm) { // Not done already
+ NetVConnection *netvc = ua_txn->get_netvc();
+ /** TODO: we want to put tunnel.has_consumer_besides_client back in here.
+ *
+ * Currently, our HTTP/2 to origin branch does not work with background
+ * fill. Trying to keep the tunnel around to fill the cache after the
+ * client aborts currently results in a connection leak. This has to be
+ * fixed, but for now we're avoiding this relatively new feature (Spring
+ * 2021) to stop the leak.
+ *
+ * Uncomment this code to reinstate background fill once it is fixed.
Review Comment:
@duke8253 is currently working on fixing this. Our hope/intention is to fix this as an additional patch on this PR before merging.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921825055
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1545,10 +1739,14 @@ Http2ConnectionState::release_stream()
// Can't do this because we just destroyed right here ^,
// or we can use a local variable to do it.
// session = nullptr;
+ } else if (shutdown_state == HTTP2_SHUTDOWN_IN_PROGRESS && fini_event == nullptr) {
Review Comment:
This is concerning because it reverts a fix.
See https://github.com/apache/trafficserver/pull/7267
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921855305
##########
proxy/http/HttpProxyServerMain.cc:
##########
@@ -50,6 +50,9 @@
HttpSessionAccept *plugin_http_accept = nullptr;
HttpSessionAccept *plugin_http_transparent_accept = nullptr;
+extern std::function<PoolableSession *()> Create_h1_server_session;
Review Comment:
Why don't you make the first lowercase? These are not types but variables.
##########
proxy/http/HttpProxyServerMain.cc:
##########
@@ -50,6 +50,9 @@
HttpSessionAccept *plugin_http_accept = nullptr;
HttpSessionAccept *plugin_http_transparent_accept = nullptr;
+extern std::function<PoolableSession *()> Create_h1_server_session;
Review Comment:
Why don't you make the first letters lowercase? These are not types but variables.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921865432
##########
proxy/http/HttpSM.cc:
##########
@@ -3405,6 +3657,66 @@ HttpSM::tunnel_handler_ua(int event, HttpTunnelConsumer *c)
return 0;
}
+int
+HttpSM::tunnel_handler_trailer_ua(int event, HttpTunnelConsumer *c)
Review Comment:
Is it possible to add support for trailer before or after H2 to Origin separately? I guess it was added for gRPC, but I assume it'll be used for H1 as well. And if we support trailer header, that should be explicitly stated on release note.
--
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] shinrich commented on a diff in pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
shinrich commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r961783915
##########
proxy/http2/Http2ServerSession.cc:
##########
@@ -0,0 +1,420 @@
+/** @file
+
+ Http2ServerSession.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "Http2ServerSession.h"
+#include "HttpDebugNames.h"
+#include "tscore/ink_base64.h"
+#include "Http2CommonSessionInternal.h"
+#include "HttpSessionManager.h"
+#include "P_SSLNetVConnection.h"
+
+ClassAllocator<Http2ServerSession> http2ServerSessionAllocator("http2ServerSessionAllocator");
+
+static int
+send_connection_event(Continuation *cont, int event, void *edata)
+{
+ SCOPED_MUTEX_LOCK(lock, cont->mutex, this_ethread());
+ return cont->handleEvent(event, edata);
+}
+
+Http2ServerSession::Http2ServerSession() = default;
+
+void
+Http2ServerSession::destroy()
+{
+ if (!in_destroy) {
+ in_destroy = true;
+ write_vio = nullptr;
+ this->remove_session();
+ this->release_outbound_connection_tracking();
+ REMEMBER(NO_EVENT, this->recursion)
+ Http2SsnDebug("session destroy");
+ if (_vc) {
+ _vc->do_io_close();
+ _vc = nullptr;
+ }
+ free();
+ }
+}
+
+void
+Http2ServerSession::free()
+{
+ auto mutex_thread = this->mutex->thread_holding;
+ if (Http2CommonSession::common_free(this)) {
+ HTTP2_DECREMENT_THREAD_DYN_STAT(HTTP2_STAT_CURRENT_SERVER_SESSION_COUNT, mutex_thread);
+ THREAD_FREE(this, http2ServerSessionAllocator, mutex_thread);
+ }
+}
+
+void
+Http2ServerSession::start()
+{
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
+
+ SET_HANDLER(&Http2ServerSession::main_event_handler);
+ HTTP2_SET_SESSION_HANDLER(&Http2ServerSession::state_start_frame_read);
+
+ VIO *read_vio = this->do_io_read(this, INT64_MAX, this->read_buffer);
+ write_vio = this->do_io_write(this, INT64_MAX, this->_write_buffer_reader);
+
+ this->connection_state.init(this);
+
+ // 3.5 HTTP/2 Connection Preface. Upon establishment of a TCP connection and
+ // determination that HTTP/2 will be used by both peers, each endpoint MUST
+ // send a connection preface as a final confirmation ...
+ // This is the preface string sent by the client
+ this->write_buffer->write(HTTP2_CONNECTION_PREFACE, HTTP2_CONNECTION_PREFACE_LEN);
+ total_write_len += HTTP2_CONNECTION_PREFACE_LEN;
+ // write_vio->nbytes = total_write_len;
+ write_reenable();
+ this->connection_state.send_connection_preface();
+ Http2SsnDebug("Sent Connection Preface");
+
+ this->handleEvent(VC_EVENT_READ_READY, read_vio);
+}
+
+void
+Http2ServerSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOBufferReader *reader)
+{
+ ink_assert(new_vc->mutex->thread_holding == this_ethread());
+ HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_CURRENT_SERVER_SESSION_COUNT, new_vc->mutex->thread_holding);
+ HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_TOTAL_SERVER_CONNECTION_COUNT, new_vc->mutex->thread_holding);
+ this->_milestones.mark(Http2SsnMilestone::OPEN);
+
+ // Unique client session identifier.
+ this->con_id = ProxySession::next_connection_id();
+ this->_vc = new_vc;
+ _vc->set_inactivity_timeout(HRTIME_SECONDS(Http2::accept_no_activity_timeout));
+ this->schedule_event = nullptr;
+ this->mutex = new_vc->mutex;
+
+ this->connection_state.mutex = this->mutex;
+
+ TLSEarlyDataSupport *eds = dynamic_cast<TLSEarlyDataSupport *>(new_vc);
+ if (eds != nullptr) {
+ this->read_from_early_data = eds->get_early_data_len();
+ Debug("ssl_early_data", "read_from_early_data = %" PRId64, this->read_from_early_data);
+ }
+
+ Http2SsnDebug("session born, netvc %p", this->_vc);
+
+ this->_vc->set_tcp_congestion_control(CLIENT_SIDE);
+
+ this->read_buffer = iobuf ? iobuf : new_MIOBuffer(HTTP2_HEADER_BUFFER_SIZE_INDEX);
+ this->read_buffer->water_mark = connection_state.local_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE);
+ this->_read_buffer_reader = reader ? reader : this->read_buffer->alloc_reader();
+
+ // Set write buffer size to max size of TLS record (16KB)
+ // 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_reader = this->write_buffer->alloc_reader();
+ this->_write_size_threshold = index_to_buffer_size(buffer_block_size_index) * Http2::write_size_threshold;
+
+ this->_handle_if_ssl(new_vc);
+
+ do_api_callout(TS_HTTP_SSN_START_HOOK);
+
+ this->add_session();
+}
+
+// implement that. After we send a GOAWAY, there
+// are scenarios where we would like to complete the outstanding streams.
+
+void
+Http2ServerSession::do_io_close(int alerrno)
+{
+ REMEMBER(NO_EVENT, this->recursion)
+
+ if (!this->connection_state.is_state_closed()) {
+ Http2SsnDebug("session closed");
+ this->remove_session();
+
+ ink_assert(this->mutex->thread_holding == this_ethread());
+ send_connection_event(&this->connection_state, HTTP2_SESSION_EVENT_FINI, this);
+
+ // Destroy will be called from connection_state.release_stream() once the number of active streams goes to 0
+ }
+}
+
+int
+Http2ServerSession::main_event_handler(int event, void *edata)
+{
+ ink_assert(this->mutex->thread_holding == this_ethread());
+ int retval;
+
+ recursion++;
+
+ Event *e = static_cast<Event *>(edata);
+ if (e == schedule_event) {
+ schedule_event = nullptr;
+ }
+
+ Http2SsnDebug("main_event_handler=%d edata=%p", event, edata);
+
+ switch (event) {
+ case VC_EVENT_READ_COMPLETE:
+ case VC_EVENT_READ_READY: {
+ bool is_zombie = connection_state.get_zombie_event() != nullptr;
+ retval = (this->*session_handler)(event, edata);
+ if (is_zombie && connection_state.get_zombie_event() != nullptr) {
+ Warning("Processed read event for zombie session %" PRId64, connection_id());
+ }
+ break;
+ }
+
+ case HTTP2_SESSION_EVENT_REENABLE:
+ // VIO will be reenableed in this handler
+ retval = (this->*session_handler)(VC_EVENT_READ_READY, static_cast<VIO *>(e->cookie));
+ // Clear the event after calling session_handler to not reschedule REENABLE in it
+ this->_reenable_event = nullptr;
+ break;
+
+ case VC_EVENT_ACTIVE_TIMEOUT:
+ case VC_EVENT_INACTIVITY_TIMEOUT:
+ case VC_EVENT_ERROR:
+ case VC_EVENT_EOS:
+ this->set_dying_event(event);
+ this->do_io_close();
+ retval = 0;
+ break;
+
+ case VC_EVENT_WRITE_READY:
+ case VC_EVENT_WRITE_COMPLETE:
+ this->connection_state.restart_streams();
+ if ((Thread::get_hrtime() >= this->_write_buffer_last_flush + HRTIME_MSECONDS(this->_write_time_threshold))) {
+ this->flush();
+ }
+
+ retval = 0;
+ break;
+
+ case HTTP2_SESSION_EVENT_XMIT:
+ default:
+ Http2SsnDebug("unexpected event=%d edata=%p", event, edata);
+ ink_release_assert(0);
+ retval = 0;
+ break;
+ }
+
+ if (!this->is_draining() && this->connection_state.get_shutdown_reason() == Http2ErrorCode::HTTP2_ERROR_MAX) {
+ this->connection_state.set_shutdown_state(HTTP2_SHUTDOWN_NONE);
+ }
+
+ if (this->connection_state.get_shutdown_state() == HTTP2_SHUTDOWN_NONE) {
+ if (this->is_draining()) { // For a case we already checked Connection header and it didn't exist
+ Http2SsnDebug("Preparing for graceful shutdown because of draining state");
+ this->connection_state.set_shutdown_state(HTTP2_SHUTDOWN_NOT_INITIATED);
+ } /*else if (this->connection_state.get_stream_error_rate() >
Review Comment:
Could add this back in. As I dimly recall in my testing, the shutdown due to spurious errors seemed high. But could have been due to other reasons.
--
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] shinrich commented on a diff in pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
shinrich commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r961773926
##########
proxy/http/HttpSM.cc:
##########
@@ -3405,6 +3657,66 @@ HttpSM::tunnel_handler_ua(int event, HttpTunnelConsumer *c)
return 0;
}
+int
+HttpSM::tunnel_handler_trailer_ua(int event, HttpTunnelConsumer *c)
Review Comment:
Could separate it out. What is present now has not been tested for a while (AFAIK). I did very basic tests early on with gRPC.
The goal for the support is gRPC which is only needed for HTTP/2. We will need that support very soon after the base H2 to origin is committed.
I agree that it would be nice to support for H1 as well, but do we have a use case for trailers with H1?
--
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 pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1328134685
The `http2_to_origin` AuTest will fail until the new Proxy Verifier `await` directive is merged in:
https://github.com/yahoo/proxy-verifier/pull/228
--
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] ywkaras commented on a diff in pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
ywkaras commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r937242092
##########
proxy/http/HttpTransact.h:
##########
@@ -571,12 +573,64 @@ class HttpTransact
ConnectionAttributes *server = nullptr;
ink_time_t now = 0;
ServerState_t state = STATE_UNDEFINED;
- unsigned attempts = 0;
- unsigned simple_retry_attempts = 0;
- unsigned unavailable_server_retry_attempts = 0;
- ParentRetry_t retry_type = PARENT_RETRY_NONE;
+ class Attempts
Review Comment:
This change is already in Yahoo TS. I think this somehow just got picked up.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1329593036
@bneradt Is it possible to separate out the change for ConnectionPool and ConnectingEntry?
Since this PR is for H2 to Origin, the code on this PR should not do anything bad unless H2 to Origin is enabled by having `h2` in `proxy.config.ssl.client.alpn_protocols`. I understand nobody can guarantee that, but ConnectionPool and ConnectingEntry are new and those may affect H1 because the code is used for the both protocols.
As you can see, people who are unfamiliar with H2 haven't been reviewing this PR at all, but the change can bother them. I think you should gather feedback from them as well. Otherwise, we would be blamed like "H2 to Origin made this mess where we don't need it!".
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r954014759
##########
proxy/http/HttpSM.cc:
##########
@@ -974,19 +973,34 @@ HttpSM::state_watch_for_client_abort(int event, void *data)
*/
case VC_EVENT_EOS: {
// We got an early EOS. If the tunnal has cache writer, don't kill it for background fill.
- NetVConnection *netvc = ua_txn->get_netvc();
- if (ua_txn->allow_half_open() || tunnel.has_consumer_besides_client()) {
- if (netvc) {
- netvc->do_io_shutdown(IO_SHUTDOWN_READ);
+ if (!terminate_sm) { // Not done already
+ NetVConnection *netvc = ua_txn->get_netvc();
+ /** TODO: we want to put tunnel.has_consumer_besides_client back in here.
+ *
+ * Currently, our HTTP/2 to origin branch does not work with background
+ * fill. Trying to keep the tunnel around to fill the cache after the
+ * client aborts currently results in a connection leak. This has to be
+ * fixed, but for now we're avoiding this relatively new feature (Spring
+ * 2021) to stop the leak.
+ *
+ * Uncomment this code to reinstate background fill once it is fixed.
Review Comment:
@duke8253 provided a fix for background fill which I have now incorporated into 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: 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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r941034088
##########
proxy/http2/Http2Stream.cc:
##########
@@ -435,23 +477,22 @@ Http2Stream::do_io_close(int /* flags */)
REMEMBER(NO_EVENT, this->reentrancy_count);
Http2StreamDebug("do_io_close");
+ // if (this->is_state_writeable()) { // Let the other end know we are going away
+ // this->get_connection_state().send_rst_stream_frame(_id, Http2ErrorCode::HTTP2_ERROR_NO_ERROR);
+ //}
+
// When we get here, the SM has initiated the shutdown. Either it received a WRITE_COMPLETE, or it is shutting down. Any
// remaining IO operations back to client should be abandoned. The SM-side buffers backing these operations will be deleted
// by the time this is called from transaction_done.
closed = true;
- if (_proxy_ssn && this->is_client_state_writeable()) {
- // Make sure any trailing end of stream frames are sent
- // We will be removed at send_data_frames or closing connection phase
- Http2ClientSession *h2_proxy_ssn = static_cast<Http2ClientSession *>(this->_proxy_ssn);
- SCOPED_MUTEX_LOCK(lock, h2_proxy_ssn->mutex, this_ethread());
- h2_proxy_ssn->connection_state.send_data_frames(this);
- }
+ // Adjust state, so we don't process any more data
+ _state = Http2StreamState::HTTP2_STREAM_STATE_CLOSED;
Review Comment:
Not a blocker since there is already one place that does similar thing, but I think we should not change `_state` like this without actual activity on H2 protocol, because we won't be able to know the last state of a stream, which should be CLOSED state on a clean close even without this line.
--
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] shinrich commented on a diff in pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
shinrich commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r961721593
##########
proxy/hdrs/HdrToken.cc:
##########
@@ -230,7 +230,10 @@ static HdrTokenFieldInfo _hdrtoken_strs_field_initializers[] = {
{"Strict-Transport-Security", MIME_SLOTID_NONE, MIME_PRESENCE_NONE, (HTIF_MULTVALS)},
{"Subject", MIME_SLOTID_NONE, MIME_PRESENCE_SUBJECT, HTIF_NONE},
{"Summary", MIME_SLOTID_NONE, MIME_PRESENCE_SUMMARY, HTIF_NONE},
- {"TE", MIME_SLOTID_TE, MIME_PRESENCE_TE, (HTIF_COMMAS | HTIF_MULTVALS | HTIF_HOPBYHOP)},
+ // Need to figure out why this cannot be handled as hop by hop. If it is hop-by-hop
+ // the information is not propagated for gRPC
+ //{"TE", MIME_SLOTID_TE, MIME_PRESENCE_TE, (HTIF_COMMAS | HTIF_MULTVALS | HTIF_HOPBYHOP)},
+ {"TE", MIME_SLOTID_TE, MIME_PRESENCE_TE, (HTIF_COMMAS | HTIF_MULTVALS)},
Review Comment:
True. I'd leave the comment, so we have it when debugging gRPC. But leave the code along. There must be another way to deal with the gRPC case. Or perhaps the other changes made for #7473 fixed the gRPC case as well.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r967488274
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1756,16 +1980,26 @@ Http2ConnectionState::send_headers_frame(Http2Stream *stream)
Http2StreamDebug(session, stream->get_id(), "Send HEADERS frame");
- HTTPHdr *resp_hdr = &stream->response_header;
- http2_convert_header_from_1_1_to_2(resp_hdr);
+ // For outbound streams, set the ID if it has not yet already been set
+ // Need to defer setting the stream ID to avoid another later created stream
+ // sending out first. This may cause the peer to issue a stream or connection
+ // error (new stream less that the greatest we have seen so far)
+ this->set_stream_id(stream);
+
+ HTTPHdr *send_hdr = stream->get_send_header();
+ if (stream->expect_send_trailer()) {
+ // Which is a no-op conversion
+ } else {
+ http2_convert_header_from_1_1_to_2(send_hdr);
+ }
- uint32_t buf_len = resp_hdr->length_get() * 2; // Make it double just in case
+ uint32_t buf_len = send_hdr->length_get() * 2; // Make it double just in case
Review Comment:
Sounds good: #9084.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921850376
##########
proxy/hdrs/HdrToken.cc:
##########
@@ -230,7 +230,10 @@ static HdrTokenFieldInfo _hdrtoken_strs_field_initializers[] = {
{"Strict-Transport-Security", MIME_SLOTID_NONE, MIME_PRESENCE_NONE, (HTIF_MULTVALS)},
{"Subject", MIME_SLOTID_NONE, MIME_PRESENCE_SUBJECT, HTIF_NONE},
{"Summary", MIME_SLOTID_NONE, MIME_PRESENCE_SUMMARY, HTIF_NONE},
- {"TE", MIME_SLOTID_TE, MIME_PRESENCE_TE, (HTIF_COMMAS | HTIF_MULTVALS | HTIF_HOPBYHOP)},
+ // Need to figure out why this cannot be handled as hop by hop. If it is hop-by-hop
+ // the information is not propagated for gRPC
+ //{"TE", MIME_SLOTID_TE, MIME_PRESENCE_TE, (HTIF_COMMAS | HTIF_MULTVALS | HTIF_HOPBYHOP)},
+ {"TE", MIME_SLOTID_TE, MIME_PRESENCE_TE, (HTIF_COMMAS | HTIF_MULTVALS)},
Review Comment:
This partially(?) reverts #7473, and it doesn't seem like a right direction. The question should be figured out before 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] maskit commented on a diff in pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921925481
##########
proxy/http2/Http2ServerSession.cc:
##########
@@ -0,0 +1,420 @@
+/** @file
+
+ Http2ServerSession.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "Http2ServerSession.h"
+#include "HttpDebugNames.h"
+#include "tscore/ink_base64.h"
+#include "Http2CommonSessionInternal.h"
+#include "HttpSessionManager.h"
+#include "P_SSLNetVConnection.h"
+
+ClassAllocator<Http2ServerSession> http2ServerSessionAllocator("http2ServerSessionAllocator");
+
+static int
+send_connection_event(Continuation *cont, int event, void *edata)
+{
+ SCOPED_MUTEX_LOCK(lock, cont->mutex, this_ethread());
+ return cont->handleEvent(event, edata);
+}
+
+Http2ServerSession::Http2ServerSession() = default;
+
+void
+Http2ServerSession::destroy()
+{
+ if (!in_destroy) {
+ in_destroy = true;
+ write_vio = nullptr;
+ this->remove_session();
+ this->release_outbound_connection_tracking();
+ REMEMBER(NO_EVENT, this->recursion)
+ Http2SsnDebug("session destroy");
+ if (_vc) {
+ _vc->do_io_close();
+ _vc = nullptr;
+ }
+ free();
+ }
+}
+
+void
+Http2ServerSession::free()
+{
+ auto mutex_thread = this->mutex->thread_holding;
+ if (Http2CommonSession::common_free(this)) {
+ HTTP2_DECREMENT_THREAD_DYN_STAT(HTTP2_STAT_CURRENT_SERVER_SESSION_COUNT, mutex_thread);
+ THREAD_FREE(this, http2ServerSessionAllocator, mutex_thread);
+ }
+}
+
+void
+Http2ServerSession::start()
+{
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
+
+ SET_HANDLER(&Http2ServerSession::main_event_handler);
+ HTTP2_SET_SESSION_HANDLER(&Http2ServerSession::state_start_frame_read);
+
+ VIO *read_vio = this->do_io_read(this, INT64_MAX, this->read_buffer);
+ write_vio = this->do_io_write(this, INT64_MAX, this->_write_buffer_reader);
+
+ this->connection_state.init(this);
+
+ // 3.5 HTTP/2 Connection Preface. Upon establishment of a TCP connection and
+ // determination that HTTP/2 will be used by both peers, each endpoint MUST
+ // send a connection preface as a final confirmation ...
+ // This is the preface string sent by the client
+ this->write_buffer->write(HTTP2_CONNECTION_PREFACE, HTTP2_CONNECTION_PREFACE_LEN);
+ total_write_len += HTTP2_CONNECTION_PREFACE_LEN;
+ // write_vio->nbytes = total_write_len;
+ write_reenable();
+ this->connection_state.send_connection_preface();
+ Http2SsnDebug("Sent Connection Preface");
+
+ this->handleEvent(VC_EVENT_READ_READY, read_vio);
+}
+
+void
+Http2ServerSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOBufferReader *reader)
+{
+ ink_assert(new_vc->mutex->thread_holding == this_ethread());
+ HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_CURRENT_SERVER_SESSION_COUNT, new_vc->mutex->thread_holding);
+ HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_TOTAL_SERVER_CONNECTION_COUNT, new_vc->mutex->thread_holding);
+ this->_milestones.mark(Http2SsnMilestone::OPEN);
+
+ // Unique client session identifier.
+ this->con_id = ProxySession::next_connection_id();
+ this->_vc = new_vc;
+ _vc->set_inactivity_timeout(HRTIME_SECONDS(Http2::accept_no_activity_timeout));
+ this->schedule_event = nullptr;
+ this->mutex = new_vc->mutex;
+
+ this->connection_state.mutex = this->mutex;
+
+ TLSEarlyDataSupport *eds = dynamic_cast<TLSEarlyDataSupport *>(new_vc);
+ if (eds != nullptr) {
+ this->read_from_early_data = eds->get_early_data_len();
Review Comment:
A client never receives early data. We don't need this block.
--
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 pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1320767226
[approve ci autest]
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921843701
##########
proxy/logging/LogAccess.cc:
##########
@@ -1412,7 +1412,7 @@ LogAccess::marshal_client_provided_cert(char *buf)
{
int provided_cert = 0;
if (m_http_sm) {
- auto txn = m_http_sm->get_ua_txn();
+ auto txn = m_http_sm->ua_txn;
Review Comment:
I don't think this is an intentional change, but let's use the getter.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921905654
##########
proxy/ProxySession.cc:
##########
@@ -313,3 +315,21 @@ ProxySession::support_sni() const
{
return _vc ? _vc->support_sni() : false;
}
+
+PoolableSession *
+ProxySession::protocol_creation(NetVConnection *netvc)
+{
+ // Figure out what protocol was negotiated
+ int proto_index = SessionProtocolNameRegistry::INVALID;
+ SSLNetVConnection *sslnetvc = dynamic_cast<SSLNetVConnection *>(netvc);
Review Comment:
Actually, I don't see any reason to receive a NetVC here. Just a protocol id is enough. Let's deal with NetVC on HttpSM side.
How about this? I want function names to have verbs.
```
PoolableSession *
ProxySession::create_outbound_sesion(int protocol_id) {
auto iter = ProtocolSessionCreateMap.find(protocol_id);
ink_release_assert(iter != ProtocolSessionCreateMap.end());
return iter->second();
}
```
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921953327
##########
proxy/http/HttpTunnel.cc:
##########
@@ -984,11 +986,14 @@ HttpTunnel::producer_run(HttpTunnelProducer *p)
p->handler_state = HTTP_SM_POST_SUCCESS;
}
}
+ Debug("http_tunnel", "Start write vio %ld bytes", c_write);
// Start the writes now that we know we will consume all the initial data
c->write_vio = c->vc->do_io_write(this, c_write, c->buffer_reader);
ink_assert(c_write > 0);
if (c->write_vio == nullptr) {
consumer_handler(VC_EVENT_ERROR, c);
+ } else if (c->write_vio->ntodo() == 0 && c->alive) {
Review Comment:
Not just this line, but I don't get why these changes to HttpTunnel is needed for H2 to Origin. I'm wondering (and concerning) if these affect H1 and client transactions.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r953266505
##########
proxy/http2/Http2Stream.h:
##########
@@ -164,17 +196,22 @@ class Http2Stream : public ProxyTransaction
Http2StreamState _state = Http2StreamState::HTTP2_STREAM_STATE_IDLE;
int64_t _http_sm_id = -1;
- HTTPHdr _req_header;
- MIOBuffer _request_buffer = CLIENT_CONNECTION_FIRST_READ_BUFFER_SIZE_INDEX;
- int64_t read_vio_nbytes;
+ HTTPHdr _recv_header;
+ MIOBuffer _recv_buffer = CLIENT_CONNECTION_FIRST_READ_BUFFER_SIZE_INDEX;
VIO read_vio;
VIO write_vio;
History<HISTORY_DEFAULT_SIZE> _history;
Milestones<Http2StreamMilestone, static_cast<size_t>(Http2StreamMilestone::LAST_ENTRY)> _milestones;
- bool trailing_header = false;
- bool has_body = false;
+ bool _possible_trailing_header = false;
+ bool _expect_send_trailer = false;
+ bool _expect_receive_trailer = false;
+
+ bool has_body = false;
+
+ bool _outbound_flag = false;
Review Comment:
Yes, good observation. Thank you. I updated the function to query the session whether it is outbound or not.
--
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 pull request #8963: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1405497777
> This is an approval for changes around H2.
>
> I'm not sure about connection management part. I still think we should make the change separately, but if that's not an option, please wait for another approval from someone else.
Thank you @maskit . Again, I appreciate all the feedback you've provided on this feature.
@keesspoelstra is carefully reviewing and testing this in the meantime too. I'll wait upon at least his review.
--
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 pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1332830704
> As you can see, people who are unfamiliar with H2 haven't been reviewing this PR at all, but the change can bother them. I think you should gather feedback from them as well. Otherwise, we would be blamed like "H2 to Origin made this mess where we don't need it!".
Just to follow up on this, after mentioning this suggestion in the PR/issue call this Monday, it was suggested that @keesspoelstra would be a good candidate to review this PR as well. He mentioned that he already has been following it and would spend some time looking at that patch.
Thank you again, @maskit , for the time you have spent reviewing 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] bneradt commented on a diff in pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r951930573
##########
proxy/http/HttpTransact.h:
##########
@@ -571,12 +573,64 @@ class HttpTransact
ConnectionAttributes *server = nullptr;
ink_time_t now = 0;
ServerState_t state = STATE_UNDEFINED;
- unsigned attempts = 0;
- unsigned simple_retry_attempts = 0;
- unsigned unavailable_server_retry_attempts = 0;
- ParentRetry_t retry_type = PARENT_RETRY_NONE;
+ class Attempts
Review Comment:
Just to follow up on this: I just rebased this branch on top of latest 10-Dev. 10-Dev now has Walt's `Attempts` work on it, so that is no longer in 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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1329840562
> The `http2_to_origin` AuTest will fail until the new Proxy Verifier `await` directive is merged in:
> [yahoo/proxy-verifier#228](https://github.com/yahoo/proxy-verifier/pull/228)
https://github.com/apache/trafficserver/pull/9221
--
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 pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1330857181
[approve ci fedora]
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r937219432
##########
proxy/http/HttpTransact.h:
##########
@@ -571,12 +573,64 @@ class HttpTransact
ConnectionAttributes *server = nullptr;
ink_time_t now = 0;
ServerState_t state = STATE_UNDEFINED;
- unsigned attempts = 0;
- unsigned simple_retry_attempts = 0;
- unsigned unavailable_server_retry_attempts = 0;
- ParentRetry_t retry_type = PARENT_RETRY_NONE;
+ class Attempts
Review Comment:
I just realized this change is similar to #8776. I guess merging it helps minimizing this PR? @ywkaras
--
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] shinrich commented on a diff in pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
shinrich commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r961780232
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1545,10 +1739,14 @@ Http2ConnectionState::release_stream()
// Can't do this because we just destroyed right here ^,
// or we can use a local variable to do it.
// session = nullptr;
+ } else if (shutdown_state == HTTP2_SHUTDOWN_IN_PROGRESS && fini_event == nullptr) {
+ session->do_clear_session_active();
+ fini_event = this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_FINI);
} else if (session->get_proxy_session()->is_active()) {
// If the number of clients is 0, HTTP2_SESSION_EVENT_FINI is not received or sent, and session is active,
// then mark the connection as inactive
session->do_clear_session_active();
+ session->set_no_activity_timeout();
Review Comment:
I recall running into idle H2 sessions timing out. Definitely for outbound connections. I don't recall if I saw it for inbound or not. I assume this would be an issue for incoming H2 sessions as well. But probably more apparent in the case of origin sessions sitting in a reuse pool.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921892219
##########
proxy/ProxySession.cc:
##########
@@ -313,3 +315,21 @@ ProxySession::support_sni() const
{
return _vc ? _vc->support_sni() : false;
}
+
+PoolableSession *
+ProxySession::protocol_creation(NetVConnection *netvc)
+{
+ // Figure out what protocol was negotiated
+ int proto_index = SessionProtocolNameRegistry::INVALID;
+ SSLNetVConnection *sslnetvc = dynamic_cast<SSLNetVConnection *>(netvc);
Review Comment:
Let's not depend on SSLNetVC. `ALPNSupport` should be enough here.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921837459
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1545,10 +1739,14 @@ Http2ConnectionState::release_stream()
// Can't do this because we just destroyed right here ^,
// or we can use a local variable to do it.
// session = nullptr;
+ } else if (shutdown_state == HTTP2_SHUTDOWN_IN_PROGRESS && fini_event == nullptr) {
+ session->do_clear_session_active();
+ fini_event = this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_FINI);
} else if (session->get_proxy_session()->is_active()) {
// If the number of clients is 0, HTTP2_SESSION_EVENT_FINI is not received or sent, and session is active,
// then mark the connection as inactive
session->do_clear_session_active();
+ session->set_no_activity_timeout();
Review Comment:
This should be safe, but I'm wondering if inactivity timeout currently works for incoming H2 sessions... Even if it works, the timeout might be based on another setting.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921805249
##########
proxy/http2/Http2Stream.h:
##########
@@ -164,17 +196,22 @@ class Http2Stream : public ProxyTransaction
Http2StreamState _state = Http2StreamState::HTTP2_STREAM_STATE_IDLE;
int64_t _http_sm_id = -1;
- HTTPHdr _req_header;
- MIOBuffer _request_buffer = CLIENT_CONNECTION_FIRST_READ_BUFFER_SIZE_INDEX;
- int64_t read_vio_nbytes;
+ HTTPHdr _recv_header;
+ MIOBuffer _recv_buffer = CLIENT_CONNECTION_FIRST_READ_BUFFER_SIZE_INDEX;
VIO read_vio;
VIO write_vio;
History<HISTORY_DEFAULT_SIZE> _history;
Milestones<Http2StreamMilestone, static_cast<size_t>(Http2StreamMilestone::LAST_ENTRY)> _milestones;
- bool trailing_header = false;
- bool has_body = false;
+ bool _possible_trailing_header = false;
+ bool _expect_send_trailer = false;
+ bool _expect_receive_trailer = false;
+
+ bool has_body = false;
+
+ bool _outbound_flag = false;
Review Comment:
This name is confusing a little, because a stream can be initiated by a server on an inbound connection (e.g. Server Push). Since a stream (ProxyTransaction) knows its owner session, we should be able to ask the session whether this stream is on an inbound session or an outbound session.
I'd remove this variable, but please rename it accordingly if you want to keep it.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921938919
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1278,8 +1333,115 @@ Http2ConnectionState::state_closed(int event, void *edata)
return 0;
}
+bool
+Http2ConnectionState::is_peer_concurrent_stream_ub() const
+{
+ return client_streams_in_count >= (peer_settings.get(HTTP2_SETTINGS_MAX_CONCURRENT_STREAMS)) * 0.9;
+}
+
+bool
+Http2ConnectionState::is_peer_concurrent_stream_lb() const
+{
+ return client_streams_in_count <= (peer_settings.get(HTTP2_SETTINGS_MAX_CONCURRENT_STREAMS)) / 2;
+}
+
+void
+Http2ConnectionState::set_stream_id(Http2Stream *stream)
+{
+ if (stream->get_transaction_id() < 0) {
+ Http2StreamId stream_id = (latest_streamid_in == 0) ? 3 : latest_streamid_in + 2;
+ stream->set_transaction_id(stream_id);
+ latest_streamid_in = stream_id;
+ }
+}
+
+Http2Stream *
+Http2ConnectionState::create_initiating_stream(bool client_streamid, Http2Error &error)
Review Comment:
It seems like `client_streamid` is always `true`, and `error` is not used on the caller side.
--
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 closed pull request #8963: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt closed pull request #8963: Http2 to origin
URL: https://github.com/apache/trafficserver/pull/8963
--
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 pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1312009739
> > If we don't support Server Push on origin side for now, we need to indicate it by sending SETTINGS_ENABLE_PUSH:0.
>
> Yes, this is a good observation. I'll work on this when #9085 is finished.
Updated to set `SETTINGS_ENABLE_PUSH` to 0. Good observation.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r954172607
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1545,10 +1739,14 @@ Http2ConnectionState::release_stream()
// Can't do this because we just destroyed right here ^,
// or we can use a local variable to do it.
// session = nullptr;
+ } else if (shutdown_state == HTTP2_SHUTDOWN_IN_PROGRESS && fini_event == nullptr) {
Review Comment:
Thanks. Good catch. The merge is pretty large and I suppose I accidentally pulled this in thinking that I wanted the additional code, not the removal of it.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921885168
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1756,16 +1980,26 @@ Http2ConnectionState::send_headers_frame(Http2Stream *stream)
Http2StreamDebug(session, stream->get_id(), "Send HEADERS frame");
- HTTPHdr *resp_hdr = &stream->response_header;
- http2_convert_header_from_1_1_to_2(resp_hdr);
+ // For outbound streams, set the ID if it has not yet already been set
+ // Need to defer setting the stream ID to avoid another later created stream
+ // sending out first. This may cause the peer to issue a stream or connection
+ // error (new stream less that the greatest we have seen so far)
+ this->set_stream_id(stream);
+
+ HTTPHdr *send_hdr = stream->get_send_header();
+ if (stream->expect_send_trailer()) {
+ // Which is a no-op conversion
+ } else {
+ http2_convert_header_from_1_1_to_2(send_hdr);
+ }
- uint32_t buf_len = resp_hdr->length_get() * 2; // Make it double just in case
+ uint32_t buf_len = send_hdr->length_get() * 2; // Make it double just in case
Review Comment:
Would make another PR for this kind of name changes (e.g. send_hdr, peer_settings, local_settings, etc.)? That would reduce the volume of this PR (which is great for reviewers so they can focus on the logical changes for H2 to Origin), and would also ease maintaining this branch.
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1756,16 +1980,26 @@ Http2ConnectionState::send_headers_frame(Http2Stream *stream)
Http2StreamDebug(session, stream->get_id(), "Send HEADERS frame");
- HTTPHdr *resp_hdr = &stream->response_header;
- http2_convert_header_from_1_1_to_2(resp_hdr);
+ // For outbound streams, set the ID if it has not yet already been set
+ // Need to defer setting the stream ID to avoid another later created stream
+ // sending out first. This may cause the peer to issue a stream or connection
+ // error (new stream less that the greatest we have seen so far)
+ this->set_stream_id(stream);
+
+ HTTPHdr *send_hdr = stream->get_send_header();
+ if (stream->expect_send_trailer()) {
+ // Which is a no-op conversion
+ } else {
+ http2_convert_header_from_1_1_to_2(send_hdr);
+ }
- uint32_t buf_len = resp_hdr->length_get() * 2; // Make it double just in case
+ uint32_t buf_len = send_hdr->length_get() * 2; // Make it double just in case
Review Comment:
Would you make another PR for this kind of name changes (e.g. send_hdr, peer_settings, local_settings, etc.)? That would reduce the volume of this PR (which is great for reviewers so they can focus on the logical changes for H2 to Origin), and would also ease maintaining this branch.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921921688
##########
proxy/http2/Http2ServerSession.cc:
##########
@@ -0,0 +1,420 @@
+/** @file
+
+ Http2ServerSession.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "Http2ServerSession.h"
+#include "HttpDebugNames.h"
+#include "tscore/ink_base64.h"
+#include "Http2CommonSessionInternal.h"
+#include "HttpSessionManager.h"
+#include "P_SSLNetVConnection.h"
Review Comment:
But this session behaves as a client and `TLSEarlyDataSupport` only supports server side at the moment. We can simply get rid of it for 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] bneradt commented on a diff in pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r951935655
##########
proxy/http/HttpSM.cc:
##########
@@ -974,19 +973,34 @@ HttpSM::state_watch_for_client_abort(int event, void *data)
*/
case VC_EVENT_EOS: {
// We got an early EOS. If the tunnal has cache writer, don't kill it for background fill.
- NetVConnection *netvc = ua_txn->get_netvc();
- if (ua_txn->allow_half_open() || tunnel.has_consumer_besides_client()) {
- if (netvc) {
- netvc->do_io_shutdown(IO_SHUTDOWN_READ);
+ if (!terminate_sm) { // Not done already
+ NetVConnection *netvc = ua_txn->get_netvc();
+ /** TODO: we want to put tunnel.has_consumer_besides_client back in here.
+ *
+ * Currently, our HTTP/2 to origin branch does not work with background
+ * fill. Trying to keep the tunnel around to fill the cache after the
+ * client aborts currently results in a connection leak. This has to be
+ * fixed, but for now we're avoiding this relatively new feature (Spring
+ * 2021) to stop the leak.
+ *
+ * Uncomment this code to reinstate background fill once it is fixed.
Review Comment:
By the way, CI is currently failing the autest for background_fill precisely because of this issue. So, to be clear, I don't intend to move forward with this PR until we get this feature working.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r962054198
##########
proxy/hdrs/HdrToken.cc:
##########
@@ -230,7 +230,10 @@ static HdrTokenFieldInfo _hdrtoken_strs_field_initializers[] = {
{"Strict-Transport-Security", MIME_SLOTID_NONE, MIME_PRESENCE_NONE, (HTIF_MULTVALS)},
{"Subject", MIME_SLOTID_NONE, MIME_PRESENCE_SUBJECT, HTIF_NONE},
{"Summary", MIME_SLOTID_NONE, MIME_PRESENCE_SUMMARY, HTIF_NONE},
- {"TE", MIME_SLOTID_TE, MIME_PRESENCE_TE, (HTIF_COMMAS | HTIF_MULTVALS | HTIF_HOPBYHOP)},
+ // Need to figure out why this cannot be handled as hop by hop. If it is hop-by-hop
+ // the information is not propagated for gRPC
+ //{"TE", MIME_SLOTID_TE, MIME_PRESENCE_TE, (HTIF_COMMAS | HTIF_MULTVALS | HTIF_HOPBYHOP)},
+ {"TE", MIME_SLOTID_TE, MIME_PRESENCE_TE, (HTIF_COMMAS | HTIF_MULTVALS)},
Review Comment:
Thank you Susan for the history. I added `HTIF_MULTVALS` back in and added a TODO comment.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r941053989
##########
proxy/http/HttpSM.cc:
##########
@@ -974,19 +973,34 @@ HttpSM::state_watch_for_client_abort(int event, void *data)
*/
case VC_EVENT_EOS: {
// We got an early EOS. If the tunnal has cache writer, don't kill it for background fill.
- NetVConnection *netvc = ua_txn->get_netvc();
- if (ua_txn->allow_half_open() || tunnel.has_consumer_besides_client()) {
- if (netvc) {
- netvc->do_io_shutdown(IO_SHUTDOWN_READ);
+ if (!terminate_sm) { // Not done already
+ NetVConnection *netvc = ua_txn->get_netvc();
+ /** TODO: we want to put tunnel.has_consumer_besides_client back in here.
+ *
+ * Currently, our HTTP/2 to origin branch does not work with background
+ * fill. Trying to keep the tunnel around to fill the cache after the
+ * client aborts currently results in a connection leak. This has to be
+ * fixed, but for now we're avoiding this relatively new feature (Spring
+ * 2021) to stop the leak.
+ *
+ * Uncomment this code to reinstate background fill once it is fixed.
Review Comment:
It's been over a year since this TODO comment. Any progress?
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r941081313
##########
proxy/http/ConnectingEntry.cc:
##########
@@ -0,0 +1,157 @@
+/** @file
+
+ Server side connection management.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+
+ */
+
+#include "ConnectingEntry.h"
+#include "HttpSM.h"
+
+ConnectingEntry::~ConnectingEntry()
+{
+ if (_netvc_read_buffer != nullptr) {
+ free_MIOBuffer(_netvc_read_buffer);
+ _netvc_read_buffer = nullptr;
+ }
+}
+
+int
+ConnectingEntry::state_http_server_open(int event, void *data)
+{
+ Debug("http_connect", "entered inside ConnectingEntry::state_http_server_open");
+
+ switch (event) {
+ case NET_EVENT_OPEN: {
+ _netvc = static_cast<NetVConnection *>(data);
+ UnixNetVConnection *vc = static_cast<UnixNetVConnection *>(_netvc);
+ ink_release_assert(_pending_action == nullptr || _pending_action->continuation == vc->get_action()->continuation);
+ _pending_action = nullptr;
+ Debug("http_connect", "ConnectingEntrysetting handler for TCP handshake");
Review Comment:
The message implies code around this line is for TCP connections. Is it true?
I'm not going to say make this work with QUIC connections right now but I'm very sad if QUIC is completely out of your scope and we have to redesign it when we support QUIC to Origin. Wording that is not protocol-specific would help understanding what condition/state is expected.
More specifically, I'm wondering if TLS handshake has to be undone, because QUIC can't do TLS handshake separately.
Anyways, I don't want to make this change with H2-to-Origin at once. Connection pool should work without H2.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r940918841
##########
proxy/http2/HTTP2.cc:
##########
@@ -599,6 +610,9 @@ http2_convert_header_from_1_1_to_2(HTTPHdr *headers)
} else {
field->value_set(headers->m_heap, headers->m_mime, value, value_len);
}
+ // Remove the host header field, redundant to the authority field
+ // For istio/envoy, having both was causing 404 responses
Review Comment:
This is interesting, because RFC 9113 says:
> An intermediary that forwards a request over HTTP/2 MAY retain any
Host header field.
Removing `Host` header should be fine, but I wonder why it causes 404. It could be because of mismatch between the generated `:authority` pseudo header and the retained `Host` header.
The RFC also says:
> Clients MUST NOT generate a request with a Host header field that
differs from the ":authority" pseudo-header field. A server
SHOULD treat a request as malformed if it contains a Host header
field that identifies an entity that differs from the entity in
the ":authority" pseudo-header field.
> An intermediary that forwards a request over HTTP/2 MUST construct
an ":authority" pseudo-header field using the authority
information from the control data of the original request, unless
the original request's target URI does not contain authority
information (in which case it MUST NOT generate ":authority").
Note that the Host header field is not the sole source of this
information; see Section 7.2 of [[HTTP](https://datatracker.ietf.org/doc/html/rfc9113#ref-HTTP)].
and ATS does gather the information from the request line if available. We might want to check generated `:authority` is really appropriate.
I don't remember why we didn't remove `Host` header. There may be no reason, but it might be because removing it is unnecessary/optional.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r940852025
##########
mgmt/RecordsConfig.cc:
##########
@@ -1354,6 +1358,8 @@ static const RecordElement RecordsConfig[] =
,
{RECT_CONFIG, "proxy.config.http2.no_activity_timeout_in", RECD_INT, "120", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
+ {RECT_CONFIG, "proxy.config.http2.no_activity_timeout_out", RECD_INT, "120", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
Review Comment:
Need documentation.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921774918
##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -3956,6 +3956,13 @@ Client-Related Configuration
Enables (``1``) or disables (``0``) TLSv1_3 in the ATS client context. If not specified, enabled by default
+.. ts:cv:: CONFIG proxy.config.ssl.client.alpn_protocol STRING ""
Review Comment:
I pointed out the same thing on the previous PR, but I'm not sure if users want to set "ALPN string".
The ALPN protocol IDs are inconsistent; The ID for HTTP/1.1 is "http/1.1" but not "h1.1" where ones for HTTP/2 and 3 are "h2" and "h3". I don't think these IDs are user friendly.
Even if we use those raw IDs, the doc should say what IDs are available. Obviously we can't set "spdy/1", although it's a registered valid ID. We shouldn't send "h3" on a regular TLS connection, and similarly we shouldn't send "h2" on a QUIC connection.
Also, the order of IDs is unclear if a user is unfamiliar with ALPN.
I don't have a good idea right now, but I think we need discussion about this setting at minimum. Changing this setting would be an incompatible change and we wouldn't be able to change it until 11.0 once we make 10.0 release with this setting.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921963287
##########
iocore/net/ALPNSupport.cc:
##########
@@ -144,3 +144,36 @@ ALPNSupport::registerNextProtocolSet(SSLNextProtocolSet *s, const SessionProtoco
this->npnSet = s;
npnSet->create_npn_advertisement(protoenabled, &npn, &npnsz);
}
+
+bool
+ALPNSupport::process_alpn_protocols(const std::string_view protocols, unsigned char *client_alpn_protocols, int &alpn_array_len)
Review Comment:
It would be nice if you could put this into some module that parses setting values. Maybe RecHttp.cc, which parses `server_ports`.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1185111999
Since H2 to Origin has been unmarged for very long time, I'm slightly lowering a bar for merging this.
I still think issue https://github.com/apache/trafficserver/issues/5230 should be fixed eventually (not just for performance but also privacy), but I'm going to let you off the hook. I actually have an incomplete patch (which works for simple use case at minimum) to pass an `HttpHdr` for request to `HttpSM` without marshaling and unmarshaling. Maybe we can add `sensitive` flag later to keep HPACK header representation. For now, I'd like to see some warning or note on the doc that says header representation won't be kept and sensitive headers can be stored into HPACK dynamic table.
--
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] shinrich commented on a diff in pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
shinrich commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r961710854
##########
proxy/http/ConnectingEntry.cc:
##########
@@ -0,0 +1,157 @@
+/** @file
+
+ Server side connection management.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+
+ */
+
+#include "ConnectingEntry.h"
+#include "HttpSM.h"
+
+ConnectingEntry::~ConnectingEntry()
+{
+ if (_netvc_read_buffer != nullptr) {
+ free_MIOBuffer(_netvc_read_buffer);
+ _netvc_read_buffer = nullptr;
+ }
+}
+
+int
+ConnectingEntry::state_http_server_open(int event, void *data)
+{
+ Debug("http_connect", "entered inside ConnectingEntry::state_http_server_open");
+
+ switch (event) {
+ case NET_EVENT_OPEN: {
+ _netvc = static_cast<NetVConnection *>(data);
+ UnixNetVConnection *vc = static_cast<UnixNetVConnection *>(_netvc);
+ ink_release_assert(_pending_action == nullptr || _pending_action->continuation == vc->get_action()->continuation);
+ _pending_action = nullptr;
+ Debug("http_connect", "ConnectingEntrysetting handler for TCP handshake");
Review Comment:
That is just a working thing. Can remove TCP. Will need to test this in QUIC case. There is a specific sequency of events that get sent as the TCP/TLS handshake works out. Will probably be slightly different in the QUIC case.
In the meantime, this should be changed to more neutral language like "connection handshake" instead of "TCP handshake"
##########
proxy/http/ConnectingEntry.cc:
##########
@@ -0,0 +1,157 @@
+/** @file
+
+ Server side connection management.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+
+ */
+
+#include "ConnectingEntry.h"
+#include "HttpSM.h"
+
+ConnectingEntry::~ConnectingEntry()
+{
+ if (_netvc_read_buffer != nullptr) {
+ free_MIOBuffer(_netvc_read_buffer);
+ _netvc_read_buffer = nullptr;
+ }
+}
+
+int
+ConnectingEntry::state_http_server_open(int event, void *data)
+{
+ Debug("http_connect", "entered inside ConnectingEntry::state_http_server_open");
+
+ switch (event) {
+ case NET_EVENT_OPEN: {
+ _netvc = static_cast<NetVConnection *>(data);
+ UnixNetVConnection *vc = static_cast<UnixNetVConnection *>(_netvc);
+ ink_release_assert(_pending_action == nullptr || _pending_action->continuation == vc->get_action()->continuation);
+ _pending_action = nullptr;
+ Debug("http_connect", "ConnectingEntrysetting handler for TCP handshake");
Review Comment:
That is just a wording thing. Can remove TCP. Will need to test this in QUIC case. There is a specific sequency of events that get sent as the TCP/TLS handshake works out. Will probably be slightly different in the QUIC case.
In the meantime, this should be changed to more neutral language like "connection handshake" instead of "TCP handshake"
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r951931106
##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -3956,6 +3956,13 @@ Client-Related Configuration
Enables (``1``) or disables (``0``) TLSv1_3 in the ATS client context. If not specified, enabled by default
+.. ts:cv:: CONFIG proxy.config.ssl.client.alpn_protocol STRING ""
Review Comment:
This work got separated out into PR https://github.com/apache/trafficserver/pull/8972 and is now merged.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r967715304
##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -4096,7 +4103,17 @@ HTTP/2 Configuration
:reloadable:
:units: bytes
- The initial window size for inbound connections.
+ The initial window size for inbound connection streams.
+
+.. ts:cv:: CONFIG proxy.config.http2.session_initial_window_size_in INT 0
Review Comment:
Since #8203 has conflicts, I opened a new PR wherein I can resolve the conflicts: #9085.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r944706108
##########
proxy/http/HttpTransact.h:
##########
@@ -571,12 +573,64 @@ class HttpTransact
ConnectionAttributes *server = nullptr;
ink_time_t now = 0;
ServerState_t state = STATE_UNDEFINED;
- unsigned attempts = 0;
- unsigned simple_retry_attempts = 0;
- unsigned unavailable_server_retry_attempts = 0;
- ParentRetry_t retry_type = PARENT_RETRY_NONE;
+ class Attempts
Review Comment:
Sorry for the delay. Yes, I picked this change into HTTP/2 to origin because we had it internally and it fixes a problem we saw in the connect attempts calculation.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1270935657
If we don't support Server Push on origin side for now, we need to indicate it by sending SETTINGS_ENABLE_PUSH:0.
> SETTINGS_ENABLE_PUSH (0x02): This setting can be used to enable or
disable server push. A server MUST NOT send a PUSH_PROMISE frame
if it receives this parameter set to a value of 0; see
[Section 8.4](https://datatracker.ietf.org/doc/html/rfc9113#section-8.4). A client that has both set this parameter to 0 and
had it acknowledged MUST treat the receipt of a PUSH_PROMISE frame
as a connection error ([Section 5.4.1](https://datatracker.ietf.org/doc/html/rfc9113#section-5.4.1)) of type PROTOCOL_ERROR.
> The initial value of SETTINGS_ENABLE_PUSH is 1. For a client,
this value indicates that it is willing to receive PUSH_PROMISE
frames.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921810678
##########
proxy/http2/HTTP2.h:
##########
@@ -247,8 +252,8 @@ enum Http2SettingsIdentifier {
HTTP2_SETTINGS_INITIAL_WINDOW_SIZE = 4,
HTTP2_SETTINGS_MAX_FRAME_SIZE = 5,
HTTP2_SETTINGS_MAX_HEADER_LIST_SIZE = 6,
-
- HTTP2_SETTINGS_MAX
+ HTTP2_SETTINGS_MAX, // Really just the max of the "densely numbered" core id's
+ HTTP2_SETTINGS_GRPC_ALLOW_TRUE_BINARY_METADATA = 0xfe03,
Review Comment:
This doesn't seem like to be used.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921867932
##########
proxy/http/HttpTransact.h:
##########
@@ -571,12 +573,64 @@ class HttpTransact
ConnectionAttributes *server = nullptr;
ink_time_t now = 0;
ServerState_t state = STATE_UNDEFINED;
- unsigned attempts = 0;
- unsigned simple_retry_attempts = 0;
- unsigned unavailable_server_retry_attempts = 0;
- ParentRetry_t retry_type = PARENT_RETRY_NONE;
+ class Attempts
Review Comment:
Would you make this change separately?
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r940942582
##########
proxy/http2/HTTP2.cc:
##########
@@ -446,10 +451,16 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers)
if (MIMEField *field = headers->field_find(HTTP2_VALUE_AUTHORITY, HTTP2_LEN_AUTHORITY);
field != nullptr && field->value_is_valid()) {
int authority_len;
- const char *authority = field->value_get(&authority_len);
+ // Set the host header field
+ MIMEField *host = headers->field_find(MIME_FIELD_HOST, MIME_LEN_HOST);
+ if (host == nullptr) {
+ host = headers->field_create(MIME_FIELD_HOST, MIME_LEN_HOST);
+ headers->field_attach(host);
Review Comment:
Okay, `Host` header was unnecessary for ATS internal process if the request URL is absolute form, but adding it makes sense.
This block is fine at all and no change is needed, but I realized one thing while I'm checking RFC 9110. According to the RFC, `Host` header should be at the beginning of a header field list.
> A user agent that sends Host SHOULD send it as the first
field in the header section of a request.
That makes sense and we do that for pseudo headers though, that's not easy to do with our `MIMEHdr` structure, because it doesn't allow us to insert/prepend headers.
I also checked RFC 9113 (HTTP/2) and it says:
> All pseudo-header fields MUST appear in a field block before all
regular field lines. Any request or response that contains a pseudo-
header field that appears in a field block after a regular field line
MUST be treated as malformed ([Section 8.1.1](https://datatracker.ietf.org/doc/html/rfc9113#section-8.1.1)).
So it's MUST for pseudo headers, but it's SHOULD for `Host` header. It would be nice if we could have `Host` header at the beginning but I don't think it has to be done on 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: 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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r940849845
##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -4096,7 +4103,17 @@ HTTP/2 Configuration
:reloadable:
:units: bytes
- The initial window size for inbound connections.
+ The initial window size for inbound connection streams.
+
+.. ts:cv:: CONFIG proxy.config.http2.session_initial_window_size_in INT 0
Review Comment:
This is #8203, right? This is obviously for incoming connections and should be merged separately. I was -0 on the PR. Take a look at the discussion on #8199 as well.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r974986588
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1275,6 +1331,99 @@ Http2ConnectionState::state_closed(int event, void *edata)
return 0;
}
+bool
+Http2ConnectionState::is_peer_concurrent_stream_ub() const
+{
+ return client_streams_in_count >= (peer_settings.get(HTTP2_SETTINGS_MAX_CONCURRENT_STREAMS)) * 0.9;
Review Comment:
I don't understand why these magic thresholds are needed. Although an implementation must not exceed the limit on purpose, it doesn't need to consider a possibility that a limit change is on wire.
> The values in the SETTINGS frame MUST be processed in the order they
appear, with no other frame processing between values. Unsupported
settings MUST be ignored. Once all values have been processed, the
recipient MUST immediately emit a SETTINGS frame with the ACK flag
set. Upon receiving a SETTINGS frame with the ACK flag set, the
sender of the altered settings can rely on the values from the oldest
unacknowledged SETTINGS frame having been applied.
> If the sender of a SETTINGS frame does not receive an acknowledgment
within a reasonable amount of time, it MAY issue a connection error
([Section 5.4.1](https://datatracker.ietf.org/doc/html/rfc9113#section-5.4.1)) of type SETTINGS_TIMEOUT. In setting a timeout, some
allowance needs to be made for processing delays at the peer; a
timeout that is solely based on the round-trip time between endpoints
might result in spurious errors.
The RFC also says:
> An endpoint that wishes to reduce the value of
SETTINGS_MAX_CONCURRENT_STREAMS to a value that is below the current
number of open streams can either close streams that exceed the new
value or allow streams to complete.
It's not a something that need to be taken care by stream initiator
--
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] shinrich commented on a diff in pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
shinrich commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r961725683
##########
proxy/http/ConnectingEntry.cc:
##########
@@ -0,0 +1,157 @@
+/** @file
+
+ Server side connection management.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+
+ */
+
+#include "ConnectingEntry.h"
+#include "HttpSM.h"
+
+ConnectingEntry::~ConnectingEntry()
+{
+ if (_netvc_read_buffer != nullptr) {
+ free_MIOBuffer(_netvc_read_buffer);
+ _netvc_read_buffer = nullptr;
+ }
+}
+
+int
+ConnectingEntry::state_http_server_open(int event, void *data)
+{
+ Debug("http_connect", "entered inside ConnectingEntry::state_http_server_open");
+
+ switch (event) {
+ case NET_EVENT_OPEN: {
+ _netvc = static_cast<NetVConnection *>(data);
+ UnixNetVConnection *vc = static_cast<UnixNetVConnection *>(_netvc);
+ ink_release_assert(_pending_action == nullptr || _pending_action->continuation == vc->get_action()->continuation);
+ _pending_action = nullptr;
+ Debug("http_connect", "ConnectingEntrysetting handler for TCP handshake");
Review Comment:
Noticed that some Debug messages in HttpSM also refer to TCP. They should also be cleaned up.
--
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 pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1320777548
[approve ci autest]
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r967708734
##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -4096,7 +4103,17 @@ HTTP/2 Configuration
:reloadable:
:units: bytes
- The initial window size for inbound connections.
+ The initial window size for inbound connection streams.
+
+.. ts:cv:: CONFIG proxy.config.http2.session_initial_window_size_in INT 0
Review Comment:
Yes, correct. This PR pulls in that change. I agree that it's a good idea to keep it separate. I'll re-open #8203.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r967710960
##########
proxy/http/HttpSM.cc:
##########
@@ -3405,6 +3657,66 @@ HttpSM::tunnel_handler_ua(int event, HttpTunnelConsumer *c)
return 0;
}
+int
+HttpSM::tunnel_handler_trailer_ua(int event, HttpTunnelConsumer *c)
Review Comment:
Just to wrap up this thread, we can work on trailer/gRPC further in a separate PR. Thank you, @maskit and @shinrich, for clarifying 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] maskit commented on a diff in pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r940977582
##########
proxy/http2/HTTP2.cc:
##########
@@ -599,6 +610,9 @@ http2_convert_header_from_1_1_to_2(HTTPHdr *headers)
} else {
field->value_set(headers->m_heap, headers->m_mime, value, value_len);
}
+ // Remove the host header field, redundant to the authority field
+ // For istio/envoy, having both was causing 404 responses
Review Comment:
I found a TODO comment at right after your changes (on line 651). We can remove the comment.
Also, I finished reviewing changes in `http2_convert_header_from_2_to_1_1` and `http2_convert_header_from_1_1_to_2`. No additional change is needed except the TODO comment I mentioned above. If you make a separate PR for these changes, I can approve it immediately.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r940977582
##########
proxy/http2/HTTP2.cc:
##########
@@ -599,6 +610,9 @@ http2_convert_header_from_1_1_to_2(HTTPHdr *headers)
} else {
field->value_set(headers->m_heap, headers->m_mime, value, value_len);
}
+ // Remove the host header field, redundant to the authority field
+ // For istio/envoy, having both was causing 404 responses
Review Comment:
I found a TODO comment at right after your changes (on line 651). We can remove the comment.
Also, I finished reviewing changes in `http2_convert_header_from_2_to_1_1` and `http2_convert_header_from_1_1_to_2`. No additional change is needed except the TODO comment I mentioned above. If you make a separate PR for these changes, I can approve it immediately.
I'd say we can make the changes on master. Since we both modified the functions and my change is going to be on master, that would be easier to manage than having two different changes on master and 10-Dev.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r941021258
##########
proxy/http2/Http2ServerSession.cc:
##########
@@ -0,0 +1,420 @@
+/** @file
+
+ Http2ServerSession.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "Http2ServerSession.h"
+#include "HttpDebugNames.h"
+#include "tscore/ink_base64.h"
+#include "Http2CommonSessionInternal.h"
+#include "HttpSessionManager.h"
+#include "P_SSLNetVConnection.h"
+
+ClassAllocator<Http2ServerSession> http2ServerSessionAllocator("http2ServerSessionAllocator");
+
+static int
+send_connection_event(Continuation *cont, int event, void *edata)
+{
+ SCOPED_MUTEX_LOCK(lock, cont->mutex, this_ethread());
+ return cont->handleEvent(event, edata);
+}
+
+Http2ServerSession::Http2ServerSession() = default;
+
+void
+Http2ServerSession::destroy()
+{
+ if (!in_destroy) {
+ in_destroy = true;
+ write_vio = nullptr;
+ this->remove_session();
+ this->release_outbound_connection_tracking();
+ REMEMBER(NO_EVENT, this->recursion)
+ Http2SsnDebug("session destroy");
+ if (_vc) {
+ _vc->do_io_close();
+ _vc = nullptr;
+ }
+ free();
+ }
+}
+
+void
+Http2ServerSession::free()
+{
+ auto mutex_thread = this->mutex->thread_holding;
+ if (Http2CommonSession::common_free(this)) {
+ HTTP2_DECREMENT_THREAD_DYN_STAT(HTTP2_STAT_CURRENT_SERVER_SESSION_COUNT, mutex_thread);
+ THREAD_FREE(this, http2ServerSessionAllocator, mutex_thread);
+ }
+}
+
+void
+Http2ServerSession::start()
+{
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
+
+ SET_HANDLER(&Http2ServerSession::main_event_handler);
+ HTTP2_SET_SESSION_HANDLER(&Http2ServerSession::state_start_frame_read);
+
+ VIO *read_vio = this->do_io_read(this, INT64_MAX, this->read_buffer);
+ write_vio = this->do_io_write(this, INT64_MAX, this->_write_buffer_reader);
+
+ this->connection_state.init(this);
+
+ // 3.5 HTTP/2 Connection Preface. Upon establishment of a TCP connection and
+ // determination that HTTP/2 will be used by both peers, each endpoint MUST
+ // send a connection preface as a final confirmation ...
+ // This is the preface string sent by the client
+ this->write_buffer->write(HTTP2_CONNECTION_PREFACE, HTTP2_CONNECTION_PREFACE_LEN);
+ total_write_len += HTTP2_CONNECTION_PREFACE_LEN;
+ // write_vio->nbytes = total_write_len;
+ write_reenable();
+ this->connection_state.send_connection_preface();
+ Http2SsnDebug("Sent Connection Preface");
+
+ this->handleEvent(VC_EVENT_READ_READY, read_vio);
+}
+
+void
+Http2ServerSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOBufferReader *reader)
+{
+ ink_assert(new_vc->mutex->thread_holding == this_ethread());
+ HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_CURRENT_SERVER_SESSION_COUNT, new_vc->mutex->thread_holding);
+ HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_TOTAL_SERVER_CONNECTION_COUNT, new_vc->mutex->thread_holding);
+ this->_milestones.mark(Http2SsnMilestone::OPEN);
+
+ // Unique client session identifier.
+ this->con_id = ProxySession::next_connection_id();
+ this->_vc = new_vc;
+ _vc->set_inactivity_timeout(HRTIME_SECONDS(Http2::accept_no_activity_timeout));
+ this->schedule_event = nullptr;
+ this->mutex = new_vc->mutex;
+
+ this->connection_state.mutex = this->mutex;
+
+ TLSEarlyDataSupport *eds = dynamic_cast<TLSEarlyDataSupport *>(new_vc);
+ if (eds != nullptr) {
+ this->read_from_early_data = eds->get_early_data_len();
+ Debug("ssl_early_data", "read_from_early_data = %" PRId64, this->read_from_early_data);
+ }
+
+ Http2SsnDebug("session born, netvc %p", this->_vc);
+
+ this->_vc->set_tcp_congestion_control(CLIENT_SIDE);
+
+ this->read_buffer = iobuf ? iobuf : new_MIOBuffer(HTTP2_HEADER_BUFFER_SIZE_INDEX);
+ this->read_buffer->water_mark = connection_state.local_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE);
+ this->_read_buffer_reader = reader ? reader : this->read_buffer->alloc_reader();
+
+ // Set write buffer size to max size of TLS record (16KB)
+ // 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_reader = this->write_buffer->alloc_reader();
+ this->_write_size_threshold = index_to_buffer_size(buffer_block_size_index) * Http2::write_size_threshold;
+
+ this->_handle_if_ssl(new_vc);
+
+ do_api_callout(TS_HTTP_SSN_START_HOOK);
+
+ this->add_session();
+}
+
+// implement that. After we send a GOAWAY, there
+// are scenarios where we would like to complete the outstanding streams.
+
+void
+Http2ServerSession::do_io_close(int alerrno)
+{
+ REMEMBER(NO_EVENT, this->recursion)
+
+ if (!this->connection_state.is_state_closed()) {
+ Http2SsnDebug("session closed");
+ this->remove_session();
+
+ ink_assert(this->mutex->thread_holding == this_ethread());
+ send_connection_event(&this->connection_state, HTTP2_SESSION_EVENT_FINI, this);
+
+ // Destroy will be called from connection_state.release_stream() once the number of active streams goes to 0
+ }
+}
+
+int
+Http2ServerSession::main_event_handler(int event, void *edata)
+{
+ ink_assert(this->mutex->thread_holding == this_ethread());
+ int retval;
+
+ recursion++;
+
+ Event *e = static_cast<Event *>(edata);
+ if (e == schedule_event) {
+ schedule_event = nullptr;
+ }
+
+ Http2SsnDebug("main_event_handler=%d edata=%p", event, edata);
+
+ switch (event) {
+ case VC_EVENT_READ_COMPLETE:
+ case VC_EVENT_READ_READY: {
+ bool is_zombie = connection_state.get_zombie_event() != nullptr;
+ retval = (this->*session_handler)(event, edata);
+ if (is_zombie && connection_state.get_zombie_event() != nullptr) {
+ Warning("Processed read event for zombie session %" PRId64, connection_id());
+ }
+ break;
+ }
+
+ case HTTP2_SESSION_EVENT_REENABLE:
+ // VIO will be reenableed in this handler
+ retval = (this->*session_handler)(VC_EVENT_READ_READY, static_cast<VIO *>(e->cookie));
+ // Clear the event after calling session_handler to not reschedule REENABLE in it
+ this->_reenable_event = nullptr;
+ break;
+
+ case VC_EVENT_ACTIVE_TIMEOUT:
+ case VC_EVENT_INACTIVITY_TIMEOUT:
+ case VC_EVENT_ERROR:
+ case VC_EVENT_EOS:
+ this->set_dying_event(event);
+ this->do_io_close();
+ retval = 0;
+ break;
+
+ case VC_EVENT_WRITE_READY:
+ case VC_EVENT_WRITE_COMPLETE:
+ this->connection_state.restart_streams();
+ if ((Thread::get_hrtime() >= this->_write_buffer_last_flush + HRTIME_MSECONDS(this->_write_time_threshold))) {
+ this->flush();
+ }
+
+ retval = 0;
+ break;
+
+ case HTTP2_SESSION_EVENT_XMIT:
+ default:
+ Http2SsnDebug("unexpected event=%d edata=%p", event, edata);
+ ink_release_assert(0);
+ retval = 0;
+ break;
+ }
+
+ if (!this->is_draining() && this->connection_state.get_shutdown_reason() == Http2ErrorCode::HTTP2_ERROR_MAX) {
+ this->connection_state.set_shutdown_state(HTTP2_SHUTDOWN_NONE);
+ }
+
+ if (this->connection_state.get_shutdown_state() == HTTP2_SHUTDOWN_NONE) {
+ if (this->is_draining()) { // For a case we already checked Connection header and it didn't exist
+ Http2SsnDebug("Preparing for graceful shutdown because of draining state");
+ this->connection_state.set_shutdown_state(HTTP2_SHUTDOWN_NOT_INITIATED);
+ } /*else if (this->connection_state.get_stream_error_rate() >
Review Comment:
But if we are going to say H2 to Origin is experimental support, like QUIC, we can ignore security concerns for 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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r941017142
##########
proxy/http2/Http2ServerSession.cc:
##########
@@ -0,0 +1,420 @@
+/** @file
+
+ Http2ServerSession.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "Http2ServerSession.h"
+#include "HttpDebugNames.h"
+#include "tscore/ink_base64.h"
+#include "Http2CommonSessionInternal.h"
+#include "HttpSessionManager.h"
+#include "P_SSLNetVConnection.h"
+
+ClassAllocator<Http2ServerSession> http2ServerSessionAllocator("http2ServerSessionAllocator");
+
+static int
+send_connection_event(Continuation *cont, int event, void *edata)
+{
+ SCOPED_MUTEX_LOCK(lock, cont->mutex, this_ethread());
+ return cont->handleEvent(event, edata);
+}
+
+Http2ServerSession::Http2ServerSession() = default;
+
+void
+Http2ServerSession::destroy()
+{
+ if (!in_destroy) {
+ in_destroy = true;
+ write_vio = nullptr;
+ this->remove_session();
+ this->release_outbound_connection_tracking();
+ REMEMBER(NO_EVENT, this->recursion)
+ Http2SsnDebug("session destroy");
+ if (_vc) {
+ _vc->do_io_close();
+ _vc = nullptr;
+ }
+ free();
+ }
+}
+
+void
+Http2ServerSession::free()
+{
+ auto mutex_thread = this->mutex->thread_holding;
+ if (Http2CommonSession::common_free(this)) {
+ HTTP2_DECREMENT_THREAD_DYN_STAT(HTTP2_STAT_CURRENT_SERVER_SESSION_COUNT, mutex_thread);
+ THREAD_FREE(this, http2ServerSessionAllocator, mutex_thread);
+ }
+}
+
+void
+Http2ServerSession::start()
+{
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
+
+ SET_HANDLER(&Http2ServerSession::main_event_handler);
+ HTTP2_SET_SESSION_HANDLER(&Http2ServerSession::state_start_frame_read);
+
+ VIO *read_vio = this->do_io_read(this, INT64_MAX, this->read_buffer);
+ write_vio = this->do_io_write(this, INT64_MAX, this->_write_buffer_reader);
+
+ this->connection_state.init(this);
+
+ // 3.5 HTTP/2 Connection Preface. Upon establishment of a TCP connection and
+ // determination that HTTP/2 will be used by both peers, each endpoint MUST
+ // send a connection preface as a final confirmation ...
+ // This is the preface string sent by the client
+ this->write_buffer->write(HTTP2_CONNECTION_PREFACE, HTTP2_CONNECTION_PREFACE_LEN);
+ total_write_len += HTTP2_CONNECTION_PREFACE_LEN;
+ // write_vio->nbytes = total_write_len;
+ write_reenable();
+ this->connection_state.send_connection_preface();
+ Http2SsnDebug("Sent Connection Preface");
+
+ this->handleEvent(VC_EVENT_READ_READY, read_vio);
+}
+
+void
+Http2ServerSession::new_connection(NetVConnection *new_vc, MIOBuffer *iobuf, IOBufferReader *reader)
+{
+ ink_assert(new_vc->mutex->thread_holding == this_ethread());
+ HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_CURRENT_SERVER_SESSION_COUNT, new_vc->mutex->thread_holding);
+ HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_TOTAL_SERVER_CONNECTION_COUNT, new_vc->mutex->thread_holding);
+ this->_milestones.mark(Http2SsnMilestone::OPEN);
+
+ // Unique client session identifier.
+ this->con_id = ProxySession::next_connection_id();
+ this->_vc = new_vc;
+ _vc->set_inactivity_timeout(HRTIME_SECONDS(Http2::accept_no_activity_timeout));
+ this->schedule_event = nullptr;
+ this->mutex = new_vc->mutex;
+
+ this->connection_state.mutex = this->mutex;
+
+ TLSEarlyDataSupport *eds = dynamic_cast<TLSEarlyDataSupport *>(new_vc);
+ if (eds != nullptr) {
+ this->read_from_early_data = eds->get_early_data_len();
+ Debug("ssl_early_data", "read_from_early_data = %" PRId64, this->read_from_early_data);
+ }
+
+ Http2SsnDebug("session born, netvc %p", this->_vc);
+
+ this->_vc->set_tcp_congestion_control(CLIENT_SIDE);
+
+ this->read_buffer = iobuf ? iobuf : new_MIOBuffer(HTTP2_HEADER_BUFFER_SIZE_INDEX);
+ this->read_buffer->water_mark = connection_state.local_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE);
+ this->_read_buffer_reader = reader ? reader : this->read_buffer->alloc_reader();
+
+ // Set write buffer size to max size of TLS record (16KB)
+ // 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_reader = this->write_buffer->alloc_reader();
+ this->_write_size_threshold = index_to_buffer_size(buffer_block_size_index) * Http2::write_size_threshold;
+
+ this->_handle_if_ssl(new_vc);
+
+ do_api_callout(TS_HTTP_SSN_START_HOOK);
+
+ this->add_session();
+}
+
+// implement that. After we send a GOAWAY, there
+// are scenarios where we would like to complete the outstanding streams.
+
+void
+Http2ServerSession::do_io_close(int alerrno)
+{
+ REMEMBER(NO_EVENT, this->recursion)
+
+ if (!this->connection_state.is_state_closed()) {
+ Http2SsnDebug("session closed");
+ this->remove_session();
+
+ ink_assert(this->mutex->thread_holding == this_ethread());
+ send_connection_event(&this->connection_state, HTTP2_SESSION_EVENT_FINI, this);
+
+ // Destroy will be called from connection_state.release_stream() once the number of active streams goes to 0
+ }
+}
+
+int
+Http2ServerSession::main_event_handler(int event, void *edata)
+{
+ ink_assert(this->mutex->thread_holding == this_ethread());
+ int retval;
+
+ recursion++;
+
+ Event *e = static_cast<Event *>(edata);
+ if (e == schedule_event) {
+ schedule_event = nullptr;
+ }
+
+ Http2SsnDebug("main_event_handler=%d edata=%p", event, edata);
+
+ switch (event) {
+ case VC_EVENT_READ_COMPLETE:
+ case VC_EVENT_READ_READY: {
+ bool is_zombie = connection_state.get_zombie_event() != nullptr;
+ retval = (this->*session_handler)(event, edata);
+ if (is_zombie && connection_state.get_zombie_event() != nullptr) {
+ Warning("Processed read event for zombie session %" PRId64, connection_id());
+ }
+ break;
+ }
+
+ case HTTP2_SESSION_EVENT_REENABLE:
+ // VIO will be reenableed in this handler
+ retval = (this->*session_handler)(VC_EVENT_READ_READY, static_cast<VIO *>(e->cookie));
+ // Clear the event after calling session_handler to not reschedule REENABLE in it
+ this->_reenable_event = nullptr;
+ break;
+
+ case VC_EVENT_ACTIVE_TIMEOUT:
+ case VC_EVENT_INACTIVITY_TIMEOUT:
+ case VC_EVENT_ERROR:
+ case VC_EVENT_EOS:
+ this->set_dying_event(event);
+ this->do_io_close();
+ retval = 0;
+ break;
+
+ case VC_EVENT_WRITE_READY:
+ case VC_EVENT_WRITE_COMPLETE:
+ this->connection_state.restart_streams();
+ if ((Thread::get_hrtime() >= this->_write_buffer_last_flush + HRTIME_MSECONDS(this->_write_time_threshold))) {
+ this->flush();
+ }
+
+ retval = 0;
+ break;
+
+ case HTTP2_SESSION_EVENT_XMIT:
+ default:
+ Http2SsnDebug("unexpected event=%d edata=%p", event, edata);
+ ink_release_assert(0);
+ retval = 0;
+ break;
+ }
+
+ if (!this->is_draining() && this->connection_state.get_shutdown_reason() == Http2ErrorCode::HTTP2_ERROR_MAX) {
+ this->connection_state.set_shutdown_state(HTTP2_SHUTDOWN_NONE);
+ }
+
+ if (this->connection_state.get_shutdown_state() == HTTP2_SHUTDOWN_NONE) {
+ if (this->is_draining()) { // For a case we already checked Connection header and it didn't exist
+ Http2SsnDebug("Preparing for graceful shutdown because of draining state");
+ this->connection_state.set_shutdown_state(HTTP2_SHUTDOWN_NOT_INITIATED);
+ } /*else if (this->connection_state.get_stream_error_rate() >
Review Comment:
I don't think this part should be completely removed. If ATS is used as a forward proxy, it could be attacked by malicious/misbehaving origin servers.
There's no graceful shutdown mechanism defined on the spec, but we could stop making new requests on the connection that has high error rate to make the connection inactive and close it sooner rather than later.
We could also mark the origin server as a bad H2 server and use H1 on future connections. It should work as fallback mechanism. I think we need this kind of marking in the future anyway to choose a transport protocol (TCP or QUIC) for an origin server.
--
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 pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1187873571
I appreciate the thoughtful review, @maskit. And thank you, too, for repeating some of your comments from the previous PR. I'm working through your observations now and will spin off other PRs per your suggestions.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r951932193
##########
iocore/net/ALPNSupport.cc:
##########
@@ -144,3 +144,36 @@ ALPNSupport::registerNextProtocolSet(SSLNextProtocolSet *s, const SessionProtoco
this->npnSet = s;
npnSet->create_npn_advertisement(protoenabled, &npn, &npnsz);
}
+
+bool
+ALPNSupport::process_alpn_protocols(const std::string_view protocols, unsigned char *client_alpn_protocols, int &alpn_array_len)
Review Comment:
Yes, this is good feedback. We addressed this in the separate ALPN PR: https://github.com/apache/trafficserver/pull/8972.
--
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 pull request #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1281376096
> If we don't support Server Push on origin side for now, we need to indicate it by sending SETTINGS_ENABLE_PUSH:0.
Yes, this is a good observation. I'll work on this when #9085 is finished.
--
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 pull request #8963: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#issuecomment-1414107908
Going to close this and re-open on master now that 10-Dev is master.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921777790
##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -3956,6 +3956,13 @@ Client-Related Configuration
Enables (``1``) or disables (``0``) TLSv1_3 in the ATS client context. If not specified, enabled by default
+.. ts:cv:: CONFIG proxy.config.ssl.client.alpn_protocol STRING ""
Review Comment:
I just remembered I made this comment as well on the previous PR.
> Let's separate out the outbound ALPN part. We still need to discuss how to enable protocols, but the feature itself should work without H2 (e.g. use http/1.1 but not 1.0), and it should be testable.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921918321
##########
proxy/http2/Http2ServerSession.cc:
##########
@@ -0,0 +1,420 @@
+/** @file
+
+ Http2ServerSession.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "Http2ServerSession.h"
+#include "HttpDebugNames.h"
+#include "tscore/ink_base64.h"
+#include "Http2CommonSessionInternal.h"
+#include "HttpSessionManager.h"
+#include "P_SSLNetVConnection.h"
Review Comment:
We should avoid including "P_SSLNetVConnection.h", which brings a lot of dependency. You could include "TLSEarlyDataSupport.h" instead.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921953327
##########
proxy/http/HttpTunnel.cc:
##########
@@ -984,11 +986,14 @@ HttpTunnel::producer_run(HttpTunnelProducer *p)
p->handler_state = HTTP_SM_POST_SUCCESS;
}
}
+ Debug("http_tunnel", "Start write vio %ld bytes", c_write);
// Start the writes now that we know we will consume all the initial data
c->write_vio = c->vc->do_io_write(this, c_write, c->buffer_reader);
ink_assert(c_write > 0);
if (c->write_vio == nullptr) {
consumer_handler(VC_EVENT_ERROR, c);
+ } else if (c->write_vio->ntodo() == 0 && c->alive) {
Review Comment:
Not just this line, but I don't get why these changes to HttpTunnel is needed for H2 to Origin. I'm wondering (and concerned) if these affect H1 and client transactions.
--
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 #8963: Http2 to origin
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #8963:
URL: https://github.com/apache/trafficserver/pull/8963#discussion_r921945112
##########
proxy/http2/Http2CommonSession.cc:
##########
@@ -172,6 +171,7 @@ void
Http2CommonSession::flush()
{
if (this->_pending_sending_data_size > 0) {
+ total_write_len += this->_pending_sending_data_size;
Review Comment:
`total_write_len` doesn't seem to be used.
--
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