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

🌱 enable https download in ipxe #507

Closed
wants to merge 1 commit into from
Closed

Conversation

defo89
Copy link

@defo89 defo89 commented May 10, 2024

What this PR does / why we need it:
I am trying to use TLS in every part of the deployment chain and it seems that by default HTTPS is disabled in iPXE image.

image

As per https://ipxe.org/buildcfg/download_proto_https this commit would enable support (tested this in our environment).

@metal3-io-bot metal3-io-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 10, 2024
@metal3-io-bot
Copy link
Contributor

Hi @defo89. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@metal3-io-bot metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 10, 2024
@tuminoid
Copy link
Member

/ok-to-test

@metal3-io-bot metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 13, 2024
@tuminoid
Copy link
Member

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

1 similar comment
@tuminoid
Copy link
Member

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

@dtantsur
Copy link
Member

/approve
/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2024
@tuminoid
Copy link
Member

/retest

@tuminoid
Copy link
Member

/retest
CI should be better now.

Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

LGTM, but
/cc @Rozzii

@metal3-io-bot metal3-io-bot requested a review from Rozzii May 16, 2024 11:26
@Rozzii
Copy link
Member

Rozzii commented May 16, 2024

/hold, for https you need to also supply the certificates unless the specific url is compatible with ipxe's default credential chain (Mozzilla) also you have to configure the dnsmasq server of ironic with the same certs, we have already tooling to build https enabled ipxe on the fly or to use pre-built ipxe firmware https://github.com/metal3-io/utility-images.

What you are editing @defo89 in this PR is the default / statically compiled ipxe firmware (that is why it is compiled at container build time) that is suitable for those who have no special requirements, for those who need more advanced ipxe firmware I would recommend to deploy the ipxebuilder as an init container of the Ironic pod or just patch the Dockerfile downstream but IMO we should in no scenario edit the upstream docker file to get this functionality.

@Rozzii
Copy link
Member

Rozzii commented May 16, 2024

/hold

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 16, 2024
@metal3-io-bot
Copy link
Contributor

metal3-io-bot commented May 16, 2024

@defo89: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
metal3-centos-e2e-integration-test-main a6d12bb link true /test metal3-centos-e2e-integration-test-main

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@defo89
Copy link
Author

defo89 commented May 22, 2024

I would recommend to deploy the ipxebuilder as an init container of the Ironic pod or just patch the Dockerfile downstream
We use this in production - problem with this approach would be delayed pod readiness on reschedule/failure of the node previously running this pod.

@Rozzii Would you consider an option to override the IPXE image location per env var like its done with IPA_BASEURI env? E.g. I could provide them via volume. My issue atm is that ironic-image provides all means to leverage TLS (I could do it for ironic and httpd) and only this bit is missing.

@Rozzii
Copy link
Member

Rozzii commented May 22, 2024

I would recommend to deploy the ipxebuilder as an init container of the Ironic pod or just patch the Dockerfile downstream
We use this in production - problem with this approach would be delayed pod readiness on reschedule/failure of the node previously running this pod.

@Rozzii Would you consider an option to override the IPXE image location per env var like its done with IPA_BASEURI env? E.g. I could provide them via volume. My issue atm is that ironic-image provides all means to leverage TLS (I could do it for ironic and httpd) and only this bit is missing.

I plan to document this topic a bit more (or a lot more) in the future but I think we have such variables:

IPXE_CUSTOM_FIRMWARE_DIR can be used to specify where should the dnsmasq container find the custom firmware files
IPXE_SSL_PROTOCOL is should set the SSL/TLS versions that
IPXE_CERT_FILE the should be the same as the one embedded in the firmware
IPXE_KEY_FILE same as above
IPXE_TLS_PORT by default 8084 this is the port that the pxe firmware will use to call "home" and download the pxe files and the ramdisk

You are AFAIK the first user who would like to run things with custom PXE firmware so please keep me in the loop if you run into some issues.

@Rozzii
Copy link
Member

Rozzii commented Jun 28, 2024

Would it be okay to close this ticket @defo89 ?

@defo89
Copy link
Author

defo89 commented Jul 4, 2024

@Rozzii sure, we can close it. Thanks!

@defo89 defo89 closed this Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
Status: Done / Closed
Development

Successfully merging this pull request may close these issues.

None yet

5 participants