Add common probe path in autoscaler, networking and activator by shreejad · Pull Request #7445 · knative/serving

shreejad · 2020-03-31T18:48:02Z

Fixes #5918

In PR 6505, a new probe path was added in activator code which can be whitelisted
This PR adds a common probe path '/healthz' in networking, autoscaler and activator (modifies previous custom path) to be whitelisted

…tivator

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

shreejad · 2020-03-31T18:50:48Z

/assign @vagababov

vagababov

/assign @JRBANCEL
For pkg/network changes.

pkg/network/status/status.go

pkg/reconciler/autoscaling/kpa/scaler.go

vagababov · 2020-03-31T19:40:56Z

pkg/reconciler/autoscaling/kpa/scaler.go

+
+	httpDest := url.URL{
+		Scheme: "http",
+		Host:   fmt.Sprintf("%s:%d", svc, port),


Suggested change

Host: fmt.Sprintf("%s:%d", svc, port),

Host: svc + ":" + strconv.Itoa(port),

I'd also recommend doing

u, _ := url.Parse(fmt.Sprintf("http://%s:%d/", svc, port)) u.Path = path.Join(u.Path, probePath) return u.String()

perhaps with error handling, but we kind of know it won't fail here...

But why "u.Path = path.Join(u.Path, probePath)" since we already know u.Path should be empty by definition in above line

Is it empty? Or is it "/"? Your path does it have "/" prefix or not. This takes care of all the cases.

hmm, makes sense

pkg/network/status/status.go

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

JRBANCEL · 2020-03-31T21:01:29Z

test/config/security/policy.yaml

@@ -46,4 +46,6 @@ spec:
      - excludedPaths:
        - prefix: /metrics
        - prefix: /_internal/knative/activator/probe


Is there value in having different paths?

I actually have no idea, and was wondering too if everything will work with a common path. @tcnghia @vagababov ?

we don't check paths. But if we're going generic, then we should pick a generic name :)

Having a single path probably makes more sense because it makes writing policies easier and it is more future proof (when we add or remove components).

Makes sense. Should the common path be something that we expect customer services won't have in their their application logic? Or can it be very short like "\probe". I had assumed that we had a complicated name with "_knative\internal..." to reduce chances of collision with customer service paths

@yolocs do you have a recommendation here?

May be something like /healthz would work. Also, if we allow this to be customized then our customers can avoid a collision even for shorter paths.

Given we intercept the request it is of less importance if it matches (still a problem if the network rule matches).
But having something like /__probe__ should mostly work.

I would prefer /healthz as it seems like pretty common path for health check. My thinking is for something default, it's better to meet most common path. If a user runs into a conflict, the config is there for them to override.

As @vagababov said, the path can be anything, so let's use something simple and if possible that is somewhat "standard" in the Kubernetes world. /healthz seems appropriate.

markusthoemmes · 2020-04-01T06:15:51Z

pkg/network/status/status.go

 			return dialContext(ctx, network, net.JoinHostPort(item.podIP, item.podPort))
 		}}

+	probeUrl, _ := url.Parse(item.url.String())


Please add a comment that it's safe to ignore the error because this is a "deepCopy" of the URL. Maybe even put it into it's own deepCopy(in *url.URL) *url.URL function for clarity?

Made changes in new commit

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

Unable to apply comments to the following files:

pkg/network/status/status.go

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

pkg/reconciler/autoscaling/kpa/scaler.go

Co-Authored-By: Matt Moore <mattmoor@vmware.com>

vagababov · 2020-04-01T16:31:06Z

you can avoid those errors by installing autoformat on save for whatever editor you're using, unless it's pico or something :)

pkg/reconciler/autoscaling/kpa/scaler.go

knative-test-reporter-robot · 2020-04-02T00:28:35Z

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:

_go_tests.DataRaceAnalysis
pkg/reconciler/route.TestGlobalResyncOnUpdateDomainConfigMap

vagababov

Just one nit.

pkg/network/status/status.go

Co-Authored-By: Victor Agababov <vagababov@gmail.com>

vagababov

/lgtm
/approve

knative-prow-robot · 2020-04-02T02:24:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shreejad, vagababov

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:

~~pkg/activator/OWNERS~~ [vagababov]
~~pkg/network/OWNERS~~ [vagababov]
~~pkg/reconciler/autoscaling/OWNERS~~ [vagababov]
~~test/OWNERS~~ [vagababov]

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

knative-metrics-robot · 2020-04-02T02:24:44Z

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/activator/net/revision_backends.go	92.5%	93.1%	0.6
pkg/network/status/status.go	98.3%	98.4%	0.1

Autoscaler, activator and KIngres Prober commonly use the probe path `/healthz`, but they defined it in each package. This patch adds const value for ProbePath and use it from them. Related to knative#7445

* Use common const value for probe path Autoscaler, activator and KIngres Prober commonly use the probe path `/healthz`, but they defined it in each package. This patch adds const value for ProbePath and use it from them. Related to #7445 * Fix lint error

Changed probe path in autoscaler and networking in compliance with ac…

3965c61

…tivator

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 31, 2020

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 31, 2020

knative-prow-robot requested review from ZhiminXiang, chizhg and yanweiguo March 31, 2020 18:48

knative-prow-robot added 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 labels Mar 31, 2020

mattmoor reviewed Mar 31, 2020

View reviewed changes

knative-prow-robot assigned vagababov Mar 31, 2020

vagababov reviewed Mar 31, 2020

View reviewed changes

knative-prow-robot assigned JRBANCEL Mar 31, 2020

mattmoor reviewed Mar 31, 2020

View reviewed changes

Format Go code

a623b87

shreejad force-pushed the probepath branch from 1a3b87c to a623b87 Compare March 31, 2020 20:13

mattmoor reviewed Mar 31, 2020

View reviewed changes

JRBANCEL reviewed Mar 31, 2020

View reviewed changes

markusthoemmes reviewed Apr 1, 2020

View reviewed changes

mattmoor reviewed Apr 1, 2020

View reviewed changes

Changed probe path to /healthz

bedc563

shreejad force-pushed the probepath branch from f4a9f58 to bedc563 Compare April 1, 2020 12:11

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 Apr 1, 2020

mattmoor reviewed Apr 1, 2020

View reviewed changes

pkg/reconciler/autoscaling/kpa/scaler.go Outdated Show resolved Hide resolved

Update pkg/reconciler/autoscaling/kpa/scaler.go Format Go code

94ef72c

Co-Authored-By: Matt Moore <mattmoor@vmware.com>

vagababov reviewed Apr 1, 2020

View reviewed changes

pkg/reconciler/autoscaling/kpa/scaler.go Outdated Show resolved Hide resolved

Few formatting changes

00f85ba

knative-prow-robot added 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 Apr 2, 2020

vagababov reviewed Apr 2, 2020

View reviewed changes

pkg/network/status/status.go Outdated Show resolved Hide resolved

shreejad changed the title ~~Changed probe path in autoscaler and networking in compliance with activator path~~ Add common probe path in autoscaler, networking and activator Apr 2, 2020

Update pkg/network/status/status.go - Change comment

07de7c8

Co-Authored-By: Victor Agababov <vagababov@gmail.com>

vagababov reviewed Apr 2, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2020

knative-prow-robot merged commit 33e843e into knative:master Apr 2, 2020

shreejad mentioned this pull request Apr 2, 2020

Use better probe path than "/" for activator and KPA #5918

Closed

nak3 mentioned this pull request Jun 15, 2020

Use common const value for probe path #8316

Merged

nak3 mentioned this pull request Jun 22, 2020

Add doc on how to use Istio Authorization with Knative and Istio mesh mode knative/docs#2583

Merged

	Host: fmt.Sprintf("%s:%d", svc, port),
	Host: svc + ":" + strconv.Itoa(port),

Conversation

shreejad commented Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

shreejad commented Mar 31, 2020

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vagababov commented Apr 1, 2020

Uh oh!

Uh oh!

knative-test-reporter-robot commented Apr 2, 2020

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Apr 2, 2020

Uh oh!

knative-metrics-robot commented Apr 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

shreejad commented Mar 31, 2020 •

edited

Loading