add messageValidationContext into CommonFactoryContext so xDS could u… by stevenzzzz · Pull Request #9621 · envoyproxy/envoy

stevenzzzz · 2020-01-09T04:09:59Z

Signed-off-by: Xin Zhuang stevenzzz@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: add messageValidationContext into CommonFactoryContext so xDS could use the correct messageValidator.

Risk Level: LOW, no behavior change.
Testing: unit test adjusted
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue] #9635

…se dynamic validator Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz · 2020-01-09T15:17:41Z

/assign htuch

htuch · 2020-01-09T19:28:37Z

source/common/router/rds_impl.cc

    RouteConfigProviderManagerImpl& route_config_provider_manager)
    : config_(new ConfigImpl(config, factory_context.getServerFactoryContext(),
-                             factory_context.messageValidationVisitor(), true)),
+                             factory_context.messageValidationContext().staticValidationVisitor(),


I don't think this is right. You can have a static inline route in a dynamic Listener, for example, and we should be applying dynamic validation logic.

revert it.
In the long run, I will probably make Listener validator passed down to RDS and its children object. (so that RDS doesn't depend on Listener).

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

…mote-validator Signed-off-by: Xin Zhuang <stevenzzz@google.com>

htuch · 2020-01-14T16:59:14Z

source/server/filter_chain_manager_impl.h

  envoy::config::core::v3alpha::TrafficDirection direction() const override;
  TimeSource& timeSource() override;
  ProtobufMessage::ValidationVisitor& messageValidationVisitor() override;
+  ProtobufMessage::ValidationContext& messageValidationContext() override;


Why do we have both validation visitor and context as peers?

Why do we have both validation visitor and context as peers?

removing of validator() will be in another PR, lots of files will be modified as I plan to pass the signal "added_via_api" (or a vlidator parameter) down from Listener to everythin.

Trying to figure out how this works.. do you have a draft PR to see the end results? As mentioned off-line, I'd like to:

Thread validators around, vs. context+flag unless the flag is needed for some other purpose.

Ensure that it's easy for consumers to automatically "do the right thing", without having to think too hard, otherwise we get folks picking random validators via copy+paste.

^^ guided the existing design. We need a bunch of comments to explain this interim solution in any case, as it's pretty confusing why both are there right now.

#9779 I have it now.
That PR doesn't include the removal of FactoryContext::messageValidationVisitor() tho.
So if we want to remove that later, we will need to pipe a picked validator from listenerImpl down to HCM::Config.

stale · 2020-01-21T21:55:00Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

stevenzzzz · 2020-01-22T16:35:53Z

still on my Radar. WIP

…

On Tue, Jan 21, 2020 at 4:55 PM stale[bot] ***@***.***> wrote: This pull request has been automatically marked as stale because it has not had activity in the last 7 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! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9621?email_source=notifications&email_token=AA2I6THIRIBGGQULLREM62TQ65VLLA5CNFSM4KESFJZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJRM5YA#issuecomment-576900832>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2I6TGKQRC5EUGOSTDEOV3Q65VLLANCNFSM4KESFJZQ> .

-- Xin Zhuang

junr03 · 2020-01-28T18:59:01Z

will review once merge conflicts updated.

/wait

stevenzzzz · 2020-01-28T19:39:06Z

hi @junr03 could you please look at #9779 instead, so that one has a little bit more change contained, but is more clear reflection on my final goal of "making longlive srds/rds subscriptions".

stale · 2020-02-04T19:52:27Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

stale · 2020-02-11T20:10:44Z

This pull request has been automatically closed because it has not had activity in the last 14 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!

stevenzzzz added 3 commits January 8, 2020 23:09

add messageValidationContext into CommonFactoryContext so xDS could u…

d21e1fd

…se dynamic validator Signed-off-by: Xin Zhuang <stevenzzz@google.com>

fix format

db33091

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

add default action for serverFactoryContext and FactoryContext mocks

e148ac1

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

repokitteh-read-only bot assigned htuch Jan 9, 2020

htuch reviewed Jan 9, 2020

View reviewed changes

htuch added the waiting:any label Jan 9, 2020

use factoryContext.validator for static route config provider

455dc26

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

repokitteh-read-only bot removed the waiting:any label Jan 9, 2020

Merge branch 'master' of https://github.com/envoyproxy/envoy into pro…

f51355e

…mote-validator Signed-off-by: Xin Zhuang <stevenzzz@google.com>

htuch reviewed Jan 14, 2020

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 21, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 22, 2020

stevenzzzz mentioned this pull request Jan 24, 2020

listener, srds, rds, vhds: replace FactoryContext with ServerFactoryContext in router/... #9779

Merged

yanavlasov requested a review from junr03 January 28, 2020 15:28

junr03 self-assigned this Jan 28, 2020

repokitteh-read-only bot added the waiting label Jan 28, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 4, 2020

stale bot closed this Feb 11, 2020

stevenzzzz deleted the promote-validator branch March 4, 2020 02:46

Conversation

stevenzzzz commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenzzzz commented Jan 9, 2020

Uh oh!

htuch Jan 9, 2020

Choose a reason for hiding this comment

Uh oh!

stevenzzzz Jan 9, 2020

Choose a reason for hiding this comment

Uh oh!

htuch Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

stevenzzzz Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

htuch Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

stevenzzzz Jan 22, 2020

Choose a reason for hiding this comment

Uh oh!

stale bot commented Jan 21, 2020

Uh oh!

stevenzzzz commented Jan 22, 2020 via email

Uh oh!

junr03 commented Jan 28, 2020

Uh oh!

stevenzzzz commented Jan 28, 2020

Uh oh!

stale bot commented Feb 4, 2020

Uh oh!

stale bot commented Feb 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stevenzzzz commented Jan 9, 2020 •

edited

Loading