-
Notifications
You must be signed in to change notification settings - Fork 335
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.
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
Add CheckEndpointState method to SpoofingClient #2166
Changes from 5 commits
65e355a
ae94058
eeafd0d
50aedcf
3831cd6
5356437
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -254,7 +254,25 @@ func (sc *SpoofingClient) WaitForEndpointState( | |
desc string, | ||
opts ...RequestOption) (*Response, error) { | ||
|
||
defer logging.GetEmitableSpan(ctx, "WaitForEndpointState/"+desc).End() | ||
return sc.endpointState( | ||
ctx, | ||
url, | ||
inState, | ||
desc, | ||
func(req *http.Request, check ResponseChecker) (*Response, error) { return sc.Poll(req, check) }, | ||
"WaitForEndpointState", | ||
opts...) | ||
} | ||
|
||
func (sc *SpoofingClient) endpointState( | ||
ctx context.Context, | ||
url *url.URL, | ||
inState ResponseChecker, | ||
desc string, | ||
f func(*http.Request, ResponseChecker) (*Response, error), | ||
logName string, | ||
opts ...RequestOption) (*Response, error) { | ||
defer logging.GetEmitableSpan(ctx, logName+"/"+desc).End() | ||
|
||
if url.Scheme == "" || url.Host == "" { | ||
return nil, fmt.Errorf("invalid URL: %q", url.String()) | ||
|
@@ -269,5 +287,54 @@ func (sc *SpoofingClient) WaitForEndpointState( | |
opt(req) | ||
} | ||
|
||
return sc.Poll(req, inState) | ||
return f(req, inState) | ||
} | ||
|
||
func (sc *SpoofingClient) Check(req *http.Request, inState ResponseChecker) (*Response, error) { | ||
traceContext, span := trace.StartSpan(req.Context(), "SpoofingClient-Trace") | ||
defer span.End() | ||
rawResp, err := sc.Client.Do(req.WithContext(traceContext)) | ||
if err != nil { | ||
sc.Logf("NOT Retrying %s: %v", req.URL.String(), err) | ||
return nil, err | ||
} | ||
defer rawResp.Body.Close() | ||
|
||
body, err := ioutil.ReadAll(rawResp.Body) | ||
if err != nil { | ||
return nil, err | ||
} | ||
rawResp.Header.Add(zipkin.ZipkinTraceIDHeader, span.SpanContext().TraceID.String()) | ||
|
||
resp := &Response{ | ||
Status: rawResp.Status, | ||
StatusCode: rawResp.StatusCode, | ||
Header: rawResp.Header, | ||
Body: body, | ||
} | ||
ok, err := inState(resp) | ||
if err != nil { | ||
return resp, fmt.Errorf("response: %s did not pass checks: %w", resp, err) | ||
} | ||
if ok { | ||
sc.logZipkinTrace(resp) | ||
return resp, nil | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The be consistent with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's very similar to the Poll method but I think resp is guaranteed to be non nil at this point so I just moved the log line out of the |
||
return nil, err | ||
} | ||
|
||
func (sc *SpoofingClient) CheckEndpointState( | ||
ctx context.Context, | ||
url *url.URL, | ||
inState ResponseChecker, | ||
desc string, | ||
opts ...RequestOption) (*Response, error) { | ||
return sc.endpointState( | ||
ctx, | ||
url, | ||
inState, | ||
desc, | ||
sc.Check, | ||
"CheckEndpointState", | ||
opts...) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,254 @@ | ||
/* | ||
Copyright 2021 The Knative Authors | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
// spoof contains logic to make polling HTTP requests against an endpoint with optional host spoofing. | ||
|
||
package spoof | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"net/http" | ||
"net/url" | ||
|
||
"testing" | ||
|
||
corev1 "k8s.io/api/core/v1" | ||
v1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"knative.dev/pkg/client/injection/kube/client/fake" | ||
) | ||
|
||
type fakeTransport struct{} | ||
|
||
func (ft *fakeTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
return &http.Response{ | ||
Status: "200 ok", | ||
StatusCode: 200, | ||
Header: http.Header{}, | ||
Body: http.NoBody, | ||
}, nil | ||
} | ||
|
||
type countCalls struct { | ||
calls int32 | ||
} | ||
|
||
func (c *countCalls) count(rc ResponseChecker) ResponseChecker { | ||
return func(resp *Response) (done bool, err error) { | ||
c.calls++ | ||
return rc(resp) | ||
} | ||
} | ||
|
||
func TestSpoofingClient_CheckEndpointState(t *testing.T) { | ||
ingress := &corev1.Service{ | ||
ObjectMeta: v1.ObjectMeta{ | ||
Name: "istio-ingressgateway", | ||
Namespace: "istio-system", | ||
}, | ||
Status: corev1.ServiceStatus{ | ||
LoadBalancer: corev1.LoadBalancerStatus{ | ||
Ingress: []corev1.LoadBalancerIngress{ | ||
{ | ||
Hostname: "host", | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
type args struct { | ||
url *url.URL | ||
inState ResponseChecker | ||
desc string | ||
opts []RequestOption | ||
} | ||
tests := []struct { | ||
name string | ||
args args | ||
wantErr bool | ||
wantCalls int32 | ||
}{{ | ||
name: "Non matching response doesn't trigger a second check", | ||
args: args{ | ||
url: &url.URL{ | ||
Host: "fake.knative.net", | ||
Scheme: "http", | ||
}, | ||
inState: func(resp *Response) (done bool, err error) { | ||
return false, nil | ||
}, | ||
}, | ||
wantErr: false, | ||
wantCalls: 1, | ||
}, { | ||
name: "Error response doesn't trigger a second check", | ||
args: args{ | ||
url: &url.URL{ | ||
Host: "fake.knative.net", | ||
Scheme: "http", | ||
}, | ||
inState: func(resp *Response) (done bool, err error) { | ||
return false, fmt.Errorf("response error") | ||
}, | ||
}, | ||
wantErr: true, | ||
wantCalls: 1, | ||
}, { | ||
name: "OK response doesn't trigger a second check", | ||
args: args{ | ||
url: &url.URL{ | ||
Host: "fake.knative.net", | ||
Scheme: "http", | ||
}, | ||
inState: func(resp *Response) (done bool, err error) { | ||
return true, nil | ||
}, | ||
}, | ||
wantErr: false, | ||
wantCalls: 1, | ||
}} | ||
for _, tt := range tests { | ||
_, fKlient := fake.With(context.TODO(), ingress) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would just create the k8s fake client directly vs pulling in the injection library |
||
t.Run(tt.name, func(t *testing.T) { | ||
sc, err := New( | ||
context.TODO(), | ||
fKlient, | ||
t.Logf, | ||
"some.svc.knative.dev", | ||
|
||
false, | ||
"host", | ||
1, | ||
1, | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'll be simpler to setup the tests if you just construct a |
||
if err != nil { | ||
t.Fatalf("Spoofing client not created: %v", err) | ||
} | ||
sc.Client = &http.Client{ | ||
Transport: &fakeTransport{}, | ||
} | ||
counter := countCalls{} | ||
_, err = sc.CheckEndpointState(context.TODO(), tt.args.url, counter.count(tt.args.inState), tt.args.desc, tt.args.opts...) | ||
if (err != nil) != tt.wantErr { | ||
t.Errorf("SpoofingClient.CheckEndpointState() error = %v, wantErr %v", err, tt.wantErr) | ||
return | ||
} | ||
if counter.calls != tt.wantCalls { | ||
t.Errorf("Expected ResponseChecker to be invoked %d time but got invoked %d", tt.wantCalls, counter.calls) | ||
} | ||
}) | ||
} | ||
} | ||
|
||
func TestSpoofingClient_WaitForEndpointState(t *testing.T) { | ||
ingress := &corev1.Service{ | ||
ObjectMeta: v1.ObjectMeta{ | ||
Name: "istio-ingressgateway", | ||
Namespace: "istio-system", | ||
}, | ||
Status: corev1.ServiceStatus{ | ||
LoadBalancer: corev1.LoadBalancerStatus{ | ||
Ingress: []corev1.LoadBalancerIngress{ | ||
{ | ||
Hostname: "host", | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
type args struct { | ||
url *url.URL | ||
inState ResponseChecker | ||
desc string | ||
opts []RequestOption | ||
} | ||
tests := []struct { | ||
name string | ||
args args | ||
wantErr bool | ||
wantCalls int32 | ||
}{{ | ||
name: "OK response doesn't trigger a second request", | ||
args: args{ | ||
url: &url.URL{ | ||
Host: "fake.knative.net", | ||
Scheme: "http", | ||
}, | ||
inState: func(resp *Response) (done bool, err error) { | ||
return true, nil | ||
}, | ||
}, | ||
wantErr: false, | ||
wantCalls: 1, | ||
}, { | ||
name: "Error response doesn't trigger more requests", | ||
args: args{ | ||
url: &url.URL{ | ||
Host: "fake.knative.net", | ||
Scheme: "http", | ||
}, | ||
inState: func(resp *Response) (done bool, err error) { | ||
return false, fmt.Errorf("response error") | ||
}, | ||
}, | ||
wantErr: true, | ||
wantCalls: 1, | ||
}, { | ||
name: "Non matching response triggers more requests", | ||
args: args{ | ||
url: &url.URL{ | ||
Host: "fake.knative.net", | ||
Scheme: "http", | ||
}, | ||
inState: func(resp *Response) (done bool, err error) { | ||
return false, nil | ||
}, | ||
}, | ||
wantErr: true, | ||
wantCalls: 3, | ||
}} | ||
for _, tt := range tests { | ||
_, fKlient := fake.With(context.TODO(), ingress) | ||
t.Run(tt.name, func(t *testing.T) { | ||
sc, err := New( | ||
context.TODO(), | ||
fKlient, | ||
t.Logf, | ||
"some.svc.knative.dev", | ||
|
||
false, | ||
"host", | ||
1, | ||
1, | ||
) | ||
if err != nil { | ||
t.Fatalf("Spoofing client not created: %v", err) | ||
} | ||
sc.Client = &http.Client{ | ||
Transport: &fakeTransport{}, | ||
} | ||
counter := countCalls{} | ||
_, err = sc.WaitForEndpointState(context.TODO(), tt.args.url, counter.count(tt.args.inState), tt.args.desc, tt.args.opts...) | ||
if (err != nil) != tt.wantErr { | ||
t.Errorf("SpoofingClient.CheckEndpointState() error = %v, wantErr %v", err, tt.wantErr) | ||
return | ||
} | ||
if counter.calls != tt.wantCalls { | ||
t.Errorf("Expected ResponseChecker to be invoked %d time but got invoked %d", tt.wantCalls, counter.calls) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit confusing to call this
waitForEndpointStateWithTimeout
but there's await
argument that makes this check.I think we should just change this to only create the spoof client and then in the respective
Wait
Check
methods callclient.Wait/Check