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.

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

agent/http: refactor proxy for simpler error handling #271

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

roobre
Copy link
Member

@roobre roobre commented Aug 1, 2023

Description

This PR introduces a small refactor in the HTTP proxy, so it is easier to read the logic behind delays, faults, and proxies.

Fixes #252

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make test) and all tests pass.
  • I have run relevant e2e test locally (make e2e-xxx for agent, disruptors, kubernetes or cluster related changes)
  • Any dependent changes have been merged and published in downstream modules

@roobre roobre requested a review from pablochacin August 1, 2023 14:03
Copy link
Collaborator

@pablochacin pablochacin left a comment

Choose a reason for hiding this comment

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

Hi @roobre Overall, the changes make sense and improve the code, except for the usage of a channel for waiting. I suggest an alternative approach that I think is simpler to reason about.

}()
}
// proxy proxies a request to the upstream URL.
// Request is performed immediately, but response won't be copied to the client until wait has been consumed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find confusing this wait channel. I think it obscures the purpose of the proxy which should in my opinion only do one thing.

I suggest that instead of passing this argument, you use a wrapper function that waits before (or after) the given delay and then calls a function passed as an argument, given that both proxy and fault has the same signature.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main purpose of the wait channel is to allow to start waiting after the request has been made. This way, the latency to the upstream service is "included" in the fault latency. I wasn't able to figure out a way of doing this with a wrapper, as this wait necessarily needs to happen after performing the request, but before writing the response.

Do you have an idea for how to get rid of the channel but maintain this feature?

Otherwise, we can just drop the feature and always sleep before initiating the request upstream.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have any off the top of my head, but at the very least, I would pass the delay and make the wait inside the functions instead of passing a channel. That makes the purpose of the argument clear. I don't see any advantage on using a channel.

_, _ = io.Copy(rw, response.Body)
}

// fault consumes wait and then writes to rw the configured error code and body.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment here. I find this wait argument confusing and cannot see how it improves or simplifies the code or the error handling.

@roobre roobre requested a review from pablochacin August 2, 2023 17:59
@roobre roobre force-pushed the http-proxy-refactor branch 2 times, most recently from bc492c1 to 4c63bff Compare August 2, 2023 21:22
Copy link
Collaborator

@pablochacin pablochacin left a comment

Choose a reason for hiding this comment

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

LGTM. Made a couple of comments about the naming of some functions because I find them odd.

@roobre
Copy link
Member Author

roobre commented Aug 5, 2023

Rebased and squashed, feel free to merge if you don't have further concerns @pablochacin.

@pablochacin pablochacin merged commit 72c4e48 into main Aug 7, 2023
6 checks passed
@pablochacin pablochacin deleted the http-proxy-refactor branch August 7, 2023 11:12
pablochacin added a commit that referenced this pull request Aug 11, 2023
pablochacin added a commit that referenced this pull request Aug 11, 2023
roobre added a commit that referenced this pull request Aug 16, 2023
roobre added a commit that referenced this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return a meaningful errors if agent cannot connect upstream host
2 participants