Fix sysconfig render when set-name is missing

[akutz]

Proposed Commit Message

Fix sysconfig render when set-name is missing

This patch addresses an issue where the absence of set-name
in a network configuration potentially results in an unintended
network configuration.

LP: #1855945

Additional Context

NA

Test Steps

make clean_pyc && PYTHONPATH="$(pwd)" python3 -m \
  pytest -v --log-level=DEBUG \
  -k test_iface_name_from_device_with_matching_mac_address \
  tests/unittests/test_net.py

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[TheRealFalcon]

Thanks for this fix. Overall LGTM.

The for loop is juuuust starting to get long enough that I'd consider putting the code you added into its own method/function, but I think it's ok as-is too. I'll leave it up to you if you want to change it.

Re: the circular import, I'm not a huge fan of the proposed solution as it adds more indirection and imports an underscore prefixed name.

Given that your current solution is already a workaround, I think a simpler (though maybe slightly hackier) workaround is to move the get_interfaces_by_mac import from the top of network_state.py to the top of the handle_ethernets method where you added it.

Long term, I think that most of the functions at the bottom of network_state.py (is_ipv6_addr through mask_and_ipv5_to_bcast_addr) probably make more sense in cloudinit/net/__init__.py. That's obviously a bigger refactor though, and not one we should do in this PR.

There are a number of test failures due to the moved function, but I think if we just move the import they should be fine.

[akutz]

Thanks for this fix. Overall LGTM.

The for loop is juuuust starting to get long enough that I'd consider putting the code you added into its own method/function, but I think it's ok as-is too. I'll leave it up to you if you want to change it.

Re: the circular import, I'm not a huge fan of the proposed solution as it adds more indirection and imports an underscore prefixed name.

Yeah, neither was I, but I also am not so actively involved with the project that I felt comfortable to refactor things too heavily.

Given that your current solution is already a workaround, I think a simpler (though maybe slightly hackier) workaround is to move the get_interfaces_by_mac import from the top of network_state.py to the top of the handle_ethernets method where you added it.

I thought about that, but get_interfaces_by_mac is a wrapper for other functions. Oh, are you suggesting relocating the actual import statement, not the method itself?

Long term, I think that most of the functions at the bottom of network_state.py (is_ipv6_addr through mask_and_ipv5_to_bcast_addr) probably make more sense in cloudinit/net/__init__.py. That's obviously a bigger refactor though, and not one we should do in this PR.

Indeed. See my previous comment about refactoring.

There are a number of test failures due to the moved function, but I think if we just move the import they should be fine.

I cannot figure out why as I added a shim to the original location. Why does moving the import fix things? Oh, because of the global symbol being changed?

[akutz]

I pushed #1336 to handle the refactored network functions. Once approved, I can rebase this PR on that one.

[TheRealFalcon]

Oh, are you suggesting relocating the actual import statement, not the method itself?

Yes, but since you did the refactor instead (thanks for that!), not needed.

[TheRealFalcon]

Thanks @akutz . Assuming CI passes, I'll merge this soon.