details: removing spaces (#13423)

Making sure response details don't muck up access logs by removing any spaces.

Risk Level: Low (changes some details)
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

source/common/http/conn_manager_impl.cc

@@ -245,7 +245,7 @@ void ConnectionManagerImpl::handleCodecError(absl::string_view error) {
   // GOAWAY.
   doConnectionClose(Network::ConnectionCloseType::FlushWriteAndDelay,
                     StreamInfo::ResponseFlag::DownstreamProtocolError,
-                    absl::StrCat("codec error: ", error));
+                    absl::StrCat("codec error:", error));
 }
 
 void ConnectionManagerImpl::createCodec(Buffer::Instance& data) {

source/common/stream_info/stream_info_impl.h

@@ -14,6 +14,8 @@
 #include "common/http/request_id_extension_impl.h"
 #include "common/stream_info/filter_state_impl.h"
 
+#include "absl/strings/str_replace.h"
+
 namespace Envoy {
 namespace StreamInfo {
 
@@ -118,7 +120,9 @@ struct StreamInfoImpl : public StreamInfo {
   }
 
   void setResponseCodeDetails(absl::string_view rc_details) override {
-    response_code_details_.emplace(rc_details);
+    const absl::flat_hash_map<std::string, std::string> replacement{
+        {" ", "_"}, {"\t", "_"}, {"\f", "_"}, {"\v", "_"}, {"\n", "_"}, {"\r", "_"}};
+    response_code_details_.emplace(absl::StrReplaceAll(rc_details, replacement));
   }
 
   const absl::optional<std::string>& connectionTerminationDetails() const override {

source/extensions/filters/common/rbac/utility.cc

@@ -16,7 +16,6 @@ RoleBasedAccessControlFilterStats generateStats(const std::string& prefix, Stats
 }
 
 std::string responseDetail(const std::string& policy_id) {
-  // TODO(alyssawilk): put this as a StreamInfo utility and apply to all response details.
   // Replace whitespaces in policy_id with '_' to avoid breaking the access log (inconsistent number
   // of segments between log entries when the separator is whitespace).
   const absl::flat_hash_map<std::string, std::string> replacement{

test/common/http/conn_manager_impl_test_2.cc

@@ -228,7 +228,7 @@ TEST_F(HttpConnectionManagerImplTest, FrameFloodError) {
 
   EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> Http::Status {
     conn_manager_->newStream(response_encoder_);
-    return bufferFloodError("too many outbound frames.");
+    return bufferFloodError("too many outbound frames");
   }));
 
   EXPECT_CALL(response_encoder_.stream_, removeCallbacks(_));
@@ -242,7 +242,7 @@ TEST_F(HttpConnectionManagerImplTest, FrameFloodError) {
       .WillOnce(Invoke([](const HeaderMap*, const HeaderMap*, const HeaderMap*,
                           const StreamInfo::StreamInfo& stream_info) {
         ASSERT_TRUE(stream_info.responseCodeDetails().has_value());
-        EXPECT_EQ("codec error: too many outbound frames.",
+        EXPECT_EQ("codec_error:too_many_outbound_frames",
                   stream_info.responseCodeDetails().value());
       }));
   // Kick off the incoming data.

test/common/stream_info/stream_info_impl_test.cc

@@ -272,6 +272,14 @@ TEST_F(StreamInfoImplTest, ConnectionID) {
   EXPECT_EQ(id, stream_info.connectionID());
 }
 
+TEST_F(StreamInfoImplTest, Details) {
+  StreamInfoImpl stream_info(test_time_.timeSystem());
+  EXPECT_FALSE(stream_info.responseCodeDetails().has_value());
+  stream_info.setResponseCodeDetails("two words");
+  ASSERT_TRUE(stream_info.responseCodeDetails().has_value());
+  EXPECT_EQ(stream_info.responseCodeDetails().value(), "two_words");
+}
+
 } // namespace
 } // namespace StreamInfo
 } // namespace Envoy