Egress Rules Refactoring 2 by vadimeisenbergibm · Pull Request #1887 · istio/istio

vadimeisenbergibm · 2017-11-28T09:01:16Z

What this PR does / why we need it:
Improves egress rules handling code.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:
It is resubmission of #1863, rebased.

Release note:

codecov · 2017-11-28T09:13:06Z

Codecov Report

Merging #1887 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1887      +/-   ##
==========================================
- Coverage   81.16%   81.15%   -0.02%     
==========================================
  Files         189      187       -2     
  Lines       19396    18891     -505     
==========================================
- Hits        15743    15331     -412     
+ Misses       3187     3128      -59     
+ Partials      466      432      -34

Flag	Coverage Δ
#broker	`45.51% <ø> (ø)`	⬆️
#mixer	`82.42% <ø> (-0.03%)`	⬇️
#pilot	`80.35% <100%> (-0.05%)`	⬇️
#security	`90.39% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pilot/model/config.go	`67.63% <ø> (-3.98%)`	⬇️
pilot/model/validation.go	`92.16% <100%> (ø)`	⬆️
pilot/model/egress_rules.go	`100% <100%> (ø)`
pilot/proxy/envoy/config.go	`92.08% <100%> (+0.02%)`	⬆️
pilot/platform/kube/queue.go	`82.69% <0%> (-7.7%)`	⬇️
mixer/adapter/prometheus/server.go	`89.83% <0%> (-5.09%)`	⬇️
pilot/platform/kube/controller.go	`51.85% <0%> (-3.04%)`	⬇️
pilot/platform/kube/inject/http.go
pilot/platform/kube/inject/inject.go
pilot/platform/kube/inject/configmap.go
... and 5 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 8743921...d6f9a1b. Read the comment docs.

codecov · 2017-11-28T09:13:14Z

Codecov Report

Merging #1887 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1887      +/-   ##
==========================================
- Coverage   81.16%   81.13%   -0.03%     
==========================================
  Files         189      186       -3     
  Lines       19396    18883     -513     
==========================================
- Hits        15743    15321     -422     
+ Misses       3187     3130      -57     
+ Partials      466      432      -34

Flag	Coverage Δ
#broker	`44.44% <ø> (-1.07%)`	⬇️
#mixer	`82.42% <ø> (-0.03%)`	⬇️
#pilot	`80.32% <100%> (-0.08%)`	⬇️
#security	`90.39% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pilot/model/config.go	`67.63% <ø> (-3.98%)`	⬇️
pilot/model/validation.go	`92.16% <100%> (ø)`	⬆️
pilot/model/egress_rules.go	`100% <100%> (ø)`
pilot/proxy/envoy/config.go	`92.08% <100%> (+0.02%)`	⬆️
pilot/platform/kube/queue.go	`82.69% <0%> (-7.7%)`	⬇️
mixer/adapter/prometheus/server.go	`89.83% <0%> (-5.09%)`	⬇️
pilot/platform/kube/controller.go	`51.85% <0%> (-3.04%)`	⬇️
pilot/platform/kube/inject/inject.go
pilot/platform/kube/inject/http.go
broker/pkg/version/version.go
... and 5 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 8743921...1bc2dfe. Read the comment docs.

vadimeisenbergibm · 2017-11-28T11:34:48Z

@ijsnellf I applied your comments for #1863, in this rebased PR.
Note that string in 117ea6e#diff-9d998bcb98e2ea8ae4a360f52661eaabL886 is still split since it is long.

ijsnellf · 2017-11-28T15:14:45Z

pilot/model/egress_rules.go

+
+	// domains - a map where keys are of the form domain:port and values are the keys of
+
+	// domains - a map where keys are of the form domain:port and values are the keys of


duplicated lines here

ijsnellf · 2017-11-28T15:20:30Z

Looks good to me, except for the nit

vadimeisenbergibm · 2017-11-28T15:23:26Z

@ijsnellf Good catch with the duplicated line, thanks! Fixed in 1bc2dfe

vadimeisenbergibm · 2017-11-28T16:00:18Z

@ijsnellf Could you please approve the PR and add LGTM label?

ijsnellf · 2017-11-28T16:52:45Z

/lgtm

istio-merge-robot · 2017-11-28T16:52:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ijsnellf

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

Details

Needs approval from an approver in each of these OWNERS Files:

~~pilot/OWNERS~~ [ijsnellf]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

…ble (istio#1887)

added model/egress_rules.go - put egress rules helper functions there

d6f9a1b

istio-testing added the release-note-none label Nov 28, 2017

googlebot added the cla: yes label Nov 28, 2017