Skip to content

Explicit HTTP request method "Request"#223

Draft
subpop wants to merge 2 commits intoRedHatInsights:mainfrom
subpop:explicit-http-request
Draft

Explicit HTTP request method "Request"#223
subpop wants to merge 2 commits intoRedHatInsights:mainfrom
subpop:explicit-http-request

Conversation

@subpop
Copy link
Contributor

@subpop subpop commented Apr 1, 2024

Add a new "com.redhat.Yggdrasil1.Dispatcher1.Request" D-Bus method. This
method transmits data using an HTTP request. The existing "Transmit"
method now exclusively transmits data over the configured transport
(typically MQTT). Workers no longer need to set "RemoteContent" to true
or false. A worker that previously relied on the "RemoteContent" feature
will now receive the URL directly as the message body. It is the
worker's responsibility to unpack the message body and get the URL. The
"Request" method is available to provide an authenticated HTTP client to
make HTTP requests easier, though a worker is not required to use it.
Workers are free to choose either of the "Transmit" or the "Request"
method to return data; they are no longer forced to use one method or
the other.

@subpop subpop changed the title Explict HTTP request method "Request" Explicit HTTP request method "Request" Apr 16, 2024
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

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

I know that this is still draft, but I have few comments.

responseCode = resp.Code
responseMetadata = resp.Metadata
responseData = resp.Data
case <-time.After(1 * time.Second):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is timeout only one second? I think that this is too low value. Could it be configured somehow? Could it be e.g. part of MQTT message sent to worker? If the timeout would not be part of MQTT message, then some default value would be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the separation of Transmit and Request, this timeout now applies only
to the configured data transport's implementation of Tx. This timeout is
waiting for a response to the underlying transport Tx call. In practice, this
happens instantaneously with the MQTT transport because the MQTT transport does
not support responses. The HTTP transport does respond to Transmit requests
with the response of the underlying HTTP request. This is an unfortunate side
effect of supporting both MQTT and HTTP as data transports.

I am not sure whether it is worth exposing this as a configurable value. Maybe
we could re-use the existing config.HTTPTimeout configuration value?

@jirihnidek
Copy link
Contributor

I also tried to test it with https server and it did not work. Let's try this:

echo '{"method": "GET", "url": "https://httpbin.org/get", "headers": {"User-Agent": "yggdrasil-http-worker"}}' | \
    go run ./cmd/yggctl generate data-message --directive http - | \
    pub -broker tcp://localhost:1883 -topic yggdrasil/$(hostname)/data/in

@subpop subpop force-pushed the explicit-http-request branch 2 times, most recently from 8cbc29b to a97f019 Compare April 25, 2024 16:06
@subpop subpop force-pushed the explicit-http-request branch from a97f019 to 5a308c9 Compare July 10, 2024 16:09
subpop added 2 commits July 10, 2024 13:10
Add the User-Agent header to the HTTP request before calling
http.Client.Do. This ensures that callers of this Do method will have
the User-Agent header set automatically.

Signed-off-by: Link Dupont <link@sub-pop.net>
Add a new "com.redhat.Yggdrasil1.Dispatcher1.Request" D-Bus method. This
method transmits data using an HTTP request. The existing "Transmit"
method now exclusively transmits data over the configured transport
(typically MQTT). Workers no longer need to set "RemoteContent" to true
or false. A worker that previously relied on the "RemoteContent" feature
will now receive the URL directly as the message body. It is the
worker's responsibility to unpack the message body and get the URL. The
"Request" method is available to provide an authenticated HTTP client to
make HTTP requests easier, though a worker is not required to use it.
Workers are free to choose either of the "Transmit" or the "Request"
method to return data; they are no longer forced to use one method or
the other.

Signed-off-by: Link Dupont <link@sub-pop.net>
@subpop subpop force-pushed the explicit-http-request branch from 5a308c9 to 3b2bf3d Compare July 10, 2024 17:10
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