feat: intercept ssh keyManager facade in model proxy #1523
Conversation
kian99
force-pushed
the
intercept-ssh-command-followup-2
branch
3 times, most recently
from
c9abbad to
bceb99e
Compare
The model proxy will intercept calls to the keyManager facade and persist user keys in JIMM rather than passing these calls along to the Juju controller. This is being done in order to support the SSH proxy efforts. Ideally, in Juju 4 these methods would be done on the controller api and all this logic would move into the jujuapi package.
kian99
force-pushed
the
intercept-ssh-command-followup-2
branch
from
bceb99e to
58d722c
Compare
internal/rpcproxy/sshkeys.go
Outdated
| return jujuparams.StringsResults{}, err | ||
| } | ||
|
|
||
| var formatter func(key sshkeys.PublicKey) string |
There was a problem hiding this comment.
is this what juju does? it seems super suspicious.. i would expect formatting to happen in the cmd package
There was a problem hiding this comment.
Yes it's all done server side - https://github.com/juju/juju/blob/3.6/apiserver/facades/client/keymanager/keymanager.go#L111
It seems to be like this because of legacy reasons, i.e. the return type is a list of strings rather than some well defined type that the client can then format.
internal/rpcproxy/sshkeys.go
Outdated
| @@ -0,0 +1,123 @@ | |||
| // Copyright 2025 Canonical. | |||
There was a problem hiding this comment.
could we rename this file to keymanagerfacade.go?
There was a problem hiding this comment.
What about just keymanager.go? Changing it to that for now.
| // with the authenticated user. | ||
| func (s *keyManagerFacade) AddKeys(ctx context.Context, args jujuparams.ModifyUserSSHKeys) (jujuparams.ErrorResults, error) { | ||
| var res []jujuparams.ErrorResult | ||
| errF := func(err error, msg string) jujuparams.ErrorResult { |
There was a problem hiding this comment.
suggestion: just have func(msg string) jujuparams.ErrorResult.. you are already creating a message string every time you call errF, so appending the err.Error() each time, would be a minor change.
There was a problem hiding this comment.
Good point, as I was changing it I noticed jujuparams.Error also accepts an error code. If I want to fill that in I need to keep the error in the signature. I'm now populating the error code, so I've mostly left things the same, let me know what you think.
There was a problem hiding this comment.
then perhaps
func(msg, errorCode string) jujuparams.ErrorResult {
return jujuparams.ErrorResult{Error: &jujuparams.Error{
Code: string(errors.ErrorCode(err)),
Message: msg,
}}
}
but just a suggestion, i won't insist on it..
| // with the authenticated user. | ||
| func (s *keyManagerFacade) AddKeys(ctx context.Context, args jujuparams.ModifyUserSSHKeys) (jujuparams.ErrorResults, error) { | ||
| var res []jujuparams.ErrorResult | ||
| errF := func(err error, msg string) jujuparams.ErrorResult { |
There was a problem hiding this comment.
then perhaps
func(msg, errorCode string) jujuparams.ErrorResult {
return jujuparams.ErrorResult{Error: &jujuparams.Error{
Code: string(errors.ErrorCode(err)),
Message: msg,
}}
}
but just a suggestion, i won't insist on it..
Description
Currently this PR builds on #1521.
This PR allows JIMM to intercept model calls for the SSH KeyManager facade and direct the calls to JIMM's business logic rather than forwarding the calls to the backing Juju controllers.
This is being done in order to support the SSH proxy efforts.
Ideally, in Juju 4 these methods would be done on the controller api and all this logic would move into the jujuapi package. I've left comments to that effect. If before that point we want to refactor the model proxy then the logic has been cleanly split so that the code you see inside
internal/rpcproxy/sshkeys.gois all the logic/code that would go inside ofjujuapi.I've manually tested the changes by spinning up JIMM and adding a Juju 3.5 controller and tried the following commands (not in this specific order):
Fixes JUJU-7346
Engineering checklist