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

Fix: Lambda json parsing #328

Merged
merged 12 commits into from
Apr 23, 2024
Merged

Fix: Lambda json parsing #328

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c25e6a6
messy early commit: reduce test deps, annotate api gateway transforme…
ben-taussig-solo Apr 1, 2024
f2023a5
safety adjustments to api_gateway_transformer.cc
ben-taussig-solo Apr 11, 2024
ec23571
clean up some comments in api_gateway_transformer.cc
ben-taussig-solo Apr 11, 2024
139b60d
messy update to transformer_teest.cc
ben-taussig-solo Apr 11, 2024
6e13d10
remove duplicate log line
ben-taussig-solo Apr 18, 2024
4fb54a2
clean up unnecessary tests, todos
ben-taussig-solo Apr 18, 2024
4e0f5d5
reintroduce transform_single_and_multi_value_headers test
ben-taussig-solo Apr 18, 2024
bed3b56
remove other unintentionally removed tests
ben-taussig-solo Apr 18, 2024
ef4310a
additional test cleanup
ben-taussig-solo Apr 18, 2024
087fd76
fix whitespace changes + address remaining comments in tests
ben-taussig-solo Apr 18, 2024
6764863
Merge branch 'main' into lambda-json-parsing
ben-taussig-solo Apr 18, 2024
a44b7e3
add changelog entry
ben-taussig-solo Apr 18, 2024
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
33 changes: 25 additions & 8 deletions source/extensions/transformers/aws_lambda/api_gateway_transformer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,14 @@ void ApiGatewayTransformer::transform_response(
// set response status code
if (json_body.contains("statusCode")) {
uint64_t status_value;
try {
status_value = json_body["statusCode"].get<uint64_t>();
} catch (std::exception& exception){
ENVOY_STREAM_LOG(debug, "Error parsing statusCode: " + std::string(exception.what()), stream_filter_callbacks);
ApiGatewayError error = {500, "500", "Non-integer status code"};
if (!json_body["statusCode"].is_number_unsigned()) {
// add duplicate log line to not break tests for now
ENVOY_STREAM_LOG(debug, "cannot parse non unsigned integer status code", stream_filter_callbacks);
ENVOY_STREAM_LOG(debug, "received status code with value: " + json_body["statusCode"].dump(), stream_filter_callbacks);
ApiGatewayError error = {500, "500", "cannot parse non unsigned integer status code"};
return ApiGatewayTransformer::format_error(*response_headers, body, error, stream_filter_callbacks);
}
status_value = json_body["statusCode"].get<uint64_t>();
response_headers->setStatus(status_value);
} else {
response_headers->setStatus(DEFAULT_STATUS_VALUE);
Expand Down Expand Up @@ -132,17 +133,33 @@ void ApiGatewayTransformer::transform_response(
if (json_body.contains("multiValueHeaders")) {
const auto& multi_value_headers = json_body["multiValueHeaders"];
if (!multi_value_headers.is_object()) {
ENVOY_STREAM_LOG(debug, "invalid multi headers object", stream_filter_callbacks);
ApiGatewayError error = {500, "500", "invalid multi headers object"};
ENVOY_STREAM_LOG(debug, "invalid multiValueHeaders object", stream_filter_callbacks);
ApiGatewayError error = {500, "500", "invalid multiValueHeaders object"};
return ApiGatewayTransformer::format_error(*response_headers, body, error, stream_filter_callbacks);
}

for (json::const_iterator it = multi_value_headers.cbegin(); it != multi_value_headers.cend(); it++) {
const auto& header_key = it.key();
const auto& header_values = it.value();

// need to validate that header_values is an array/iterable
if (!header_values.is_array()) {
// if it's an object, we reject the request
if (header_values.is_object()) {
ENVOY_STREAM_LOG(debug, "invalid multi header value object", stream_filter_callbacks);
ApiGatewayError error = {500, "500", "invalid multi header value object"};
return ApiGatewayTransformer::format_error(*response_headers, body, error, stream_filter_callbacks);
}

ENVOY_STREAM_LOG(debug, "warning: using non-array value for multi header value", stream_filter_callbacks);
}
for (json::const_iterator inner_it = header_values.cbegin(); inner_it != header_values.cend(); inner_it++) {
const auto& header_value = inner_it.value().get<std::string>();
std::string header_value;
if (inner_it.value().is_string()) {
header_value = inner_it.value().get<std::string>();
} else {
header_value = inner_it.value().dump();
ben-taussig-solo marked this conversation as resolved.
Show resolved Hide resolved
}
add_response_header(*response_headers, header_key, header_value, stream_filter_callbacks, true);
}
}
Expand Down
2 changes: 0 additions & 2 deletions test/extensions/transformers/aws_lambda/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,5 @@ envoy_gloo_cc_test(
"//api/envoy/config/transformer/aws_lambda/v2:pkg_cc_proto",
"//source/extensions/transformers/aws_lambda:api_gateway_transformer_lib",
"@envoy//test/mocks/http:http_mocks",
"@envoy//test/mocks/server:server_mocks",
"@envoy//test/test_common:utility_lib",
],
)
296 changes: 290 additions & 6 deletions test/extensions/transformers/aws_lambda/transformer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,8 @@
#include "source/extensions/transformers/aws_lambda/api_gateway_transformer.h"

#include "test/mocks/http/mocks.h"
#include "test/mocks/server/mocks.h"
#include "test/test_common/utility.h"

#include "fmt/format.h"
#include "nlohmann/json.hpp"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
Comment on lines -6 to -12
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed (along with the changes to the BUILD file) to improve compilation times for these tests

on my machine, this reduced the time to compile small changes to the already-cached suite from 80s to 60s


using testing::_;
using testing::AtLeast;
Expand Down Expand Up @@ -305,7 +300,296 @@ TEST(ApiGatewayTransformer, error) {
EXPECT_EQ(response_headers.get(Http::LowerCaseString("x-amzn-errortype"))[0]->value().getStringView(), "500");
}

// helper used in multi value headers type safety tests
// - bodyPtr: json payload in the format used by AWS API Gateway/returned from an upstream Lambda
// - expected_error_message: if present, expect that an error message will be logged that contains this string
// - expected_multi_value_header: if present, expect that the first value of the 'test-multi-header' header in the response will be equal to this string,
void test_multi_value_headers_transformation(
std::unique_ptr<Buffer::OwnedImpl> bodyPtr,
std::string expected_error_message = "Error transforming response: [json.exception.type_error.302] type must be string, but is null",
std::string expected_multi_value_header = "multi-value-0,multi-value-1,multi-value-2") {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would prefer to see these as external const values

Http::TestRequestHeaderMapImpl headers{{":method", "GET"},
{":authority", "www.solo.io"},
{"x-test", "789"},
{":path", "/users/123"}};
Http::TestResponseHeaderMapImpl response_headers{};
ApiGatewayTransformer transformer;
NiceMock<Http::MockStreamDecoderFilterCallbacks> filter_callbacks_{};

if (expected_error_message.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would prefer to use std::optional instead of empty strings as sentinel values

transformer.transform(response_headers, &headers, *bodyPtr, filter_callbacks_);
auto lowercase_multi_header_name = Http::LowerCaseString("test-multi-header");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would be nice to move "test-multi-header" to an external const value

auto header_values = response_headers.get(lowercase_multi_header_name);

if (expected_multi_value_header.empty()) {
EXPECT_EQ(header_values.empty(), true);
return;
}

EXPECT_EQ(header_values.empty(), false);
auto str_header_value = header_values[0]->value().getStringView();
EXPECT_EQ(expected_multi_value_header, str_header_value);
} else {
EXPECT_LOG_CONTAINS(
"debug",
expected_error_message,
transformer.transform(response_headers, &headers, *bodyPtr, filter_callbacks_)
);
}
}

// helper used in status code type safety tests
// - bodyPtr: json payload in the format used by AWS API Gateway/returned from an upstream Lambda
// - expected_error_message: if present, expect that an error message will be logged that contains this string
// - expected_status_code: if present, expect that the status code in the response headers will be equal to this string
void test_status_code_transform(
std::unique_ptr<Buffer::OwnedImpl> bodyPtr,
std::string expected_error_message = "Error transforming response: [json.exception.type_error.302] type must be number, but is",
std::string expected_status_code = "200") {
Http::TestRequestHeaderMapImpl headers{{":method", "GET"},
{":authority", "www.solo.io"},
{"x-test", "789"},
{":path", "/users/123"}};
Http::TestResponseHeaderMapImpl response_headers{};
ApiGatewayTransformer transformer;
NiceMock<Http::MockStreamDecoderFilterCallbacks> filter_callbacks_{};

if (expected_error_message.empty()) {
transformer.transform(response_headers, &headers, *bodyPtr, filter_callbacks_);
EXPECT_EQ(expected_status_code, response_headers.getStatusValue());
} else {
EXPECT_LOG_CONTAINS(
"debug",
expected_error_message,
transformer.transform(response_headers, &headers, *bodyPtr, filter_callbacks_)
);
}
}

///////////////////////////////////////////
// multi value headers type safety tests //
///////////////////////////////////////////
TEST(ApiGatewayTransformer, transform_null_multi_value_header) {
test_multi_value_headers_transformation(
std::make_unique<Buffer::OwnedImpl>(R"json({
"multiValueHeaders": {
"test-multi-header": null
}
})json"),
"",
""
);
}

TEST(ApiGatewayTransformer, transform_int_multi_value_header) {
test_multi_value_headers_transformation(
std::make_unique<Buffer::OwnedImpl>(R"json({
"multiValueHeaders": {
"test-multi-header": 123
}
})json"),
"",
"123"
);
}

TEST(ApiGatewayTransformer, transform_float_multi_value_header) {
test_multi_value_headers_transformation(
std::make_unique<Buffer::OwnedImpl>(R"json({
"multiValueHeaders": {
"test-multi-header": 123.456
}
})json"),
"",
"123.456"
);
}

TEST(ApiGatewayTransformer, transform_object_multi_value_header) {
test_multi_value_headers_transformation(
std::make_unique<Buffer::OwnedImpl>(R"json({
"multiValueHeaders": {
"test-multi-header": {"test": "test-value"}
}
})json"),
"Returning error with message: invalid multi header value object",
""
);
}

TEST(ApiGatewayTransformer, transform_list_multi_value_header) {
test_multi_value_headers_transformation(
std::make_unique<Buffer::OwnedImpl>(R"json({
"multiValueHeaders": {
"test-multi-header": ["test-value"]
}
})json"),
"",
"test-value"
);
}

TEST(ApiGatewayTransformer, transform_list_with_null_multi_value_header) {
test_multi_value_headers_transformation(
std::make_unique<Buffer::OwnedImpl>(R"json({
"multiValueHeaders": {
"test-multi-header": ["test-value", null]
}
})json"),
"",
"test-value,null"
);
}

TEST(ApiGatewayTransformer, transform_list_with_int_multi_value_header) {
test_multi_value_headers_transformation(
std::make_unique<Buffer::OwnedImpl>(R"json({
"multiValueHeaders": {
"test-multi-header": ["test-value", 123]
}
})json"),
"",
"test-value,123"
);
}

TEST(ApiGatewayTransformer, transform_list_with_float_multi_value_header) {
test_multi_value_headers_transformation(
std::make_unique<Buffer::OwnedImpl>(R"json({
"multiValueHeaders": {
"test-multi-header": ["test-value", 123.456]
}
})json"),
"",
"test-value,123.456"
);
}

TEST(ApiGatewayTransformer, transform_list_with_object_multi_value_header) {
test_multi_value_headers_transformation(
std::make_unique<Buffer::OwnedImpl>(R"json({
"multiValueHeaders": {
"test-multi-header": ["test-value", {"test": "test-value"}]
}
})json"),
"",
"test-value,{\"test\":\"test-value\"}"
);
}

TEST(ApiGatewayTransformer, transform_list_with_list_multi_value_header) {
test_multi_value_headers_transformation(
std::make_unique<Buffer::OwnedImpl>(R"json({
"multiValueHeaders": {
"test-multi-header": ["test-value", ["test-value"]]
}
})json"),
"",
"test-value,[\"test-value\"]"
);
}

TEST(ApiGatewayTransformer, transform_list_with_list_with_null_multi_value_header) {
test_multi_value_headers_transformation(
std::make_unique<Buffer::OwnedImpl>(R"json({
"multiValueHeaders": {
"test-multi-header": ["test-value", [null]]
}
})json"),
"",
"test-value,[null]"
);
}

///////////////////////////////////
// status code type safety tests //
///////////////////////////////////
TEST(ApiGatewayTransformer, transform_null_status_code) {
test_status_code_transform(
std::make_unique<Buffer::OwnedImpl>(R"json({
"statusCode": null
})json"),
"cannot parse non unsigned integer status code",
""
);
}

TEST(ApiGatewayTransformer, transform_string_status_code) {
test_status_code_transform(
std::make_unique<Buffer::OwnedImpl>(R"json({
"statusCode": "200"
})json"),
"cannot parse non unsigned integer status code",
""
);
}

TEST(ApiGatewayTransformer, transform_string_non_int_status_code) {
test_status_code_transform(
std::make_unique<Buffer::OwnedImpl>(R"json({
"statusCode": "200fasdfasdf"
})json"),
"cannot parse non unsigned integer status code",
""
);
}

TEST(ApiGatewayTransformer, transform_int_status_code) {
test_status_code_transform(
std::make_unique<Buffer::OwnedImpl>(R"json({
"statusCode": 200
})json"),
"",
"200"
);
}

TEST(ApiGatewayTransformer, transform_negative_int_status_code) {
test_status_code_transform(
std::make_unique<Buffer::OwnedImpl>(R"json({
"statusCode": -200
})json"),
"cannot parse non unsigned integer status code",
""
);
}


// note to reviewers: as it stands, this is a breaking change
// we used to support float input for status code (which would be rounded down to the nearest integer)
// this PR propses that we reject float input for status code
TEST(ApiGatewayTransformer, transform_float_status_code) {
test_status_code_transform(
std::make_unique<Buffer::OwnedImpl>(R"json({
"statusCode": 201.6
})json"),
"cannot parse non unsigned integer status code",
""
);
}

TEST(ApiGatewayTransformer, transform_object_status_code) {
test_status_code_transform(
std::make_unique<Buffer::OwnedImpl>(R"json({
"statusCode": {"test": "test-value"}
})json"),
"cannot parse non unsigned integer status code",
""
);
}

TEST(ApiGatewayTransformer, transform_list_status_code) {
test_status_code_transform(
std::make_unique<Buffer::OwnedImpl>(R"json({
"statusCode": ["test-value"]
})json"),
"cannot parse non unsigned integer status code",
""
);
}


} // namespace AwsLambda
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
} // namespace Envoy
Loading