Skip to content

Revert #6801 - breaks upgrade from 0.8#6848

Merged
costinm merged 1 commit intoistio:release-1.0from
costinm:10-fixes
Jul 4, 2018
Merged

Revert #6801 - breaks upgrade from 0.8#6848
costinm merged 1 commit intoistio:release-1.0from
costinm:10-fixes

Conversation

@costinm
Copy link
Copy Markdown
Contributor

@costinm costinm commented Jul 4, 2018

Reverts change breaking 0.8 upgrade test:

[2018-07-04 14:53:43.296][25][warning][config] bazel-out/k8-opt/bin/external/envoy/source/common/config/_virtual_includes/grpc_mux_subscription_lib/common/config/grpc_mux_subscription_impl.h:70] gRPC config for type.googleapis.com/envoy.api.v2.RouteConfiguration rejected: Unable to parse JSON as proto (INVALID_ARGUMENT:: Cannot find field.): {"mixer_attributes":{"attributes":{"destination.service.host":{"string_value":"istio-pilot.istio-system.svc.cluster.local"},"destination.service":{"string_value":"istio-pilot.istio-system.svc.cluster.local"}}},"disable_check_calls":true,"forward_attributes":{"attributes":{"destination.service":{"string_value":"istio-pilot.istio-system.svc.cluster.local"},"destination.service.host":{"string_value":"istio-pilot.istio-system.svc.cluster.local"}}}}

Can be added back in 1.1 or with some detection or in a backward-compatible way.

@costinm
Revert istio#6801 - breaks upgrade from 0.8
da9d923 
Remove noisy log, makes pilot hard to debug.
@istio-testing istio-testing requested a review from ijsnellf July 4, 2018 15:21
rshriram
rshriram approved these changes Jul 4, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@rshriram rshriram left a comment

Choose a reason for hiding this comment

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

/lgtm

@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Jul 4, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm, rshriram

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@costinm costinm merged commit bfe4526 into istio:release-1.0 Jul 4, 2018
@kyessenov
Copy link
Copy Markdown
Contributor

kyessenov commented Jul 4, 2018

Really? @douglas-reid this is going to break telemetry, and does not even fix the problem with new fields in filter configs...

@kyessenov
Copy link
Copy Markdown
Contributor

kyessenov commented Jul 4, 2018

@hklai we can't be removing features on holidays... Changes like this need to be discussed with TOC and appropriate leads.

@hklai
Copy link
Copy Markdown
Contributor

hklai commented Jul 5, 2018

@kyessenov upgrade from 0.8 is also critical and we cannot break that as well.

By commenting these few lines, are we breaking any 1.0 P0 feature? If so, we really should have a test for it (and this should break that test). Do you have any idea how this can/should be fixed? is it possible to add a version check?

@kyessenov
Copy link
Copy Markdown
Contributor

kyessenov commented Jul 5, 2018 via email

@kyessenov
Copy link
Copy Markdown
Contributor

kyessenov commented Jul 6, 2018

@hklai please revert this. It does not solve the upgrade problem...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@douglas-reid douglas-reid Awaiting requested review from douglas-reid

@ijsnellf ijsnellf Awaiting requested review from ijsnellf

2 more reviewers

@rshriram rshriram rshriram approved these changes

@quanjielin quanjielin quanjielin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rshriram rshriram

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@costinm @istio-testing @kyessenov @hklai @rshriram @quanjielin @googlebot