Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions source/common/common/logger.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ namespace Envoy {
namespace Logger {

// clang-format off
// TODO: find out a way for extensions to register new logger IDs
#define ALL_LOGGER_IDS(FUNCTION) \
FUNCTION(admin) \
FUNCTION(aws) \
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a TODO to figure out some way for extensions to register new logger IDs?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't been looking at these PRs but just wanted to note that in #5580 we decided to not add aws: #5580 (comment)

So I'm surprised this got back in. @ivitjuk It would make sense to have the sigv4 signer use it again since the logging the signer does it very AWS-specific.

Copy link
Member Author

@ivitjuk ivitjuk Jun 20, 2019

Choose a reason for hiding this comment

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

Makes sense to me. I'll make a PR to switch signer to the AWS logger.

FUNCTION(assert) \
FUNCTION(backtrace) \
FUNCTION(client) \
Expand Down
16 changes: 16 additions & 0 deletions source/extensions/filters/http/common/aws/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,19 @@ envoy_cc_library(
"//source/common/http:headers_lib",
],
)

envoy_cc_library(
name = "region_provider_interface",
hdrs = ["region_provider.h"],
external_deps = ["abseil_optional"],
)

envoy_cc_library(
name = "region_provider_impl_lib",
srcs = ["region_provider_impl.cc"],
hdrs = ["region_provider_impl.h"],
deps = [
":region_provider_interface",
"//source/common/common:logger_lib",
],
)
33 changes: 33 additions & 0 deletions source/extensions/filters/http/common/aws/region_provider.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#pragma once

#include "envoy/common/pure.h"

#include "absl/types/optional.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace Common {
namespace Aws {

/**
* Interface for classes capable of discovering the AWS region from the execution environment.
*/
class RegionProvider {
public:
virtual ~RegionProvider() = default;

/**
* Discover and return the AWS region.
* @return AWS region, or nullopt if unable to discover the region.
*/
virtual absl::optional<std::string> getRegion() PURE;
};

typedef std::shared_ptr<RegionProvider> RegionProviderSharedPtr;

} // namespace Aws
} // namespace Common
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
30 changes: 30 additions & 0 deletions source/extensions/filters/http/common/aws/region_provider_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#include "extensions/filters/http/common/aws/region_provider_impl.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace Common {
namespace Aws {

static const char AWS_REGION[] = "AWS_REGION";

StaticRegionProvider::StaticRegionProvider(const std::string& region) : region_(region) {}

absl::optional<std::string> StaticRegionProvider::getRegion() {
return absl::optional<std::string>(region_);
}

absl::optional<std::string> EnvironmentRegionProvider::getRegion() {
const auto region = std::getenv(AWS_REGION);
if (region == nullptr) {
return absl::nullopt;
}
ENVOY_LOG(debug, "Found environment region {}={}", AWS_REGION, region);
return absl::optional<std::string>(region);
}

} // namespace Aws
} // namespace Common
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
38 changes: 38 additions & 0 deletions source/extensions/filters/http/common/aws/region_provider_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#pragma once

#include "common/common/logger.h"

#include "extensions/filters/http/common/aws/region_provider.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace Common {
namespace Aws {

/**
* Retrieve AWS region name from the environment
*/
class EnvironmentRegionProvider : public RegionProvider, public Logger::Loggable<Logger::Id::aws> {
public:
absl::optional<std::string> getRegion() override;
};

/**
* Return statically configured AWS region name
*/
class StaticRegionProvider : public RegionProvider {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, use-case wise, can you explain more about the difference or the need of this Static vs Environment RegionProvider?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, please check the closed PR we are trying to break into smaller pieces here:

https://github.com/envoyproxy/envoy/pull/5546/files#diff-18af3435579912d71a19beaecd97d832R42

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I think to provide inline comments here probably helpful?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added documentation at the class level.

public:
StaticRegionProvider(const std::string& region);

absl::optional<std::string> getRegion() override;

private:
const std::string region_;
};

} // namespace Aws
} // namespace Common
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
9 changes: 9 additions & 0 deletions test/extensions/filters/http/common/aws/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,12 @@ envoy_cc_test(
"//test/test_common:utility_lib",
],
)

envoy_cc_test(
name = "region_provider_impl_test",
srcs = ["region_provider_impl_test.cc"],
deps = [
"//source/extensions/filters/http/common/aws:region_provider_impl_lib",
"//test/test_common:environment_lib",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#include "extensions/filters/http/common/aws/region_provider_impl.h"

#include "test/test_common/environment.h"

#include "gtest/gtest.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace Common {
namespace Aws {

class EnvironmentRegionProviderTest : public testing::Test {
public:
virtual ~EnvironmentRegionProviderTest() { TestEnvironment::unsetEnvVar("AWS_REGION"); }

EnvironmentRegionProvider provider_;
};

class StaticRegionProviderTest : public testing::Test {
public:
StaticRegionProviderTest() : provider_("test-region") {}

StaticRegionProvider provider_;
};

TEST_F(EnvironmentRegionProviderTest, SomeRegion) {
TestEnvironment::setEnvVar("AWS_REGION", "test-region", 1);
EXPECT_EQ("test-region", provider_.getRegion().value());
}

TEST_F(EnvironmentRegionProviderTest, NoRegion) { EXPECT_FALSE(provider_.getRegion().has_value()); }

TEST_F(StaticRegionProviderTest, SomeRegion) {
EXPECT_EQ("test-region", provider_.getRegion().value());
}

} // namespace Aws
} // namespace Common
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
14 changes: 12 additions & 2 deletions test/test_common/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ void TestEnvironment::setEnvVar(const std::string& name, const std::string& valu
if (!overwrite) {
size_t requiredSize;
const int rc = ::getenv_s(&requiredSize, nullptr, 0, name.c_str());
ASSERT_EQ(rc, 0);
ASSERT_EQ(0, rc);
if (requiredSize != 0) {
return;
}
Expand All @@ -324,7 +324,17 @@ void TestEnvironment::setEnvVar(const std::string& name, const std::string& valu
ASSERT_EQ(0, rc);
#else
const int rc = ::setenv(name.c_str(), value.c_str(), overwrite);
ASSERT_EQ(rc, 0);
ASSERT_EQ(0, rc);
#endif
}

void TestEnvironment::unsetEnvVar(const std::string& name) {
#ifdef WIN32
const int rc = ::_putenv_s(name.c_str(), "");
ASSERT_EQ(0, rc);
#else
const int rc = ::unsetenv(name.c_str());
ASSERT_EQ(0, rc);
#endif
}

Expand Down
5 changes: 5 additions & 0 deletions test/test_common/environment.h
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,11 @@ class TestEnvironment {
* Set environment variable. Same args as setenv(2).
*/
static void setEnvVar(const std::string& name, const std::string& value, int overwrite);

/**
* Removes environment variable. Same args as unsetenv(3).
*/
static void unsetEnvVar(const std::string& name);
};

} // namespace Envoy