openstack: Add attribute OPENSTACK_INSTANCE_UUID #931

pierreprinetti · 2023-05-25T15:46:20Z

In the OpenStack attributes, the field OPENSTACK_INSTANCE_ID reports
the instance ID fetched from the EC2-compatibility-layer metadata.
However, that identifier (in the format i-something) does not match
the one used to refer to machines in the OpenStack API.

With this patch, I propose to add a new OpenStack attribute
OPENSTACK_INSTANCE_UUID that reports the actual identifier for
OpenStack instances in the context of the OpenStack API.

Implementation-wise, this patch adds enables fetching the UUID from both
the config-drive and from the Nova metadata service endpoint. In order
to retain compatibility with old versions of the metadata service, this
patch uses the old endpoint 2012-08-10.

Fixes #930

pierreprinetti · 2023-05-26T09:34:54Z

I have tested this patch on an OpenStack VM.

Metadata endpoint:

$ ./afterburn --provider openstack --attributes ./attr-metadata.txt
May 26 09:23:19.498 WARN failed to locate config-drive, using the metadata service API instead
May 26 09:23:19.518 INFO Fetching http://169.254.169.254/openstack/2012-08-10/meta_data.json: Attempt #1
May 26 09:23:20.110 INFO Fetch successful
May 26 09:23:20.110 INFO Fetching http://169.254.169.254/latest/meta-data/hostname: Attempt #1
May 26 09:23:20.115 INFO Fetch successful
May 26 09:23:20.115 INFO Fetching http://169.254.169.254/latest/meta-data/instance-id: Attempt #1
May 26 09:23:20.120 INFO Fetch successful
May 26 09:23:20.120 INFO Fetching http://169.254.169.254/latest/meta-data/instance-type: Attempt #1
May 26 09:23:20.124 INFO Fetch successful
May 26 09:23:20.124 INFO Fetching http://169.254.169.254/latest/meta-data/local-ipv4: Attempt #1
May 26 09:23:20.129 INFO Fetch successful
May 26 09:23:20.129 INFO Fetching http://169.254.169.254/latest/meta-data/public-ipv4: Attempt #1
May 26 09:23:20.133 INFO Fetch successful
$ cat attr-metadata.txt
AFTERBURN_OPENSTACK_INSTANCE_TYPE=m1.medium
AFTERBURN_OPENSTACK_INSTANCE_ID=i-0000003b
AFTERBURN_OPENSTACK_IPV4_LOCAL=172.16.0.159
AFTERBURN_OPENSTACK_INSTANCE_UUID=4d7a54fe-95eb-4789-ac9a-3529618505ec
AFTERBURN_OPENSTACK_IPV4_PUBLIC=192.168.3.16
AFTERBURN_OPENSTACK_HOSTNAME=mule

Config drive:

# ./afterburn --provider openstack --attributes ./attr-cd.txt
# cat attr-cd.txt
AFTERBURN_OPENSTACK_INSTANCE_TYPE=m1.medium
AFTERBURN_OPENSTACK_IPV4_LOCAL=172.16.0.159
AFTERBURN_OPENSTACK_HOSTNAME=mule
AFTERBURN_OPENSTACK_INSTANCE_UUID=4d7a54fe-95eb-4789-ac9a-3529618505ec
AFTERBURN_OPENSTACK_IPV4_PUBLIC=
AFTERBURN_OPENSTACK_INSTANCE_ID=i-0000003b

mdbooth · 2023-05-26T10:03:07Z

Could do with a couple of unit tests (one each for network and config drive), but I think it's in good enough shape to remove the scary warning in the description.

I agree with the design decision to add a new parameter rather than fix the existing one. Although the existing value can't be used anywhere in the openstack API, it does still potentially serve as an opaque unique identifier: changing it could potentially break any server which had previously registered the old value.

pierreprinetti · 2023-05-26T10:10:22Z

Could do with a couple of unit tests

Aaaand here they are!

pierreprinetti · 2023-05-26T10:23:53Z

I believe this patch to be ready for a review. Shall I assign @bgilbert?

src/providers/openstack/mock_tests.rs

docs/usage/attributes.md

mdbooth · 2023-05-26T16:24:58Z

src/providers/openstack/mock_tests.rs

+        .mock("GET", "/openstack/2012-08-10/meta_data.json")
+        .with_status(200)
+        .with_header("content-type", "application/json")
+        .with_body(r#"{"uuid": "99dcf33b-6eb5-4acf-9abb-d81723e0c949", "public_keys": {"pprinett": "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQC/Zc+pyRoKWVS1CVx2soRe7Ia7RGkASpXZE495H2XuS3gF4Vb9JdATkJ4NURmAQhUN8TYCbfqTcf2Fpm/5mtG6Dn8Avua1mXgNRvrufVYBvWkOn9qPEYkwELSG0WcqjpISwsKRAU0d7Mcqo0aDmp/wpgbXHCBG03Q8w0uhRTdI7jEj/EzatbtkgUJnE79OHUImxdG+11oKH0Ul2lQlSo+pMa0fwS3GrHKIxxZmMckSpT+gua0AttuGpr+JYIZKNkoWn7bMqpYpJAlKxjzaB6ympaIAOgd/CxXnXPzc8ntIm/MubDUbGzo7dpq39MmxtWb1bahS/zeyFq0ZR43FbHexZyj21PiurtFOXl93r+wgBLBcVjBogi23p8i+SrjOVb+nJJ5XEUaLDYYE7T4xnrmT/T0ODQQbh6W+F+/mCSwOpRsZV0+FYcWg34InQM177eBweAv5Lwtct6B0xzCCXYpNTjWpBUe46dP7FO0ltzD57CGglRyx7whff96Og7Zx61/YfR/zrfI4NYiP+4EbiN24NuTn0DvND/anCpvA1Zpsd7bNMvL9YrlCRD2lfcQl3p6Kqi48jLNpqjEMYEmgYzzGjE1hFuVqdjY63bAmD3+NPlfARsgbMiPpLAmb3nE5FTCWktUTE8fnoWIojUYm7/4HoVQlwWxRhLKji5NiJGMgYw== cardno:9_040_793\n"}, "keys": [{"name": "pprinett", "type": "ssh", "data": "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQC/Zc+pyRoKWVS1CVx2soRe7Ia7RGkASpXZE495H2XuS3gF4Vb9JdATkJ4NURmAQhUN8TYCbfqTcf2Fpm/5mtG6Dn8Avua1mXgNRvrufVYBvWkOn9qPEYkwELSG0WcqjpISwsKRAU0d7Mcqo0aDmp/wpgbXHCBG03Q8w0uhRTdI7jEj/EzatbtkgUJnE79OHUImxdG+11oKH0Ul2lQlSo+pMa0fwS3GrHKIxxZmMckSpT+gua0AttuGpr+JYIZKNkoWn7bMqpYpJAlKxjzaB6ympaIAOgd/CxXnXPzc8ntIm/MubDUbGzo7dpq39MmxtWb1bahS/zeyFq0ZR43FbHexZyj21PiurtFOXl93r+wgBLBcVjBogi23p8i+SrjOVb+nJJ5XEUaLDYYE7T4xnrmT/T0ODQQbh6W+F+/mCSwOpRsZV0+FYcWg34InQM177eBweAv5Lwtct6B0xzCCXYpNTjWpBUe46dP7FO0ltzD57CGglRyx7whff96Og7Zx61/YfR/zrfI4NYiP+4EbiN24NuTn0DvND/anCpvA1Zpsd7bNMvL9YrlCRD2lfcQl3p6Kqi48jLNpqjEMYEmgYzzGjE1hFuVqdjY63bAmD3+NPlfARsgbMiPpLAmb3nE5FTCWktUTE8fnoWIojUYm7/4HoVQlwWxRhLKji5NiJGMgYw== cardno:9_040_793\n"}], "hostname": "mule", "name": "mule", "launch_index": 0, "availability_zone": "nova"}"#)


I'll leave this up to the maintainers, but given that there's already a fixtures directory containing related data I feel this blob would be better placed there and read from file.

If I understand correctly, the fixture folder contains files meant to be passed to the tests as files. While we could move this string literal into a .json file and dynamically load it in the test, I wouldn't necessarily see that as an improvement.

I agree that this isn't especially legible here, and the fixture could be nice to reuse for additional tests in the future. Let's move it into the fixtures directory and use with_body_from_file().

lentzi90

Looks good to me! I'm not familiar enough with the code base (or rust for that matter) to say anything about the specifics, but on a high level it looks like a good solution.
Thanks for working on this @pierreprinetti ! 🙂

mdbooth · 2023-05-30T08:53:36Z

/lgtm

I'm hoping we can get an E2E test of this solving our root issue from @tormath1 later.

tormath1 · 2023-05-30T16:09:36Z

FWIW I tested with a patched Flatcar image in the Cluster API OpenStack project - the UUID is fetched as expected (kubernetes-sigs/cluster-api-provider-openstack#1564 (comment)).

pierreprinetti · 2023-06-06T09:13:25Z

@bgilbert @prestist Do you think we can get the tests executed, so that if there's anything failing I can maybe try and fix before you review?

prestist · 2023-06-06T13:51:06Z

@pierreprinetti

Firstly thank you for doing this!

Second, regarding your question about getting the tests working:
They are fixed by #938

If you do a git push -f it will re trigger all the CI. It looks like a simple re-run is not working at the moment.

pierreprinetti · 2023-06-06T14:28:43Z

If you do a git push -f it will re trigger all the CI. It looks like a simple re-run is not working at the moment.

OK! I have run the below command in the expectation that the tests will now run:

git commit --amend --no-edit && git push --force

pierreprinetti · 2023-06-06T15:50:19Z

Are we missing an /ok-to-test label or something?

prestist · 2023-06-06T20:10:25Z

So since you have not contributed to the repo before it looks like GitHub wanted a maintainer to approve the CI for you to run. You can read about it here => https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

prestist

Generally looks good to me, I have a few nits and a few questions.

Thank you so much for putting this up!

docs/release-notes.md

docs/usage/attributes.md

src/providers/openstack/configdrive.rs

pierreprinetti · 2023-06-09T18:02:27Z

Thanks for the review, @prestist! I think I have applied all of your suggestions.

bgilbert

This looks good! Thanks @pierreprinetti for the contribution and @mdbooth for the reviews. Just a few nits and then I think we can land this.

bgilbert · 2023-06-11T23:08:23Z

src/providers/openstack/mock_tests.rs

+        .mock("GET", "/openstack/2012-08-10/meta_data.json")
+        .with_status(200)
+        .with_header("content-type", "application/json")
+        .with_body(r#"{"uuid": "99dcf33b-6eb5-4acf-9abb-d81723e0c949", "public_keys": {"pprinett": "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQC/Zc+pyRoKWVS1CVx2soRe7Ia7RGkASpXZE495H2XuS3gF4Vb9JdATkJ4NURmAQhUN8TYCbfqTcf2Fpm/5mtG6Dn8Avua1mXgNRvrufVYBvWkOn9qPEYkwELSG0WcqjpISwsKRAU0d7Mcqo0aDmp/wpgbXHCBG03Q8w0uhRTdI7jEj/EzatbtkgUJnE79OHUImxdG+11oKH0Ul2lQlSo+pMa0fwS3GrHKIxxZmMckSpT+gua0AttuGpr+JYIZKNkoWn7bMqpYpJAlKxjzaB6ympaIAOgd/CxXnXPzc8ntIm/MubDUbGzo7dpq39MmxtWb1bahS/zeyFq0ZR43FbHexZyj21PiurtFOXl93r+wgBLBcVjBogi23p8i+SrjOVb+nJJ5XEUaLDYYE7T4xnrmT/T0ODQQbh6W+F+/mCSwOpRsZV0+FYcWg34InQM177eBweAv5Lwtct6B0xzCCXYpNTjWpBUe46dP7FO0ltzD57CGglRyx7whff96Og7Zx61/YfR/zrfI4NYiP+4EbiN24NuTn0DvND/anCpvA1Zpsd7bNMvL9YrlCRD2lfcQl3p6Kqi48jLNpqjEMYEmgYzzGjE1hFuVqdjY63bAmD3+NPlfARsgbMiPpLAmb3nE5FTCWktUTE8fnoWIojUYm7/4HoVQlwWxRhLKji5NiJGMgYw== cardno:9_040_793\n"}, "keys": [{"name": "pprinett", "type": "ssh", "data": "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQC/Zc+pyRoKWVS1CVx2soRe7Ia7RGkASpXZE495H2XuS3gF4Vb9JdATkJ4NURmAQhUN8TYCbfqTcf2Fpm/5mtG6Dn8Avua1mXgNRvrufVYBvWkOn9qPEYkwELSG0WcqjpISwsKRAU0d7Mcqo0aDmp/wpgbXHCBG03Q8w0uhRTdI7jEj/EzatbtkgUJnE79OHUImxdG+11oKH0Ul2lQlSo+pMa0fwS3GrHKIxxZmMckSpT+gua0AttuGpr+JYIZKNkoWn7bMqpYpJAlKxjzaB6ympaIAOgd/CxXnXPzc8ntIm/MubDUbGzo7dpq39MmxtWb1bahS/zeyFq0ZR43FbHexZyj21PiurtFOXl93r+wgBLBcVjBogi23p8i+SrjOVb+nJJ5XEUaLDYYE7T4xnrmT/T0ODQQbh6W+F+/mCSwOpRsZV0+FYcWg34InQM177eBweAv5Lwtct6B0xzCCXYpNTjWpBUe46dP7FO0ltzD57CGglRyx7whff96Og7Zx61/YfR/zrfI4NYiP+4EbiN24NuTn0DvND/anCpvA1Zpsd7bNMvL9YrlCRD2lfcQl3p6Kqi48jLNpqjEMYEmgYzzGjE1hFuVqdjY63bAmD3+NPlfARsgbMiPpLAmb3nE5FTCWktUTE8fnoWIojUYm7/4HoVQlwWxRhLKji5NiJGMgYw== cardno:9_040_793\n"}], "hostname": "mule", "name": "mule", "launch_index": 0, "availability_zone": "nova"}"#)


I agree that this isn't especially legible here, and the fixture could be nice to reuse for additional tests in the future. Let's move it into the fixtures directory and use with_body_from_file().

src/providers/openstack/network.rs

docs/usage/attributes.md

src/providers/openstack/network.rs

pierreprinetti · 2023-06-12T08:27:55Z

Here's another try! Let's see if we get closer... 😁

bgilbert

LGTM!

src/providers/openstack/mock_tests.rs

In the OpenStack attributes, the field `OPENSTACK_INSTANCE_ID` reports the instance ID fetched from the EC2-compatibility-layer metadata. However, that identifier (in the format `i-something`) does not match the one used to refer to machines in the OpenStack API. With this patch, I propose to add a new OpenStack attribute `OPENSTACK_INSTANCE_UUID` that reports the actual identifier for OpenStack instances in the context of the OpenStack API. Implementation-wise, this patch adds enables fetching the UUID from both the config-drive and from the Nova metadata service endpoint. In order to retain compatibility with old versions of the metadata service, this patch uses the old endpoint `2012-08-10`. Fixes coreos#930

pierreprinetti mentioned this pull request May 25, 2023

🌱 Templates: Set provider ID for flatcar directly via kubelet kubernetes-sigs/cluster-api-provider-openstack#1564

Closed

3 tasks

pierreprinetti force-pushed the openstack_instance_uuid branch from 16d6ff3 to ec1046d Compare May 26, 2023 09:24

pierreprinetti force-pushed the openstack_instance_uuid branch from ec1046d to 92b76a5 Compare May 26, 2023 10:09

pierreprinetti force-pushed the openstack_instance_uuid branch from 92b76a5 to 7525052 Compare May 26, 2023 10:19

pierreprinetti changed the title ~~openstack: Add OPENSTACK_INSTANCE_UUID~~ openstack: Add attribute OPENSTACK_INSTANCE_UUID May 26, 2023

pierreprinetti force-pushed the openstack_instance_uuid branch from 7525052 to c4c66b4 Compare May 26, 2023 10:20

mdbooth reviewed May 26, 2023

View reviewed changes

src/providers/openstack/mock_tests.rs Outdated Show resolved Hide resolved

src/providers/openstack/mock_tests.rs Show resolved Hide resolved

pierreprinetti force-pushed the openstack_instance_uuid branch 2 times, most recently from 209c215 to 70badef Compare May 26, 2023 13:07

pierreprinetti commented May 26, 2023

View reviewed changes

docs/usage/attributes.md Outdated Show resolved Hide resolved

mdbooth reviewed May 26, 2023

View reviewed changes

pierreprinetti force-pushed the openstack_instance_uuid branch from 70badef to 37058a3 Compare May 30, 2023 08:04

lentzi90 approved these changes May 30, 2023

View reviewed changes

tormath1 mentioned this pull request May 30, 2023

[RFE] upgrade coreos-base/afterburn flatcar/Flatcar#1049

Closed

bgilbert self-requested a review June 1, 2023 04:08

pierreprinetti force-pushed the openstack_instance_uuid branch from 37058a3 to 2566a39 Compare June 6, 2023 14:27

pierreprinetti force-pushed the openstack_instance_uuid branch from 2566a39 to 6418459 Compare June 6, 2023 21:58

prestist reviewed Jun 9, 2023

View reviewed changes

docs/release-notes.md Outdated Show resolved Hide resolved

docs/usage/attributes.md Outdated Show resolved Hide resolved

src/providers/openstack/configdrive.rs Outdated Show resolved Hide resolved

src/providers/openstack/configdrive.rs Show resolved Hide resolved

pierreprinetti force-pushed the openstack_instance_uuid branch from 6418459 to c4ba9f7 Compare June 9, 2023 18:00

bgilbert reviewed Jun 11, 2023

View reviewed changes

pierreprinetti force-pushed the openstack_instance_uuid branch from c4ba9f7 to c27b26d Compare June 12, 2023 08:26

bgilbert reviewed Jun 12, 2023

View reviewed changes

src/providers/openstack/mock_tests.rs Outdated Show resolved Hide resolved

pierreprinetti force-pushed the openstack_instance_uuid branch from c27b26d to 6a06a02 Compare June 12, 2023 20:32

bgilbert approved these changes Jun 12, 2023

View reviewed changes

bgilbert enabled auto-merge June 12, 2023 20:35

bgilbert merged commit 8cbe7f6 into coreos:main Jun 12, 2023

pierreprinetti deleted the openstack_instance_uuid branch June 12, 2023 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openstack: Add attribute OPENSTACK_INSTANCE_UUID #931

openstack: Add attribute OPENSTACK_INSTANCE_UUID #931

pierreprinetti commented May 25, 2023 •

edited

Loading

pierreprinetti commented May 26, 2023 •

edited

Loading

mdbooth commented May 26, 2023

pierreprinetti commented May 26, 2023

pierreprinetti commented May 26, 2023

mdbooth May 26, 2023

pierreprinetti May 30, 2023

bgilbert Jun 11, 2023

lentzi90 left a comment

mdbooth commented May 30, 2023

tormath1 commented May 30, 2023

pierreprinetti commented Jun 6, 2023

prestist commented Jun 6, 2023 •

edited

Loading

pierreprinetti commented Jun 6, 2023

pierreprinetti commented Jun 6, 2023 •

edited

Loading

prestist commented Jun 6, 2023

prestist left a comment

pierreprinetti commented Jun 9, 2023

bgilbert left a comment

bgilbert Jun 11, 2023

pierreprinetti commented Jun 12, 2023

bgilbert left a comment

openstack: Add attribute OPENSTACK_INSTANCE_UUID #931

openstack: Add attribute OPENSTACK_INSTANCE_UUID #931

Conversation

pierreprinetti commented May 25, 2023 • edited Loading

pierreprinetti commented May 26, 2023 • edited Loading

mdbooth commented May 26, 2023

pierreprinetti commented May 26, 2023

pierreprinetti commented May 26, 2023

mdbooth May 26, 2023

Choose a reason for hiding this comment

pierreprinetti May 30, 2023

Choose a reason for hiding this comment

bgilbert Jun 11, 2023

Choose a reason for hiding this comment

lentzi90 left a comment

Choose a reason for hiding this comment

mdbooth commented May 30, 2023

tormath1 commented May 30, 2023

pierreprinetti commented Jun 6, 2023

prestist commented Jun 6, 2023 • edited Loading

pierreprinetti commented Jun 6, 2023

pierreprinetti commented Jun 6, 2023 • edited Loading

prestist commented Jun 6, 2023

prestist left a comment

Choose a reason for hiding this comment

pierreprinetti commented Jun 9, 2023

bgilbert left a comment

Choose a reason for hiding this comment

bgilbert Jun 11, 2023

Choose a reason for hiding this comment

pierreprinetti commented Jun 12, 2023

bgilbert left a comment

Choose a reason for hiding this comment

pierreprinetti commented May 25, 2023 •

edited

Loading

pierreprinetti commented May 26, 2023 •

edited

Loading

prestist commented Jun 6, 2023 •

edited

Loading

pierreprinetti commented Jun 6, 2023 •

edited

Loading