Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions source/common/common/logger.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ namespace Logger {
// clang-format off
#define ALL_LOGGER_IDS(FUNCTION) \
FUNCTION(admin) \
FUNCTION(aws) \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there any reason not to use http?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Given where the code is now, it could just be classified as http. I'm fine doing that. :)

FUNCTION(assert) \
FUNCTION(backtrace) \
FUNCTION(client) \
Expand Down
47 changes: 47 additions & 0 deletions source/extensions/filters/http/common/aws/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
licenses(["notice"]) # Apache 2

load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_library",
"envoy_package",
)

envoy_package()

envoy_cc_library(
name = "signer_lib",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: if this is going to only ever be a pure interface, signer_interface would be more consistent with other conventions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good to know. I thought that convention was only used the public headers. e.g. include/envoy.

hdrs = ["signer.h"],
deps = [
"//include/envoy/http:message_interface",
],
)

envoy_cc_library(
name = "signer_impl_lib",
srcs = ["signer_impl.cc"],
hdrs = ["signer_impl.h"],
external_deps = ["ssl"],
deps = [
":credentials_provider_lib",
":region_provider_lib",
":signer_lib",
"//source/common/buffer:buffer_lib",
"//source/common/common:assert_lib",
"//source/common/common:hex_lib",
"//source/common/common:logger_lib",
"//source/common/common:stack_array",
"//source/common/common:utility_lib",
"//source/common/http:headers_lib",
],
)

envoy_cc_library(
name = "credentials_provider_lib",
hdrs = ["credentials_provider.h"],
external_deps = ["abseil_optional"],
)

envoy_cc_library(
name = "region_provider_lib",
hdrs = ["region_provider.h"],
)
66 changes: 66 additions & 0 deletions source/extensions/filters/http/common/aws/credentials_provider.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
#pragma once

#include <string>

#include "envoy/common/pure.h"

#include "absl/types/optional.h"

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

class Credentials {
public:
Credentials() = default;

~Credentials() = default;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: probably don't need this if we're not doing polymorphic inheritance.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the destructor, but I do use the no-args constructor.


Credentials(const std::string& access_key_id, const std::string& secret_access_key)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: we tend to use absl::string_view rather than const std::string& in new code, as it's strictly more general. You can still turn this into a string copy below.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

clang-tidy will suggest pass by value here and std::move below for constructors like this.
https://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html

By doing this you may save one copy if caller does move too.

: access_key_id_(access_key_id), secret_access_key_(secret_access_key) {}

Credentials(const std::string& access_key_id, const std::string& secret_access_key,
const std::string& session_token)
: access_key_id_(access_key_id), secret_access_key_(secret_access_key),
session_token_(session_token) {}

void setAccessKeyId(const std::string& access_key_id) {
access_key_id_ = absl::optional<std::string>(access_key_id);
}

const absl::optional<std::string>& accessKeyId() const { return access_key_id_; }

void setSecretAccessKey(const std::string& secret_key) {
secret_access_key_ = absl::optional<std::string>(secret_key);
}

const absl::optional<std::string>& secretAccessKey() const { return secret_access_key_; }

void setSessionToken(const std::string& session_token) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need both setters and constructors for these fields? Could this be an immutable object?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't need them. I can remove them.

session_token_ = absl::optional<std::string>(session_token);
}

const absl::optional<std::string>& sessionToken() const { return session_token_; }

private:
absl::optional<std::string> access_key_id_;
absl::optional<std::string> secret_access_key_;
absl::optional<std::string> session_token_;
};

class CredentialsProvider {
public:
virtual ~CredentialsProvider() = default;

virtual Credentials getCredentials() PURE;
};

typedef std::shared_ptr<CredentialsProvider> CredentialsProviderSharedPtr;

} // namespace Aws
} // namespace Common
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
26 changes: 26 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,26 @@
#pragma once

#include "envoy/common/pure.h"

#include "absl/types/optional.h"

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

class RegionProvider {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had a look at how this works in #5546. This allows both static and environment to specify region. I guess my main question is "do we really need this?". Specifically, Envoy's Node ID includes locality information (and a variety of ways to specify, from bootstrap config to CLI). Can we pull region from that in the interest of economy of configuration mechanism and making this more consistent with the rest of Envoy config?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My intention was to eventually add a region provider impl that pulls the region from an AWS-endpoint, much like we pull the credentials.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the larger point is "shouldn't we be synthesizing the Node locality in the bootstrap or CLI for Envoys based on the AWS endpoint data source?" If you do that, there is no need to manage data sources for this information in the extension.

Credentials can also be placed in Node metadata, but the limitation there is that if they change dynamically during the lifetime of the Envoy, they can become stale. For region, this should be static for the lifetime of an Envoy.

@lavignes lavignes Jan 15, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for not addressing that larger point. I think it makes sense. I'll pipe the node metadata through in the later PR. I'll eliminate this class, especially since it will probably just provide static data for the foreseeable future.

public:
virtual ~RegionProvider() = default;

virtual absl::optional<std::string> getRegion() PURE;
};

typedef std::shared_ptr<RegionProvider> RegionProviderSharedPtr;

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

#include "envoy/common/pure.h"
#include "envoy/http/message.h"

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

class Signer {
public:
virtual ~Signer() = default;

/**
* Sign an AWS request.
* @param message an
*/
virtual void sign(Http::Message& message) const PURE;
};

typedef std::shared_ptr<Signer> SignerSharedPtr;

} // namespace Aws
} // namespace Common
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
Loading