Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/template: Clouding provider headers #4189

Merged
merged 2 commits into from
Dec 8, 2015
Merged

provider/template: Clouding provider headers #4189

merged 2 commits into from
Dec 8, 2015

Conversation

sthulb
Copy link

@sthulb sthulb commented Dec 7, 2015

Adds headers to the parts and MIME-Version to the overall document.

So, it appears that mime/multipart has some issues: https://golang.org/src/mime/multipart/writer.go#L97

Because it doesn't sort headers before writing them, they could end up out of order, this would mean the diff could change.

The original implmentation was missing headers to denote mime version &
content transfer encoding, this caused issues with EC2.

Signed-off-by: Simon Thulbourn <[email protected]>
@jen20
Copy link
Contributor

jen20 commented Dec 7, 2015

Hi @sthulb, any reason that the tests are removed?

@sthulb
Copy link
Author

sthulb commented Dec 7, 2015

I've actually added them back locally.

I've forked the mime/multipart package to sort part headers, something being merged in to go for 1.7 with my patch.

I'll write a better summary when I'm back on my laptop.

On 7 Dec 2015, at 17:51, James Nugent [email protected] wrote:

Hi @sthulb, any reason that the tests are removed?


Reply to this email directly or view it on GitHub.

@sthulb
Copy link
Author

sthulb commented Dec 7, 2015

It turns out that go doesn't sort MIMEHeaders before writing them, so this obviously breaks terraform since terraform relied on diffs if resources being the same all of the time.

I've made a patch and I'll submit it tomorrow morning/tonight to Go for their 1.7 release (sometime in Feb). In the mean time, I'll keep a fork of the mime/multipart package for terraform with my patch in.

Go issue: golang/go#13522

@sthulb
Copy link
Author

sthulb commented Dec 7, 2015

Gerrit patch: https://go-review.googlesource.com/#/c/17497/

@sthulb
Copy link
Author

sthulb commented Dec 7, 2015

@jen20 I'd consider this complete now. :)

@sthulb sthulb changed the title Cloudinit provider headers provider/template: Clouding provider headers Dec 8, 2015
@jen20
Copy link
Contributor

jen20 commented Dec 8, 2015

@sthulb That change makes sense - hopefully it will make it into the next release of Go. In the meantime using your fork with the same changeset seems fine to me. I'll merge this into the feature branch in the HashiCorp repo, and we can review from there! Thanks!

jen20 added a commit that referenced this pull request Dec 8, 2015
provider/template: Clouding provider headers
@jen20 jen20 merged commit f1cdb67 into hashicorp:cloudinit-provider Dec 8, 2015
@ghost
Copy link

ghost commented Apr 29, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants