[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: myidpt

Assign the PR to them by writing /assign @myidpt in a comment when ready.

The full list of commands accepted by this bot can be found here.

@ldemailly yes, I would like to patch and test, this is why I asked you to rebase that PR

I checked with Envoy and this is what we are supposed to do, this is the correct Envoy behaviour. We should have done this already. Do you have some arguments to support why this is not a good idea?

that's good points to further explore and refine, but what is the exact downside of that flag (if it works and it does convert even some classes of errors to non errors) applied incrementally ?

Let's test this - if it does indeed reduces the 503 ( I don't think we have 404s any more ) - it's great.
That is what Harvey and Piotr suggested - we may do other things to improve further.

for historical context, here is the chat on envoy-dev I had with Matt on october 5th:

laurent [3:53 PM]
we kind of need a quick fix for those 404s when rds comes before cds, any creative suggestions ? (edited)
(that doesn't involve switching to grpc and ads)
maybe some way for envoy to verify routes are not dead ends and ignore the ones that are dead ends (or trigger a synchronous rds refresh when found such dead end)

mklein [3:56 PM]
put a hack in pilot to add a sleep or something
between refreshes
or sequence them
with sleeps
is that possible?

laurent [3:57 PM]
partially, it doesn't cover the case where we have multiple pilots and one of them is lagging behind in getting updates from k8s
also it 100% delays updates by 1 cycle when the race is client side

mklein [3:58 PM]
“also it 100% delays updates by 1 cycle when the race is client side” what does that mean?

laurent [3:59 PM]
if lucky, the operator makes a change, the changes propagate in the next refresh cycle (say 10s) in the right order
on average it takes 5s for changes to show up with x% of case where it's broken for another 10s
if we delay it'll always take 15s or such for all changes

mklein [4:00 PM]
I’m not sure there is any easy solution here other than switching to ADS if you need atomic/sequenced updates

laurent [4:01 PM]
do you think we can't hack (in our branch maybe if needed) a "validation" to rds that checks the cds is there and spew unhappyness in the logs about bad routes being ignored for now, instead of 404s to the user ? (edited)
I have not looked at envoy code or architecture so my apologies if what I say is complete nonsense
asking for your advice on anything creative that would be quick (as a side note someone here mentioned it should be 503 and not 404 to avoid end user app making a decision that something is truly gone)

mklein [4:03 PM]
unfortunately it doesn’t really work that way. All updates inside Envoy are atomic. There is no going back
by the time you know it’s bad, there is nothing to go back to

laurent [4:04 PM]
if the json is not formatted well; you don't apply it right ? so can't that validation do a bit more semantic-ish validation (even if the change can be the other direction, the cds could disappear, but just trying to address the most common, observed issue not cover 100% correctness) (edited)

mklein [4:05 PM]
ah
yes
there is a hack here you can do
one sec and I have you covered
brb

laurent [4:05 PM]
woot - thx, take your time.

mklein [4:08 PM]
@laurent see here: https://envoyproxy.github.io/envoy/configuration/http_conn_man/route_config/route_config.html#config-http-conn-man-route-table
set “validate_clusters” to true in your response
for RDS it defaults to false
if you set to true, envoy will reject update
and will keep using current route table

laurent [4:09 PM]
oh cool ! it's already there ! fantastic
let me get a test repro'ing the user report and then try this, that's awesome

If it reduces / eliminates 503, without breaking routes - it's a good change.
We have eventual consistency - some delays are ok, next RDS update will take effect.

For now - this seem the best way to improve the 503 situation. It may not be perfect,
and we may need additional changes in envoy or pilot for 0.7 (for example delay the
RDS on pilot side) - but I don't see any clear harm.

Why wouldn't we validate_cluster ? If the cluster is invalid/missing, what's the point in accepting the route ?

Yes, Matt's advice to sequence RDS update is also possible if this one doesn't solve all 503.
We can easily delay the clearCache for routes - it's better to have correct behavior slower
than 503s.

Again, this doesn't eliminate the problem. In lyft's world, clusters are static. You don't create new ones dynamically. What it does, it replaces with two problems 1) slower effective RDS updates 2) 404/503 prior to EDS refresh. It's a trade-off to choose which set of problems you like.

How is it a fix if it does not solve the problem of inconsistent xDS config?

If after this change we don't see 503 errors - it is a fix for the 503 bug, which affects real users.
I don't know what other problem you have regarding xDS - it is eventually consistent, so a future
RDS update will get the new route.

Kuat: have you tested with this option on and noticed any failure ?

I understand you believe ADS is the only possible solution - but if this or sequencing
the RDS/CDS improves things, I think it is still a win and there is no valid reason to block this.

Please provide some evidence this has positive effect and a flag to control the behavior. I would also want to limit this to ingress since mesh-internal traffic we have client under control.

Mesh internal traffic can also see invalid clusters.

Laurent has a test reproducing 503 - so we'll test with it. If the fix solves it - I don't think
we need a 'return 503' (or invalid_clusters) flag is really needed.

I think this deserves a flag since it can have some serious consequences. I'm thinking that envoy-to-envoy there are better ways to hide these 503s.

for anyone who want to test/compare : this PR's pilot:

        image: gcr.io/istio-testing/pilot:31c44885a0825713110bc92c5b6d85235e26a767

vs

        image: gcr.io/istio-release/pilot:0.6.0-pre20180213-09-15-00

I'm about to test it with #1041

Codecov Report

❗ No coverage uploaded for pull request base (master@e6d05c5). Click here to learn what that means.
The diff coverage is 62%.

@@           Coverage Diff            @@
##             master   #3482   +/-   ##
========================================
  Coverage          ?     75%           
========================================
  Files             ?     303           
  Lines             ?   27538           
  Branches          ?       0           
========================================
  Hits              ?   20612           
  Misses            ?    5593           
  Partials          ?    1333

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6d05c5...65622f8. Read the comment docs.

Closing in favor of #4024 , since the set of files changed got messed up by the merge.

I don't see validate_cluster=true in #4024 (which seems to be about the timeouts?)

@andraxylia: The following tests failed, say /retest to rerun them all:

Rebase messed up the set of files, closing this PR and opening a new one from the same branch #4031

looks like it's #4039 now