Skip to content

thrift: configurable validate_clusters to override the default cluste…#20874

Merged
htuch merged 6 commits into
envoyproxy:mainfrom
JuniorHsu:configValidateClusters
Apr 27, 2022
Merged

thrift: configurable validate_clusters to override the default cluste…#20874
htuch merged 6 commits into
envoyproxy:mainfrom
JuniorHsu:configValidateClusters

Conversation

@JuniorHsu
Copy link
Copy Markdown
Contributor

…r validation

Signed-off-by: kuochunghsu kuochunghsu@pinterest.com

Commit Message:
This is a continuation of #20577.

Additional Description:
Risk Level: low
Testing: unit test
Docs Changes: proto
Release Notes: current.rst

…r validation

Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@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: #20874 was opened by JuniorHsu.

see: more, trace.

Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu
Copy link
Copy Markdown
Contributor Author

cc @rgs1 @tkovacs-2

Copy link
Copy Markdown
Member

@rgs1 rgs1 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Just a few nits.

"unknown thrift cluster");
}

TEST_F(ThriftConnectionManagerTest, UnknownClusterWithNegativeValidateClusters) {
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: UnknownClusterWithValidateClustersDisabled

"unknown thrift weighted cluster");
}

TEST_F(ThriftConnectionManagerTest, UnknownWeightedClusterWithNegativeValidateClusters) {
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: UnknownWeightedClusterWithValidateClustersDisabled

"unknown thrift shadow cluster");
}

TEST_F(ThriftConnectionManagerTest, UnknownMirrorPolicyClusterWithNegativeValidateClusters) {
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: UnknownMirrorPolicyClusterWithValidateClustersDisabled

Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
@JuniorHsu JuniorHsu requested a review from rgs1 April 19, 2022 19:02
rgs1
rgs1 previously approved these changes Apr 19, 2022
Copy link
Copy Markdown
Member

@rgs1 rgs1 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Over to @zuercher for merging.

Comment thread api/envoy/extensions/filters/network/thrift_proxy/v3/route.proto
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.

API LGTM modulo nit.

Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
// :ref:`trds
// <envoy_v3_api_field_extensions.filters.network.thrift_proxy.v3.ThriftProxy.trds>`
// option. Users may wish to override the default behavior in certain cases (for example when
// using TRDS with a static route table).
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.

@htuch
What was wrong with the CDS in this comment?
In this form it is meaningless, TRDS means dynamic routing table.

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.

Sorry, you're right, let's revert.

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.

@htuch a gentle ping for another api modulo review

This reverts commit 6e04d54.

Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>
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

Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

I went ahead and resolved the trivial merge conflict. I'd hoped that would revoke all the previous approvals, but it seems to have.

@JuniorHsu
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 #20874 (comment) was created by @JuniorHsu.

see: more, trace.

@htuch
Copy link
Copy Markdown
Member

htuch commented Apr 27, 2022

/lgtm api

@htuch htuch merged commit 0be448e into envoyproxy:main Apr 27, 2022
@JuniorHsu JuniorHsu deleted the configValidateClusters branch April 27, 2022 04:33
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
envoyproxy#20874)

This is a continuation of envoyproxy#20577.

Additional Description:
Risk Level: low
Testing: unit test

Signed-off-by: kuochunghsu <kuochunghsu@pinterest.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.

5 participants