Code review comment for lp:~jwharshaw/juju-core/fixlogbuild

I completely agree. My approach was a very quick approach to solving the
problem, but as you have pointed out, does have some issues. I do like the
approach you suggested, and I will get right on to implementing it. Thanks
for your feedback!
On Mar 19, 2014 5:34 AM, <email address hidden> wrote:

> Thanks very much for working on this. It's much appreciated, and
> something that really needs to be done. I have a suggestion for an
> alternative approach below. Also, this should really have a test - I
> suggest a test in apiserver that makes some of the relevant calls and
> uses the LogMatches checker to check that the messages look appropriate.
>
>
>
> https://codereview.appspot.com/77500045/diff/1/state/
> apiserver/apiserver.go
> File state/apiserver/apiserver.go (right):
>
> https://codereview.appspot.com/77500045/diff/1/state/
> apiserver/apiserver.go#newcode121
> state/apiserver/apiserver.go:121: r :=
> regexp.MustCompile(`"Password":"(.*?)"`)
> I'm not that keen on this approach - since this happens on every rpc
> request and reply (even if it does happen in debugging mode only), I
> think it is worth being a bit more efficient here.
>
> Also, this won't work correctly if the password contains a quote
> character, we don't need to compile the regexp on every request, and
> some secrets aren't named "Password".
>
> I *think* we should be able to do a better job by type-switching on
> body. It will be some more code, but we can potentially do a better job.
>
> Something like this, perhaps?
>
> func (n *requestNotifier) ServerRequest(hdr *rpc.Header, body
> interface{}) {
> if hdr.Request.Type == "Pinger" && hdr.Request.Action == "Ping" {
> return
> }
> body = elideSecrets(body)
> logger.Debugf("<- [%X] %s %s", n.id, n.tag(),
> jsoncodec.DumpRequest(hdr, body))
> }
>
> func (n *requestNotifier) ServerReply(req rpc.Request, hdr *rpc.Header,
> body interface{}, timeSpent time.Duration) {
> if hdr.Request.Type == "Pinger" && hdr.Request.Action == "Ping" {
> return
> }
> body = elideSecrets(body)
> logger.Debugf("-> [%X] %s %s %s %s[%q].%s", n.id, n.tag(),
> timeSpent, jsoncodec.DumpRequest(hdr, body), req.Type, req.Id,
> req.Action)
> }
>
> func (n *requestNotifier) elideSecrets(body interface{}) interface{} {
> switch body := body.(type) {
> case params.EntityPasswords:
> newBody := params.EntityPasswords{}
> newBody.Changes = append(newBody.Changes, body.Changes)
> for i := range newBody.Changes {
> newBody.Changes[i].Password = "<elided>"
> }
> return newBody
> case params.EnvironmentSet:
> newBody := params.EnvironmentSet{
> Config: make(map[string]interface{})
> }
> // TODO change environs.Environ so we can find secret
> // attributes without needing a full environment
> // configuration. In the meantime, just hide
> // everything.
> for attr := range body.Config {
> newBody.Config[attr] = "<elided>"
> }
> case params.EnvironmentGetResults:
> newBody := params.EnvironmentGetResults{
> Config: make(map[string]interface{})
> }
> // TODO as above
> for attr := range body.Config {
> newBody.Config[attr] = "<elided>"
> }
> cfg, err := config.New(body.Config)
> if err != nil {
> return body
> }
> case params.Creds:
> body.Password = "<elided>"
> return body
> }
> return body
> }
>
> https://codereview.appspot.com/77500045/
>