Egress Rules refactoring by vadimeisenbergibm · Pull Request #1863 · istio/istio

vadimeisenbergibm · 2017-11-27T11:51:20Z

What this PR does / why we need it:
Refactors handling egress rules to enable effective handling of multiple TCP and HTTP protocols.

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:
This PR is required to enable mongo and redis TCP protocols.

Release note:

…ess_rules.go added more helper functions to egress_rules.go

fixed a typo, added model module

moved RejectConflictingEgressRules there

istio-merge-robot · 2017-11-27T11:51:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: crazytan

Assign the PR to them by writing /assign @crazytan in a comment when ready.

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

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

codecov · 2017-11-27T12:02:51Z

Codecov Report

Merging #1863 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1863      +/-   ##
==========================================
+ Coverage    81.9%   81.95%   +0.04%     
==========================================
  Files         188      190       +2     
  Lines       18715    18755      +40     
==========================================
+ Hits        15328    15370      +42     
  Misses       2960     2960              
+ Partials      427      425       -2

Flag	Coverage Δ
#broker	`45.51% <ø> (+1.06%)`	⬆️
#mixer	`82.46% <ø> (-0.02%)`	⬇️
#pilot	`82.41% <100%> (+0.13%)`	⬆️
#security	`90.39% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pilot/model/config.go	`97.77% <ø> (-0.4%)`	⬇️
pilot/proxy/envoy/config.go	`92.2% <100%> (+0.02%)`	⬆️
pilot/model/validation.go	`97.1% <100%> (ø)`	⬆️
pilot/model/egress_rules.go	`100% <100%> (ø)`
mixer/adapter/prometheus/server.go	`89.83% <0%> (-5.09%)`	⬇️
broker/pkg/version/version.go	`100% <0%> (ø)`
mixer/adapter/stackdriver/metric/bufferedClient.go	`92.5% <0%> (+2.5%)`	⬆️
pilot/platform/kube/queue.go	`90.38% <0%> (+7.69%)`	⬆️

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 cc39692...4b7c4de. Read the comment docs.

not capitalized, does not end in punctuation

ijsnellf · 2017-11-27T17:13:43Z

pilot/model/egress_rules.go

+
+func protocolMapAsString(protocolMap map[Protocol]bool) string {
+	first := true
+	result := ""


maybe use &bytes.Buffer{} here to speed up the string concatenation?: https://golang.org/pkg/bytes/#Buffer

ijsnellf · 2017-11-27T17:19:54Z

pilot/model/egress_rules.go

+)
+
+// RejectConflictingEgressRules rejects rules that have the destination which is equal to
+// the destionation of some other rule.


destionation -> destination

ijsnellf · 2017-11-27T17:25:22Z

pilot/model/validation.go

+		if cidrDestinationService &&
+			!IsEgressRulesSupportedTCPProtocol(Protocol(strings.ToUpper(port.Protocol))) {
+			errs = multierror.Append(errs, fmt.Errorf("Only the following protocols can be defined for "+
+				"CIDR destination service notation "+egressRulesSupportedTCPProtocols()+


nit: use string formatting to build the entire string instead of a mix of string concatenation and string formatting?

istio-merge-robot · 2017-11-28T05:35:21Z

@vadimeisenbergibm PR needs rebase

Automatic merge from submit-queue. Make linking the kubeconfig idempotent. Previously there were situations where we needed to manually unlink the config, e.g., after build errors. ```release-note NONE ```

Automatic merge from submit-queue. Update mockapi package name to match new home **What this PR does / why we need it**: The `mockapi` package was created by moving files. During that move, the package statements at the top of the files were not updated to match. This PR fixes that issue. **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**: **Release note**: ```release-note-none ```

…#1866) * added ConvertCaseInsensitiveStringToProtocol and ProtocolUnsupported * remove code duplication in convertProtocol functions * use ConvertCaseInsensitiveStringToProtocol instead of strings.ToUpper in conversion of a string to model.Protocol, since there are not-all-uppercase protocols like Mongo and Redis * removed unused metadata values * added TestConvertCaseInsensitiveStringToProtocol * replaced shadowing variable * Revert "replaced shadowing variable" This reverts commit 6a9981f. * replaced shadowing variable

…/community. (istio#1871) Having this file present helps our community-friendly rating that GitHub maintains.

…io#1845) 1. create corresponding CRD for HTTPAPISpec, HTTPAPISpecBinding, QuotaSpec, and QuotaSpecBinding (see https://github.com/istio/api/blob/master/mixer/v1/config/client/api_spec.proto) 2. add corresponding validation code for new model types 3. update istioConfigStore to select HTTPAPISpec and QuotaSpec config associated with destination service instances using the corresponding HTTPAPISpec and QuotaSpec bindings. 4. distribute the HTTPAPI and Quota spec to proxy as part of mixerclient filter configura

Automatic merge from submit-queue. remove lintconfig.json, that's generated **What this PR does / why we need it**: **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**: **Release note**: ```release-note none ```

Automatic merge from submit-queue. Support one handler for mulitple GCP services and fixes - Allow one handler handles multiple GCP services - Expand metric support - Minor fixes in reportBuilder

Automatic merge from submit-queue. Switch Mixer tracing package to use jaeger library. **What this PR does / why we need it**: This PR switches Mixer from using the openzipkin tracing library to the jaeger tracing library for tracing Mixer functions. This switch accomplishes a few key items: - Adds Mixer support for Jaeger backends - Removes the need for a custom fork of the OpenZipkin library - Adds better flag support for tracing control - Enables development of Jaeger adapter (library conflict with OpenZipkin library). It deprecates the `--traceOutput` flag and replaces it with several additional flags: - `zipkinURL`, - `jaegerURL` - `logTraceSpans` **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # Fixes istio/old_mixer_repo#1352 **Special notes for your reviewer**: **Release note**: ```release-note Add Jaeger support to Mixer. ```

…ess_rules.go added more helper functions to egress_rules.go

googlebot · 2017-11-28T06:51:45Z

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 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 cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

istio-testing · 2017-11-28T06:52:18Z

@vadimeisenbergibm: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-presubmit.sh	`4b7c4de`	link	`/test istio-presubmit`

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vadimeisenbergibm · 2017-11-28T06:53:46Z

Will rebase in a new branch and open as a separate PR.

vadimeisenbergibm added 7 commits November 27, 2017 09:53

refactored config.go - extracted egress rule helper function into egr…

1c577b2

…ess_rules.go added more helper functions to egress_rules.go

changed validation to use egress_rules helper functions

8d1777d

use IsEgrressRuleSupportedTCPProtocol in pilot/proxy/envoy/config.go

fffb60a

reversed protocol check condition

82e44fa

fixed a typo, added model module

added egress_rules_test

f43468a

moved RejectConflictingEgressRules there

fixed formatting

9a79638

use specific protocol of the egress rules instead of TCP

78013fa

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

googlebot added the cla: yes label Nov 27, 2017

vadimeisenbergibm added 2 commits November 27, 2017 14:06

fixed an error string according to golint

9db0789

not capitalized, does not end in punctuation

added comment describing IsEgressRuleSupported*Protocol functions

72f4db7

vadimeisenbergibm changed the title ~~[WIP] Egress refactoring~~ Egress refactoring Nov 27, 2017

vadimeisenbergibm requested a review from rshriram November 27, 2017 12:44

vadimeisenbergibm changed the title ~~Egress refactoring~~ Egress Rules refactoring Nov 27, 2017

removed redundant duplicated comment

8df1f89

ijsnellf reviewed Nov 27, 2017

View reviewed changes

yutongz added the do-not-merge/post-submit label Nov 28, 2017

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 28, 2017

yutongz added the do-not-merge/post-submit label Nov 28, 2017

Joerg Schad and others added 8 commits November 28, 2017 08:47

Make linking the kubeconfig idempotent. (istio#1852)

e7a0fc5

Automatic merge from submit-queue. Make linking the kubeconfig idempotent. Previously there were situations where we needed to manually unlink the config, e.g., after build errors. ```release-note NONE ```

Add a local CONTRIBUTING.md file that points to the main one on istio…

b671c67

…/community. (istio#1871) Having this file present helps our community-friendly rating that GitHub maintains.

consolidate scripts in bin directory in istio components (istio#1875)

13464a7

Support one handler for mulitple GCP services and fixes (istio#1867)

2dfd15e

Automatic merge from submit-queue. Support one handler for mulitple GCP services and fixes - Allow one handler handles multiple GCP services - Expand metric support - Minor fixes in reportBuilder

douglas-reid and others added 4 commits November 28, 2017 08:48

refactored config.go - extracted egress rule helper function into egr…

c90c3ba

…ess_rules.go added more helper functions to egress_rules.go

changed validation to use egress_rules helper functions

0191a12

fixed formatting

4b7c4de

googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes labels Nov 28, 2017

vadimeisenbergibm closed this Nov 28, 2017

vadimeisenbergibm mentioned this pull request Nov 28, 2017

Egress Rules Refactoring 2 #1887

Merged

kyessenov pushed a commit to kyessenov/istio that referenced this pull request Aug 13, 2018

Update istio.dep as valid json (istio#1863)

8f33827

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Egress Rules refactoring#1863

Egress Rules refactoring#1863
vadimeisenbergibm wants to merge 22 commits intoistio:masterfrom
vadimeisenbergibm:egress_refactoring

vadimeisenbergibm commented Nov 27, 2017

Uh oh!

istio-merge-robot commented Nov 27, 2017

Uh oh!

codecov bot commented Nov 27, 2017 •

edited

Loading

Uh oh!

ijsnellf Nov 27, 2017

Uh oh!

ijsnellf Nov 27, 2017

Uh oh!

ijsnellf Nov 27, 2017

Uh oh!

istio-merge-robot commented Nov 28, 2017

Uh oh!

googlebot commented Nov 28, 2017

Uh oh!

istio-testing commented Nov 28, 2017

Uh oh!

vadimeisenbergibm commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Conversation

vadimeisenbergibm commented Nov 27, 2017

Uh oh!

istio-merge-robot commented Nov 27, 2017

Uh oh!

codecov bot commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ijsnellf Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

ijsnellf Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

ijsnellf Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Nov 28, 2017

Uh oh!

googlebot commented Nov 28, 2017

Uh oh!

istio-testing commented Nov 28, 2017

Uh oh!

vadimeisenbergibm commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

codecov bot commented Nov 27, 2017 •

edited

Loading