[troglodyne]

• Adds support for clouds.yaml usage for auth, very useful in CI context or for use with Trog::Provisioner (soon).
• Make floating IPs optional, because they are (and certainly at cPanel our setup now makes it optional).
• Fixes bug in uploads of cloud-config (it needed the content to get B64'd)
• Fleshes out the API surface more, as it was only partially implemented here.

Related PR (what I wrote this to serve):
Troglodyne-Internet-Widgets/trog-provisioner#121

That said I probably can use this within cPanel's QA testsuite or other CI workflows we have for those cases where terraform's openstack driver just doesn't have the functionality to get what you want done (as it generally only handles vm spawn/lifecyle, not things like image management, etc.)

[Koan-Bot]

Previous review — superseded by a newer review below.

[Koan-Bot]

Warning

Important issues found.

• BUILDARGS auth-passthrough uses ref, not blessed — a clouds.yaml hash silently becomes the auth object

[atoomic]

note: #29 will fix CI
let's focus on the #1 issue there

[troglodyne]

Addressed, thanks

[Koan-Bot]

@atoomic: note: #29 will fix CI

Issue #1 resolved. BUILDARGS now guards on blessed($args[0]->{'auth'}), not ref. A clouds.yaml cloud entry falls through to OpenStack::Client::Auth->new as before. New negative test at t/01_base.t covers {auth => {username => 'x'}}. Nothing blocking left — remaining findings are all suggestions.

[Koan-Bot]

@troglodyne: Addressed, thanks

Verified. blessed() guard correct, test covers the unblessed case. Three earlier suggestions still open: list_images has no test and returns a bare list (count in scalar context), the three volume-attachment methods skip the $uid guard their siblings have, and POD/Changes/dist.ini prereqs cover none of the new surface.

[Koan-Bot]

PR Review — Add several features missing from OpenStack::MetaAPI

A substantial, well-tested expansion of the API surface — fourteen new routes, a Cinder service class, base64 user_data, and an optional floating IP — with the previously blocking BUILDARGS guard now correctly fixed. Everything remaining is non-blocking.

• BUILDARGS now uses blessed(), and the new negative test pins that a clouds.yaml-shaped auth hash still falls through to OpenStack::Client::Auth
• Volume.pm resolving volumev3/v2/volume from the live catalogue instead of a hardcoded type is the right call, and the fixture covers all three
• Asserting on the captured POST body rather than the response is the correct level for a field the API encodes on the caller's behalf
• list_images remains the only new route with no test, and returns a bare list where its siblings collapse to a scalar
• POD, README, Changes, and the dist.ini prereq list still cover none of the new surface

---

✅ Resolved since last review (2)

Previously-flagged issues verified fixed

• lib/OpenStack/MetaAPI.pm:57 BUILDARGS auth-passthrough uses ref, not blessed — a clouds.yaml hash silently becomes the auth object
• lib/OpenStack/MetaAPI.pm:233 POD and Changes not updated for the new surface

---

🟢 Suggestions

1. list_images is still the only new route with no test, and returns a bare list
lib/OpenStack/MetaAPI/API/Images.pm:48-51

This was raised on the previous revision and is unchanged. t/service-extras.t covers the other twelve new routes; list_images has none — and it is the method the amended images() die message now points users at.

Two things a test would pin down:

• Return shape breaks the house convention. OpenStack::Client::all ends in return @items, so list_images returns a raw list. Every other lister here goes through Roles::Listable::_list, which collapses to a scalar for <=1 result (lib/OpenStack/MetaAPI/Roles/Listable.pm:71). So my $snaps = $api->list_images(instance_uuid => $uid) — the query the comment directly above advertises — yields the count, not a hashref or arrayref. Routing through _list(['/images', 'images'], \%query) would give the same pagination plus the collapse, the regex filters, and the spec-driven query-param handling for free. Note _list's $uri !~ m{^/v} guard means /images would still reach root_uri and resolve to v2/images, same as today.
• The URI is worth pinning. root_uri('/images') yields v2/images only because version_prefix is 'v2' and the Glance endpoint carries no version (lib/OpenStack/MetaAPI/API/Service.pm:79-98). A last_http_request() assertion, as service-extras.t uses elsewhere, would lock that in.

sub list_images {
    my ($self, %query) = @_;

    return $self->client->all($self->root_uri('/images'), 'images', \%query);
}

2. server_volumes / attach_volume / detach_volume still don't validate $uid
lib/OpenStack/MetaAPI/API/Compute.pm:123-134

Also unchanged from the previous revision. server_action (line 78), create_image (line 93), and every Cinder method in the new Volume.pm guard their required ids with unless defined ... && length .... These three guard $volume_id but never $uid.

With $uid undef, root_uri("/servers/$uid/os-volume_attachments") interpolates an empty string — a Use of uninitialized value warning under use warnings — and the request lands on .../v2.1/servers//os-volume_attachments. The caller gets a Nova 404 instead of the clear die its siblings produce.

One line each keeps the new surface internally consistent:

die "server id is required by attach_volume" unless defined $uid && length $uid;

sub server_volumes {
    my ($self, $uid) = @_;

    my $out = $self->get($self->root_uri("/servers/$uid/os-volume_attachments"));

3. block_device_mapping_v2 can be passed but cannot actually boot from a volume
lib/OpenStack/MetaAPI.pm:124-129

The comment above this loop says the field carries "the volumes it boots or carries" — but the boot case is unreachable through create_vm.

• image is still a hard requirement (die "'image' name or id is required by create_vm" at line 57), and imageRef => $image->{id} is unconditionally included in the create_server payload at line 92.
• Nova rejects a request that carries both a non-empty imageRef and a block_device_mapping_v2 entry with boot_index: 0 sourced from a volume or snapshot, with 400 Block Device Mapping is Invalid: Boot sequence for the instance and image/block device mapping combination is not valid.

So the pass-through works for attaching extra volumes at create time, which is genuinely useful, but a caller following the comment into a boot-from-volume call gets a 400 with no hint from this module about why.

Either narrow the comment to what the field can actually do here, or let image be omitted when a boot_index: 0 mapping is present and send imageRef => '' in that case.

    foreach my $field (qw{availability_zone metadata block_device_mapping_v2}) {
        push @extra, ($field => $opts{$field}) if defined $opts{$field};
    }

4. POD, README, Changes, and dist.ini prereqs not updated for the new surface
lib/OpenStack/MetaAPI.pm:275-288

Still open from the previous revision. README.md is generated from this POD ([ReadmeAnyFromPod] in dist.ini), so the POD is this distribution's only user-facing documentation. The PR adds fourteen routes, a new API::Volume service, and four create_vm options, none of them documented:

• The create_vm synopsis still lists network_for_floating_ip as a plain argument with no note that it is now optional — a headline change of this PR.
• user_data, availability_zone, metadata, and block_device_mapping_v2 are undiscoverable. user_data in particular is base64-encoded for the caller; double-encoding is the natural mistake and produces a VM that boots with garbage cloud-init.
• "Available functions / methods" lists every existing route; the new ones are absent, including the whole Cinder surface.
• Changes still carries only the {{NEXT}} stanza.
• MIME::Base64 is now a runtime dependency and isn't under [Prereqs / RuntimeRequires] in dist.ini. It is core so nothing breaks, but that block is hand-maintained (AutoPrereqs is off), so declaring it keeps the list honest.

=head2 $api->create_vm( %args )

Create one server from one image with one floating IP, wait for the server to be ready.

---

Checklist

• Input validation at boundaries — suggestion ＃2
• New branches covered by tests — suggestion ＃1
• Public API docs kept in sync with code — suggestion ＃4
• Return conventions consistent across the object — suggestion ＃1
• Auth object passthrough rejects unblessed config hashes
• Backward compatible for existing create_vm callers
• New routes resolve to real service methods without name collisions
• No hardcoded secrets or credentials

---

Automated review by Skuggi HEAD=d10099a 7 min 30s

[Koan-Bot]

Tip

No blocking issues found — ready to merge.