Use Shriram's env for better upgrade fix.#6894
Conversation
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
CLAs look good, thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: costinm The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## release-1.0 #6894 +/- ##
============================================
- Coverage 71% 71% -<1%
============================================
Files 370 371 +1
Lines 31663 32010 +347
============================================
+ Hits 22305 22540 +235
- Misses 8433 8540 +107
- Partials 925 930 +5
Continue to review full report at Codecov.
|
|
@kyessenov or @Mandar-Jog ? |
| func modifyOutboundRouteConfig(in *plugin.InputParams, httpRoute route.Route) route.Route { | ||
| if !has10Config(in.Node) { | ||
| return httpRoute | ||
| } |
There was a problem hiding this comment.
why this short circuiting? we had per-filter config stuff in 0.8
There was a problem hiding this comment.
Are you sure ? I saw errors on 0.8 accepting routes.
There was a problem hiding this comment.
@qiwzhang / @kyessenov ^^^ .. I distinctly remember that the mixer was the first to get the per-route filter config.
rshriram
left a comment
There was a problem hiding this comment.
LGTM mostly. Not sure about one particular change.
|
Yes, mixer filter supported per route config since last year.
…On Mon, Jul 9, 2018, 7:08 AM Shriram Rajagopalan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pilot/pkg/networking/plugin/mixer/mixer.go
<#6894 (comment)>:
> func modifyOutboundRouteConfig(in *plugin.InputParams, httpRoute route.Route) route.Route {
+ if !has10Config(in.Node) {
+ return httpRoute
+ }
@qiwzhang <https://github.com/qiwzhang> / @kyessenov
<https://github.com/kyessenov> ^^^ .. I distinctly remember that the
mixer was the first to get the per-route filter config.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6894 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJGIxkMYfZHfv4N1ZsURlfkOZwIXxQVTks5uE2PDgaJpZM4VGDKR>
.
|
|
|
After enabling the config we're discussing I got the error above. |
|
Forward attributes in per-route service config is the only new field, not
known to 0.8 envoy.
…On Mon, Jul 9, 2018 at 12:04 PM Costin Manolache ***@***.***> wrote:
After enabling the config we're discussing I got the error above.
How about someone who is familiar with 080 config generation makes a
separate PR to generate whatever attributes are needed - maybe per route
works but only some of the attributes are supported.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6894 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJGIxjGdAbbfk1aHNh8kbPuMSqIIkbr_ks5uE6kngaJpZM4VGDKR>
.
|
|
Ok, will add it back in separate PR, so this part can get into today build. I doubt lots of people will test the upgrade in next build or notice the missing metrics. |
The reminder of the upgrade fix - RDS routes, plus conditionals on the new node environment that
detects 1.0.
With this a 0.8 sidecar will accept all configs from 1.0 pilot - so prod can be upgraded/rolled without traffic lost.
For now just using the presence of the env - in 1.1 we may refine this and use per-plugin or capabilities.
As a reminder, upgrade is critical, there are users with 0.8 in production and in google (and everywhere else) ability to gradually roll a new version is required.