Skip to content

ext_authz: add support for passing destination service labels#16955

Merged
lizan merged 20 commits intoenvoyproxy:mainfrom
ramaraochavali:fix/authz_labels
Jul 12, 2021
Merged

ext_authz: add support for passing destination service labels#16955
lizan merged 20 commits intoenvoyproxy:mainfrom
ramaraochavali:fix/authz_labels

Conversation

@ramaraochavali
Copy link
Copy Markdown
Contributor

Commit Message:add support for passing destination service labels
Additional Description: add support for passing destination service labels
Risk Level: Low
Testing: Updated
Docs Changes: Updated
Release Notes: Added
Fixes #16898

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16955 was opened by ramaraochavali.

see: more, trace.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Copy Markdown
Contributor Author

@lizan can you PTAL when you get chance?


// Optional labels that will be passed to :ref:`labels<envoy_v3_api_field_service.auth.v3.AttributeContext.Peer.labels>` in
// :ref:`destination<envoy_v3_api_field_service.auth.v3.AttributeContext.destination>` if provided.
map<string, string> destination_labels = 15;
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 you expect these fields to be set at per route config so ext_authz get different labels per route? I'm thinking if we should use cluster metadata or something else instead of directly pass this in the 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.

Right now there is no such use case. But we never know. Using cluster metadata seems an interesting idea. What is the advantage of using cluster metadata than this? Is n't cluster metadata need to be configured for every service if the choice of using ext authz is done by few services using envoy filter or request authentication (in Istio world)? Are there any downsides of specifying in config?

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.

Just thinking loud, I was assuming this labels would be overlapping with cluster metadata so it blows the config size in LDS/RDS. I'd rather put them in a generic place but ext_authz specific.

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.

Another idea I was thinking was since these are destination labels - Could we use bootstrap metadata some how? If labels are there in bootstrap metadata - we just pass it - May be we will have to agree on some standard 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.

Another idea I was thinking was since these are destination labels - Could we use bootstrap metadata some how? If labels are there in bootstrap metadata - we just pass it - May be we will have to agree on some standard key?

Yeah that's my confusing point as well, if bootstrap metadata works why not use them? We can agree on standard key, or make the key configurable here.

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.

Ok. I think that makes sense to me. Should we start with agree on a key for now and if we need add config in future here?

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.

Should we standardize on LABELS as the key to start with?

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.

@lizan What do you think about adding a config "bootstrap_metadata_labels_key" and pass labels from that key?

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Copy Markdown
Contributor Author

@lizan moved to use bootstrap metadata key. Can you PTAL?

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Copy link
Copy Markdown
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

/lgtm api

Comment on lines +92 to +93
* dns resolver: added *DnsResolverOptions* protobuf message to reconcile all of the DNS lookup option flags. By setting the configuration option :ref:`use_tcp_for_dns_lookups <envoy_v3_api_field_config.core.v3.DnsResolverOptions.use_tcp_for_dns_lookups>` as true we can make the underlying dns resolver library to make only TCP queries to the DNS servers and by setting the configuration option :ref:`no_default_search_domain <envoy_v3_api_field_config.core.v3.DnsResolverOptions.no_default_search_domain>` as true the DNS resolver library will not use the default search domains.
* dns resolver: added *DnsResolutionConfig* to combine :ref:`dns_resolver_options <envoy_v3_api_field_config.core.v3.DnsResolutionConfig.dns_resolver_options>` and :ref:`resolvers <envoy_v3_api_field_config.core.v3.DnsResolutionConfig.resolvers>` in a single protobuf message. The field *resolvers* can be specified with a list of DNS resolver addresses. If specified, DNS client library will perform resolution via the underlying DNS resolvers. Otherwise, the default system resolvers (e.g., /etc/resolv.conf) will be used.
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.

??

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16955 (comment) was created by @ramaraochavali.

see: more, trace.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16955 (comment) was created by @ramaraochavali.

see: more, trace.

@ramaraochavali
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16955 (comment) was created by @ramaraochavali.

see: more, trace.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@lizan lizan merged commit a25ff10 into envoyproxy:main Jul 12, 2021
@ramaraochavali ramaraochavali deleted the fix/authz_labels branch July 13, 2021 03:52
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
…roxy#16955)

Additional Description: add support for passing destination service labels
Risk Level: Low
Testing: Updated
Docs Changes: Updated
Release Notes: Added
Fixes envoyproxy#16898

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
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.

Enhance ext authz filter to pass labels for peer info

4 participants