Skip to content

Add support for translating ifcfg kernel cmdline argument to ip#1957

Merged
teclator merged 4 commits intoinfo_paramfrom
agama_dracut_ifcfg
Feb 1, 2025
Merged

Add support for translating ifcfg kernel cmdline argument to ip#1957
teclator merged 4 commits intoinfo_paramfrom
agama_dracut_ifcfg

Conversation

@teclator
Copy link
Copy Markdown
Contributor

@teclator teclator commented Jan 28, 2025

Problem

As there is no linuxrc in Agama we wanted to explore the possibility of translating linuxrc network-related boot arguments into the dracut equivalent (which is the supported and recommended method for the new installation media).

Solution

A basic translation of the ifcfg boot argument has being implemented as a prof of concept but as the nm-initrd-generator parses de cmdline as a cmdline hook it is too early for doing some kind of checks about the devices present in the system and trying to match against the device name and the mac address therefore any kind of pattern except the '*' wildcard.

Supported examples

  • ifcfg=*=dhcp
    ip=dhcp

  • ifcfg=eth0=dhcp
    ip=eth0:dhcp

  • ifcfg=eth0.10=192.168.0.100/24,192.168.0.1
    vlan=eth0.10:eth0 ip=192.168.0.100::192.168.0.1:24::eth0.10

  • ifcfg="eth0=192.168.0.33/24 10.0.0.100/24,192.168.0.1,192.168.0.1 10.0.0.1,suse.de"
    ip=192.168.0.33::192.168.0.1:24::eth0 nameserver=192.168.0.1 nameserver=10.0.0.1 ip=10.0.0.100:::24::eth0

The parser is also added as a cmdline hook, we explored the possibility to call it as a initqueue/settle hook, in that case we could try to call the nm_generate_connections function directly passing it the getcmdline result. At that point we can check the devices present in /sys/class/net/ doing some filtering based in the name or mac address but for example the try function requires to run the configuration in order to check if the interface was configured or not for continuing trying... for that kind of support we might need to add it to NetworkManager and the nm-initrd-generator.

Testing

  • Tested manually

@teclator teclator marked this pull request as ready for review January 29, 2025 09:15
@teclator teclator requested a review from jreidinger January 29, 2025 09:15
Copy link
Copy Markdown
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

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

I'm approving it because reading the code it makes sense, but be aware I do not know too much bash scripting.

Also, I trust in the fact that it has been manually tested. Which make me thing that in a no far future we could evaluate the introduction of a shell testing tool like https://github.com/shellspec/shellspec or similar. Just saying 🙄

Copy link
Copy Markdown
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

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

Question: Does it deserves an entry in some changelog file?

@teclator
Copy link
Copy Markdown
Contributor Author

Question: Does it deserves an entry in some changelog file?

Probably yes, I will add it, we probably should add an unique entry as this PR is against Jozef branch.

@dgdavid
Copy link
Copy Markdown
Contributor

dgdavid commented Jan 30, 2025

Question: Does it deserves an entry in some changelog file?

Probably yes, I will add it, we probably should add an unique entry as this PR is against Jozef branch.

Ah, right. I overlooked that small detail :) Thanks!

done

if [[ $# -eq 0 ]]; then
echo "IFCFG 0 options given, must be wrong"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the text looks a bit strange..I would write something like "no parameters given to ifcfg option. Skipping" ( or what action happen after return 1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

was adde mainly for debugging we could even skip it.. or modify as suggested

if str_starts "$1" "dhcp"; then
autoconf="$1"
if [ "$autoconf" = "dhcp4" ]; then
echo "AUTOCONF"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this looks like debug output, not?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yep, look like a leftover

case $autoconf in
"dhcp" | "dhcp6")
if [ "$interface" = "*" ]; then
echo "ip=${1}" >>"/etc/cmdline.d/40-agama-network.conf"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not $conf_path here? it should be same, not?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

should be there ... probably missing when modified...

fi
;;
*)
echo "No supported option ${1}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would write "Unsupported option ${1}"

echo "nameserver=${nameserver%% *}" >>$conf_path
nameserver="${nameserver#* }"
done
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have to say that without automatic testing I would be very scared to touch this file to avoid breaking anything.

@teclator teclator merged commit 7d4bfd8 into info_param Feb 1, 2025
@teclator teclator deleted the agama_dracut_ifcfg branch February 1, 2025 17:29
@imobachgs imobachgs mentioned this pull request Feb 26, 2025
imobachgs added a commit that referenced this pull request Feb 26, 2025
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.

3 participants