Endpoint metadata support from static config #160

iffyio · 2020-12-14T17:54:21Z

Parses generic yaml key values into a json value and in order to
use as the endpoint's metadata.
Values under the quilkin.dev key is reserved for quilkin usage.
Extracts token into the endpoint object since we don't want to traverse
the json value on each lookup.
Removes name and connection ID from the static Endpoint config struct.

Work on #10
Fixes #152

Bulk of the changes are updates to old references and using the new one, most of the relevant changes are around server/mod.rs, config/ and xds/

Parses generic yaml key values into a json value and in order to use as the endpoint's metadata. Values under the `quilkin.dev` key is reserved for quilkin usage. Extracts token into the endpoint object since we don't want to traverse the json value on each lookup. Removes name and connection ID from the static Endpoint config struct. Work on #10 Fixes #152

markmandel · 2020-12-15T03:16:10Z

src/cluster/mod.rs

@@ -31,6 +32,7 @@ pub(crate) mod cluster_manager {
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct Endpoint {
    pub address: SocketAddr,
+    pub tokens: HashSet<Vec<u8>>,


Should we be capturing tokens at all here, since this seems like it's all handled by the Filters?

If you can define metadata on an Endpoint in the static section - is that not enough? ~~Especially since the key for the token lookup or storage is configurable in TokenRouter?~~ (Edit: got this confused with dynamic metadata)

Should the Filter define which metadata value has all the tokens in it. so it is configurable? Rather than have it be hard-coded? (with a standard default as well)

The main reason was to pre-process it so that tokens are simple and efficient to query. If we treat it as other metadata then the token router or any other filter that needs the info would need to traverse the json structure and scan a list each time.
Re pulling it out of the metadata, we can consider these as special cases since its primarily quilkin that needs them, compared to other metadata that the control plane might pass to a user's custom filter for example.

Should the Filter define which metadata value has all the tokens in it. so it is configurable? Rather than have it be hard-coded? (with a standard default as well)

I'm not following the advantage of having it configurable? if e.g a user filter needs the tokens accessible on a particular field they can still put them under the desired key in the metadata without affecting any quilkin use case but I can't see why one would want it to be configurable if its already present in the endpoint itself?

The main reason was to pre-process it so that tokens are simple and efficient to query.

That makes sense - it's a performance concern. We will need to document how we use xDS really well, and include this 👍

I'm not following the advantage of having it configurable?

That's a fair point. I am wondering if at some point someone might want it to be configurable at the config.yaml file level - basically as a system wide configuration parameter -- but we could also cross that bridge when we come to it.

markmandel · 2020-12-17T00:04:54Z

src/cluster/mod.rs

@@ -31,6 +32,7 @@ pub(crate) mod cluster_manager {
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct Endpoint {
    pub address: SocketAddr,
+    pub tokens: HashSet<Vec<u8>>,


Just a mental note for me "tokens" as a name totally makes sense, since we have a TokenRouter 👍

markmandel · 2020-12-17T00:05:55Z

src/config/endpoints.rs

@@ -14,7 +14,11 @@
 *  limitations under the License.
 */

-use crate::config::EndPoint;
+pub const ENDPOINT_METADATA_KEY_PREFIX: &str = "quilkin.dev";
+pub const ENDPOINT_METADATA_TOKEN_KEY: &str = "endpoint.tokens";


Suggested change

pub const ENDPOINT_METADATA_TOKEN_KEY: &str = "endpoint.tokens";

pub const ENDPOINT_METADATA_TOKEN_KEY: &str = "tokens";

Is "endpoint" redundant? The metadata is on the Endpoint already?

Indeed, fixed!

markmandel · 2020-12-17T00:06:18Z

src/config/endpoints.rs

@@ -28,7 +32,7 @@ pub struct IndexOutOfRangeError;

 /// Endpoints represents the set of all known upstream endpoints.
 #[derive(Clone, Debug, PartialEq)]
-pub struct Endpoints(Arc<Vec<EndPoint>>);
+pub struct Endpoints(Arc<Vec<Endpoint>>);


That a nice naming.

markmandel · 2020-12-17T00:07:45Z

src/config/endpoints.rs

@@ -14,7 +14,11 @@
 *  limitations under the License.
 */

-use crate::config::EndPoint;
+pub const ENDPOINT_METADATA_KEY_PREFIX: &str = "quilkin.dev";


I feel like we have "quilkin.dev" as a prefix in quite a few places around -- do we want to find somewhere central for this? Some kind of centrally located "KEY_PREFIX" const?

The issue preventing reusing currently is const values must be assigned literals in rust so we can't it easily unfortunately, the only other usage today is capturedbytes' quilkin.dev/captured_bytes and we can't concatenate the prefix since that's a const :P not currently sure if there's a workaround atm

src/config/metadata.rs

markmandel · 2020-12-17T00:10:50Z

src/extensions/filter_registry.rs

-            address: addr,
-            connection_ids: vec![],
-        };
+        let endpoint = Endpoint::from_address(addr);


Sooo much cleaner 👍

markmandel · 2020-12-17T00:11:50Z

src/extensions/filters/debug.rs

@@ -103,7 +103,7 @@ impl Filter for Debug {
    }

    fn on_upstream_receive(&self, ctx: UpstreamContext) -> Option<UpstreamResponse> {
-        info!(self.log, "received endpoint packet"; "endpoint" => ctx.endpoint.name.clone(),
+        info!(self.log, "received endpoint packet";


Should we output the endpoint address and port here? Since that would be the unique identifier? Otherwise, we don't know where the information is coming from.

Added socket addres to log message

markmandel · 2020-12-17T00:13:00Z

src/proxy/server/error.rs

@@ -35,6 +36,7 @@ impl Display for Error {
            Error::Initialize(reason) => write!(f, "failed to startup properly: {}", reason),
            Error::Session(inner) => write!(f, "session error: {}", inner),
            Error::Bind(inner) => write!(f, "failed to bind to port: {}", inner),
+            Error::InvalidEndpointConfig(reason) => write!(f, "{}", reason),


Suggested change

Error::InvalidEndpointConfig(reason) => write!(f, "{}", reason),

Error::InvalidEndpointConfig(reason) => write!(f, "invalid endpoint configuration: {}", reason),

🤷 ?

…metadata

markmandel

LGTM!

iffyio requested a review from markmandel December 14, 2020 17:54

google-cla bot added the cla: yes label Dec 14, 2020

markmandel reviewed Dec 15, 2020

View reviewed changes

markmandel reviewed Dec 17, 2020

View reviewed changes

iffyio added 2 commits December 17, 2020 20:47

Rename token key

f2a45eb

Merge remote-tracking branch 'origin/master' into iu/static-endpoint-…

92c3e61

…metadata

markmandel approved these changes Jan 13, 2021

View reviewed changes

markmandel merged commit 0cd6eb4 into master Jan 13, 2021

markmandel deleted the iu/static-endpoint-metadata branch January 13, 2021 00:37

markmandel added area/user-experience Pertaining to developers trying to use Quilkin, e.g. cli interface, configuration, etc kind/feature New feature or request labels Jan 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Endpoint metadata support from static config #160

Endpoint metadata support from static config #160

iffyio commented Dec 14, 2020

markmandel Dec 15, 2020 •

edited

Loading

iffyio Dec 15, 2020

markmandel Dec 17, 2020

markmandel Dec 17, 2020

markmandel Dec 17, 2020

iffyio Dec 17, 2020

markmandel Dec 17, 2020

markmandel Dec 17, 2020

iffyio Dec 17, 2020

markmandel Dec 17, 2020

markmandel Dec 17, 2020

iffyio Dec 17, 2020

markmandel Dec 17, 2020

iffyio Dec 17, 2020

markmandel left a comment

	pub const ENDPOINT_METADATA_TOKEN_KEY: &str = "endpoint.tokens";
	pub const ENDPOINT_METADATA_TOKEN_KEY: &str = "tokens";

	Error::InvalidEndpointConfig(reason) => write!(f, "{}", reason),
	Error::InvalidEndpointConfig(reason) => write!(f, "invalid endpoint configuration: {}", reason),

Endpoint metadata support from static config #160

Endpoint metadata support from static config #160

Conversation

iffyio commented Dec 14, 2020

markmandel Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmandel left a comment

Choose a reason for hiding this comment

markmandel Dec 15, 2020 •

edited

Loading