-
Notifications
You must be signed in to change notification settings - Fork 15
ci: add ci workflow #102
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
base: main
Are you sure you want to change the base?
ci: add ci workflow #102
Conversation
Summary of ChangesHello @yih-redhat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a foundational Continuous Integration (CI) workflow to automatically validate the FIDO Device Onboard (FDO) process for Fedora bootc images. By integrating this workflow, the project ensures that any new changes are tested against a full device onboarding scenario, from image creation and VM provisioning to the successful completion of the FDO handshake, thereby enhancing reliability and preventing regressions in the critical onboarding path. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new CI workflow for integration testing go-fdo-server. The workflow sets up a test environment, builds a custom bootc ISO image, provisions a VM, and runs FDO onboarding tests. While this is a great addition for automated testing, the review has identified a critical security vulnerability due to a committed private SSH key. Additionally, there are several high and medium severity issues concerning script robustness, resource management, and maintainability that should be addressed to make the CI process more reliable and secure.
fce199e to
a5d5ddc
Compare
|
Hi @mmartinv , I found the integration test script in this pull request is also executed in e2e test (which should not) and caused failures in check jobs. |
dcec3dc to
7bcd10d
Compare
mmartinv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we do not use current packit integration instead of testing-farm-as-github-action so we don't have to build the RPMs twice? If we use packit integration the correct server rpms will be already installed, we don't need to install them.
The test uses functions from the utils.sh but the test doesn't look like existing tests at all. Would it be possible to move all the common functions to the utils.sh file and make the proper integration?
7447a65 to
a7bf865
Compare
test/fmf/plans/bootc-e2e.fmf
Outdated
|
|
||
| discover: | ||
| how: fmf | ||
| test: bootc-test-onboarding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| test: bootc-test-onboarding | |
| filter: tag:bootc |
Similar change needs to be done for rpm-e2e plan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| tag: | ||
| - fdo | ||
| - e2e | ||
| - onboarding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - onboarding | |
| - onboarding | |
| - bootc |
Similar change needs to be done to all the rpm tests so we can filter the tests by a meaningful tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
test/fmf/plans/bootc-e2e.fmf
Outdated
| processors: ">= 2" | ||
| memory: ">= 6 GB" | ||
| disk: ">= 20 GB" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be wrong, just add the required CPUs/Memory/Disk, no >= symbols
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without >=, tmt reports error when prepare vm.
With >=, tmt is more flexible to provide vms.
test/bootc/utils.sh
Outdated
| sed -i "/%post --erroronfail/i\ | ||
| sshkey --username admin \"${ssh_key_pub}\"" "${new_ks_file}" | ||
| sed -i "/bootc switch/a\ | ||
| go-fdo-client --blob /boot/device_credential --debug device-init http://${manufacturer_ip}:8038 --device-info=iot-device --key ec256" "${new_ks_file}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| go-fdo-client --blob /boot/device_credential --debug device-init http://${manufacturer_ip}:8038 --device-info=iot-device --key ec256" "${new_ks_file}" | |
| go-fdo-client --blob /boot/device_credential --debug device-init ${manufacturer_protocol}://${manufacturer_ip}:${manufacturer_port} --device-info=iot-device --key ec256" "${new_ks_file}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
test/bootc/utils.sh
Outdated
| configure_libvirt | ||
|
|
||
| # Configure firewall | ||
| configure_firewall |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the libvirtd service and firewalld services to the services array with the variables at the beginning of this file.
services+=("libvirtd" "firewalld")
Then declare the {start,stop,configure}_service_{libvirtd,firewalld} functions and implement them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried it after we discussed, but failed due to some kind of libvirt service ip not found error. Didn't remember the exact error message, but seems the configure_service_libvirt seems to look for service ip.
test/bootc/utils.sh
Outdated
| log_info "Install required packages" | ||
| local packages=(golang make podman jq qemu-img qemu-kvm libvirt-client libvirt-daemon-kvm libvirt-daemon virt-install ansible-core firewalld lorax gobject-introspection) | ||
| dnf install -y "${packages[@]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move the installation of each package to the function that needs it and they need to be installed if not running on tmt: if the test is running in tmt the packages must be installed in the prepare section so they are available to all the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
- install all required packages in tmt plan file.
- move packages to functions that needs it, and add condition if [[ ! -v "PACKIT_COPR_RPMS" ]] to install it.
test/bootc/utils.sh
Outdated
| log_info "Adding host entries for FDO services in vm" | ||
| sudo ssh "${ssh_options[@]}" -i "${ssh_key}" "admin@${guest_ip}" \ | ||
| 'echo -e "192.168.100.1 manufacturer\n192.168.100.1 rendezvous\n192.168.100.1 owner" | sudo tee -a /etc/hosts > /dev/null' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do this in the libvirt configuration function, Please see: https://serverfault.com/questions/976107/how-to-assign-vm-hostname-by-host-definition-in-libvirt-virtual-network-interf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to add go-fdo-server hostname and ip address in /etc/hosts file of virtual machine provisioned in the test script, so the go-fdo-client in virtual machine can connect to go-fdo-servers to finish fido device onboard.
I think there is no libvirt configuration in the bootc virtual machine.
test/bootc/utils.sh
Outdated
| local os_variant=$1 | ||
| local boot_args=$2 | ||
| local guest_ip="192.168.100.50" | ||
| sudo qemu-img create -f qcow2 "/var/lib/libvirt/images/disk.qcow2" 10G | ||
| sudo restorecon -Rv /var/lib/libvirt/images/ | ||
| sudo virt-install --name="fdo-bootc" \ | ||
| --disk "path=/var/lib/libvirt/images/disk.qcow2,format=qcow2" \ | ||
| --ram 3072 \ | ||
| --vcpus 2 \ | ||
| --network "network=integration,mac=34:49:22:B0:83:30" \ | ||
| --os-type linux \ | ||
| --os-variant "${os_variant}" \ | ||
| --cdrom "/var/lib/libvirt/images/install.iso" \ | ||
| --boot "${boot_args}" \ | ||
| --nographics \ | ||
| --noautoconsole \ | ||
| --wait=-1 \ | ||
| --noreboot | ||
|
|
||
| log_info "Starting VM..." | ||
| sudo virsh start "fdo-bootc" | ||
|
|
||
| # Wait for SSH | ||
| if ! wait_for_ssh $guest_ip; then | ||
| return 1 | ||
| fi | ||
|
|
||
| # FDO onboarding process | ||
| log_info "Get device initialization voucher guid" | ||
| guid=$(curl --fail --silent "${manufacturer_url}/api/v1/vouchers" | jq -r '.[0].guid') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to a run_device_initialization function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/bootc/utils.sh
Outdated
| log_info "Sending Ownership Voucher to the Owner" | ||
| send_manufacturer_ov_to_owner "${manufacturer_url}" "${guid}" "${owner_url}" | ||
|
|
||
| log_info "Setting or updating Owner Redirect Info (RVTO2Addr)" | ||
| set_or_update_owner_redirect_info "${owner_url}" "${owner_service_name}" "${owner_dns}" "${owner_port}" | ||
|
|
||
| sleep 60 | ||
|
|
||
| log_info "Running FIDO Device Onboard" | ||
| sudo ssh "${ssh_options[@]}" -i "${ssh_key}" "admin@${guest_ip}" \ | ||
| 'set -o pipefail; sudo go-fdo-client --blob /boot/device_credential onboard --key ec256 --kex ECDH256 --debug | tee /tmp/onboarding.log' | ||
|
|
||
| log_info "Check Device Onboard result" | ||
| result=$(sudo ssh "${ssh_options[@]}" -i "${ssh_key}" "admin@${guest_ip}" 'cat /tmp/onboarding.log') | ||
| if [[ ! "$result" =~ "FIDO Device Onboard Complete" ]]; then | ||
| return 1 | ||
| fi | ||
|
|
||
| return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to the run_test function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
1e61daf to
24f9e68
Compare
Signed-off-by: Yi He <[email protected]>
This pull request:
Setup and start go-fdo-server, includes manufacture, rendevous and owner
build a fedora 43 bootc anaconda-iso image with go-fdo-client installed
provision a vm with anaconda-iso file
device initialize during provision in %post kickstart file
after vm boot, TO0 is triggered manually in vm
after TO0, onboarding is triggered manually in vm
Please be noted the last two steps should run automatically after vm boot, will update test script later once go-fdo-client can do it automatically.