provider/azure: strip off UTF-8 BOM from responses #6835

Merged
merged 1 commit into from Jan 19, 2017

Conversation

Projects
None yet
3 participants
Member

axw commented Jan 19, 2017

When handling responses from the AAD REST endpoint,
strip off the UTF-8 BOM, if any, from JSON responses.
These are only sometimes returned.

Fixes https://bugs.launchpad.net/juju/+bug/1657448

Member

axw commented Jan 19, 2017

QA

add-credential azure, interactive, 10 times

provider/azure/internal/ad/client.go
+ if _, err := io.Copy(&b, resp.Body); err != nil {
+ return err
+ }
+ var decodeErr error
@rogpeppe

rogpeppe Jan 19, 2017

Owner

Given that we're now reading the whole body, why not:

 var utf8BOM = []byte("\ufeff")    // global
 data, err := ioutil.ReadAll(resp.Body)
 if err != nil ...
 if err := json.Unmarshal(bytes.TrimPrefix(data, utf8BOM), &oe); err != nil ... {
 }
 resp.Body = io.NopCloser(bytes.NewReader(data))

?

BTW I think it might be better to strip the BOM in all JSON decoding code - surely
this isn't the only place that can include it?

@axw

axw Jan 19, 2017

Member

BTW I think it might be better to strip the BOM in all JSON decoding code - surely
this isn't the only place that can include it?

Maybe, but we don't control the other code. That will need to go in go-autorest. The reason this code exists at all is because the AAD (Azure Active Directory) API endpoint is a little different to the other Azure API endpoints. Different error structures. So I wouldn't be surprised if this is the only place that the BOM shows up, because it's probably getting routed to a different backend.

@axw

axw Jan 19, 2017

Member

As for the simplification: I'm intentionally preserving the behaviour of leaving the body as-is for the caller. But I think you're right, we should just chuck out the BOM and pretend it was never there. It's no use to anyone.

EDIT: and now I see yours preserves it too, I'm just a bit thick. I'll go with that.

Owner

rogpeppe commented Jan 19, 2017

LGTM with one thought, thanks.

provider/azure: strip off UTF-8 BOM from responses
When handling responses from the AAD REST endpoint,
strip off the UTF-8 BOM, if any, from JSON responses.
These are only sometimes returned.

Fixes https://bugs.launchpad.net/juju/+bug/1657448
Member

axw commented Jan 19, 2017

$$bill$$

Contributor

jujubot commented Jan 19, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 7e38b44 into juju:develop Jan 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment