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

feat(akamai): unzip user content #1934

Merged
merged 1 commit into from
Oct 14, 2024
Merged

feat(akamai): unzip user content #1934

merged 1 commit into from
Oct 14, 2024

Conversation

guilhem
Copy link

@guilhem guilhem commented Sep 6, 2024

This pull request introduces functionality to handle compressed userdata in the Linode Metadata Service and includes the implementation of a utility function to decompress gzip data if needed. The most important changes include modifications to the fetchConfig function and the addition of a new utility function for gzip decompression.

Enhancements to userdata handling:

New utility function:

  • internal/providers/util/unzip.go: Added a new utility function GunzipIfNeeded to handle gzip decompression, including a helper function hasGzipMagicNumber to check for gzip magic numbers.

@guilhem guilhem force-pushed the linode-gzip branch 2 times, most recently from 6ae071d to 0d03da3 Compare September 6, 2024 13:51
@guilhem guilhem force-pushed the linode-gzip branch 2 times, most recently from fe468da to e6d24f9 Compare September 11, 2024 15:36
@guilhem guilhem marked this pull request as ready for review September 11, 2024 15:38
Copy link
Contributor

@tormath1 tormath1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me from a functional aspect. It has been tested successfully with Flatcar with both configuration (gzipped and not gzipped):

$ linode-cli linodes create \
    --metadata.user_data "$(cat config.json | gzip | base64 -w0)" \
    --label flatcar \
    ...
$ linode-cli linodes create \
    --metadata.user_data "$(cat config.json | base64 -w0)" \
    --label flatcar \
    ...

On the instance:

Flatcar Container Linux by Kinvolk developer 9999.0.0+tormath1-ignition-akamai for Akamai
core@172-236-99-29 ~ $ sudo cat /var/run/ignition.json
{"ignition":{"config":{"replace":{"verification":{}}},"proxy":{},"security":{"tls":{}},"timeouts":{},"version":"3.5.0-experimental"},"kernelArguments...

internal/providers/akamai/akamai.go Show resolved Hide resolved
@jlebon
Copy link
Member

jlebon commented Sep 13, 2024

Doesn't the response include a Content-Encoding header? We can enhance the fetcher such that if we pass a flag via FetchOptions (or e.g. set Compression to a special value), it automatically respects Content-Encoding and decompresses it. It would then probably make sense to set this flag for all providers and not just Akamai.

@tormath1
Copy link
Contributor

Doesn't the response include a Content-Encoding header? We can enhance the fetcher such that if we pass a flag via FetchOptions (or e.g. set Compression to a special value), it automatically respects Content-Encoding and decompresses it. It would then probably make sense to set this flag for all providers and not just Akamai.

At the moment it does not seem that Akamai is sending a Content-Encoding header:

core@172-233-211-243 ~ $ curl --silent -H "Metadata-Token: $TOKEN" http://169.254.169.254/v1/user-data | base64 -d | gzip -c -d -
{"ignition":{"version":"3.3.0"},"passwd":{"users":[{"name":"core","sshAuthorizedKeys":["ssh-rsa ...
core@172-233-211-243 ~ $ curl --head -H "Metadata-Token: $TOKEN" http://169.254.169.254/v1/user-data
HTTP/1.1 200 OK
Connection: keep-alive
Content-Length: 936
Content-Type: text/html; charset=utf-8
Date: Tue, 17 Sep 2024 08:02:27 GMT
Retry-After: 1
Server: nginx/1.18.0
X-Ratelimit-Limit: 10
X-Ratelimit-Remaining: 9
X-Ratelimit-Reset: 1726560149

@guilhem do you think that's something Akamai could consider?

It would then probably make sense to set this flag for all providers and not just Akamai.

That's discussed here I think: #1933 and from what I understand it's not the first time this topic is raised on Ignition. I'm just concerned about relying entirely on this Content-Encoding header - as it's up to each provider to respect this approach...

@guilhem
Copy link
Author

guilhem commented Sep 17, 2024

I will speak to the metadata team and ask about this header :)

@guilhem
Copy link
Author

guilhem commented Sep 25, 2024

So the header will not be added soon.
Another problem with this header is that we are embedding gzip in base64, so content is more text than gzip.

What can we do to have this PR merged?
Can we see a work on fetcher for another PR?

@guilhem guilhem requested a review from eljohnson92 October 1, 2024 08:13
@guilhem
Copy link
Author

guilhem commented Oct 1, 2024

@jlebon can we have a review? :)

Copy link
Collaborator

@prestist prestist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks great! Thank you for contributing and thank you for testing @tormath1 ❤️

Functionally it lgtm I have some small nits that I dont feel very strong about but would like to at least bring them up.

And should we add a comment about #1934 (comment) and link an rfe or somthing else

)

var (
gzipMagic = []byte{0x1F, 0x8B, 0x08}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is a little bit ambiguous, small nit. Maybe we can call it something like gzipSigniture or add a comment to explain the variable.

Preferably the former, and if we do change the name I think it would clean up some of the helper functions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I change the way to test unzip by using a standard function: http.DetectContentType.

I also added test to be sure that it's working as expected

@prestist is it something better than with “magic” variables :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guilhem absolutely! 💯

internal/providers/util/unzip.go Outdated Show resolved Hide resolved
@guilhem guilhem force-pushed the linode-gzip branch 2 times, most recently from dc6434a to 7e05458 Compare October 8, 2024 21:50
@prestist
Copy link
Collaborator

prestist commented Oct 9, 2024

@guilhem you have my approval, I just need to see green, looks like we have failing unit tests?

@guilhem
Copy link
Author

guilhem commented Oct 9, 2024

@guilhem you have my approval, I just need to see green, looks like we have failing unit tests?

Awesome 😎
Sadly it's a failure in all PR.

Copy link
Collaborator

@prestist prestist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again, and looking into whats going on with CI now. Thank you for bringing that to my attention.

Tracking bug here #1950

@prestist
Copy link
Collaborator

prestist commented Oct 11, 2024

@guilhem if you rebase your pr should be good now
#1952 landed.

@guilhem
Copy link
Author

guilhem commented Oct 11, 2024

@prestist rebase done.
I see Golang unit tests ok, so just waiting for all the other tests and it should be fine to merge.

@prestist prestist enabled auto-merge October 14, 2024 12:53
@prestist prestist merged commit 768e850 into coreos:main Oct 14, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants