feat: Support URI sources in `write_files` module #5505

LRitzdorf · 2024-07-10T22:20:36Z

Proposed Commit Message

feat(write_files): support URI content sources

This change adds an optional `source` key to the `write_files` module,
allowing users to specify a URI from which to load file contents. This
facilitates more flexible multi-part configurations, as file contents
can be managed via external sources such as independent Git
repositories.

Fixes GH-5500

Additional Context

Resolves #5500

Test Steps

Unit tests, included, cover:

file:// and HTTP URI functionality
Fallback behavior (i.e. use inline content if the provided URI fails to return usable data)
Updated schema validation

Merge type

Squash merge using "Proposed Commit Message"
Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

LRitzdorf · 2024-07-10T22:49:40Z

As I continue to work on these changes, I have a few questions for anyone with real cloud-init experience:

Do we want to support templating of the source URI?
The phone_home module seems to do this, but... only with the instance ID?
Which, if any, of the other kwargs supported by readurl should be exposed as module parameters?
Retry behavior and headers, in particular, stick out to me as settings that the user might want to control. On the other hand, how many parameters is too many?

This isn't really an appropriate example, since the `write_files` module now supports URL sources. (Also, this example wrote to `/tmp`, which conflicts with other advice on the examples page.)

...rather than passing a Paths object, which is just pain. Also, this way lets us pass None as well, if desired.

LRitzdorf · 2024-07-11T23:25:52Z

I have ~~one~~ (now two) new tests currently, which access content from a file:// URI. There should probably also be one for a "real" HTTP(S) URI as well, but I'm not sure about setting that up — would we also need to launch a lightweight HTTP server? Is there already some kind of support for this in the testing framework?

We should also test reading from a "real" (i.e. HTTP[S]) URI, though setting this up might be... involved. Would we need a local server?

And this is why we test.

TheRealFalcon

Thanks for this contribution! Overall, things look good here. I left a few inline comments as well as answers to your questions here.

Do we want to support templating of the source URI?

cloud-config supports jinja templating, so module specific templating shouldn't be necessary (if I understood the question correctly). The cc_phone_home snippet you found is very old and likely predates the ability to use jinja.

Which, if any, of the other kwargs supported by readurl should be exposed as module parameters?

That's a great question with no great answer. I prefer to be opinionated as to remove the number of options that need to be specified. The retry defaults you provided seem reasonable, though I'd throw a comment in specifying that they're completely arbitrary and just seemed like good defaults. It makes sense to include headers for any kind of token or basic auth, but I don't think we need more options than that. To accomodate this, I think it'd be better to make source a dictionary. It can then have subkeys like uri which would be required and headers that would be optional. If in the future we decide we need more than that, we can then add them to the dictionary.

I have one (now two) new tests currently, which access content from a file:// URI. There should probably also be one for a "real" HTTP(S) URI as well, but I'm not sure about setting that up — would we also need to launch a lightweight HTTP server? Is there already some kind of support for this in the testing framework?

For unit tests, we use responses to mock the server responses. For this effort I think that should be sufficient. Can we also get a test for the fallback behavior?

Let me know if you have any extra questions!

cloudinit/config/cc_write_files.py

LRitzdorf · 2024-07-12T20:37:41Z

Thanks for the feedback — that's very useful, especially since I'm new to cloud-init. Especially the "source as a dictionary" bit; that'll be much cleaner. Will have updates soon!

(Also, side note: there's already a test for the fallback behavior; that's the second one I alluded to in my previous edit. It currently uses a file URI, but that could change if desired.)

TheRealFalcon · 2024-07-12T21:18:17Z

(Also, side note: there's already a test for the fallback behavior; that's the second one I alluded to in my previous edit. It currently uses a file URI, but that could change if desired.)

Ah, sorry, I missed that. That works!

In response to PR feedback.

i.e. if the URL fails and no `content` was provided, don't even create a file.

This really made the linting system mad. Oops.

This is really important with such a large codebase, isn't it?

LRitzdorf · 2024-07-12T23:16:22Z

All right, comments addressed — and a fair bit of formatting fixed; tests under older Pythons should actually pass now!

FWIW, I did try changing the fallback test to use an HTTP URI, but that takes several seconds to fail (due to going through the OS's networking stack), so I've left it with a file URI. Not worth slowing down the tests for a tiny bit of added realism.

TheRealFalcon

Thanks for the updates! I left a few more minor inline comments, but I think it's really close now. I'm also approving the CI runs, so check for any failures there too.

cloudinit/config/cc_write_files.py

doc/module-docs/cc_write_files/example6.yaml

cloudinit/config/cc_write_files.py

LRitzdorf · 2024-07-18T23:59:43Z

Great, thanks! I've been running tests manually on my system as well, but it looks like I missed ruff. That's fixed now, along with your other comments!

holmanb

@LRitzdorf Thanks for this! I see a small change that we should make to the schema before merging. See my comments below.

cloudinit/config/schemas/schema-cloud-config-v1.json

holmanb

This looks fine to me. Thanks @LRitzdorf, and welcome to cloud-init!

TheRealFalcon

LGTM! Thanks for the contribution @LRitzdorf !

This change adds an optional `source` key to the `write_files` module, allowing users to specify a URI from which to load file contents. This facilitates more flexible multi-part configurations, as file contents can be managed via external sources such as independent Git repositories. Fixes canonicalGH-5500

This change adds an optional `source` key to the `write_files` module, allowing users to specify a URI from which to load file contents. This facilitates more flexible multi-part configurations, as file contents can be managed via external sources such as independent Git repositories. Fixes GH-5500

LRitzdorf added 4 commits July 10, 2024 14:47

cla: add LRitzdorf to CLA signers file

53cfae8

feat(write_files): add initial support for URL source

fc856f8

feat(write_files): set retries, SSL params for URL source

66b19ca

feat(write_files): document source parameter in jsonschema

bf777ea

LRitzdorf added 3 commits July 10, 2024 17:15

docs(write_files): describe URL source usage

d0422d7

docs(runcmd): remove wget example

44839b6

This isn't really an appropriate example, since the `write_files` module now supports URL sources. (Also, this example wrote to `/tmp`, which conflicts with other advice on the examples page.)

feat(write_files): pass SSL details dict directly

05cdc73

...rather than passing a Paths object, which is just pain. Also, this way lets us pass None as well, if desired.

LRitzdorf added 3 commits July 12, 2024 08:51

test(write_files): read content from file:// URI

561128e

We should also test reading from a "real" (i.e. HTTP[S]) URI, though setting this up might be... involved. Would we need a local server?

fix(write_files): properly retrieve content from URI

792a58e

And this is why we test.

test(write_files): fall back to "content" when "source" unavailable

c5adce9

LRitzdorf marked this pull request as ready for review July 12, 2024 15:02

TheRealFalcon added the CLA signed The submitter of the PR has signed the CLA label Jul 12, 2024

TheRealFalcon requested changes Jul 12, 2024

View reviewed changes

cloudinit/config/cc_write_files.py Outdated Show resolved Hide resolved

cloudinit/config/cc_write_files.py Outdated Show resolved Hide resolved

cloudinit/config/cc_write_files.py Outdated Show resolved Hide resolved

LRitzdorf added 7 commits July 12, 2024 15:21

fix(write_files): extract raw, not UTF-8, data from URL

4f97281

fix(write_files): function-ize "read URL with fallback"

9d25d9e

In response to PR feedback.

fix(write_files): don't fall back to empty content

1c27f77

i.e. if the URL fails and no `content` was provided, don't even create a file.

feat(write_files): support URI load with HTTP headers

ce9dbe6

test(write_files): load content from HTTP URI

6989bc9

fix(write_files): use typing's Optional, not union

3048a06

This really made the linting system mad. Oops.

fix(write_files): appease auto-formatter

519332e

This is really important with such a large codebase, isn't it?

LRitzdorf added 2 commits July 15, 2024 08:50

fix(write_files): headers is object, not array of them

662dc25

test(write_files): add headers to HTTP test case

1c55f6c

LRitzdorf requested a review from TheRealFalcon July 16, 2024 17:16

TheRealFalcon requested changes Jul 18, 2024

View reviewed changes

cloudinit/config/cc_write_files.py Show resolved Hide resolved

doc/module-docs/cc_write_files/example6.yaml Outdated Show resolved Hide resolved

cloudinit/config/cc_write_files.py Outdated Show resolved Hide resolved

fix(write_files): appease ruff format check

9c42a15

LRitzdorf added 3 commits July 18, 2024 16:24

fix(write_files): use less confusable variable names

041ed49

doc(write_files): update usage example with headers

3ae34bd

fix(write_files): write an empty file if no content or URI

48c0072

LRitzdorf requested a review from TheRealFalcon July 19, 2024 15:10

holmanb requested changes Jul 19, 2024

View reviewed changes

cloudinit/config/schemas/schema-cloud-config-v1.json Show resolved Hide resolved

fix(write_files): force source.headers values to be strings

368a214

holmanb approved these changes Jul 19, 2024

View reviewed changes

TheRealFalcon approved these changes Jul 22, 2024

View reviewed changes

TheRealFalcon merged commit 7c2d4fd into canonical:main Jul 22, 2024
23 checks passed

LRitzdorf deleted the write-files-url-source branch August 5, 2024 18:09

alexlovelltroy mentioned this pull request Aug 23, 2024

Inventory-Driven Post Boot Configuration OpenCHAMI/roadmap#42

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support URI sources in `write_files` module #5505

feat: Support URI sources in `write_files` module #5505

LRitzdorf commented Jul 10, 2024 •

edited

Loading

LRitzdorf commented Jul 10, 2024

LRitzdorf commented Jul 11, 2024 •

edited

Loading

TheRealFalcon left a comment

LRitzdorf commented Jul 12, 2024

TheRealFalcon commented Jul 12, 2024

LRitzdorf commented Jul 12, 2024

TheRealFalcon left a comment

LRitzdorf commented Jul 18, 2024

holmanb left a comment

holmanb left a comment

TheRealFalcon left a comment

feat: Support URI sources in write_files module #5505

feat: Support URI sources in write_files module #5505

Conversation

LRitzdorf commented Jul 10, 2024 • edited Loading

Proposed Commit Message

Additional Context

Test Steps

Merge type

LRitzdorf commented Jul 10, 2024

LRitzdorf commented Jul 11, 2024 • edited Loading

TheRealFalcon left a comment

Choose a reason for hiding this comment

LRitzdorf commented Jul 12, 2024

TheRealFalcon commented Jul 12, 2024

LRitzdorf commented Jul 12, 2024

TheRealFalcon left a comment

Choose a reason for hiding this comment

LRitzdorf commented Jul 18, 2024

holmanb left a comment

Choose a reason for hiding this comment

holmanb left a comment

Choose a reason for hiding this comment

TheRealFalcon left a comment

Choose a reason for hiding this comment

feat: Support URI sources in `write_files` module #5505

feat: Support URI sources in `write_files` module #5505

LRitzdorf commented Jul 10, 2024 •

edited

Loading

LRitzdorf commented Jul 11, 2024 •

edited

Loading