Decompress GZIP'd user data #1762
Conversation
cartermckinnon
force-pushed
the
gzip-userdata
branch
3 times, most recently
from
176e80f
to
706489f
Compare
cartermckinnon
force-pushed
the
gzip-userdata
branch
from
706489f
to
3fceead
Compare
cartermckinnon
force-pushed
the
gzip-userdata
branch
from
3fceead
to
d741996
Compare
@ndbaker1 latest rev adds support for this, PTAL. |
There was a problem hiding this comment.
I rewrote the unit tests for this config provider; they're more verbose but I think being explicit about the input is more clear and lets us more easily add test cases in the future.
There was a problem hiding this comment.
Did you do any integration testing here? Or is that covered by the existing integration tests?
There was a problem hiding this comment.
i like the change, thanks for skipping over the junk to setup mime documents and testing the actual provider interface
| type userDataConfigProvider struct { | ||
| userDataProvider userDataProvider | ||
| } |
There was a problem hiding this comment.
This is for testing, it allows us to mock out the IMDS calls
| return &userDataConfigProvider{} | ||
| return &userDataConfigProvider{ | ||
| userDataProvider: &imdsUserDataProvider{}, | ||
| } | ||
| } | ||
|
|
||
| func (ics *userDataConfigProvider) Provide() (*internalapi.NodeConfig, error) { |
There was a problem hiding this comment.
Why is ics the name for userDataConfigProvider?
There was a problem hiding this comment.
i think it's an artifact from when it was something like imdsConfigProvider? 🤣
maybe we could just generally name the config provider references cp
There was a problem hiding this comment.
tbh i like just calling it this but that's a Java-ism that gophers detest
There was a problem hiding this comment.
Haha. Let's change it to something that makes sense :P
| if part.Header.Get(contentEncodingHeader) == "gzip" { | ||
| decompressedNodeConfigPart, err := decompressIfGZIP(nodeConfigPart) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| nodeConfigPart = decompressedNodeConfigPart | ||
| } |
There was a problem hiding this comment.
Why do you need to make a check that it's gzip and then gracefully handle it not being gzip?
There was a problem hiding this comment.
wouldn't just calling decompressIfGZIP in all cases work better since its fallible in constant time and safe either way?
To not hit this branch you'd have to set the nodeConfig mediaType but forget to add the gzip encoding header, which im sure would only happen on accident. maybe handling it explicitly is more "proper", but not sure if its necessary
There was a problem hiding this comment.
MIME semantics dictate that if the section is GZIP'd, this header needs to be there. If a user (or library) is creating MIME documents that use GZIP but omit this header, that should fail because it's not conformant to the spec.
Generally, folks are only using GZIP compression via a library which should implement this properly -- folks aren't hand-writing and hand-compressing their MIME docs.
There was a problem hiding this comment.
Did you do any integration testing here? Or is that covered by the existing integration tests?
| if part.Header.Get(contentEncodingHeader) == "gzip" { | ||
| decompressedNodeConfigPart, err := decompressIfGZIP(nodeConfigPart) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| nodeConfigPart = decompressedNodeConfigPart | ||
| } |
There was a problem hiding this comment.
wouldn't just calling decompressIfGZIP in all cases work better since its fallible in constant time and safe either way?
To not hit this branch you'd have to set the nodeConfig mediaType but forget to add the gzip encoding header, which im sure would only happen on accident. maybe handling it explicitly is more "proper", but not sure if its necessary
| return &userDataConfigProvider{} | ||
| return &userDataConfigProvider{ | ||
| userDataProvider: &imdsUserDataProvider{}, | ||
| } | ||
| } | ||
|
|
||
| func (ics *userDataConfigProvider) Provide() (*internalapi.NodeConfig, error) { |
There was a problem hiding this comment.
i think it's an artifact from when it was something like imdsConfigProvider? 🤣
maybe we could just generally name the config provider references cp
There was a problem hiding this comment.
i like the change, thanks for skipping over the junk to setup mime documents and testing the actual provider interface
| type userDataProvider interface { | ||
| GetUserData() ([]byte, error) | ||
| } |
There was a problem hiding this comment.
nice way to make testing the provider interface easier 👍
|
/ci |
|
@cartermckinnon roger that! I've dispatched a workflow. 👍 |
|
@cartermckinnon the workflow that you requested has completed. 🎉
|
|
One flake:
Not related |
Issue #, if available:
Fixes #1734
Description of changes:
Adds support for GZIP-compressed user data.
The following scenarios are supported:
NodeConfigthat is compressed with GZIP.Content-Encoding: gzipheader).By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.