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.

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

xds: Add a KeyValueStore-backed xDS delegate extension to contrib #77

Draft
wants to merge 1 commit into
base: xds_persistent_factory
Choose a base branch
from

Conversation

abeyad
Copy link
Owner

@abeyad abeyad commented Aug 7, 2022

An xDS delegate extension point was added in
envoyproxy#22473 to enable custom behavior
upon receiving and loading xDS resources. This change creates an
implementation of the XdsResourcesDelegate interface that is backed by a
KeyValueStore.

The intended use case is to enable persisting xDS resources and loading
them on startup in Envoy Mobile, in the event that the xDS control plane
is unreachable.

Signed-off-by: Ali Beyad abeyad@google.com

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

An xDS delegate extension point was added in
envoyproxy#22473 to enable custom behavior
upon receiving and loading xDS resources.  This change creates an
implementation of the XdsResourcesDelegate interface that is backed by a
KeyValueStore.

The intended use case is to enable persisting xDS resources and loading
them on startup in Envoy Mobile, in the event that the xDS control plane
is unreachable.

Signed-off-by: Ali Beyad <abeyad@google.com>
Copy link

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

very cool! only high level comments (and looking forward to your TODO for comments)

// distinct key namespace.
constexpr char KEY_PREFIX[] = "XDS_CONFIG";
// The delimiter between parts of the key.
constexpr char DELIMITER[] = "*+";

Choose a reason for hiding this comment

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

definitely worth checking if there's constraints on prefs key naming where + or URL characters won't work (if we have to base64 encode or some such)

const std::string filename = TestEnvironment::temporaryPath("dns_cache.txt");
::unlink(filename.c_str());

return fmt::format(R"EOF(

Choose a reason for hiding this comment

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

in general we want to addconfigModifiers or use prior const config


std::string bootstrapConfig() {
const std::string filename = TestEnvironment::temporaryPath("dns_cache.txt");
::unlink(filename.c_str());

Choose a reason for hiding this comment

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

yaaas. Came here to make this comment, and you already did it, haha

static constexpr char RTDS_CLUSTER_NAME[] = "rtds_cluster";
static constexpr char CLIENT_CERT_NAME[] = "client_cert";

std::string bootstrapConfig() {

Choose a reason for hiding this comment

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

I do think this is worth unit tests as well, as they're easier for testing updates, eviction etc.

@abeyad
Copy link
Owner Author

abeyad commented Aug 17, 2022

/cc @goaway FYI, still a WIP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants