Reject overlapping Gateway HTTPS hosts by howardjohn · Pull Request #13731 · istio/istio

howardjohn · 2019-04-30T18:36:48Z

We already had some unit tests on master for this, so I pulled those in as well

* Adding unit tests for gateway * Fixing the lint issue * Fixing the copyright year * Making changes suggested in the reviews. Changes the name of a function and location of another.

googlebot · 2019-04-30T18:36:51Z

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 or co-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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

howardjohn · 2019-04-30T18:40:03Z

@mbanikazemi are you ok with us merging your commit in? not really sure if this is necessary but oh well

howardjohn · 2019-04-30T23:06:47Z

/test all

To fix prow flake

costinm · 2019-05-01T00:15:49Z

pilot/pkg/model/gateway.go

 					currentProto := ParseProtocol(p[0].Port.Protocol)
 					if currentProto != protocol || !protocol.IsHTTP() {
-						log.Debugf("skipping server on gateway %s port %s.%d.%s: conflict with existing server %s.%d.%s",
+						log.Warnf("skipping server on gateway %s port %s.%d.%s: conflict with existing server %s.%d.%s",


The general approach is to add a metric - users are not expected to read the warnings.

Don’t change this. This gets extremely spammy as it’s called often.

costinm

/lgtm

istio-testing · 2019-05-01T00:16:51Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/OWNERS~~ [costinm]

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

rshriram · 2019-05-01T01:55:30Z

pilot/pkg/model/gateway.go

+					tlsHostsByPort[s.Port.Number] = map[string]struct{}{}
+				}
+				if duplicateHosts := checkDuplicates(s.Hosts, tlsHostsByPort[s.Port.Number]); len(duplicateHosts) != 0 {
+					log.Warnf("MergeGateways: skipping server on gateway %s, duplicate host names: %v", gatewayName, duplicateHosts)


rshriram · 2019-05-01T01:56:31Z

pilot/pkg/model/gateway.go

+				}
+				if duplicateHosts := checkDuplicates(s.Hosts, tlsHostsByPort[s.Port.Number]); len(duplicateHosts) != 0 {
+					log.Warnf("MergeGateways: skipping server on gateway %s, duplicate host names: %v", gatewayName, duplicateHosts)
+					continue


Why do you have to reject the entire server? Just remove the host or reject the host.

I think it's a good idea, but I am not sure it is the right one. Do we guarantee (or want to) guarantee atomicity of config? What I mean is it ok to accept part of a config, or should we accept all or nothing?

I worry that if we just remove the host, the config, which is invalid and should he fixed, may go unnoticed for longer.

I do see the appeal of rejecting just the bad host though

thats a fair point. thats what we do for the port names as well. The thing is, the duplicate host stuff worked before (dunno how but somehow envoy didn't barf).
I guess its okay. Better to reject the entire server than just one host, as the server has certs and other stuff.

In 1.1.2 and below it was "working", but Envoy would just pick an arbitrary one (I think the last one). However it could crash envoy in some instances - see #13735.

In 1.1.4 we picked up this PR envoyproxy/envoy#6022 which always blocks it

rshriram

please use debug log statements as this code is called in several places for every gateway in the system. It will get extremely spammy in Pilot logs.

istio-testing · 2019-05-01T15:03:09Z

New changes are detected. LGTM label has been removed.

istio-testing · 2019-05-01T15:39:30Z

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

Test name	Commit	Details	Rerun command
prow/istio-pilot-multicluster-e2e.sh	`cfc6230`	link	`/test istio-pilot-multicluster-e2e`

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.

howardjohn · 2019-05-01T17:37:28Z

@costinm @rshriram dropped the logs and replaced with prometheus metrics, should be better now in visibility and no log spam, lgtm got removed though

duderino · 2019-05-01T17:49:33Z

@mbanikazemi already gave consent to merge his code to master (in #12792). That means we can cherry pick his change into the release-1.1 branch

googlebot · 2019-05-01T17:49:51Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

duderino · 2019-05-01T18:18:04Z

@costinm and @rshriram please lgtm after the merge

mbanikazemi and others added 2 commits April 30, 2019 08:56

Adding unit tests for gateway (istio#12792)

ab77d00

* Adding unit tests for gateway * Fixing the lint issue * Fixing the copyright year * Making changes suggested in the reviews. Changes the name of a function and location of another.

Reject overlapping Gateway HTTPS hosts

ef4939a

howardjohn requested review from costinm and rshriram April 30, 2019 18:36

googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Apr 30, 2019

howardjohn requested review from duderino and lambdai April 30, 2019 23:23

costinm reviewed May 1, 2019

View reviewed changes

istio-testing assigned costinm May 1, 2019

costinm approved these changes May 1, 2019

View reviewed changes

istio-testing added the lgtm label May 1, 2019

istio-testing added the approved label May 1, 2019

rshriram reviewed May 1, 2019

View reviewed changes

rshriram approved these changes May 1, 2019

View reviewed changes

Drop log level, add metric

cfc6230

istio-testing removed the lgtm label May 1, 2019

duderino added cla: yes and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels May 1, 2019

duderino merged commit 9b6d31b into istio:release-1.1 May 1, 2019

howardjohn mentioned this pull request May 9, 2019

When Duplicate entry of domain curl any domain return 404 #13940

Closed

Conversation

howardjohn commented Apr 30, 2019

Uh oh!

googlebot commented Apr 30, 2019

Uh oh!

howardjohn commented Apr 30, 2019

Uh oh!

howardjohn commented Apr 30, 2019

Uh oh!

costinm May 1, 2019

Choose a reason for hiding this comment

Uh oh!

rshriram May 1, 2019

Choose a reason for hiding this comment

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

istio-testing commented May 1, 2019

Uh oh!

rshriram May 1, 2019

Choose a reason for hiding this comment

Uh oh!

rshriram May 1, 2019

Choose a reason for hiding this comment

Uh oh!

howardjohn May 1, 2019 • 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!

rshriram May 1, 2019

Choose a reason for hiding this comment

Uh oh!

howardjohn May 1, 2019

Choose a reason for hiding this comment

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

istio-testing commented May 1, 2019

Uh oh!

istio-testing commented May 1, 2019

Uh oh!

howardjohn commented May 1, 2019

Uh oh!

duderino commented May 1, 2019

Uh oh!

googlebot commented May 1, 2019

Uh oh!

duderino commented May 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

howardjohn May 1, 2019 •

edited

Loading