Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fatal error if no udp threads but using quic #11673

Closed
wants to merge 1 commit into from
Closed

Fatal error if no udp threads but using quic #11673

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/traffic_server/traffic_server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2215,6 +2215,21 @@ main(int /* argc ATS_UNUSED */, const char **argv)
REC_ReadConfigInteger(num_of_udp_threads, "proxy.config.udp.threads");
}

ats_scoped_str server_ports(255);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

255 chosen arbitrarily - would appreciate guidance of a better number

*server_ports = '\0';
REC_ReadConfigString(server_ports, "proxy.config.http.server_ports", 255);
bool quic_server_port = strstr(server_ports, "/quic/") != server_ports;

#if TS_USE_QUIC == 0
if (quic_server_port) {
Fatal("proxy.config.http.server_ports listening for quic but traffic_server wasn't built with quic support.");
Copy link
Member

Choose a reason for hiding this comment

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

This check could be done as a part of this block, if we want to have a specialized message for QUIC (the block already has code for unrecognized options).

trafficserver/src/records/RecHttp.cc

Lines 367 to 463 in fc5918e

for (auto item : values) {
if (isdigit(item[0])) { // leading digit -> port value
char *ptr;
int port = strtoul(item, &ptr, 10);
if (ptr == item) {
// really, this shouldn't happen, since we checked for a leading digit.
Warning("Mangled port value '%s' in port configuration '%s'", item, opts);
} else if (port <= 0 || 65536 <= port) {
Warning("Port value '%s' out of range (1..65535) in port configuration '%s'", item, opts);
} else {
m_port = port;
zret = true;
}
} else if (nullptr != (value = this->checkPrefix(item, OPT_FD_PREFIX, OPT_FD_PREFIX_LEN))) {
char *ptr; // tmp for syntax check.
int fd = strtoul(value, &ptr, 10);
if (ptr == value) {
Warning("Mangled file descriptor value '%s' in port descriptor '%s'", item, opts);
} else {
m_fd = fd;
zret = true;
}
} else if (nullptr != (value = this->checkPrefix(item, OPT_INBOUND_IP_PREFIX, OPT_INBOUND_IP_PREFIX_LEN))) {
if (0 == ip.load(value)) {
m_inbound_ip = ip;
} else {
Warning("Invalid IP address value '%s' in port descriptor '%s'", item, opts);
}
} else if (nullptr != (value = this->checkPrefix(item, OPT_OUTBOUND_IP_PREFIX, OPT_OUTBOUND_IP_PREFIX_LEN))) {
if (swoc::IPAddr addr; addr.load(value)) {
this->m_outbound = addr;
} else {
Warning("Invalid IP address value '%s' in port descriptor '%s'", item, opts);
}
} else if (0 == strcasecmp(OPT_COMPRESSED, item)) {
m_type = TRANSPORT_COMPRESSED;
} else if (0 == strcasecmp(OPT_BLIND_TUNNEL, item)) {
m_type = TRANSPORT_BLIND_TUNNEL;
} else if (0 == strcasecmp(OPT_IPV6, item)) {
m_family = AF_INET6;
af_set_p = true;
} else if (0 == strcasecmp(OPT_IPV4, item)) {
m_family = AF_INET;
af_set_p = true;
} else if (0 == strcasecmp(OPT_SSL, item)) {
m_type = TRANSPORT_SSL;
#if TS_USE_QUIC == 1
} else if (0 == strcasecmp(OPT_QUIC, item)) {
m_type = TRANSPORT_QUIC;
#endif
} else if (0 == strcasecmp(OPT_PLUGIN, item)) {
m_type = TRANSPORT_PLUGIN;
} else if (0 == strcasecmp(OPT_PROXY_PROTO, item)) {
m_proxy_protocol = true;
} else if (0 == strcasecmp(OPT_TRANSPARENT_INBOUND, item)) {
#if TS_USE_TPROXY
m_inbound_transparent_p = true;
#else
Warning("Transparency requested [%s] in port descriptor '%s' but TPROXY was not configured.", item, opts);
#endif
} else if (0 == strcasecmp(OPT_TRANSPARENT_OUTBOUND, item)) {
#if TS_USE_TPROXY
m_outbound_transparent_p = true;
#else
Warning("Transparency requested [%s] in port descriptor '%s' but TPROXY was not configured.", item, opts);
#endif
} else if (0 == strcasecmp(OPT_TRANSPARENT_FULL, item)) {
#if TS_USE_TPROXY
m_inbound_transparent_p = true;
m_outbound_transparent_p = true;
#else
Warning("Transparency requested [%s] in port descriptor '%s' but TPROXY was not configured.", item, opts);
#endif
} else if (0 == strcasecmp(OPT_TRANSPARENT_PASSTHROUGH, item)) {
#if TS_USE_TPROXY
m_transparent_passthrough = true;
#else
Warning("Transparent pass-through requested [%s] in port descriptor '%s' but TPROXY was not configured.", item, opts);
#endif
} else if (0 == strcasecmp(OPT_ALLOW_PLAIN, item)) {
m_allow_plain = true;
} else if (0 == strcasecmp(OPT_MPTCP, item)) {
if (mptcp_supported()) {
m_mptcp = true;
} else {
Warning("Multipath TCP requested [%s] in port descriptor '%s' but it is not supported by this host.", item, opts);
}
} else if (nullptr != (value = this->checkPrefix(item, OPT_HOST_RES_PREFIX, OPT_HOST_RES_PREFIX_LEN))) {
this->processFamilyPreference(value);
host_res_set_p = true;
} else if (nullptr != (value = this->checkPrefix(item, OPT_PROTO_PREFIX, OPT_PROTO_PREFIX_LEN))) {
this->processSessionProtocolPreference(value);
sp_set_p = true;
} else {
Warning("Invalid option '%s' in proxy port descriptor '%s'", item, opts);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maskit Yeah good suggestion! I tried looking into the records code to see a good place to put this logic in but couldn't find a good place. I see if I can make progress with your suggestion here.

}
#endif

if (quic_server_port && num_of_udp_threads == 0) {
Fatal("proxy.config.http.server_ports listening for quic but proxy.config.udp.threads is 0.");
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we should use Fatal. Even port configuration error is just a warning. It depends on the use case for sure, but QUIC being unavailable is not the end of the world.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maskit I think you might be right w.r.t warning - I bounced between fatal and warning a few times before proposing.

but QUIC being unavailable is not the end of the world.

Well, QUIC being unavailable when you've specifically asked ATS to serve quic could be considered to be the end of the world - it would certainly be surprising enough that I'd think we'd want to catch it sooner rather than later

Copy link
Member

Choose a reason for hiding this comment

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

How you catch misconfiguration and how ATS behave while configuration may be wrong are different things. You could watch for a warning to catch misconfiguration, or you could also watch for the UDP ports. Ideally you should periodically send health check requests. Look at the code I mentioned above. You can try to enable mptcp where mptcp is unavailable on the host and that does not stop ATS. Lack of QUIC does not threaten the safety. QUIC can be unavailable even with proper configuration if middle boxes block it. I still think Fatal is too much.

}

udpNet.register_event_type();
if (num_of_udp_threads) {
udpNet.start(num_of_udp_threads, stacksize);
Expand Down