Skip to content

rate_limit_quota: [API] Support Envoy node info in RateLimitQuotaUsageReports#24014

Closed
tyxia wants to merge 5 commits intoenvoyproxy:mainfrom
tyxia:rlqs_envoy_node
Closed

rate_limit_quota: [API] Support Envoy node info in RateLimitQuotaUsageReports#24014
tyxia wants to merge 5 commits intoenvoyproxy:mainfrom
tyxia:rlqs_envoy_node

Conversation

@tyxia
Copy link
Copy Markdown
Member

@tyxia tyxia commented Nov 15, 2022

Even though our (Google) RLQS server doesn't need this, there are needs/use cases from other RLQS customers that the RQLS server will receive reports from many envoy nodes across various regions, clusters and tenants. And the customers will use this Envoy node which contains all of this information to distinguish the tenant/region/cluster and to apply the appropriate rate limiting policies.

This PR addresses #23585.

Signed-off-by: tyxia tyxia@google.com

Risk Level: LOW
Testing: N/A

…geReports`

Signed-off-by: tyxia <tyxia@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24014 was opened by tyxia.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #24014 was opened by tyxia.

see: more, trace.

Comment thread api/envoy/service/rate_limit_quota/v3/rlqs.proto Outdated
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Nov 16, 2022

/retest

deflake

@repokitteh-read-only
Copy link
Copy Markdown

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

🐱

Caused by: a #24014 (comment) was created by @tyxia.

see: more, trace.

@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Nov 16, 2022

@htuch Thanks for the review! PTAL.

(The CI failure is caused by CI/CD infra that one agent is not responding... I will re-test it before submitting this PR).

@tyxia tyxia marked this pull request as ready for review November 16, 2022 15:41
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Nov 16, 2022

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #24014 (comment) was created by @tyxia.

see: more, trace.

@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Nov 17, 2022

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #24014 (comment) was created by @tyxia.

see: more, trace.

Copy link
Copy Markdown
Member

@htuch htuch 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

@htuch
Copy link
Copy Markdown
Member

htuch commented Nov 18, 2022

@tyxia do you want to fold in the implementation PR into this one? I'm pretty sure that combined this is a small PR.

@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Nov 18, 2022

@tyxia do you want to fold in the implementation PR into this one? I'm pretty sure that combined this is a small PR.

Yea, the implementation should be pretty straightforward. We just need to set this info by the Node from localInfo.
The only issue is that the baseline impl PR (#22135 ) has not been merged yet, so there is really no implementation code to add at this point :)
We can either 1) check in this API proto change first 2) Or hold it off until baseline impl is merged. Either works for me

@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Nov 19, 2022

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #24014 (comment) was created by @tyxia.

see: more, trace.

@htuch
Copy link
Copy Markdown
Member

htuch commented Nov 21, 2022

Yep, I suggest merging in the implementation with this PR and let's review together.

@jmarantz
Copy link
Copy Markdown
Contributor

/wait

@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 24, 2022
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Dec 28, 2022

/wait

I will update this PR once other parts of RLQS are ready.

@github-actions github-actions Bot removed the stale stalebot believes this issue/PR has not been touched recently label Dec 28, 2022
@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 27, 2023
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Jan 27, 2023

I will work on this once other parts of impl are merged. Then I will merge corresponding impl into this proto change, as Harvey suggested.

@github-actions github-actions Bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 28, 2023
@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 27, 2023
@zuercher
Copy link
Copy Markdown
Member

Is this PR still in progress?

@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Feb 27, 2023

Is this PR still in progress?

Yes, this PR is currently on-hold to wait for other pieces of project to be merged first. But I will revisit this PR soon.

@github-actions github-actions Bot removed the stale stalebot believes this issue/PR has not been touched recently label Feb 27, 2023
@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 29, 2023
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 6, 2023

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot closed this Apr 6, 2023
@tyxia tyxia deleted the rlqs_envoy_node branch December 26, 2024 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api stale stalebot believes this issue/PR has not been touched recently waiting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants