Skip to content
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

feat: collect AWS public IPv4 as cannonical fact #3732

Closed

Conversation

ezr-ondrej
Copy link
Member

@ezr-ondrej ezr-ondrej commented Apr 6, 2023

All Pull Requests:

Check all that apply:

  • Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
  • Is this PR to correct an issue?
  • Is this PR an enhancement?

Complete Description of Additions/Changes:

This tries to add an AWS public IP address, that is not known to the machine itself.

Issue: #3731

Refs ESSNTL-4588

This is yet a draft, I'd love to hear if my approach would work here, so far I've pulled just IP as it's easier do demonsrate and agree upon my intended approach :)

facts = dict(
insights_id=valid_uuid_or_None(_safe_parse(insights_id)),
machine_id=valid_uuid_or_None(_safe_parse(machine_id)),
bios_uuid=valid_uuid_or_None(_safe_parse(bios_uuid)),
subscription_manager_id=valid_uuid_or_None(submanid.data if submanid else None),
ip_addresses=ips.data if ips else [],
ip_addresses=ips_list,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The canonical facts JSON schema is not (to my knowledge) strictly defined anywhere, but it is assumed to include a certain set of keys (in other words, it follows a de facto standard). There are other implementations in code that (for instance, rhc and in cloud-connector) that assume the current schema is adhered to. I suggest you take care when making changes to canonical facts. This change is safe, since you don't appear to be altering the key strings at all, but the above history of canonical facts is important to keep in mind.

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the intention was not to modify canonical facts, this looks like a copy and paste mistake. From what I understand, AWS/Azure/GCP id and types are present here because they provide a good value for host identity, these are unique idents. Our use case is different tho - we want to report a public IP address which can very often change, for example when user shutdowns and starts an instance again (a new public IP is assigned) or it can be even manually reconfigured by user.

Therefore we only want to register the new datasource and send it along with other facts. This is done via RegistryPoint() function if I understand correctly.

@xiangce
Copy link
Contributor

xiangce commented Apr 7, 2023

@ezr-ondrej - This PR added a new Spec to the core collection, for any new specs, it's required to get it approved before collecting it. Could you please go to the insights-core-assets Gitlab repo to get it approved? I added the link of the repo to the ESSNTL-4588, please have a look.

@lzap lzap mentioned this pull request Apr 12, 2023
3 tasks
@lzap
Copy link
Contributor

lzap commented Apr 12, 2023

@ezr-ondrej can you close this one in fav of: #3741

I am taking over, thanks!

@ezr-ondrej
Copy link
Member Author

Happy to, thanks for taking over 🧡

@ezr-ondrej ezr-ondrej closed this Apr 13, 2023
@ezr-ondrej ezr-ondrej deleted the add_aws_public_ip branch April 13, 2023 17:31
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.

5 participants