chore: move model proxy to separate pkg #1521
Conversation
The model proxy is currently housed within the rpc package. Originally I did this because the model proxy relied on the Juju RPC message definition declared in the rpc package. But this is likely not the right place for the model proxy as it has nothing to do with the dialer logic in the rpc package. Another supporting argument that the model proxy was in the wrong package is that the jimm package relies on the rpc package for dialing logic, but does not rely on the model proxy at all. And in fact, instead, the model proxy indirectly relies on the jimm package for business logic.
kian99
force-pushed
the
intercept-ssh-commands-followup-1
branch
from
1fd4cb0 to
6d1fe9c
Compare
| @@ -46,3 +48,14 @@ func proxy(src base.Stream, dst base.Stream) error { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func unexpectedReadError(err error) bool { | |||
There was a problem hiding this comment.
The streamproxy was using this bit of logic also used by the rpcproxy. So I just copied it over for now since it's reasonably small.
There was a problem hiding this comment.
i remember this logic to be the source of some flaky tests we had, we need to be careful copying this over. I would prefer to just create a rpcutils pkg instead
internal/testutils/rpctest/server.go
Outdated
There was a problem hiding this comment.
I took some of the testing logic that was shared by the (now split) rpc, rpcproxy and streamproxy packages and brought them into a new package called rpctest to reduce duplication. Just some helper functions to start a test server with a method to start a websocket connection with the test server.
internal/rpc/client_test.go
Outdated
There was a problem hiding this comment.
The tests in this file were mixed together with dialer tests and have been moved to rpcproxy_test.go since they are basic tests of the rpc proxy.
internal/rpcproxy/message.go
Outdated
There was a problem hiding this comment.
The Juju message struct has been copied for use in this package. This format isn't expected to change until Juju's API rework so I don't expect this to be an issue.
internal/rpcproxy/rpcproxy_test.go
Outdated
There was a problem hiding this comment.
These are the tests from client_test.go copied verbatim.
There was a problem hiding this comment.
This file has only 1 (large) test. It tests that the login side of the rpc proxy works so I've just put it into a file that better reflects that.
| @@ -203,16 +203,6 @@ func (c *Client) Call(ctx context.Context, facade string, version int, id, metho | |||
|
|
|||
| select { | |||
| case <-ch: | |||
| permissionsRequired, err := checkPermissionsRequired(ctx, *respMsg) | |||
There was a problem hiding this comment.
The rpc package was using checkPermissionsRequired which now lives in rpcproxy.go. I've removed this as there is no need to check for the permissionsRequired error here.
| @@ -46,3 +48,14 @@ func proxy(src base.Stream, dst base.Stream) error { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func unexpectedReadError(err error) bool { | |||
There was a problem hiding this comment.
i remember this logic to be the source of some flaky tests we had, we need to be careful copying this over. I would prefer to just create a rpcutils pkg instead
I'll hold off on an rpcutils pkg for now since I believe it would only contain that one function. |
Description
While working on intercepting SSH key methods in the model proxy and passing them to JIMM's business logic, I realised the model proxy should be in a different package. See the justification below.
The model proxy is currently housed within the rpc package. Originally I did this because the model proxy relied on the Juju RPC message definition declared in the rpc package. But this is likely not the right place for the model proxy as it has nothing to do with the dialer logic in the rpc package. Another supporting argument that the model proxy was in the wrong package is that the jimm package relies on the rpc package for dialing logic, but does not rely on the model proxy at all. And in fact, instead, the model proxy indirectly relies on the jimm package for business logic.
I also opted to move the streamproxy (used for proxying websocket connections that don't use Juju RPC like audit-logs) to its own package.
I've left some comments to explain the changes.
Partially fixes JUJU-7346
Engineering checklist