Repository navigation
Conversation
| EXTRACTORS = { | ||
| 'tgz' => Proxy::TFTP::ExtractorTgz, | ||
| 'iso' => Proxy::TFTP::ExtractorIso, | ||
| }.freeze |
There was a problem hiding this comment.
These extractors will cover Fedora, Red Hat, CentOS and clone, Debian and Ubuntu and clones. It is a good start, it is easy to add more if needed.
adamruzicka
left a comment
adamruzicka
left a comment
There was a problem hiding this comment.
just some tidbits after a quick read-through
| Proxy::TFTP.ensure_tftp_file_path(destination) | ||
|
|
||
| commands = [ | ||
| [[find_command('bsdtar'), '-xOf', archive.to_s, member], false], |
There was a problem hiding this comment.
Why bsdtar over gnu tar which is more likely to be available?
There was a problem hiding this comment.
I only found two utilities in RHEL9 which can extract ISOs without mounting: bsdtar and xorriso.
|
I did not expect such a quick reviews, I pushed an ugly draft so I can take this with me offline (not working today). Well, you guys are fast, this is a very rough code (it works but needs more love).
Yeah, I was not aware. This implementation in comparsion:
The other implementation:
There are some ideas which I like and I could inspire, e.g. using the command task for running the extraction subcommands. |
41e2210 to
eda9dea
Compare
|
Ok rebased, thanks for initial reviews. This is now ready for full reviews. Once I get approvals, I will work on the Foreman part and test this end to end. |
eda9dea to
5f59767
Compare
adamruzicka
left a comment
adamruzicka
left a comment
There was a problem hiding this comment.
Haven't tested, but it fits together nicely in my head. It still feels a bit go-esque though in some places
5f59767 to
3d50764
Compare
|
Thanks amended your comments, I actually squashed the two commits into one because I had to fix how "destination" alias works. Previously, it was only prefix so it worked like a prefix when file path was provided and for directory (ending with slash) it created the very same filename. But after I tested this also with Ubuntu, we need to be also able to download ISO alone. This is because last 3 releases of Ubuntu provides both netboot tarball and installation ISO, however, for Ubuntu installer the ISO is always required. Thus, it will make sense to download BOTH netboot tarball and the ISO: curl --fail --silent --show-error \
--header 'Content-Type: application/json' \
--data '{
"extract": {
"source": "https://releases.ubuntu.com/26.04/ubuntu-26.04-netboot-amd64.tar.gz",
"destination": "bootloader-universe/pxegrub2/ubuntu/26.04/amd64/netboot.tar.gz",
"type": "tgz",
"files": {
"bootloader-universe/pxegrub2/ubuntu/26.04/amd64/linux": "amd64/linux",
"bootloader-universe/pxegrub2/ubuntu/26.04/amd64/initrd.gz": "amd64/initrd",
"bootloader-universe/pxegrub2/ubuntu/26.04/amd64/grubx64.efi": "amd64/grubx64.efi",
"bootloader-universe/pxegrub2/ubuntu/26.04/amd64/shimx64.efi": "amd64/bootx64.efi"
},
"symlinks": {
"bootloader-universe/pxegrub2/ubuntu/26.04/amd64/boot.efi": "bootloader-universe/pxegrub2/ubuntu/26.04/amd64/grubx64.efi",
"bootloader-universe/pxegrub2/ubuntu/26.04/amd64/boot-sb.efi": "bootloader-universe/pxegrub2/ubuntu/26.04/amd64/shimx64.efi"
}
}
}' \
"${PROXY_URL}/tftp/fetch_boot_file"
# Ubuntu ISO is also needed
curl --fail --silent --show-error \
--header 'Content-Type: application/json' \
--data '{
"source": "https://releases.ubuntu.com/26.04/ubuntu-26.04-live-server-amd64.iso",
"destination": "bootloader-universe/pxegrub2/ubuntu/26.04/amd64/boot.iso"
}' \
"${PROXY_URL}/tftp/fetch_boot_file"The old flag called |
3d50764 to
a4db4c0
Compare
I don't think Foreman should send the exact path. In other places we send the smart-proxy/modules/tftp/server.rb Lines 161 to 186 in c2af3d3 |
That is exactly what the changed API point does, It also aligns how this works in Foreman core - the code there fully works with full (relative) TFTP paths when constructing the For this reason I think it is better to keep the "download this file in here" contract also for extraction. |
stejskalleos
left a comment
stejskalleos
left a comment
There was a problem hiding this comment.
First review round, not tested yet, but I plan to as a next step.
| return if response.is_a?(Net::HTTPSuccess) | ||
|
|
||
| unless response.is_a?(Net::HTTPRedirection) | ||
| raise "HTTP HEAD preflight failed for #{uri}: #{response.code} #{response.message}" |
There was a problem hiding this comment.
Could we explicitly fail when the error is Net::HTTPMethodNotAllowed or 501 Not Implemented so Foreman admins know that they have to set the tftp_http_download_preflight to false.
There was a problem hiding this comment.
There is no response for the fetch_boot_files it is async, however, you got a point that in the future code call sites without rescue could lead to 500 or something. I could do this:
class PreflightError < StandardError
attr_reader :uri, :upstream_status
def initialize(uri, message, upstream_status: nil)
@uri = uri
@upstream_status = upstream_status
super("HTTP HEAD preflight failed for #{uri}: #{message}")
end
def status_code
502
end
endThis would be ideal, but that is a lot of code for such a small helper. What you recommend would effectively mean simply returning response.value that is weird. I wish Ruby had Go error wrapping that is such a powerful thing.
You know what let me generate the PreflightError solution, that is the correct way.
| destination_provided = request_params.key?(:destination) || request_params.key?('destination') | ||
| destination = request_params[:destination] || request_params['destination'] | ||
| prefix = request_params[:prefix] || request_params['prefix'] | ||
| source = request_params[:source] || request_params['source'] || request_params[:path] || request_params['path'] |
There was a problem hiding this comment.
Can we rename it to source_iso to make the source type clearer?
There was a problem hiding this comment.
These two parameters are the original API ones, named "path" and "prefix". In the initial version of the patch, as you figured out, I had also aliases "source" and "destination". What I am trying to say is this "source" field is the original, not used for ISO/TGZ downloads at all.
Frankly, now that I see the final result, the initial assumption that it was a good idea to build this new call on the current fetch_boot_file call is not great - I think we can affort to make a completely new endpoint here because once universe is implemented for all OSes, then we can simply drop this endpoint completely.
| --header 'Content-Type: application/json' \ | ||
| --data '{ | ||
| "extract": { | ||
| "source": "https://mirror.stream.centos.org/10-stream/BaseOS/x86_64/os/images/boot.iso", |
There was a problem hiding this comment.
From the PR description:
source is just an alias for path (which remains unchanged)
But here I see the source as part of the extract, not as a new third field.
There was a problem hiding this comment.
These are actually different fields, there is "source" and "destination" (aliases) and "extract.source" and "extract.destination". I know this is confusing. As I said elsewhere now I think I should drop the modifications of the original API call and make a new one and keep the original untouched.
| --data '{ | ||
| "extract": { | ||
| "source": "https://mirror.stream.centos.org/10-stream/BaseOS/x86_64/os/images/boot.iso", | ||
| "destination": "bootloader-universe/pxegrub2/centos/10/x86_64/boot.iso", |
There was a problem hiding this comment.
Same as for source above.
| logger.info "TFTP: Queuing boot file extraction from #{options[:source]} to #{options[:archive]}" | ||
| # Keep the API asynchronous. The worker owns download completion and all | ||
| # extraction errors are logged there because the request has already ended. | ||
| Thread.new { perform_extract_boot_files(options) } |
There was a problem hiding this comment.
This is making the API endpoint asynchronous. We need to document this; otherwise, users might have the wrong impression when we return 200, only to later show an extraction error in the logs.
There was a problem hiding this comment.
I'm curious about the integration with the Foreman Create Host form. How will the Orchestration handle this extraction phase?
There was a problem hiding this comment.
This endpoint already was asynchronous (optionally), I am not changing that. But it was not obvious, let me explain.
The HttpDownloader util is async and up until now, the endpoint only called the download. Now, we need to download first, wait via the join call, and then make extraction. The extraction must run in a separate thread in order to achieve that.
| ensure_tftp_root | ||
| archive = tftp_path(archive_path) | ||
| files = options[:files] || {} | ||
| symlinks = options[:symlinks] || options[:symlink] || {} |
There was a problem hiding this comment.
Nit: Do we have to support both?
There was a problem hiding this comment.
Oh this was a typo, nice catch.
|
|
||
| post "/fetch_boot_file" do | ||
| log_halt(400, "TFTP: Failed to fetch boot file: ") { Proxy::TFTP.fetch_boot_file(params[:prefix], params[:path]) } | ||
| request_params = parse_json_body.merge(params) |
There was a problem hiding this comment.
Note to myself: With this, we support data from the body and from URL params,
while URL params will override the data sent via the body.
There was a problem hiding this comment.
Yes, the old API only supported URL params, the new one supports both.
| commands << [:bsdtar, bsdtar] if bsdtar | ||
| commands << [:xorriso, xorriso] if xorriso |
There was a problem hiding this comment.
What if both commands are available? Does it extract the iso file twice?
There was a problem hiding this comment.
Nope, few lines later the first one is picked up: commands.any?
Btw both tools are in RHEL, added to my notes to add this to packaging.
| super(args) | ||
| end | ||
|
|
||
| private |
There was a problem hiding this comment.
As a Go developer, you will surely appreciate this AI comment:
lib/proxy/http_download.rb:52-97— usingprivate/publicvisibility toggles around two small helpers is a bit go-esque; a smallprivate :preflight_http, :preflight_requestafter the constructor would be more idiomatic Ruby.
There was a problem hiding this comment.
I did not give LLM any instructions to imitate Go, not sure why it decided to do that. Sure, let's do that.
|
Ah sorry I was working on many more changes, I converted to draft until I am done with re-review. I will read all your comments, tho, thanks so far. |
|
Thanks for the review, I force-pushed. Addressed hopefully all your comments and I did revert the original API call and added a brand new one. You were apparently confused by the aliases and how it works, I think two separate endpoints are much cleaner. I am also updating the description to reflect the current implementation. |
4 participants
Adds new TFTP capabilities with extracting tarballs and ISO files. Use the test script to perform a live download test on CentOS or Debian to see it in action. See https://community.theforeman.org/t/managed-bootloader-universe/47609 for more context. The script follows the universe contract: https://docs.theforeman.org/nightly/Provisioning_Hosts/index-katello.html#grub2-uefi-boot-loader-and-the-bootloader-universe-structure
The API is idempotent - when called with same URL, it will only check if the file on server is newer, if not, download (and extraction) is skipped. If some files were deleted for any reason, extraction will be performed. The source archives are always kept in the TFTP directory.
The legacy API call was asynchronous, this is done by the downloader utility class. The new extraction code retain this behavior and spawns a new thread, then download the archive synchronously and performs the extraction logging everything to the log.
Additionally, an extra check is performed before download of any file via the utility downloader - HTTP HEAD is performed first, if the file does not exist it fails immediately. This will improve experience by emitting errors right away for incorrect URLs or misconfigured setups.
Writing of any files is now much more strict, paths are normalized, symlinked directories are not allowed, and symlinks are never extracted. But this should not affect existing contract.
It is a little bit more code that I expected, most of it are extra checks against path escape and various other attacks.
Example result when you run
extra/download_test.sh:The following symlinks are also created: