RPC provider #343

jacobweinstock · 2023-08-12T00:19:53Z

What does this PR implement/change/remove?

This adds an RPC provider. The idea of the RPC provider is to enable users the ability to interoperate with existing bespoke out of band system or as a stop gap for BMCs/protocols that bmclib does not have a provider for yet. Internal bespoke out of band systems will generally not be accepted into BMCLIB.

So users can run a small web endpoint that implements the RPC contract. The RPC provider sends a request to the user's web endpoint. The web endpoint will then translate the call to the out of band protocol/tool/etc that is needed to communicate with the BMC.

Checklist

Tests added
Similar commits squashed

The HW vendor this change applies to (if applicable)

The HW model number, product name this change applies to (if applicable)

The BMC firmware and/or BIOS versions that this change applies to (if applicable)

What version of tooling - vendor specific or opensource does this change depend on (if applicable)

Description for changelog/release notes

codecov · 2023-08-12T00:21:25Z

Codecov Report

Patch coverage: 67.36% and project coverage change: +1.75% 🎉

Comparison is base (08172f2) 42.68% compared to head (61c88e3) 44.43%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
+ Coverage   42.68%   44.43%   +1.75%     
==========================================
  Files          44       50       +6     
  Lines        3730     4015     +285     
==========================================
+ Hits         1592     1784     +192     
- Misses       1951     2027      +76     
- Partials      187      204      +17

Files Changed	Coverage Δ
option.go	`20.00% <0.00%> (-1.16%)`	⬇️
providers/rpc/experimental.go	`0.00% <0.00%> (ø)`
client.go	`55.55% <7.14%> (-3.52%)`	⬇️
providers/rpc/payload.go	`50.00% <50.00%> (ø)`
providers/rpc/http.go	`56.75% <56.75%> (ø)`
providers/rpc/signature.go	`69.38% <69.38%> (ø)`
providers/rpc/rpc.go	`77.46% <77.46%> (ø)`
providers/rpc/logging.go	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

providers/rpc/rpc.go

providers/rpc/doc.go

providers/rpc/experimental.go

providers/rpc/rpc.go

providers/rpc/http.go

client.go

chrisdoherty4

Nice to see this shaping up.

I noticed there's a fair amount of flexibility and I'm not sure I appreciate why. For example, there are options to specify the HTTP content type and HTTP method used: whats the motivation for being so flexible when we are ultimately defining the contract?

In the case of content type, given we assume JSON does it make sense to even expose that in such a raw manner? (this makes me wonder if I really understand whats going on 🤔).

providers/rpc/rpc.go

providers/rpc/payload.go

chrisdoherty4 · 2023-09-06T14:47:19Z

providers/rpc/http.go

+			c.Logger.Info("rpc notification details", kvs...)
+		}
+	}()
+	defer resp.Body.Close()


I don't recall how defer works but are you sure this closes the right handle given we reset the body on l.74?

(it may be the case that Go stores the point in memory it'll jump to on return at the time of defer, hence I'm not sure if this is a problem)

As im passing in resp *http.Response, i am already handling the close in the caller so i think i will just remove this Close. Thoughts?

providers/rpc/rpc.go

jacobweinstock · 2023-09-06T15:24:09Z

Nice to see this shaping up.

I noticed there's a fair amount of flexibility and I'm not sure I appreciate why. For example, there are options to specify the HTTP content type and HTTP method used: whats the motivation for being so flexible when we are ultimately defining the contract?

In the case of content type, given we assume JSON does it make sense to even expose that in such a raw manner? (this makes me wonder if I really understand whats going on 🤔).

Thanks for the review, Chris.

whats the motivation for being so flexible when we are ultimately defining the contract?

With the goal of the RPC provider being to provide interoperability with existing/bespoke out-of-band systems, for me, flexibility is key. POST vs PATCH or even a GET for the HTTP method will provide that flexibility while not compromising our payload contracts. The content type, while we will always send json, could have a requirement in the existing system. i.e. application/vnd.api+json or application/json or some other custom type.

chrisdoherty4 · 2023-09-08T14:14:32Z

@jacobweinstock Sounds good, thanks for the explanation.

providers/rpc/http.go

chrisdoherty4 · 2023-09-08T17:01:49Z

providers/rpc/rpc.go

+	// handle the response
+	rp, err := c.handleResponse(resp, kvs)
+	if err != nil {
+		return ResponsePayload{}, err
+	}
+
+	return rp, nil


Suggested change

// handle the response

rp, err := c.handleResponse(resp, kvs)

if err != nil {

return ResponsePayload{}, err

}

return rp, nil

// handle the response

return c.handleResponse(resp, kvs)

The idea of the RPC provider is to enable users the ability to interoperate with existing bespoke out of band system or as a stop gap for BMCs/protocols that bmclib does not have a provider for yet. Internal bespoke out of band systems will generally not be accepted into BMCLIB. So users can run a small web endpoint that implements the RPC contract. The RPC provider sends a request to the user's web endpoint. The web endpoint will then translate the call to the out of band protocol/tool/etc that is needed to communicate with the BMC. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

Hopping to help with understanding of the code base. Signed-off-by: Jacob Weinstock <[email protected]>

Reorg the Config struct to make clearer the different areas of concern. Remove internal package. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

This doc.go will detail the why of this package and its use. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

This was just experimentation. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

As bespoke out-of-band systems will require a high level of flexibility to interoperate properly, this provides even more flexibility around the RequestPayload while still maintaining a simple contract for the bmclib. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

Improvements for cleaner/clearer code. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

This is more core to the provider than the request. Used to submit the request not configure the request, so to speak. Signed-off-by: Jacob Weinstock <[email protected]>

Rename interface{} -> any. This change is compatible back to Go 1.18. Signed-off-by: Jacob Weinstock <[email protected]>

The struct name was changed so this updates the receiver variable to match. Signed-off-by: Jacob Weinstock <[email protected]>

Clarify its use with an example. Signed-off-by: Jacob Weinstock <[email protected]>

Let the caller handle the close. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

This guards against dos attacks that might send very large responses. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

Check status code and response payload in the Open method to validate the endpoint is conformant with the response contract. Update checking of the response error code as well as if it is nil. This will make sure we dont error out when a response contains a value for the error instead of just nil. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

Reading from io.Readers and then creating a nop closer and dealing with closing the readers was more difficult than it needed to be. Signed-off-by: Jacob Weinstock <[email protected]>

chrisdoherty4 · 2023-09-12T13:06:24Z

providers/rpc/rpc.go

+	}
+	defer resp.Body.Close()
+	respBuf := new(bytes.Buffer)
+	if _, err := io.Copy(respBuf, resp.Body); err != nil {


I think this may still be at risk of a DoS because Copy will read everything from Body into the buffer. That could consume a CPU and chew threw all available memory.

I think the only way to safely read is to chunk (https://pkg.go.dev/io#CopyN) checking the total bytes read as we go.

ah yeah, fair. i'll use CopyN and move the copy below the content length check.

providers/rpc/rpc.go

Moving the response body copy to after the content length check and only coping the resp.Contentlegnth should give us more defense against malicious actors. Signed-off-by: Jacob Weinstock <[email protected]>

joelrebel

Thanks for the efforts here 🚀

jacobweinstock marked this pull request as ready for review September 1, 2023 19:25

jacobweinstock requested review from joelrebel and chrisdoherty4 September 1, 2023 19:26

jacobweinstock changed the title ~~[WIP] RPC provider~~ RPC provider Sep 1, 2023

joelrebel reviewed Sep 5, 2023

View reviewed changes

providers/rpc/rpc.go Outdated Show resolved Hide resolved

providers/rpc/doc.go Show resolved Hide resolved

jacobweinstock requested a review from joelrebel September 5, 2023 16:21

joelrebel reviewed Sep 6, 2023

View reviewed changes

providers/rpc/experimental.go Outdated Show resolved Hide resolved

providers/rpc/rpc.go Outdated Show resolved Hide resolved

providers/rpc/rpc.go Outdated Show resolved Hide resolved

providers/rpc/http.go Outdated Show resolved Hide resolved

client.go Show resolved Hide resolved

chrisdoherty4 reviewed Sep 6, 2023

View reviewed changes

jacobweinstock requested review from joelrebel and chrisdoherty4 September 6, 2023 16:28

chrisdoherty4 reviewed Sep 8, 2023

View reviewed changes

providers/rpc/http.go Outdated Show resolved Hide resolved

chrisdoherty4 reviewed Sep 8, 2023

View reviewed changes

jacobweinstock force-pushed the rpc-provider branch from a7f6b65 to 1c74198 Compare September 11, 2023 15:27

jacobweinstock added 14 commits September 11, 2023 09:28

Update linter version

d428e47

Signed-off-by: Jacob Weinstock <[email protected]>

Reorganize the RPC provider:

b1fcbab

Hopping to help with understanding of the code base. Signed-off-by: Jacob Weinstock <[email protected]>

Simplify rpc code again:

f43dbfc

Reorg the Config struct to make clearer the different areas of concern. Remove internal package. Signed-off-by: Jacob Weinstock <[email protected]>

Add log message if RPC configs merge fails:

cc64911

Signed-off-by: Jacob Weinstock <[email protected]>

Add initial doc.go for rpc package:

7a7f906

This doc.go will detail the why of this package and its use. Signed-off-by: Jacob Weinstock <[email protected]>

Clean up go.mod

b07c6a5

Signed-off-by: Jacob Weinstock <[email protected]>

Revert changes to the client test:

efac18f

This was just experimentation. Signed-off-by: Jacob Weinstock <[email protected]>

Add code comments

aa6ff78

Signed-off-by: Jacob Weinstock <[email protected]>

Add RPC to supported BMC interfaces in readme

6fddc6c

Signed-off-by: Jacob Weinstock <[email protected]>

Fix code comment

121a59a

Signed-off-by: Jacob Weinstock <[email protected]>

Invert nil check, be sure to close response body:

dbb08da

Improvements for cleaner/clearer code. Signed-off-by: Jacob Weinstock <[email protected]>

Move registering the RPC to its own func

ac389b6

Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock added 8 commits September 11, 2023 09:28

Move http client to the top level:

e961fc4

This is more core to the provider than the request. Used to submit the request not configure the request, so to speak. Signed-off-by: Jacob Weinstock <[email protected]>

Rename Config -> Provider:

2ecfd71

Rename interface{} -> any. This change is compatible back to Go 1.18. Signed-off-by: Jacob Weinstock <[email protected]>

Update receiver variable:

655d6ee

The struct name was changed so this updates the receiver variable to match. Signed-off-by: Jacob Weinstock <[email protected]>

Update code comment for IncludedPayloadHeaders:

588de30

Clarify its use with an example. Signed-off-by: Jacob Weinstock <[email protected]>

Remove body.Close on passed in *http.Response:

7c6461a

Let the caller handle the close. Signed-off-by: Jacob Weinstock <[email protected]>

Fix some linting issues

8c7de93

Signed-off-by: Jacob Weinstock <[email protected]>

Add max allowed response content size:

45e4d38

This guards against dos attacks that might send very large responses. Signed-off-by: Jacob Weinstock <[email protected]>

Error on unknown content size:

cc82aad

Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock force-pushed the rpc-provider branch from 1c74198 to cc82aad Compare September 11, 2023 15:28

jacobweinstock added 4 commits September 11, 2023 14:06

Add an example for the configuring and using the RPC provider:

ffe1702

Signed-off-by: Jacob Weinstock <[email protected]>

Add doc comment on HMAC

4d9b3c7

Signed-off-by: Jacob Weinstock <[email protected]>

Copy req/resp bodies to buffers:

6440d17

Reading from io.Readers and then creating a nop closer and dealing with closing the readers was more difficult than it needed to be. Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock requested a review from chrisdoherty4 September 11, 2023 21:51

chrisdoherty4 reviewed Sep 12, 2023

View reviewed changes

providers/rpc/rpc.go Show resolved Hide resolved

Be more defensive with the response body:

61c88e3

Moving the response body copy to after the content length check and only coping the resp.Contentlegnth should give us more defense against malicious actors. Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock requested a review from chrisdoherty4 September 13, 2023 03:45

joelrebel approved these changes Sep 13, 2023

View reviewed changes

chrisdoherty4 approved these changes Sep 13, 2023

View reviewed changes

jacobweinstock added feature ready-to-merge bmclib v2 labels Sep 13, 2023

mergify bot merged commit b1092ac into bmc-toolbox:main Sep 13, 2023
7 checks passed

jacobweinstock deleted the rpc-provider branch September 13, 2023 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC provider #343

RPC provider #343

jacobweinstock commented Aug 12, 2023

codecov bot commented Aug 12, 2023 •

edited

Loading

chrisdoherty4 left a comment

chrisdoherty4 Sep 6, 2023

jacobweinstock Sep 8, 2023

jacobweinstock commented Sep 6, 2023

chrisdoherty4 commented Sep 8, 2023

chrisdoherty4 Sep 8, 2023

chrisdoherty4 Sep 12, 2023 •

edited

Loading

jacobweinstock Sep 13, 2023

joelrebel left a comment

RPC provider #343

RPC provider #343

Conversation

jacobweinstock commented Aug 12, 2023

What does this PR implement/change/remove?

Checklist

The HW vendor this change applies to (if applicable)

The HW model number, product name this change applies to (if applicable)

The BMC firmware and/or BIOS versions that this change applies to (if applicable)

What version of tooling - vendor specific or opensource does this change depend on (if applicable)

Description for changelog/release notes

codecov bot commented Aug 12, 2023 • edited Loading

Codecov Report

chrisdoherty4 left a comment

Choose a reason for hiding this comment

chrisdoherty4 Sep 6, 2023

Choose a reason for hiding this comment

jacobweinstock Sep 8, 2023

Choose a reason for hiding this comment

jacobweinstock commented Sep 6, 2023

chrisdoherty4 commented Sep 8, 2023

chrisdoherty4 Sep 8, 2023

Choose a reason for hiding this comment

chrisdoherty4 Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

jacobweinstock Sep 13, 2023

Choose a reason for hiding this comment

joelrebel left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 12, 2023 •

edited

Loading

chrisdoherty4 Sep 12, 2023 •

edited

Loading