[RHCLOUD-30604] return ansible_host with the high level connection status response

[tahmidefaz]

What?

Explain what the change is linking any relevant JIRAs or Issues.

Why?

Consider what business or engineering goal does this PR achieves.

How?

Describe how the change is implemented. Any noteable new libaries, APIs, or features.

Testing

Did you add any tests for the change?

Anything Else?

Any other notes about the PR that would be useful for the reviewer.

Secure Coding Practices Checklist Link

• https://github.com/RedHatInsights/secure-coding-checklist

Secure Coding Checklist

• Input Validation
• Output Encoding
• Authentication and Password Management
• Session Management
• Access Control
• Cryptographic Practices
• Error Handling and Logging
• Data Protection
• Communication Security
• System Configuration
• Database Security
• File Management
• Memory Management
• General Coding Practices

[dehort]

Hey Tahmid,
I talked with Rex about this a while back. He mentioned that the ansible_host field needs to be along side the host id. More info in this comment in the jira.

At this point, I think the best thing to do is to add a new field along side the existing systems field that contains the host id and ansible_host field. Maybe we call call this new field systems_details (naming things is difficult)?

It feels kind of gross to add a new field to contain this info, but we would break existing clients if we changed the existing systems field from an array of strings to an array of objects. :(

Let me know if you have any questions. I'll try to look into this a bit more closely shortly.

[tahmidefaz]

Hey Tahmid, I talked with Rex about this a while back. He mentioned that the ansible_host field needs to be along side the host id. More info in this comment in the jira.

At this point, I think the best thing to do is to add a new field along side the existing systems field that contains the host id and ansible_host field. Maybe we call call this new field systems_details (naming things is difficult)?

It feels kind of gross to add a new field to contain this info, but we would break existing clients if we changed the existing systems field from an array of strings to an array of objects. :(

Let me know if you have any questions. I'll try to look into this a bit more closely shortly.

I just moved the ansible_host under a new systems_info object. For example:

"systems_info": [
            {
                "ansible_host": "test-ansible-host",
                "host_id": "fe30b997-c15a-44a9-89df-c236c3b5c540"
            }
        ]

[dehort]

/retest

[dehort]

This is looking good.

I think there is an issue where the satellite connected hosts always have an empty string for their ansible_host even if they have a valid ansible_host in inventory.

This can be seen by modifying the inventory mock to add the ansible_host to the satellite connected host.

diff --git internal/api/connectors/inventory/inventory_mock.go internal/api/connectors/inventory/inventory_mock.go
index 7113f73..161dad8 100644
--- internal/api/connectors/inventory/inventory_mock.go
+++ internal/api/connectors/inventory/inventory_mock.go
@@ -38,6 +38,7 @@ func (this *inventoryConnectorMock) GetHostConnectionDetails(
                SatelliteInstanceID: &satelliteInstanceID,
                SatelliteVersion:    &satelliteVersion,
                SatelliteOrgID:      &satelliteOrgID,
+               AnsibleHost: &ansibleHost,
        }
 
        directConnectDetails := HostDetails{

The way I see to fix it would be to stop passing the ansibleHost parameter to the formatConnectionResponse() method and instead add the ansiblehost data to host array that is passed to that method. Of course, each method that calls the formatConnectionResponse() method will need to be modified to build that list so that it contains the host id and ansible_host.

I hope that makes sense.

This is complicated. :) Ping me if you need more details, etc.

[tahmidefaz]

This is looking good.

I think there is an issue where the satellite connected hosts always have an empty string for their ansible_host even if they have a valid ansible_host in inventory.

This can be seen by modifying the inventory mock to add the ansible_host to the satellite connected host.

diff --git internal/api/connectors/inventory/inventory_mock.go internal/api/connectors/inventory/inventory_mock.go
index 7113f73..161dad8 100644
--- internal/api/connectors/inventory/inventory_mock.go
+++ internal/api/connectors/inventory/inventory_mock.go
@@ -38,6 +38,7 @@ func (this *inventoryConnectorMock) GetHostConnectionDetails(
                SatelliteInstanceID: &satelliteInstanceID,
                SatelliteVersion:    &satelliteVersion,
                SatelliteOrgID:      &satelliteOrgID,
+               AnsibleHost: &ansibleHost,
        }
 
        directConnectDetails := HostDetails{

The way I see to fix it would be to stop passing the ansibleHost parameter to the formatConnectionResponse() method and instead add the ansiblehost data to host array that is passed to that method. Of course, each method that calls the formatConnectionResponse() method will need to be modified to build that list so that it contains the host id and ansible_host.

I hope that makes sense.

This is complicated. :) Ping me if you need more details, etc.

@dehort I was initially thinking that ansible_host is only present with direct connect host, so did not pay much attention to satellite. Anyhow, ansible_host values should get return correctly for all connection types. 👍

[dehort]

Looking good.

I left one (kind of) nit-picky comment.

[dehort]

Can this be renamed to something like HostDetails or something a bit more generic?

Its possible that we will need to add more info about the hosts later on and then this name makes less sense.

Naming is hard :(

[tahmidefaz]

I changed this to HostAtribute. Unfortunately, HostDetails is used quite extensively thoughout the codebase to mean a different thing, so didn't want to add to the confusion here.

[tahmidefaz]

@dehort I am occupied with the PlatEx stuff. If you or someone from the integrations team can take over this task, that'd be great!

[dehort]

Hey @tahmidefaz, yep, I'll take it from here. Thank you!

[dehort]

/retest