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

Split http adapter logic out of the broker #29

Merged
merged 10 commits into from
Jun 26, 2024

Conversation

BryanFauble
Copy link
Member

@BryanFauble BryanFauble commented Jun 2, 2024

Items left before this can be moved out of draft state:

  • engine.go: Determine the best way to pass along the context around the target adapter to the HTTP server. Since it's already being looked up in this code we shouldn't need to replicate the same code in the HTTP server, instead we should probably just pass along the context we found.
  • Code to be deleted out of the broker
  • sub_mgr.go: Is the implementation I wrote the best method to connect a request out to the HTTP server?
  • http server, http_client.go: I need to get some context about the target adapter for the request in the SendEvent() function. I need to know the host, method, headers, ect...
  • http server, http_client.go: When sending the response back - Will need to determine which is the correct timestamp to send back
  • Testing a full round trip -- Currently the code will, Request from cURL -> http server -> broker -> quickstart component -> broker -> http server (Dies)

Problem:

  1. The broker was responsible for executing HTTP requests that originate from components running the Kit SDK
  2. Executing HTTP requests without a "/" in it was erroring out

Solution:

  1. Copying the HTTP client code out of the broker and into the HTTP server running in the cluster
  2. Adding in a "/" if the host if empty

Testing:
Verified that running the changes allowed me to execute a full round trip and receive a response:

{
  "args": {}, 
  "data": "", 
  "files": {}, 
  "form": {}, 
  "headers": {
    "Accept-Encoding": "gzip", 
    "Host": "httpbin.org", 
    "User-Agent": "Go-http-client/2.0", 
    "X-Amzn-Trace-Id": "Root=1-66663c6c-61460ac946f419545031d61b"
  }, 
  "json": null, 
  "method": "GET", 
  "origin": "98.225.110.56", 
  "url": "https://httpbin.org/anything"
}

@BryanFauble BryanFauble requested a review from xadhatter June 2, 2024 22:29
core/component.go Outdated Show resolved Hide resolved
Copy link
Member

@xadhatter xadhatter left a comment

Choose a reason for hiding this comment

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

Overall this looks great. Thanks again for all the work. It appears there is more work to fit the adapter into the existing component mold. But I think the best approach is to massage it so it does and have the broker treat it like any other component instead of making exceptions for adapters.

api/kubernetes/v1alpha1/http_adapter_types.go Outdated Show resolved Hide resolved
components/broker/engine/engine.go Outdated Show resolved Hide resolved
core/event.go Outdated Show resolved Hide resolved
core/component.go Outdated Show resolved Hide resolved
components/httpsrv/server/server.go Outdated Show resolved Hide resolved
@BryanFauble BryanFauble changed the title Draft: Start to split http adapter logic out of the broker Split http adapter logic out of the broker Jun 16, 2024
@BryanFauble BryanFauble requested a review from xadhatter June 16, 2024 18:53
@BryanFauble BryanFauble marked this pull request as ready for review June 16, 2024 18:53
api/kubernetes/v1alpha1/http_adapter_types.go Outdated Show resolved Hide resolved
components/httpsrv/server/http_client.go Outdated Show resolved Hide resolved
api/vars.go Outdated Show resolved Hide resolved
components/broker/engine/engine.go Show resolved Hide resolved
@BryanFauble BryanFauble requested a review from xadhatter June 20, 2024 00:14
@xadhatter xadhatter merged commit 2ef325d into main Jun 26, 2024
2 checks passed
@xadhatter xadhatter deleted the split-http-adapter-logic-from-broker branch June 26, 2024 13:59
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.

2 participants