Skip to content

Improve management of CAPI images #978

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.

Sign up for GitHub

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

Merged
berendt merged 9 commits into from
Aug 12, 2024
Merged

Improve management of CAPI images #978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a9dbbaf
Invoke openstack-image-manager without shell
martinmo Aug 7, 2024
a4f5bf4
Fix hard-coded `--cloud` flag
martinmo Aug 7, 2024
8c91f7b
Allow passing `--dry-run` flag to image manager
martinmo Aug 7, 2024
5c1c2cb
Allow passing a custom `--tag` to image manager
martinmo Aug 7, 2024
11865bb
Set hypervisor_type=qemu in CAPI images
martinmo Aug 6, 2024
e53db3c
Set hw_disk_bus=scsi in CAPI images
martinmo Aug 6, 2024
2028134
Use SCS compliant CAPI image name
martinmo Aug 7, 2024
2548698
Format code with black
martinmo Aug 7, 2024
fbb4613
Version needs to be prefixed with `v`
martinmo Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions osism/commands/manage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,21 @@ def get_parser(self, prog_name):
default="https://swift.services.a.regiocloud.tech/swift/v1/AUTH_b182637428444b9aa302bb8d5a5a418c/openstack-k8s-capi-images/",
)
parser.add_argument(
"--cloud", type=str, help="Cloud name in clouds.yaml", default="openstack"
"--cloud",
type=str,
help="Cloud name in clouds.yaml (will be overruled by OS_AUTH_URL envvar)",
default="admin",
)
parser.add_argument(
"--dry-run",
action="store_true",
help="Do not perform any changes (--dry-run passed to openstack-image-manager)",
)
parser.add_argument(
"--tag",
type=str,
help="Name of the tag used to identify managed images (use openstack-image-manager's default if unset)",
default=None,
)
parser.add_argument(
"--filter",
Expand All @@ -41,6 +55,7 @@ def take_action(self, parsed_args):
base_url = parsed_args.base_url
cloud = parsed_args.cloud
filter = parsed_args.filter

@codereviewbot-ai codereviewbot-ai Bot Aug 7, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The variable name filter shadows the built-in Python function filter(). This can lead to unexpected behavior and bugs, especially if the built-in function is needed later in the code.

Recommended Solution: Rename the variable to something more descriptive, such as image_filter.

@berendt berendt Aug 7, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This probably makes sense and can be tackled In a separate PR.

tag = parsed_args.tag

if filter:
supported_cluterapi_k8s_images = [filter]
Expand Down Expand Up @@ -78,10 +93,19 @@ def take_action(self, parsed_args):
with open(f"/tmp/clusterapi/k8s-{kubernetes_release}.yml", "w+") as fp:
fp.write(result)

subprocess.call(
"/usr/local/bin/openstack-image-manager --images=/tmp/clusterapi --cloud admin --filter 'Kubernetes CAPI'",
shell=True,
)
args = [
"openstack-image-manager",
"--images=/tmp/clusterapi",
"--cloud",
cloud,
"--filter",
"ubuntu-capi-image",
]
if tag is not None:
args.extend(["--tag", tag])
if parsed_args.dry_run:
args.append("--dry-run")
subprocess.call(args)

@codereviewbot-ai codereviewbot-ai Bot Aug 7, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The use of subprocess.call can be risky if any of the arguments are derived from user input, as it may lead to command injection vulnerabilities. Although the current arguments seem to be controlled, it's safer to use subprocess.run with a list of arguments to avoid shell interpretation issues.

Recommended Solution: Replace subprocess.call with subprocess.run and ensure all arguments are passed as a list.



class ImageOctavia(Command):
Expand Down
7 changes: 4 additions & 3 deletions osism/data/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@

TEMPLATE_IMAGE_CLUSTERAPI = """---
images:
- name: Kubernetes CAPI
- name: ubuntu-capi-image
enable: true
keep: true
format: qcow2
Expand All @@ -44,17 +44,18 @@
multi: false
meta:
architecture: x86_64
hw_disk_bus: virtio
hw_disk_bus: scsi
hw_rng_model: virtio
hw_scsi_model: virtio-scsi
hw_watchdog_action: reset
hypervisor_type: qemu
os_distro: ubuntu
replace_frequency: never
uuid_validity: none
provided_until: none
tags: []
versions:
- version: "{{ image_version }}"
- version: "v{{ image_version }}"
url: "{{ image_url }}"
checksum: "{{ image_checksum }}"
build_date: {{ image_builddate }}
Expand Down