istio / old_mixer_repo

Deprecated home of Istio's Mixer and its adapters, now in istio/istio's mixer dir
https://github.com/istio/istio/tree/master/mixer
Apache License 2.0
67 stars 93 forks source link

Alter Preprocess bag handling to prevent attribute overriding #1460

Closed douglas-reid closed 7 years ago

douglas-reid commented 7 years ago

Prevent APAs from overriding attributes sent in the original request.

This adds a new method to MutableBag named PreserveMerge which takes in bags that possibly contain conflicts and does a merge in which original values are maintained and conflicts are silently dropped on the floor.

This is in response to errors reported on the dev mailing list that stemmed from having multiple values for the attribute destination.service in the attribute bag that was used to create instances.

Release note:

Prefer received attribute values to preprocess-derived attribute values.

This change is Reviewable

istio-testing commented 7 years ago

@douglas-reid: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none". Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed. Instructions for interacting with me using PR comments are available [here](https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
douglas-reid commented 7 years ago

/hold

geeknoid commented 7 years ago

/lgtm

istio-merge-robot commented 7 years ago

/lgtm cancel //PR changed after LGTM, removing LGTM. @douglas-reid @geeknoid

douglas-reid commented 7 years ago

PTAL. Updated code to address linter issues. This required a small set of changes around error-handling, but I think that is OK given initial feedback.

geeknoid commented 7 years ago

Groovy.

/lgtm

istio-merge-robot commented 7 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geeknoid

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

Needs approval from an approver in each of these OWNERS Files: - ~~[OWNERS](https://github.com/istio/mixer/blob/master/OWNERS)~~ [geeknoid] You can indicate your approval by writing `/approve` in a comment You can cancel your approval by writing `/approve cancel` in a comment
istio-merge-robot commented 7 years ago

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot commented 7 years ago

Automatic merge from submit-queue.

codecov[bot] commented 7 years ago

Codecov Report

Merging #1460 into master will decrease coverage by 0.08%. The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1460      +/-   ##
==========================================
- Coverage   84.66%   84.58%   -0.09%     
==========================================
  Files         122      122              
  Lines       11922    11980      +58     
==========================================
+ Hits        10094    10133      +39     
- Misses       1632     1646      +14     
- Partials      196      201       +5
Impacted Files Coverage Δ
pkg/api/grpcServer.go 76.17% <62.5%> (-2%) :arrow_down:
pkg/attribute/mutableBag.go 89.71% <64.7%> (-1.71%) :arrow_down:
adapter/stackdriver/metric/bufferedClient.go 90% <0%> (-2.5%) :arrow_down:
pkg/expr/func.go 97.34% <0%> (-1.17%) :arrow_down:
pkg/il/evaluator/evaluator.go 81.67% <0%> (-1.11%) :arrow_down:
pkg/il/compiler/compiler.go 88.38% <0%> (+0.19%) :arrow_up:

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 4eba741...94a9aad. Read the comment docs.