foreman icon indicating copy to clipboard operation
foreman copied to clipboard

Fixes #38391 - Allow displaying all host interfaces through API

Open adamlazik1 opened this issue 8 months ago • 2 comments

Currently, when you add an interfce to an existing host, for example with

hammer host interface create --host <hostname> --identifier test --managed false

and the try to GET request to display the interfaces with

curl --request GET --insecure --user admin:changeme https://`hostname -f`/api/v2/hosts/10/enc

you get an error about undefined method to_export. For details see the linked issue.

When creating interface with --managed false, the default class of the interface seems to be Nic::Base. Including the module exportable in the superclass gets rid of the error, the unmanaged interface is instead displayed as nil in the report. It should be possible to designate displayable attributes with attr_exportable if we agree that this would bring value to the report.

adamlazik1 avatar Apr 25 '25 09:04 adamlazik1

@ofedoren thank you for the feedback. I found out that methods in hosts controller allow nil interface type, which seems strange to me. The issue is fixed when I hardcode in the default type. I wonder if we should go with this approach or instead make type a required parameter. I suspect that the latter could possibly break workflows of some users. There is also question of what we should do with existing nil type interfaces. Would it be safe to change the type with a migration?

adamlazik1 avatar Apr 28 '25 14:04 adamlazik1

Thanks, @adamlazik1, but I still think there is something off:

#4833 was meant to fix an issue where we allowed to update the type of already created interface. This should remain. But at the same time it introduced allow_nil_type, which we are now trying to forbid 'cause it leads to either nil or Nic::Base type. With this PR we are going to make Nic::Managed to be the actual default type. I'd say we need to drop allow_nil_type.

allow_nil_type is essentially host.nil?, which seems to mean that we allow nil type in case the host is just being created. But even in the API docs we say it will be the default type if not provided 🤷.

Since we won't allow neithier nil nor base, we also need to update this line: https://github.com/theforeman/foreman/blob/develop/app/models/nic/base.rb#L349

Could we also add a test to simulate the issue is or at least is described in the RM ticket?

Sure, I removed allow_nil_type and and added a test to ensure that a default interface type is being assigned.

adamlazik1 avatar Jun 02 '25 15:06 adamlazik1

Thank you for reminding me. Updated the commit message.

adamlazik1 avatar Jun 25 '25 15:06 adamlazik1