Skip to content

Commit

Permalink
runtime: load rtds bool correctly as true/false instead of 1/0 (envoy…
Browse files Browse the repository at this point in the history
…proxy#36044)

Commit Message: Load RTDS boolean config correctly as true/false instead
of 1/0
Additional Description: Currently, RTDS boolean values are being loaded
as `1` or `0`, which is inconsistent with the expected `true` or `false`
values. This discrepancy needs to be corrected so that boolean values
are consistently loaded and represented as `true` or `false`. For
further details, please refer to the [issue
comment](envoyproxy#35762 (comment))
where this inconsistency is discussed.
Risk Level: Low (to be consistent with initially loaded true/false
value)
Testing: new Unit Test
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
[Optional Fixes #Issue] envoyproxy#35762

---------

Signed-off-by: Gordon Wang <gordonwang@lyft.com>
  • Loading branch information
gordon-wang-lyft authored Sep 11, 2024
1 parent c091ca9 commit cd3346d
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 1 deletion.
5 changes: 5 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,11 @@ bug_fixes:
the number of requests per I/O cycle is configured and an HTTP decoder filter that pauses filter chain is present. This behavior
can be reverted by setting the runtime guard ``envoy.reloadable_features.use_filter_manager_state_for_downstream_end_stream``
to false.
- area: runtime
change: |
Fixed an inconsistency in how boolean values are loaded in RTDS, where they were previously converted to "1"/"0"
instead of "true"/"false". The correct string representation ("true"/"false") will now be used. This change can be
reverted by setting the runtime guard ``envoy.reloadable_features.boolean_to_string_fix`` to false.
removed_config_or_runtime:
# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
Expand Down
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
// ASAP by filing a bug on github. Overriding non-buggy code is strongly discouraged to avoid the
// problem of the bugs being found after the old code path has been removed.
RUNTIME_GUARD(envoy_reloadable_features_allow_alt_svc_for_ips);
RUNTIME_GUARD(envoy_reloadable_features_boolean_to_string_fix);
RUNTIME_GUARD(envoy_reloadable_features_check_switch_protocol_websocket_handshake);
RUNTIME_GUARD(envoy_reloadable_features_conn_pool_delete_when_idle);
RUNTIME_GUARD(envoy_reloadable_features_consistent_header_validation);
Expand Down
8 changes: 7 additions & 1 deletion source/common/runtime/runtime_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,13 @@ SnapshotImpl::Entry SnapshotImpl::createEntry(const ProtobufWkt::Value& value,
case ProtobufWkt::Value::kBoolValue:
entry.bool_value_ = value.bool_value();
if (entry.raw_string_value_.empty()) {
entry.raw_string_value_ = absl::StrCat(value.bool_value());
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.boolean_to_string_fix")) {
// Convert boolean to "true"/"false"
entry.raw_string_value_ = value.bool_value() ? "true" : "false";
} else {
// Use absl::StrCat for backward compatibility, which converts to "1"/"0"
entry.raw_string_value_ = absl::StrCat(value.bool_value());
}
}
break;
case ProtobufWkt::Value::kStructValue:
Expand Down
35 changes: 35 additions & 0 deletions test/common/runtime/runtime_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#ifdef ENVOY_ENABLE_QUIC
#include "quiche/common/platform/api/quiche_flags.h"
#endif
ABSL_DECLARE_FLAG(bool, envoy_reloadable_features_boolean_to_string_fix);
ABSL_DECLARE_FLAG(bool, envoy_reloadable_features_reject_invalid_yaml);

using testing::_;
Expand Down Expand Up @@ -1314,6 +1315,40 @@ TEST_F(RtdsLoaderImplTest, BadConfigSource) {
"bad config");
}

TEST_F(RtdsLoaderImplTest, BooleanToStringConversionWhenFlagEnabled) {
setup();

absl::SetFlag(&FLAGS_envoy_reloadable_features_boolean_to_string_fix, true);

auto runtime = TestUtility::parseYaml<envoy::service::runtime::v3::Runtime>(R"EOF(
name: some_resource
layer:
toggle: true
)EOF");

EXPECT_CALL(rtds_init_callback_, Call());
doOnConfigUpdateVerifyNoThrow(runtime);

EXPECT_EQ("true", loader_->snapshot().get("toggle").value().get());
}

TEST_F(RtdsLoaderImplTest, BooleanToStringConversionWhenFlagDisabled) {
setup();

absl::SetFlag(&FLAGS_envoy_reloadable_features_boolean_to_string_fix, false);

auto runtime = TestUtility::parseYaml<envoy::service::runtime::v3::Runtime>(R"EOF(
name: some_resource
layer:
toggle: true
)EOF");

EXPECT_CALL(rtds_init_callback_, Call());
doOnConfigUpdateVerifyNoThrow(runtime);

EXPECT_EQ("1", loader_->snapshot().get("toggle").value().get()); // Assuming previous behavior
}

} // namespace
} // namespace Runtime
} // namespace Envoy

0 comments on commit cd3346d

Please sign in to comment.