Pull host spoofing logic into an http client by jonjohnsonjr · Pull Request #1458 · knative/serving

jonjohnsonjr · 2018-07-03T04:00:09Z

This little abstraction hoists the hostname spoofing into its own http
client so that it is reusable outside of the context of waiting for an
endpoint to return a specific response.

jonjohnsonjr · 2018-07-03T04:15:48Z

/assign @tcnghia @bobcatfish

adrcunha · 2018-07-03T14:02:50Z

Nice!

/approve
/lgtm
/woof
/hold

Holding so @bobcatfish can take a look.

google-prow-robot · 2018-07-03T14:02:51Z

@adrcunha:

Details

In response to this:

Nice!

/approve
/lgtm
/woof
/hold

Holding so @bobcatfish can take a look.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

adrcunha · 2018-07-03T16:10:40Z

/lgtm

jonjohnsonjr · 2018-07-03T16:49:00Z

/hold

Going to pull this into a separate file per #1460 (comment)

bobcatfish

Can you also update the docs in adding_tests.md? some description of this interface there might make this all crystal clear (helps for a review too! cuz then i can start with the docs before diving into the nitty gritty

bobcatfish · 2018-07-03T17:49:40Z

I like the idea of exposing a polling interface that is specifically oriented around polling response objects safely.

It took me a while (and talking with you) to understand why bodyMatcher is replacing the body of the response after reading it, but now that I understand (it's so you can poll it without changing it, and also so you can return the response object untarnished), what do you think about this idea: what if the interface to inState (i.e. the ResponseChecker) took a different object, not the actual response, but a response object you have constructed that contains the body of the response, the error code, etc.? then ppl can do whatever they want to it in inState without causing side effects (in the currently implementation, there's nothing to stop someone from avoiding bodyMatcher and doing all kinds of terrible things to the response in their inState - maybe by accident!

Oh. I like this!

bobcatfish · 2018-07-03T17:50:08Z

I think the comment describing this function could have more detail about the assumed interface, e.g. details such as the response body should not be consumed (or maybe that wont be a problem, see above), and what is returned (or you could include more of this in adding_tests.md)

I'll add a longer comment to the ResponseChecker type, let me know if that does not address your concern here.

bobcatfish · 2018-07-03T17:52:05Z

two things i am not so fond of here:

I find it very hard to tell the difference between bodyMatcher, matchesBody and eventuallyMatchesBody based on name and docs (esp. eventuallyMatchesBody b/c from the name and description i thought that function itself was doing polling)

I don't like it that these have been made with a specific usage in mind: being invoked in a polling method, but also are intended to be used in a non-polling context. i'd prefer if the part that is going to be used in a non-polling context was factored out and the poll-ready interface was only used when polling is involved

bodyMatcher

This should mostly go away with your other suggestion.

used in a non-polling context

Yeah this was a bit of a shortcut, I'll break these down.

jonjohnsonjr · 2018-07-03T20:52:06Z

/assign @mattmoor

jonjohnsonjr · 2018-07-03T20:57:53Z

I welcome any and all bikeshedding for names :)

mattmoor · 2018-07-03T20:58:52Z

/lgtm
/approve

bobcatfish

Looking good! Still would love to see some docs in adding_tests.md :D

/lgtm
/approve
/meow space
/hold

bobcatfish · 2018-07-03T20:59:58Z

bobcatfish · 2018-07-03T21:02:36Z

how do you feel about these timeouts being the SAME values in multiple files but also duplicated everywhere? i dont have any good ideas what else to do.

I don't understand, can you elaborate?

part of why you dont understand is that i derped - i thought you were adding a new set of timeouts.

as it stands these will only exist here but then also in crd_checks - i guess 2 spots isnt so bad

bobcatfish · 2018-07-03T21:02:44Z

niiiice 😎

bobcatfish · 2018-07-03T21:03:34Z

is this a pattern people use? ive never seen this before so it seems strange to me but im probably not used to it!

https://medium.com/stupid-gopher-tricks/ensuring-go-interface-satisfaction-at-compile-time-1ed158e8fa17

courtesy of @imjasonh

I'll add a comment.

bobcatfish · 2018-07-03T21:04:18Z

im interested in why this is called Interface instead of something like SpoofingClientInterface

To avoid the stutter. If another package wants to implement this, I prefer spoof.Interface to spoof.SpoofingClientInterface. If this ends up with more than one interface, I'd readily change it.

okee dokee, thanks for explaining

bobcatfish · 2018-07-03T21:05:01Z

we should probably have some docs/comments somewhere that explain the scope for this, e.g. this is cant be reused for multiple Ingresses (tied to an ingress)

Good idea, thanks.

bobcatfish · 2018-07-03T21:08:06Z

how about something like:

"EventuallyMatchesBody is the same as MatchesBody, but will return false, nil if the response does not match, which will prompt polling methods such as SpoofingClient.Poll to retry the request"

I've incorporated your comment and changed the structure of EventuallyMatchesBody to mirror MatchesBody so that it's easier to compare them. I also added a comment describing the behavior of each failure condition to make it explicit why the false case does what it does.

WDYT?

google-prow-robot · 2018-07-03T21:08:42Z

@bobcatfish:

Details

In response to this:

Looking good! Still would love to see some docs in adding_tests.md :D

/lgtm
/approve
/meow space
/hold

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

google-prow-robot · 2018-07-03T21:08:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrcunha, bobcatfish, jonjohnsonjr, mattmoor

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:

~~test/OWNERS~~ [adrcunha,bobcatfish,mattmoor]

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

This little abstraction hoists the hostname spoofing into its own http client so that it is reusable outside of the context of waiting for an endpoint to return a specific response.

jonjohnsonjr

I think that's everything, please LMK if I missed anything. Thanks for the review 🤸‍♂️

jonjohnsonjr · 2018-07-03T21:25:54Z

jonjohnsonjr · 2018-07-03T21:26:00Z

jonjohnsonjr · 2018-07-03T21:26:55Z

To avoid the stutter. If another package wants to implement this, I prefer spoof.Interface to spoof.SpoofingClientInterface. If this ends up with more than one interface, I'd readily change it.

jonjohnsonjr · 2018-07-03T21:27:16Z

https://medium.com/stupid-gopher-tricks/ensuring-go-interface-satisfaction-at-compile-time-1ed158e8fa17

courtesy of @imjasonh

I'll add a comment.

jonjohnsonjr · 2018-07-03T21:28:49Z

Good idea, thanks.

jonjohnsonjr · 2018-07-03T21:39:18Z

I don't understand, can you elaborate?

jonjohnsonjr · 2018-07-03T21:47:37Z

I've incorporated your comment and changed the structure of EventuallyMatchesBody to mirror MatchesBody so that it's easier to compare them. I also added a comment describing the behavior of each failure condition to make it explicit why the false case does what it does.

WDYT?

jonjohnsonjr · 2018-07-03T22:50:02Z

/hold cancel

This should be ready for a final look @bobcatfish 👍

bobcatfish

/lgtm

EXCITING

bobcatfish · 2018-07-03T22:51:00Z

bobcatfish · 2018-07-03T23:03:39Z

part of why you dont understand is that i derped - i thought you were adding a new set of timeouts.

as it stands these will only exist here but then also in crd_checks - i guess 2 spots isnt so bad

bobcatfish · 2018-07-03T23:03:56Z

okee dokee, thanks for explaining

bobcatfish · 2018-07-03T23:04:30Z

…e-v1.16 [release-v1.16] Update OWNERS file

google-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 3, 2018

google-prow-robot requested review from adrcunha and bobcatfish July 3, 2018 04:00

jonjohnsonjr force-pushed the spoofing-client branch from d03ddde to 222f728 Compare July 3, 2018 04:02

jonjohnsonjr changed the title ~~Pull host spoofing logc into an http client~~ Pull host spoofing logic into an http client Jul 3, 2018

google-prow-robot assigned bobcatfish and tcnghia Jul 3, 2018

This was referenced Jul 3, 2018

Add blue/green conformance test #1460

Merged

Removes 503 as a retryable status code in tests #1298

Closed

google-prow-robot assigned adrcunha Jul 3, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2018

bobcatfish reviewed Jul 3, 2018

View reviewed changes

google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2018

jonjohnsonjr force-pushed the spoofing-client branch from 2fee235 to 8fcdc8e Compare July 3, 2018 20:44

google-prow-robot assigned mattmoor Jul 3, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2018

bobcatfish approved these changes Jul 3, 2018

View reviewed changes

google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2018

Pull host spoofing logic into an http client

790b57b

This little abstraction hoists the hostname spoofing into its own http client so that it is reusable outside of the context of waiting for an endpoint to return a specific response.

jonjohnsonjr force-pushed the spoofing-client branch from 297d095 to 790b57b Compare July 3, 2018 22:15

jonjohnsonjr commented Jul 3, 2018

View reviewed changes

google-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2018

bobcatfish approved these changes Jul 3, 2018

View reviewed changes

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2018

google-prow-robot merged commit 562140f into knative:master Jul 3, 2018

jonjohnsonjr deleted the spoofing-client branch July 3, 2018 23:20

openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/knative-serving that referenced this pull request Oct 3, 2025

Merge pull request knative#1458 from serverless-qe/sync-owners-releas…

bc93ef9

…e-v1.16 [release-v1.16] Update OWNERS file

Conversation

jonjohnsonjr commented Jul 3, 2018

Uh oh!

jonjohnsonjr commented Jul 3, 2018

Uh oh!

adrcunha commented Jul 3, 2018

Uh oh!

google-prow-robot commented Jul 3, 2018

Uh oh!

adrcunha commented Jul 3, 2018

Uh oh!

jonjohnsonjr commented Jul 3, 2018

Uh oh!

bobcatfish 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!

jonjohnsonjr Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonjohnsonjr commented Jul 3, 2018

Uh oh!

jonjohnsonjr commented Jul 3, 2018

Uh oh!

mattmoor commented Jul 3, 2018

Uh oh!

bobcatfish 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!

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!

google-prow-robot commented Jul 3, 2018

Uh oh!

google-prow-robot commented Jul 3, 2018

Uh oh!

jonjohnsonjr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonjohnsonjr Jul 3, 2018 •

edited

Loading