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 2021/03/26 09:09:21 UTC
[GitHub] [trafficserver] maskit commented on a change in pull request #7622: HTTP/2 to origin
maskit commented on a change in pull request #7622:
URL: https://github.com/apache/trafficserver/pull/7622#discussion_r601977186
##########
File path: doc/admin-guide/files/records.config.en.rst
##########
@@ -3717,6 +3717,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 ""
+
+ Set the alpn string that ATS will send to origin during new connections. By default no ALPN string will be set.
+ To enable HTTP/2 communication to the origin, set this to "h2,http1.1".
Review comment:
I'd like to know why it has no ALPN string by default. Having "http1.1" (or "http1.1,http1.0") by default and adding "h2" to enable HTTP/2 sound more natural to me. Also, it may be better to mention about the order.
What will we do if we support H3 to origin? Will we internally filter the string to not offer protocols that are not supported on a transport, or have another setting for QUIC transport?
I'm not sure if users want to specify raw ALPN protocol names. For server_ports, we don't use it.
I don't think this is really going to be a problem, but ALPN protocol name may have commas and any characters. The spec says protocol names are non-empty byte strings.
##########
File path: proxy/ProxySession.cc
##########
@@ -106,7 +106,7 @@ ProxySession::state_api_callout(int event, void *data)
if (!lock.is_locked()) {
SET_HANDLER(&ProxySession::state_api_callout);
if (!schedule_event) { // Don't bother if there is already one
- schedule_event = mutex->thread_holding->schedule_in(this, HRTIME_MSECONDS(10));
+ schedule_event = this_ethread()->schedule_in(this, HRTIME_MSECONDS(10));
Review comment:
Can we make this change separately?
##########
File path: iocore/net/SSLConfig.cc
##########
@@ -177,7 +177,6 @@ set_paths_helper(const char *path, const char *filename, char **final_path, char
*final_filename = filename ? ats_stringdup(Layout::get()->relative_to(path, filename)) : nullptr;
}
}
-
Review comment:
I'm surprised that clang-format doesn't point out this. I want an empty line here.
##########
File path: proxy/ProxyTransaction.cc
##########
@@ -220,3 +220,78 @@ ProxyTransaction::has_request_body(int64_t request_content_length, bool is_chunk
{
return request_content_length > 0 || is_chunked;
}
+
+bool
+ProxyTransaction::is_read_closed() const
+{
+ return false;
+}
+
+bool
+ProxyTransaction::expect_send_trailer() const
+{
+ return false;
+}
+
+void
+ProxyTransaction::set_expect_send_trailer()
+{
+}
Review comment:
I want an empty line between functions.
##########
File path: proxy/ProxyTransaction.cc
##########
@@ -220,3 +220,78 @@ ProxyTransaction::has_request_body(int64_t request_content_length, bool is_chunk
{
return request_content_length > 0 || is_chunked;
}
+
+bool
+ProxyTransaction::is_read_closed() const
+{
+ return false;
+}
+
+bool
+ProxyTransaction::expect_send_trailer() const
+{
+ return false;
+}
+
+void
+ProxyTransaction::set_expect_send_trailer()
+{
+}
+bool
+ProxyTransaction::expect_receive_trailer() const
+{
+ return false;
+}
+
+void
+ProxyTransaction::set_expect_receive_trailer()
+{
+}
+
+void
+ProxyTransaction::attach_transaction(HttpSM *attach_sm)
+{
+ _sm = attach_sm;
+}
+
+void
+ProxyTransaction::release()
+{
+ HttpTxnDebug("[%" PRId64 "] session released by sm [%" PRId64 "]", _proxy_ssn ? _proxy_ssn->connection_id() : 0,
+ _sm ? _sm->sm_id : 0);
+
+ this->decrement_transactions_stat();
+
+ // Pass along the release to the session
+ if (_proxy_ssn) {
+ _proxy_ssn->release(this);
+ }
+}
+
+HostDBApplicationInfo::HttpVersion
+ProxyTransaction::get_version(HTTPHdr &hdr) const
+{
+ if (hdr.version_get() == HTTPVersion(1, 1)) {
+ return HostDBApplicationInfo::HTTP_VERSION_11;
+ } else if (hdr.version_get() == HTTPVersion(1, 0)) {
+ return HostDBApplicationInfo::HTTP_VERSION_10;
+ } else {
+ return HostDBApplicationInfo::HTTP_VERSION_09;
+ }
+}
+
+bool
+ProxyTransaction::allow_half_open() const
Review comment:
Can you do this separately to minimize this PR?
We should remove `allow_falf_open()` from Http2Stream and Http3Transact if we have this here.
##########
File path: proxy/http/HttpConfig.cc
##########
@@ -413,6 +413,10 @@ register_stat_callbacks()
RECP_PERSISTENT, (int)http_ua_msecs_counts_errors_pre_accept_hangups_stat,
RecRawStatSyncIntMsecsToFloatSeconds);
+ RecRegisterRawStat(http_rsb, RECT_PROCESS, "proxy.process.http.pooled_server_connections", RECD_INT, RECP_NON_PERSISTENT,
Review comment:
Seems like we could add this stat separately.
##########
File path: proxy/ProxyTransaction.cc
##########
@@ -220,3 +220,78 @@ ProxyTransaction::has_request_body(int64_t request_content_length, bool is_chunk
{
return request_content_length > 0 || is_chunked;
}
+
+bool
+ProxyTransaction::is_read_closed() const
+{
+ return false;
+}
+
+bool
+ProxyTransaction::expect_send_trailer() const
+{
+ return false;
+}
+
+void
+ProxyTransaction::set_expect_send_trailer()
+{
+}
+bool
+ProxyTransaction::expect_receive_trailer() const
+{
+ return false;
+}
+
+void
+ProxyTransaction::set_expect_receive_trailer()
+{
+}
+
+void
+ProxyTransaction::attach_transaction(HttpSM *attach_sm)
+{
+ _sm = attach_sm;
+}
+
+void
+ProxyTransaction::release()
+{
+ HttpTxnDebug("[%" PRId64 "] session released by sm [%" PRId64 "]", _proxy_ssn ? _proxy_ssn->connection_id() : 0,
+ _sm ? _sm->sm_id : 0);
+
+ this->decrement_transactions_stat();
+
+ // Pass along the release to the session
+ if (_proxy_ssn) {
+ _proxy_ssn->release(this);
+ }
+}
+
+HostDBApplicationInfo::HttpVersion
+ProxyTransaction::get_version(HTTPHdr &hdr) const
+{
+ if (hdr.version_get() == HTTPVersion(1, 1)) {
+ return HostDBApplicationInfo::HTTP_VERSION_11;
+ } else if (hdr.version_get() == HTTPVersion(1, 0)) {
+ return HostDBApplicationInfo::HTTP_VERSION_10;
+ } else {
+ return HostDBApplicationInfo::HTTP_VERSION_09;
+ }
+}
+
+bool
+ProxyTransaction::allow_half_open() const
+{
+ return false;
+}
+
+void
+ProxyTransaction::increment_transactions_stat()
Review comment:
I'd like to know why we want this empty implementation. Pure virtual function explicitly require subclasses to have appropriate implementation. With this empty implementation, we may forget to add the stat for HTTP/4.
##########
File path: proxy/http/Http1ClientTransaction.cc
##########
@@ -21,23 +21,28 @@
limitations under the License.
*/
-#include "Http1Transaction.h"
+#include "Http1ClientTransaction.h"
#include "Http1ClientSession.h"
#include "HttpSM.h"
void
-Http1Transaction::release(IOBufferReader *r)
+Http1ClientTransaction::release()
{
-}
+ _proxy_ssn->clear_session_active();
+ /*
+ // Must set this inactivity count here rather than in the session because the state machine
+ // is not available then
+ MgmtInt ka_in = _sm->t_state.txn_conf->keep_alive_no_activity_timeout_in;
+ set_inactivity_timeout(HRTIME_SECONDS(ka_in));
-void
-Http1Transaction::reset()
-{
- _sm = nullptr;
+ _proxy_ssn->ssn_last_txn_time = Thread::get_hrtime();
Review comment:
`ssn_last_txn_time` is unused. I opened #7626 to remove it.
##########
File path: proxy/ProxyTransaction.h
##########
@@ -114,15 +125,19 @@ class ProxyTransaction : public VConnection
// This function must return a non-negative number that is different for two in-progress transactions with the same proxy_ssn
// session.
//
- void set_rx_error_code(ProxyError e);
- void set_tx_error_code(ProxyError e);
+ virtual void set_rx_error_code(ProxyError e);
+ virtual void set_tx_error_code(ProxyError e);
bool support_sni() const;
/// Variables
//
HttpSessionAccept::Options upstream_outbound_options; // overwritable copy of options
+ void set_reader(IOBufferReader *reader);
Review comment:
Seems like `set_reader` is only needed on Http1Transaction. The callers are only Http1ClientSession and Http1ServerSession. Nobody should call this for Http2Stream.
##########
File path: proxy/ProxySession.h
##########
@@ -127,6 +127,7 @@ class ProxySession : public VConnection, public PluginUserArgs<TS_USER_ARGS_SSN>
// Non-Virtual Methods
NetVConnection *get_netvc() const;
+ virtual void set_netvc(NetVConnection *netvc);
Review comment:
This doesn't seem to be needed on client side. We should add `virtual` to `PoolableSession::set_netvc`.
##########
File path: proxy/PoolableSession.h
##########
@@ -72,16 +73,29 @@ class PoolableSession : public ProxySession
TSServerSessionSharingMatchMask sharing_match = TS_SERVER_SESSION_SHARING_MATCH_MASK_NONE;
TSServerSessionSharingPoolType sharing_pool = TS_SERVER_SESSION_SHARING_POOL_GLOBAL;
- // Keep track of connection limiting and a pointer to the
- // singleton that keeps track of the connection counts.
- OutboundConnTrack::Group *conn_track_group = nullptr;
+ void enable_outbound_connection_tracking(OutboundConnTrack::Group *group);
+ void release_outbound_comnection_tracking();
Review comment:
Typo: comnection
##########
File path: proxy/ProxyTransaction.cc
##########
@@ -220,3 +220,78 @@ ProxyTransaction::has_request_body(int64_t request_content_length, bool is_chunk
{
return request_content_length > 0 || is_chunked;
}
+
+bool
+ProxyTransaction::is_read_closed() const
+{
+ return false;
+}
+
+bool
+ProxyTransaction::expect_send_trailer() const
+{
+ return false;
+}
+
+void
+ProxyTransaction::set_expect_send_trailer()
+{
+}
+bool
+ProxyTransaction::expect_receive_trailer() const
+{
+ return false;
+}
+
+void
+ProxyTransaction::set_expect_receive_trailer()
+{
+}
+
+void
+ProxyTransaction::attach_transaction(HttpSM *attach_sm)
+{
+ _sm = attach_sm;
+}
+
+void
+ProxyTransaction::release()
+{
+ HttpTxnDebug("[%" PRId64 "] session released by sm [%" PRId64 "]", _proxy_ssn ? _proxy_ssn->connection_id() : 0,
+ _sm ? _sm->sm_id : 0);
+
+ this->decrement_transactions_stat();
Review comment:
This is done in `ProxyTransaction::transaction_done` too. Maybe double decrement?
##########
File path: proxy/hdrs/HdrToken.cc
##########
@@ -227,7 +227,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:
Assuming this is not harmful for non-gRPC traffic. Having gRPC support may be worth temporally removing hop-by-hop flag, but in case we do that, we should file an issue for it.
Making this change separately would make reverting the change easy in case it affects non-gRPC traffic.
##########
File path: iocore/net/I_NetVConnection.h
##########
@@ -39,6 +39,7 @@
#include "YamlSNIConfig.h"
#include "tscpp/util/TextView.h"
#include "tscore/IpMap.h"
+#include "P_ALPNSupport.h"
Review comment:
Hmmm, for MAX_ALPN_STRING? I don't want to bring in this dependency into I_NetVConnection.
I can see you want to put ALPN stuff into one place, but the limitation (30) doesn't come from ALPN spec, ALPNSupport, or the new function you added. It's a random number defined on users' side. Furthermore, appropriate size depends on ATS user's configuration.
How about having the constant value in ink_defs.h? I think it's pretty similar to TS_MAX_HOST_NAME_LEN in terms of that we need to have some number as an implementation restriction.
##########
File path: proxy/hdrs/HTTP.h
##########
@@ -580,6 +580,27 @@ class HTTPHdr : public MIMEHdr
const char *path_get(int *length ///< Storage for path length.
);
+ /** Get the URL matrix params.
+ This is a reference, not allocated.
+ @return A pointer to the matrix params or @c NULL if there is no valid URL.
+ */
+ const char *params_get(int *length ///< Storage for param length.
Review comment:
We should add these functions separately. Other changes that may happen in the future probably don't want to depend on this H2 to origin commit. Those may be backported to old versions.
##########
File path: proxy/http/Http1ClientTransaction.h
##########
@@ -0,0 +1,52 @@
+/** @file
+
+ Http1ClientTransaction.h - The Client Transaction class for Http1*
+
+ @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.
+ */
+
+#pragma once
+
+#include "Http1Transaction.h"
+
+class Http1ClientTransaction : public Http1Transaction
+{
+public:
+ using super_type = Http1Transaction;
+
+ Http1ClientTransaction() {}
+ Http1ClientTransaction(ProxySession *session) : super_type(session) {}
+
+ ////////////////////
+ // Methods
+ void release() override;
+ // void destroy() override; // todo make ~Http1Transaction()
Review comment:
Remove it.
##########
File path: proxy/http/Http1ClientTransaction.h
##########
@@ -0,0 +1,52 @@
+/** @file
+
+ Http1ClientTransaction.h - The Client Transaction class for Http1*
+
+ @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.
+ */
+
+#pragma once
+
+#include "Http1Transaction.h"
+
+class Http1ClientTransaction : public Http1Transaction
+{
+public:
+ using super_type = Http1Transaction;
+
+ Http1ClientTransaction() {}
+ Http1ClientTransaction(ProxySession *session) : super_type(session) {}
+
+ ////////////////////
+ // Methods
+ void release() override;
+ // void destroy() override; // todo make ~Http1Transaction()
+
+ bool allow_half_open() const override;
+ void transaction_done() override;
+ // int get_transaction_id() const override;
Review comment:
Remove 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org