-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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/aws: fixes double base64 encode #10871
Conversation
@@ -0,0 +1,13 @@ | |||
package aws |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than a generic utils.go
file, maybe this file should be named base64.go
and likewise for the test file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was looking for examples in other packages on what to name this file. In the aws
package, there's an auth_helper.go
. In other packages there are utils.go
, just not sure which is the best fit for this use case.
func base64Encode(data []byte) string { | ||
// Check whether the user_data is already Base64 encoded; don't double-encode | ||
if _, base64DecodeError := base64.StdEncoding.DecodeString(string(data)); base64DecodeError != nil { | ||
return base64.StdEncoding.EncodeToString(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since an error represents the expected path of not being encoded and therefore needing encoding, this may be clearer with a few more comments. // not encoded, so encode it
// already encoded
.
Or maybe splitting out an isBase64Encoded() bool
function instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good change to me, applying the same base64Encode logic consistently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
just a few comments on the comments
// Base64Encode encodes data if the input isn't already encoded useing base64.StdEncoding.EncodeToString. | ||
// If the input is already base64 encoded, return the original input unchanged. | ||
func base64Encode(data []byte) string { | ||
// Check whether the user_data is already Base64 encoded; don't double-encode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this context it's just data
, not user_data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks will fix
|
||
import "encoding/base64" | ||
|
||
// Base64Encode encodes data if the input isn't already encoded useing base64.StdEncoding.EncodeToString. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/useing/using/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops good catch
Add a utility that ensures a byte array is not doubly base64 encoded Fixes hashicorp#10786
83cf53c
to
ad56597
Compare
This LGTM, I'm going to rename the file after merge however. Thanks @curtisallen! |
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. |
Add a utility that ensures a byte array is not doubly base64 encoded
Reference https://play.golang.org/p/RnEBFCJ9h0
Fixes #10786