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

Drop special logic for kubelet probe header for istio #950

Merged
merged 4 commits into from
Mar 26, 2024
Merged

Drop special logic for kubelet probe header for istio #950

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9858961
drop special logic for kubelet probe header for istio
izabelacg Mar 25, 2024
15584ad
fix tests
izabelacg Mar 25, 2024
3210a05
add header back
izabelacg Mar 25, 2024
a3cb1e5
address feedback
izabelacg Mar 26, 2024
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
11 changes: 9 additions & 2 deletions pkg/deprecated_header.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,15 @@ const (
// probes. Istio with mTLS rewrites probes, but their probes pass a
// different user-agent. So we augment the probes with this header.
//
// Deprecated: use knative.dev/networking/pkg/http/header.KubeletProbeKey
KubeletProbeHeaderName = header.KubeletProbeKey
// Deprecated: use knative.dev/networking/pkg/http/header.UserAgentKey
KubeletProbeHeaderName = header.UserAgentKey
Copy link
Member

Choose a reason for hiding this comment

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

I'd leave this referencing header.KubeletProbeKey as before


// KubeletProbeKey is the name of the header supplied by kubelet
// probes. Istio with mTLS rewrites probes, but their probes pass a
// different user-agent. So we augment the probes with this header.
//
// Deprecated: use knative.dev/networking/pkg/http/header.UserAgentKey
KubeletProbeKey = header.UserAgentKey
Copy link
Member

Choose a reason for hiding this comment

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

Drop this addtion - we don't need to introduce a new field here


// UserAgentKey is the constant for header "User-Agent".
//
Expand Down
5 changes: 3 additions & 2 deletions pkg/http/header/header.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ const (
// KubeletProbeKey is the name of the header supplied by kubelet
// probes. Istio with mTLS rewrites probes, but their probes pass a
// different user-agent. So we augment the probes with this header.
//
// Deprecated: use knative.dev/networking/pkg/http/header.UserAgentKey
Copy link
Member

Choose a reason for hiding this comment

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

I'd be more specific in this deprecation notice and say we want to detect probes with header.UserAgentKey and KubeProbeUAPrefix

Copy link
Member Author

Choose a reason for hiding this comment

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

Made changes. Let me know if the text makes sense

KubeletProbeKey = "K-Kubelet-Probe"

// RouteTagKey is the name of the header entry which has a tag name as value.
Expand Down Expand Up @@ -126,8 +128,7 @@ func IsProbe(r *http.Request) bool {

// IsKubeletProbe returns true if the request is a Kubernetes probe.
func IsKubeletProbe(r *http.Request) bool {
return strings.HasPrefix(r.Header.Get("User-Agent"), KubeProbeUAPrefix) ||
r.Header.Get(KubeletProbeKey) != ""
return strings.HasPrefix(r.Header.Get("User-Agent"), KubeProbeUAPrefix)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave this as is for now and then remove the use of K-Kubelet-Probe after a release or two

Copy link
Member

Choose a reason for hiding this comment

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

if there's a minor drift between the queue proxy and the deployment then we want to prevent things from blowing up.

}

// RewriteHostIn removes the `Host` header from the inbound (server) request
Expand Down
4 changes: 0 additions & 4 deletions pkg/http/header/header_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,6 @@ func TestIsKubeletProbe(t *testing.T) {
if IsKubeletProbe(req) {
t.Error("Not a kubelet probe but counted as such")
}
req.Header.Set(KubeletProbeKey, "no matter")
if !IsKubeletProbe(req) {
t.Error("kubelet probe but not counted as such")
}
}

func TestKnativeProbeHeader(t *testing.T) {
Expand Down
Loading