Skip to content

swift: stop using loopback for direct responses #24402

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

Merged
alyssawilk merged 1 commit into from
Dec 8, 2022
Merged

swift: stop using loopback for direct responses #24402

Show file tree
Hide file tree
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
10 changes: 9 additions & 1 deletion mobile/library/common/config/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,6 @@ const char* config_template = R"(
- name: remote_service
domains: ["*"]
routes:
#{fake_remote_responses}
Copy link
Copy Markdown
Contributor

@Augustyniak Augustyniak Dec 8, 2022

Choose a reason for hiding this comment

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

oh, I thought that we would be able to remove a bigger portion of this fake listener as part of this step. What's preventing us from doing that (apologies if you have explained it in the community sync meeting)?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Dec 8, 2022

Choose a reason for hiding this comment

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

I was going to do that as a follow-up after I removed listener support so we can make sure it doesn't break Lyft tests first :-)

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Dec 8, 2022

Choose a reason for hiding this comment

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

so the plan is land this, then #24363 (working on a c-opt for legacy tests like the kotlin tests) then remove the listener entirely

- match: { prefix: "/" }
direct_response: { status: 404, body: { inline_string: "not found" } }
request_headers_to_remove:
Expand Down Expand Up @@ -375,6 +374,15 @@ const char* config_template = R"(
route_config:
name: api_router
virtual_hosts:
- name: remote_service
domains: ["127.0.0.1"]
routes:
#{fake_remote_responses}
- match: { prefix: "/" }
direct_response: { status: 404, body: { inline_string: "not found" } }
request_headers_to_remove:
- x-forwarded-proto
- x-envoy-mobile-cluster
)"
// The list of virtual hosts impacts directly the number of virtual cluster stats.
// That's because we create a separate set of virtual clusters stats for every
Expand Down
1 change: 1 addition & 0 deletions mobile/library/common/extensions/filters/http/local_error/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ envoy_cc_extension(
repository = "@envoy",
deps = [
":filter_cc_proto",
"//library/common/http:header_utility_lib",
"//library/common/http:internal_headers_lib",
"//library/common/types:c_types_lib",
"@envoy//envoy/http:codes_interface",
Expand Down
4 changes: 3 additions & 1 deletion mobile/library/common/extensions/filters/http/local_error/filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#include "envoy/server/filter_config.h"

#include "library/common/http/header_utility.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
Expand All @@ -18,7 +20,7 @@ Http::LocalErrorStatus LocalErrorFilter::onLocalReply(const LocalReplyData& repl
// ASSERT(reply.details_ == info.responseCodeDetails().value());
info.setResponseCode(static_cast<uint32_t>(reply.code_));

return Http::LocalErrorStatus::ContinueAndResetStream;
return Http::Utility::statusForOnLocalReply(reply, decoder_callbacks_->streamInfo());
}

} // namespace LocalError
Expand Down
1 change: 1 addition & 0 deletions mobile/library/common/extensions/filters/http/network_configuration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ envoy_cc_extension(
repository = "@envoy",
deps = [
":filter_cc_proto",
"//library/common/http:header_utility_lib",
"//library/common/http:internal_headers_lib",
"//library/common/network:connectivity_manager_lib",
"//library/common/stream_info:extra_stream_info_lib",
Expand Down
4 changes: 3 additions & 1 deletion mobile/library/common/extensions/filters/http/network_configuration/filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "source/common/network/filter_state_proxy_info.h"

#include "library/common/http/header_utility.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
Expand Down Expand Up @@ -159,7 +161,7 @@ Http::LocalErrorStatus NetworkConfigurationFilter::onLocalReply(const LocalReply
connectivity_manager_->reportNetworkUsage(extra_stream_info_->configuration_key_.value(),
network_fault);

return Http::LocalErrorStatus::ContinueAndResetStream;
return Http::Utility::statusForOnLocalReply(reply, decoder_callbacks_->streamInfo());
}

void NetworkConfigurationFilter::onDestroy() { dns_cache_handle_.reset(); }
Expand Down
2 changes: 1 addition & 1 deletion mobile/library/common/extensions/filters/http/platform_bridge/filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ Http::LocalErrorStatus PlatformBridgeFilter::onLocalReply(const LocalReplyData&
finalStreamIntel(), platform_filter_.instance_context);
}

return Http::LocalErrorStatus::ContinueAndResetStream;
return Http::Utility::statusForOnLocalReply(reply, decoder_callbacks_->streamInfo());
}

envoy_final_stream_intel PlatformBridgeFilter::finalStreamIntel() {
Expand Down
1 change: 1 addition & 0 deletions mobile/library/common/http/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ envoy_cc_library(
"//library/common/data:utility_lib",
"//library/common/types:c_types_lib",
"@envoy//envoy/buffer:buffer_interface",
"@envoy//envoy/http:filter_interface",
"@envoy//envoy/http:header_map_interface",
"@envoy//source/common/http:header_map_lib",
"@envoy//source/extensions/http/header_formatters/preserve_case:preserve_case_formatter",
Expand Down
11 changes: 11 additions & 0 deletions mobile/library/common/http/header_utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,17 @@ namespace Envoy {
namespace Http {
namespace Utility {

Http::LocalErrorStatus statusForOnLocalReply(const StreamDecoderFilter::LocalReplyData& reply,
const StreamInfo::StreamInfo& info) {
// This is a horrible hack to work around legacy swift direct response API.
// TODO(https://github.com/envoyproxy/envoy/issues/24428) remove.
if (reply.details_ == "direct_response" && info.getRequestHeaders() &&
info.getRequestHeaders()->getHostValue() == "127.0.0.1") {
return Http::LocalErrorStatus::Continue;
}
return Http::LocalErrorStatus::ContinueAndResetStream;
}

void toEnvoyHeaders(HeaderMap& envoy_result_headers, envoy_headers headers) {
Envoy::Http::StatefulHeaderKeyFormatter& formatter = envoy_result_headers.formatter().value();
for (envoy_map_size_t i = 0; i < headers.length; i++) {
Expand Down
9 changes: 9 additions & 0 deletions mobile/library/common/http/header_utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include "envoy/buffer/buffer.h"
#include "envoy/http/filter.h"
#include "envoy/http/header_map.h"

#include "library/common/types/c_types.h"
Expand All @@ -9,6 +10,14 @@ namespace Envoy {
namespace Http {
namespace Utility {

/*
* Returns the proper status code for onLocalReply
* @param reply the local reply data for the stream.
* @param stream_info the info for the stream.
*/
Http::LocalErrorStatus statusForOnLocalReply(const StreamDecoderFilter::LocalReplyData& reply,
const StreamInfo::StreamInfo& stream_info);

/**
* Transform envoy_headers to the supplied HeaderMap
* This function copies the content.
Expand Down
25 changes: 25 additions & 0 deletions mobile/test/common/integration/client_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,5 +315,30 @@ TEST_P(ClientIntegrationTest, DISABLED_Proxying) {
ASSERT_EQ(cc_.on_complete_calls, 2);
}

TEST_P(ClientIntegrationTest, DirectResponse) {
initialize();
if (version_ == Network::Address::IpVersion::v6) {
// Localhost only resolves to an ipv4 address - alas no kernel happy eyeballs.
return;
}

// Override to not validate stream intel.
stream_prototype_->setOnComplete(
[this](envoy_stream_intel, envoy_final_stream_intel final_intel) {
cc_.on_complete_received_byte_count = final_intel.received_byte_count;
cc_.on_complete_calls++;
cc_.terminal_callback->setReady();
});

default_request_headers_.setHost("127.0.0.1");
default_request_headers_.setPath("/");

stream_->sendHeaders(envoyToMobileHeaders(default_request_headers_), true);
terminal_callback_.waitReady();
ASSERT_EQ(cc_.status, "404");
ASSERT_EQ(cc_.on_headers_calls, 1);
stream_.reset();
}

} // namespace
} // namespace Envoy