Skip to content

Add common probe path in autoscaler, networking and activator #7445

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

Merged
knative-prow-robot merged 6 commits into from
Apr 2, 2020
Merged

Add common probe path in autoscaler, networking and activator #7445

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3965c61
Changed probe path in autoscaler and networking in compliance with ac…
shreejad Mar 31, 2020
a623b87
Format Go code
shreejad Mar 31, 2020
bedc563
Changed probe path to /healthz
shreejad Apr 1, 2020
94ef72c
Update pkg/reconciler/autoscaling/kpa/scaler.go Format Go code
shreejad Apr 1, 2020
00f85ba
Few formatting changes
shreejad Apr 2, 2020
07de7c8
Update pkg/network/status/status.go - Change comment
shreejad Apr 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions pkg/network/status/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ const (
// probeConcurrency defines how many probing calls can be issued simultaneously
probeConcurrency = 15
//probeTimeout defines the maximum amount of time a request will wait
Comment thread
shreejad marked this conversation as resolved.
Outdated
probeTimeout = 1 * time.Second
probeTimeout = 1 * time.Second
probePath = "/_internal/knative/networking/probe"
)

var dialContext = (&net.Dialer{Timeout: probeTimeout}).DialContext
Expand Down Expand Up @@ -356,10 +357,13 @@ func (m *Prober) processWorkItem() bool {
return dialContext(ctx, network, net.JoinHostPort(item.podIP, item.podPort))
}}

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

@markusthoemmes markusthoemmes Apr 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@shreejad shreejad Apr 1, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made changes in new commit

probeUrl.Path = probePath

ok, err := prober.Do(
item.context,
transport,
item.url.String(),
probeUrl.String(),
prober.WithHeader(network.UserAgentKey, network.IngressReadinessUserAgent),
prober.WithHeader(network.ProbeHeaderName, network.ProbeHeaderValue),
m.probeVerifier(item))
Expand Down
12 changes: 11 additions & 1 deletion pkg/reconciler/autoscaling/kpa/scaler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"net/http"
"net/url"
"time"

"knative.dev/pkg/apis/duck"
Expand Down Expand Up @@ -47,6 +48,8 @@ const (
scaleUnknown = -1
probePeriod = 1 * time.Second
probeTimeout = 45 * time.Second
probePath = "/_internal/knative/autoscaler/probe"
Comment thread
shreejad marked this conversation as resolved.
Outdated

// The time after which the PA will be re-enqueued.
// This number is small, since `handleScaleToZero` below will
// re-enqueue for the configured grace period.
Expand Down Expand Up @@ -116,7 +119,14 @@ func newScaler(ctx context.Context, psInformerFactory duck.InformerFactory, enqu
func paToProbeTarget(pa *pav1alpha1.PodAutoscaler) string {
svc := pkgnet.GetServiceHostname(pa.Status.ServiceName, pa.Namespace)
port := networking.ServicePort(pa.Spec.ProtocolType)
return fmt.Sprintf("http://%s:%d/", svc, port)

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

@vagababov vagababov Mar 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vagababov vagababov Mar 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

@shreejad shreejad Mar 31, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vagababov vagababov Mar 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@shreejad shreejad Mar 31, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, makes sense

Path: probePath,
}

return httpDest.String()
}

// activatorProbe returns true if via probe it determines that the
Expand Down
2 changes: 2 additions & 0 deletions test/config/security/policy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,6 @@ spec:
- excludedPaths:
- prefix: /metrics
- prefix: /_internal/knative/activator/probe

@JRBANCEL JRBANCEL Mar 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there value in having different paths?

@shreejad shreejad Mar 31, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vagababov vagababov Mar 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@JRBANCEL JRBANCEL Mar 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@shreejad shreejad Mar 31, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tcnghia tcnghia Mar 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@vagababov vagababov Mar 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yolocs yolocs Mar 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@JRBANCEL JRBANCEL Mar 31, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- prefix: /_internal/knative/autoscaler/probe
- prefix: /_internal/knative/networking/probe
principalBinding: USE_ORIGIN