Fix upgrade. by costinm · Pull Request #6792 · istio/istio

costinm · 2018-07-02T21:23:21Z

Disable backward-incompatible mixer config.

costinm · 2018-07-02T21:25:10Z

kyessenov

/hold
FAIL_CLOSE is not a new attribute.

costinm · 2018-07-02T21:56:09Z

Re 'close': not clear why it is needed (not controlled by anything, not use case to change it), and not clear if it was tested for upgrade. Want to reduce risks.

costinm · 2018-07-02T22:00:54Z

I mean: the mixer client should fail close for policy and open for telemetry, by default.

If we find a need to change the default, and define an API for this - we can start using the mixer plugin in pilot to generate the attribute. Until this happens - there is not point in pilot generating redundant config,
and no point in mixerclient having a different default. So this change is not needed, and won't be needed until the API is defined.

kyessenov · 2018-07-02T22:03:41Z

Per @liminw we need FAIL_CLOSE for a usable RBAC. This is not a new field, so it should not cause problems.

kyessenov · 2018-07-02T22:04:12Z

Please remove FAIL_CLOSE conditioned on a variable. The rest looks fine.

…ent in this build, we can remove in next

codecov · 2018-07-02T22:12:05Z

Codecov Report

Merging #6792 into master will decrease coverage by 1%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #6792    +/-   ##
=======================================
- Coverage      71%     71%   -<1%     
=======================================
  Files         367     367            
  Lines       31488   31477    -11     
=======================================
- Hits        22260   22233    -27     
- Misses       8334    8356    +22     
+ Partials      894     888     -6

Impacted Files	Coverage Δ
mixer/adapter/solarwinds/log_handler.go	`58% <0%> (-22%)`	⬇️
mixer/adapter/statsd/statsd.go	`93% <0%> (-2%)`	⬇️
pilot/pkg/serviceregistry/kube/controller.go	`66% <0%> (-2%)`	⬇️
mixer/adapter/prometheus/server.go	`95% <0%> (-1%)`	⬇️
mixer/adapter/opa/opa.go	`79% <0%> (-1%)`	⬇️
mixer/pkg/protobuf/yaml/resolver.go	`59% <0%> (-1%)`	⬇️
mixer/adapter/list/list.go	`100% <0%> (ø)`	⬇️
istioctl/cmd/istioctl/proxystatus.go	`7% <0%> (ø)`	⬇️
mixer/adapter/dogstatsd/dogstatsd.go	`100% <0%> (ø)`	⬆️
mixer/adapter/stdio/stdio.go	`100% <0%> (ø)`	⬆️
... and 11 more

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 e5a09e9...2276b95. Read the comment docs.

costinm · 2018-07-02T22:13:24Z

Added back for now the fail close - it is not the right way to enable RBAC, please fix it in mixerclient code and remove it from pilot. Policy and RBAC can't fail open - there is no reason for mixerclient to default to open, and no need for pilot to send redundant configs.

kyessenov · 2018-07-02T22:18:18Z

pilot/pkg/networking/plugin/mixer/mixer.go

 		CheckCluster:  model.BuildSubsetKey(model.TrafficDirectionOutbound, "", model.Hostname(policy), port),
 		ReportCluster: model.BuildSubsetKey(model.TrafficDirectionOutbound, "", model.Hostname(telemetry), port),
+	}
+	// TODO(yangminzhu): remove this after the default on client code is changed.


This is no longer possible due to backwards compatibility constraints. It will fail open for the foreseeable future if we want to maintain compatibility between older and newer envoys.

We don't want to maintain fail open - there is no such backward compat constraint.

Just that new envoy works properly ( fail close ).

kyessenov

Wouldn't hurt to document new environment variables (or even a better way to configure plugins).

/lgtm

istio-testing · 2018-07-02T22:19:24Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pilot/pkg/networking/plugin/mixer/OWNERS~~ [costinm,kyessenov]

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

costinm · 2018-07-02T23:05:26Z

Lint failure in file not touched by change, probably metalinter issue to be resolved by separate PR.

* Introducing mcpc, a command-line mcp client tool. (#6715) * Set default requests cpu resource to all deployments (#6229) * Set default requests cpu resource to all deployments * Description added * Requested CPU decreased to meet test env node capacity * Setting requested cpu to 10m as most k8s platforms are defaulting to 0m * Merge master remains * Apply the global resources to the new gateways * Updated goldens to cpu 10m * Clean up and rename the global default resources * Redundant Redis servers were removed + defer in a loop is not recommended. (#6768) * Fix upgrade. (#6792) * Fix upgrade. * Format * Move back the fail close - not clear if we have time to get mixer client in this build, we can remove in next * Add configuration to allow transformation of durations to integers. (#5416) The fluentd handler currently transforms durations to their string representation for log messages. This commit adds a configuration to override this default behaviour and tranform durations to integers of unit millisecond. * Add workload and service dashboard (#6789) * Add workload and service dashboard * fix typo * Skip service dash e2e test for now * Add rushed, missed workload dash * Touch up * Commenting out test until can replicate locally and fix * simplify fmt script (#6798) * simplify fmt script - goimports is a superset of gofmt - shouldn't need to remove spaces - now utilizing -l flag to tell us if there is a diff * always update gofmt to latest * spacing that is now considered incorrect * support hash based routing for soft session affinity (#6742) * support sticky sessions * fmt :( * update servicediscovery fakes * bump istio/api * use updated api for ring hash * support session affinity for sidecar * update destination rule yaml to reflect new api * ttl is a string * unset sdsUdsPath in configmap (#6799) * fix bug so that destination.service.** attributes are collected (#6801) * remove unnecessary generated attributes finding. (#6785) * modify docker template files for proxyv2 (#6790) * Long-running testing improvements (#6800) * Add values for config map settings, including access log. More docs. * Updates and improvements for the stress-testing configs. * Add values for config map settings, including access log. (#6797) * Add values for config map settings, including access log. More docs. * Updates and improvements for the stress-testing configs. * Address review comments * Merged wrong files * Add the setup helm file - this change now depend on the previous PR. * Sync with remote, remove accidentally added files. * Another accidental file * SNI routing from sidecar to gateway with virtual services (#6402) * quick sni matching 1st pass with no refactoring of existing code * use shriram's api sha * quick pass at using tls block * add some validation * copyright * fix lint + remove deadcode * rename protocol tcp_tls -> tls * update back to istio/api master * remove accidentally added test file * add tls block to gateway logic * add todos * basic sni wildcard implementation * add tcp, fix problems with rbac, matching * better tcp + tls validation * address code review comments * remove out of date comment * update comments * fix compile error * use tcp proxy in tcp routing * add tcp routing e2e test * add forgotten vs config file + update description of test * Comments, bug fixes Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * cleanup gateway tcp test Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * moving networking test yamls Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * tcp/tls tests Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * yaml fixes Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * fix file switcheroo * port matches Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * fix authN plugin overwriting TLS context Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * more tests - route via egress gateway Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * yaml fixes Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * initialize prom variables Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * split tests Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * final test fix hopefully Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * revert gateway tweaks Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * Add myself back to OWNERS, sort the list alphabetically. (#6830) * Upload cloudfoundry test result to GCS (#6829) * Fix typos in command-line output.

Fix upgrade.

e00d04c

costinm requested review from andraxylia, douglas-reid and hklai July 2, 2018 21:23

istio-testing requested review from kyessenov and mandarjog July 2, 2018 21:23

googlebot added the cla: yes label Jul 2, 2018

istio-testing added the approved label Jul 2, 2018

Format

412a699

kyessenov reviewed Jul 2, 2018

View reviewed changes

istio-testing added the do-not-merge/hold Block automatic merging of a PR. label Jul 2, 2018

Move back the fail close - not clear if we have time to get mixer cli…

2276b95

…ent in this build, we can remove in next

kyessenov reviewed Jul 2, 2018

View reviewed changes

istio-testing assigned kyessenov Jul 2, 2018

istio-testing added the lgtm label Jul 2, 2018

costinm removed the do-not-merge/hold Block automatic merging of a PR. label Jul 2, 2018

costinm merged commit 855fdd7 into istio:master Jul 2, 2018

Conversation

costinm commented Jul 2, 2018

Uh oh!

costinm commented Jul 2, 2018

Uh oh!

kyessenov left a comment

Choose a reason for hiding this comment

Uh oh!

costinm commented Jul 2, 2018

Uh oh!

costinm commented Jul 2, 2018

Uh oh!

kyessenov commented Jul 2, 2018

Uh oh!

kyessenov commented Jul 2, 2018

Uh oh!

codecov bot commented Jul 2, 2018

Codecov Report

Uh oh!

costinm commented Jul 2, 2018

Uh oh!

kyessenov Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

costinm Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

kyessenov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Jul 2, 2018

Uh oh!

costinm commented Jul 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kyessenov left a comment •

edited

Loading