Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix exposing cluster local names to the external gateway. #6174

Merged
merged 1 commit into from
Dec 7, 2019
Merged

Fix exposing cluster local names to the external gateway. #6174

merged 1 commit into from
Dec 7, 2019

Conversation

vagababov
Copy link
Contributor

Currently the code was not adding the service/config target (default
placeholder service) if the whole
traffic routing stanza was revision tags only.
This created a problem that the clusterLocal names were only
tag target names, if the service was marked cluster-local.
Thus, when we created the rules for ingress `` (empty tag), it translated into
serviceName target, that was not part of the cluster local list, hence
exposing it on the cluster-ingress gateway.

/assign @tcnghia @mattmoor

@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2019
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no Indicates the PR's author has not signed the CLA. label Dec 7, 2019
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 7, 2019
@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/autoscale area/networking size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 7, 2019
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/ingress/resources/virtual_service.go 86.1% 86.5% 0.4
pkg/reconciler/route/resources/ingress.go 86.8% 87.3% 0.5
pkg/reconciler/route/route.go 81.2% 81.5% 0.2

@knative-test-reporter-robot
Copy link

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-unit-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

pkg/reconciler/route/resources.TestGetDesiredServiceNames
pkg/reconciler/route/resources.TestGetDesiredServiceNames/no_traffic_defined

@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 7, 2019
@vagababov vagababov force-pushed the 20191207-fix-cluster-local branch 3 times, most recently from 656fdb5 to 1c033ce Compare December 7, 2019 20:02
@vagababov vagababov changed the title [WIP] Fix exposing cluster local names to the external gateway. Fix exposing cluster local names to the external gateway. Dec 7, 2019
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2019
@vagababov vagababov force-pushed the 20191207-fix-cluster-local branch from 1c033ce to a5cf9fe Compare December 7, 2019 20:25
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Dec 7, 2019
@knative-prow-robot knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Dec 7, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vagababov

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@tcnghia
Copy link
Contributor

tcnghia commented Dec 7, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2019
@vagababov
Copy link
Contributor Author

This should also fix #5734.

@vagababov
Copy link
Contributor Author

/retest

@knative-prow-robot knative-prow-robot merged commit fe2d208 into knative:master Dec 7, 2019
vagababov added a commit to vagababov/serving that referenced this pull request Dec 7, 2019
@vagababov
squash (knative#6174)
2a75f57
knative-prow-robot pushed a commit that referenced this pull request Dec 8, 2019
@vagababov @knative-prow-robot
squash (#6174) (#6175)
6607885
markusthoemmes pushed a commit to markusthoemmes/knative-serving that referenced this pull request Dec 10, 2019
@vagababov @markusthoemmes
squash (knative#6174) (knative#6175)
a56bb15
openshift-merge-robot pushed a commit to openshift/knative-serving that referenced this pull request Dec 10, 2019
@markusthoemmes @openshift-merge-robot
Cherry picks from 0.10.1 (#361)
4cf806b 
* squash (knative#6174) (knative#6175)

* Use prefix instead of regex for authority match in virtualservice (knative#6088) (knative#6183)

* Use prefix instead of regex for authority match in virtualservice

This patch changes to use prefix instead of regex for authority match in virtualservice.

As described in knative#6058, Istio
1.4 introduced 100 bytes limitation for the regex. So, Knative service
which has long service name or domain name, it hits the limit easily.

To fix it, this patch uses `prefix` and stop using `regex`.

Current regex in VirtualService should be able to replaced with Prefix.

CURRENT:
```
regex: ^hello-example\.default\.example\.com(?::\d{1,5})?$
```

AFTER:
```
prefix: hello-example.default.example.com
```

* Trim cluster local domain to match local
@vagababov vagababov deleted the 20191207-fix-cluster-local branch June 23, 2020 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nak3 nak3 Awaiting requested review from nak3

@tcnghia tcnghia Awaiting requested review from tcnghia

@yanweiguo yanweiguo Awaiting requested review from yanweiguo

Assignees

@tcnghia tcnghia

@mattmoor mattmoor

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/autoscale area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vagababov @googlebot @knative-metrics-robot @knative-test-reporter-robot @knative-prow-robot @tcnghia @mattmoor