microshift-image: create a booc microshift based image #201
Conversation
WalkthroughAdds a Bootc-based MicroShift deployment under deploy/microshift-bootc: container image and Containerfile, a Makefile and run script for building and running images, a Flask-based configuration service with systemd units, kustomize manifests, image builder config, and README documentation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as Browser
participant Config as config-svc (Flask)
participant Host as Host OS (systemd)
participant Boot as Boot Container
participant Kube as Kubernetes API
Browser->>Config: POST /configure (baseDomain, image)
Config->>Config: validate, build Jumpstarter CR YAML
Config->>Host: set hostname / set root password / update banner
Host-->>Config: ack
Config->>Boot: kubectl apply (using mounted kubeconfig)
Boot->>Kube: API apply CR
Kube-->>Boot: apply result
Boot-->>Config: result
Config-->>Browser: success / error
Browser->>Config: GET /kubeconfig
Config->>Boot: read kubeconfig
Boot-->>Config: kubeconfig
Config-->>Browser: kubeconfig (rewritten)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
deploy/microshift-bootc/Makefile (1)
1-1: Incomplete.PHONYdeclaration.The
clean,build-all, andpush-alltargets are missing from the.PHONYdeclaration. This can cause issues if files with these names exist.-.PHONY: help build build-config-svc build-bootc push push-config-svc push-bootc +.PHONY: help build build-config-svc build-bootc push push-config-svc push-bootc build-all push-all cleandeploy/microshift-bootc/config-svc/app.py (1)
440-475: Consider using PyYAML instead of custom implementation.The custom
json_to_yamlimplementation works for simple cases but has edge cases (multiline strings, special YAML characters beyond:/#/-). Since this is a container environment, addingpython3-pyyamlto the Containerfile would be straightforward.However, for the limited CR structure being generated, this implementation is functional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
deploy/microshift-bootc/Containerfile(1 hunks)deploy/microshift-bootc/Makefile(1 hunks)deploy/microshift-bootc/config-svc/Containerfile(1 hunks)deploy/microshift-bootc/config-svc/app.py(1 hunks)deploy/microshift-bootc/config-svc/manifest.yaml(1 hunks)deploy/microshift-bootc/config.toml(1 hunks)deploy/microshift-bootc/kustomization.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Applied to files:
deploy/microshift-bootc/kustomization.yamldeploy/microshift-bootc/config-svc/manifest.yaml
📚 Learning: 2025-05-13T19:57:56.811Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: hack/deploy_with_helm.sh:26-34
Timestamp: 2025-05-13T19:57:56.811Z
Learning: The jumpstarter-dev repository uses a custom kind cluster configuration that allows NodePort services to use non-standard ports 5080 and 5443, outside the default Kubernetes NodePort range (30000-32767).
Applied to files:
deploy/microshift-bootc/kustomization.yamldeploy/microshift-bootc/config-svc/manifest.yaml
📚 Learning: 2025-05-13T19:57:56.811Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: hack/deploy_with_helm.sh:26-34
Timestamp: 2025-05-13T19:57:56.811Z
Learning: The jumpstarter project uses a custom kind cluster configuration with an expanded NodePort range (3000-32767) and explicit port mappings for ingress (5080/5443) and gRPC services (30010/30011 mapped to 8082/8083).
Applied to files:
deploy/microshift-bootc/config-svc/manifest.yaml
📚 Learning: 2025-05-13T19:56:27.924Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml:23-26
Timestamp: 2025-05-13T19:56:27.924Z
Learning: In the jumpstarter-controller project, the router service uses the same ConfigMap as the controller service (controller-cm.yaml) even though it has been moved to its own separate deployment.
Applied to files:
deploy/microshift-bootc/config-svc/manifest.yaml
🪛 checkmake (0.2.2)
deploy/microshift-bootc/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 Checkov (3.2.334)
deploy/microshift-bootc/config-svc/manifest.yaml
[medium] 68-117: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[high] 68-117: Container should not be privileged
(CKV_K8S_16)
[medium] 68-117: Minimize the admission of root containers
(CKV_K8S_23)
[medium] 68-117: Containers should not share the host network namespace
(CKV_K8S_19)
🪛 Ruff (0.14.8)
deploy/microshift-bootc/config-svc/app.py
305-305: Do not catch blind exception: Exception
(BLE001)
306-306: Use explicit conversion flag
Replace with conversion flag
(RUF010)
318-318: Starting a process with a partial executable path
(S607)
341-341: subprocess call: check for execution of untrusted input
(S603)
342-342: Starting a process with a partial executable path
(S607)
356-356: Consider moving this statement to an else block
(TRY300)
357-357: Do not catch blind exception: Exception
(BLE001)
366-366: Do not catch blind exception: Exception
(BLE001)
374-374: subprocess call: check for execution of untrusted input
(S603)
375-375: Starting a process with a partial executable path
(S607)
380-380: Consider moving this statement to an else block
(TRY300)
385-385: Do not catch blind exception: Exception
(BLE001)
417-417: subprocess call: check for execution of untrusted input
(S603)
418-418: Starting a process with a partial executable path
(S607)
428-429: try-except-pass detected, consider logging the exception
(S110)
428-428: Do not catch blind exception: Exception
(BLE001)
435-435: Do not catch blind exception: Exception
(BLE001)
485-485: Possible binding to all interfaces
(S104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-release-0-7
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: lint-go
- GitHub Check: tests
- GitHub Check: e2e-test-operator
- GitHub Check: deploy-kind
🔇 Additional comments (5)
deploy/microshift-bootc/config-svc/Containerfile (1)
1-24: LGTM - Clean Containerfile structure.The Containerfile follows good practices: uses a specific base image version, consolidates RUN commands to reduce layers, and cleans up the dnf cache.
Minor nit: Line 18's
chmod +xis unnecessary since the script is invoked explicitly withpython3on Line 24.deploy/microshift-bootc/kustomization.yaml (1)
1-11: LGTM - Kustomization configuration is well-structured.The resource references align correctly with the files copied in the main Containerfile, and the common labels appropriately identify the deployment context.
deploy/microshift-bootc/Containerfile (1)
1-6: LGTM - Manifest packaging is correct.The Containerfile properly packages the Jumpstarter resources for MicroShift's manifest auto-loading. The numeric prefix
002-jumpstarterensures proper ordering, and file renames align with the kustomization resource references.deploy/microshift-bootc/config-svc/manifest.yaml (1)
68-116: Acknowledged: Privileged security context is intentional.The static analysis correctly flags the privileged container, hostNetwork, and root container. However, these are necessary for this service's purpose:
hostNetwork: true- Required to set the system hostname viahostnamectlprivileged: true- Required for host-level operations- hostPath volume - Needed to read the MicroShift kubeconfig
Consider adding a comment in the manifest documenting why these elevated privileges are required.
deploy/microshift-bootc/config-svc/app.py (1)
313-359: LGTM - Network detection logic is well-implemented.The function properly parses
ip routeandip addroutput to determine the default route IP. The dash-separated IP format for nip.io is correct.
| ports: | ||
| - containerPort: 8880 | ||
| hostPort: 8880 | ||
| name: http | ||
| protocol: TCP | ||
| securityContext: | ||
| privileged: true | ||
| volumeMounts: | ||
| - name: microshift-data | ||
| mountPath: /var/lib/microshift | ||
| readOnly: true | ||
| env: | ||
| - name: PORT | ||
| value: "8080" |
There was a problem hiding this comment.
Critical: Port mismatch will cause connectivity failure.
The container declares containerPort: 8880 and hostPort: 8880 (lines 92-93), but the PORT environment variable is set to 8080 (line 104). The application reads PORT to determine the listening port, so it will listen on 8080 while the container expects 8880.
Additionally, the Service (lines 128-129) targets port 8080, which won't match the hostPort declaration.
ports:
- - containerPort: 8880
- hostPort: 8880
+ - containerPort: 8080
+ hostPort: 8080
name: http
protocol: TCP📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ports: | |
| - containerPort: 8880 | |
| hostPort: 8880 | |
| name: http | |
| protocol: TCP | |
| securityContext: | |
| privileged: true | |
| volumeMounts: | |
| - name: microshift-data | |
| mountPath: /var/lib/microshift | |
| readOnly: true | |
| env: | |
| - name: PORT | |
| value: "8080" | |
| ports: | |
| - containerPort: 8080 | |
| hostPort: 8080 | |
| name: http | |
| protocol: TCP | |
| securityContext: | |
| privileged: true | |
| volumeMounts: | |
| - name: microshift-data | |
| mountPath: /var/lib/microshift | |
| readOnly: true | |
| env: | |
| - name: PORT | |
| value: "8080" |
🤖 Prompt for AI Agents
In deploy/microshift-bootc/config-svc/manifest.yaml around lines 91 to 104 (and
note related Service at lines ~128-129), there is a port mismatch: containerPort
and hostPort are 8880 but the PORT env is 8080 and the Service targets 8080;
make them consistent by choosing one port and applying it everywhere—either
change the env PORT (and Service targetPort) to "8880" to match
containerPort/hostPort, or change containerPort/hostPort to 8080 to match the
env and Service; update the Service targetPort as well so containerPort,
hostPort, env PORT, and Service targetPort all use the same numeric port.
deploy/microshift-bootc/config.toml
Outdated
| # Root user password | ||
| [[customizations.user]] | ||
| name = "root" | ||
| password = "changeme123" |
There was a problem hiding this comment.
Security concern: Hardcoded default password.
The root password changeme123 is committed to the repository. Even as a default/example, this creates risk if users deploy without changing it. Consider:
- Using an environment variable or build-time argument
- Adding a prominent warning comment
- Requiring password configuration at first boot
At minimum, add a clear warning that this must be changed before production use.
# Root user password
+# WARNING: Change this password before any production or public deployment!
[[customizations.user]]
name = "root"
-password = "changeme123"
+password = "CHANGE_ME_BEFORE_DEPLOY"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Root user password | |
| [[customizations.user]] | |
| name = "root" | |
| password = "changeme123" | |
| # Root user password | |
| # WARNING: Change this password before any production or public deployment! | |
| [[customizations.user]] | |
| name = "root" | |
| password = "CHANGE_ME_BEFORE_DEPLOY" |
🤖 Prompt for AI Agents
In deploy/microshift-bootc/config.toml around lines 7 to 10 the root password is
hardcoded ("changeme123"); remove the literal password and instead reference a
configurable value (e.g. an environment variable or build-time
variable/placeholders) so the password is injected at deploy time, add a
prominent warning comment above the user block stating this value must be
changed before production, and, if supported, implement or call out a mechanism
to require password configuration on first boot (or fail startup) so deployments
cannot proceed with a default password.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
deploy/microshift-bootc/config-svc/app.py (2)
217-234: Security: Verify Jinja2 auto-escaping is enabled.User-controlled values (current_hostname, suggested_hostname, message text) are rendered in the HTML template. While Flask's Jinja2 enables auto-escaping by default for
render_template_string, it's best practice to explicitly escape user input for defense in depth.Add explicit HTML escaping:
+import htmlAnd in the rendering code, consider explicitly escaping values or verify that auto-escaping is functioning correctly for your Flask version.
485-496: Fix apiVersion for Jumpstarter CR.The CR uses
apiVersion: 'jumpstarter.dev/v1alpha1', but based on the CRD group structure, it should useoperator.jumpstarter.dev/v1alpha1.cr = { - 'apiVersion': 'jumpstarter.dev/v1alpha1', + 'apiVersion': 'operator.jumpstarter.dev/v1alpha1', 'kind': 'Jumpstarter',deploy/microshift-bootc/Makefile (1)
14-16: Typo: "buinding" should be "building".build-bootc: ## Build the bootc image with MicroShift - @echo "Building bootc image: $(BOOTC_IMG): buinding as root to be on the container storage from root" + @echo "Building bootc image: $(BOOTC_IMG): building as root to use container storage from root" sudo podman build -t $(BOOTC_IMG) -f Containerfile ../..
🧹 Nitpick comments (4)
deploy/microshift-bootc/config-svc/config-svc.service (1)
7-17: Consider adding systemd hardening directives.The service performs privileged operations (hostname changes, password management, kubectl operations) and must run as root. Consider adding hardening directives to limit the attack surface:
[Service] Type=simple ExecStart=/usr/bin/python3 /usr/local/bin/config-svc Restart=on-failure RestartSec=5 Environment="PORT=8880" # Security and resource limits +PrivateTmp=yes +ProtectSystem=strict +ReadWritePaths=/etc/hostname /var/lib/microshift +NoNewPrivileges=yes +ProtectHome=yes StandardOutput=journal StandardError=journal SyslogIdentifier=config-svcNote:
ProtectSystem=strictwithReadWritePathsallows write access only to necessary paths while protecting the rest of the filesystem.deploy/microshift-bootc/config-svc/app.py (3)
358-364: Document the insecure-skip-tls-verify configuration.The kubeconfig is modified to add
insecure-skip-tls-verify: true, which disables TLS certificate validation. This is likely intentional for development/testing with self-signed certificates, but it reduces security.Consider adding a comment in the code explaining this choice:
# Add insecure-skip-tls-verify: true to the cluster section - # We'll add it after the server line in the cluster section + # Add after the server line to allow self-signed certificates in dev/test environments + # WARNING: This reduces security by skipping TLS certificate verification kubeconfig_content = re.sub(
457-478: Prefix unused variable with underscore.The
stdoutvariable is never used after being unpacked.- stdout, stderr = process.communicate(input=f'root:{password}\n') + _stdout, stderr = process.communicate(input=f'root:{password}\n')
529-564: Consider using PyYAML library instead of custom implementation.The custom YAML serialization reinvents functionality available in the well-tested
pyyamllibrary. Using a standard library reduces maintenance burden and potential edge-case bugs.+import yaml + def apply_jumpstarter_cr(base_domain, image): """Apply Jumpstarter Custom Resource using kubectl.""" try: # Build the CR YAML cr = { ... } # Write CR to temporary file with tempfile.NamedTemporaryFile(mode='w', suffix='.yaml', delete=False) as f: - yaml_content = json_to_yaml(cr) + yaml_content = yaml.dump(cr, default_flow_style=False) f.write(yaml_content) temp_file = f.nameThen remove the
json_to_yamlandyaml_valuefunctions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
deploy/microshift-bootc/Containerfile(1 hunks)deploy/microshift-bootc/Makefile(1 hunks)deploy/microshift-bootc/config-svc/app.py(1 hunks)deploy/microshift-bootc/config-svc/config-svc.service(1 hunks)deploy/microshift-bootc/kustomization.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/microshift-bootc/Containerfile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Applied to files:
deploy/microshift-bootc/kustomization.yamldeploy/microshift-bootc/config-svc/app.py
🪛 checkmake (0.2.2)
deploy/microshift-bootc/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 Ruff (0.14.8)
deploy/microshift-bootc/config-svc/app.py
39-39: subprocess call: check for execution of untrusted input
(S603)
40-40: Starting a process with a partial executable path
(S607)
45-45: Consider moving this statement to an else block
(TRY300)
46-46: Do not catch blind exception: Exception
(BLE001)
376-376: Do not catch blind exception: Exception
(BLE001)
377-377: Use explicit conversion flag
Replace with conversion flag
(RUF010)
385-385: Starting a process with a partial executable path
(S607)
408-408: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
423-423: Consider moving this statement to an else block
(TRY300)
424-424: Do not catch blind exception: Exception
(BLE001)
433-433: Do not catch blind exception: Exception
(BLE001)
441-441: subprocess call: check for execution of untrusted input
(S603)
442-442: Starting a process with a partial executable path
(S607)
447-447: Consider moving this statement to an else block
(TRY300)
452-452: Do not catch blind exception: Exception
(BLE001)
462-462: Starting a process with a partial executable path
(S607)
468-468: Unpacked variable stdout is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
475-475: Consider moving this statement to an else block
(TRY300)
476-476: Do not catch blind exception: Exception
(BLE001)
506-506: subprocess call: check for execution of untrusted input
(S603)
507-507: Starting a process with a partial executable path
(S607)
517-518: try-except-pass detected, consider logging the exception
(S110)
517-517: Do not catch blind exception: Exception
(BLE001)
524-524: Do not catch blind exception: Exception
(BLE001)
574-574: Possible binding to all interfaces
(S104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: e2e-tests-release-0-7
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: lint-go
- GitHub Check: deploy-kind
- GitHub Check: e2e-test-operator
- GitHub Check: tests
🔇 Additional comments (10)
deploy/microshift-bootc/kustomization.yaml (1)
1-10: LGTM!The Kustomization configuration is well-structured and follows Kubernetes labeling conventions. The resource reference and common labels are appropriate for this deployment.
deploy/microshift-bootc/Makefile (2)
18-35: LGTM!The QCOW2 image generation target is well-configured with appropriate volume mounts, security options, and the latest bootc-image-builder. The privileged mode and security options are necessary for the bootc image building process.
45-49: LGTM!The push target correctly uses non-sudo podman to leverage the user's registry credentials.
deploy/microshift-bootc/config-svc/app.py (7)
26-48: LGTM!The authentication implementation correctly restricts access to the root user and uses PAM as the primary method with a reasonable fallback to
su. The password is safely passed via stdin rather than command-line arguments.
51-68: LGTM!The HTTP Basic Authentication decorator is correctly implemented with proper 401 response and WWW-Authenticate header.
262-275: LGTM!The index route correctly retrieves system information and provides sensible defaults using nip.io for hostname suggestions.
278-331: LGTM!The configuration handler correctly validates inputs, executes all three operations (password, hostname, CR), and provides appropriate feedback. The error aggregation ensures users see all failures rather than stopping at the first error.
380-426: LGTM!The function correctly parses the default route and associated IP address, with appropriate error handling and nip.io formatting.
438-454: LGTM!The hostname configuration correctly uses
hostnamectlwith proper error handling and informative return values.
567-574: LGTM - Network binding is intentional.The service correctly binds to all interfaces (0.0.0.0) to allow remote access for configuration. This is appropriate given the service is protected by HTTP Basic Authentication and is intended for system administration.
mangelajo
force-pushed
the
microshift-image
branch
from
c0353a6 to
7bb3024
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
deploy/microshift-bootc/config-svc/app.py (1)
1129-1175: Fix Jumpstarter CRapiVersion+ apply against MicroShift explicitly.
- This still uses
apiVersion: jumpstarter.dev/v1alpha1, but the CRD group isoperator.jumpstarter.dev/v1alpha1(per prior review).kubectl applyhas no explicit--kubeconfig, so it can silently apply to the wrong cluster/context (or fail).def apply_jumpstarter_cr(base_domain, image): """Apply Jumpstarter Custom Resource using kubectl.""" try: + kubeconfig_path = '/var/lib/microshift/resources/kubeadmin/kubeconfig' # Build the CR YAML cr = { - 'apiVersion': 'jumpstarter.dev/v1alpha1', + 'apiVersion': 'operator.jumpstarter.dev/v1alpha1', 'kind': 'Jumpstarter', 'metadata': { 'name': 'jumpstarter', 'namespace': 'default' }, ... result = subprocess.run( - ['kubectl', 'apply', '-f', temp_file], + ['kubectl', '--kubeconfig', kubeconfig_path, 'apply', '-f', temp_file], capture_output=True, text=True, check=True )deploy/microshift-bootc/Makefile (1)
15-16: Fix typo in log message (“buinding” → “building”).- @echo "Building bootc image: $(BOOTC_IMG): buinding as root to be on the container storage from root" + @echo "Building bootc image: $(BOOTC_IMG): building as root to be on the container storage from root"
🧹 Nitpick comments (2)
deploy/microshift-bootc/config-svc/app.py (2)
1177-1213: Homegrown YAML emitter is too fragile for user-controlled strings; use a real YAML library.
json_to_yaml()/yaml_value()won’t correctly escape quotes/newlines/etc., and can generate invalid YAML (or surprise keys) for certain inputs. Preferyaml.safe_dump()(PyYAML or ruamel.yaml) and keep quoting/escaping correct by construction.+import yaml ... - yaml_content = json_to_yaml(cr) + yaml_content = yaml.safe_dump(cr, sort_keys=False)
1215-1229: Consider making bind host configurable (and avoid accidental LAN exposure).
Binding0.0.0.0is fine for container use, but it’s easy to expose the root-auth UI unintentionally. ConsiderBIND_HOSTenv defaulting to127.0.0.1for dev.def main(): """Main entry point.""" port = int(os.environ.get('PORT', 8080)) + bind_host = os.environ.get('BIND_HOST', '0.0.0.0') ... - app.run(host='0.0.0.0', port=port, debug=False) + app.run(host=bind_host, port=port, debug=False)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
deploy/microshift-bootc/Containerfile(1 hunks)deploy/microshift-bootc/Makefile(1 hunks)deploy/microshift-bootc/README.md(1 hunks)deploy/microshift-bootc/config-svc/app.py(1 hunks)deploy/microshift-bootc/config-svc/config-svc.service(1 hunks)deploy/microshift-bootc/config.toml(1 hunks)deploy/microshift-bootc/kustomization.yaml(1 hunks)deploy/microshift-bootc/run-microshift.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- deploy/microshift-bootc/kustomization.yaml
- deploy/microshift-bootc/config-svc/config-svc.service
- deploy/microshift-bootc/Containerfile
- deploy/microshift-bootc/config.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Applied to files:
deploy/microshift-bootc/config-svc/app.py
🪛 checkmake (0.2.2)
deploy/microshift-bootc/Makefile
[warning] 86-86: Target body for "clean" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 LanguageTool
deploy/microshift-bootc/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # MicroShift Bootc Deployment This directory contains the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~35-~35: Ensure spelling is correct
Context: ...ended ## Quick Start ### 1. Build the Bootc Image bash make bootc-build Th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
deploy/microshift-bootc/README.md
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
184-184: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.8)
deploy/microshift-bootc/config-svc/app.py
40-40: subprocess call: check for execution of untrusted input
(S603)
41-41: Starting a process with a partial executable path
(S607)
46-46: Consider moving this statement to an else block
(TRY300)
47-47: Do not catch blind exception: Exception
(BLE001)
78-127: Possible hardcoded password assigned to: "PASSWORD_REQUIRED_TEMPLATE"
(S105)
841-841: subprocess call: check for execution of untrusted input
(S603)
842-842: Starting a process with a partial executable path
(S607)
898-898: Do not catch blind exception: Exception
(BLE001)
899-899: Use explicit conversion flag
Replace with conversion flag
(RUF010)
949-949: Do not catch blind exception: Exception
(BLE001)
950-950: Use explicit conversion flag
Replace with conversion flag
(RUF010)
980-980: Do not catch blind exception: Exception
(BLE001)
990-990: Starting a process with a partial executable path
(S607)
1013-1013: subprocess call: check for execution of untrusted input
(S603)
1014-1014: Starting a process with a partial executable path
(S607)
1028-1028: Consider moving this statement to an else block
(TRY300)
1029-1029: Do not catch blind exception: Exception
(BLE001)
1038-1038: Do not catch blind exception: Exception
(BLE001)
1046-1046: subprocess call: check for execution of untrusted input
(S603)
1047-1047: Starting a process with a partial executable path
(S607)
1052-1052: Consider moving this statement to an else block
(TRY300)
1057-1057: Do not catch blind exception: Exception
(BLE001)
1067-1067: Starting a process with a partial executable path
(S607)
1073-1073: Unpacked variable stdout is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1080-1080: Consider moving this statement to an else block
(TRY300)
1081-1081: Do not catch blind exception: Exception
(BLE001)
1124-1124: Do not catch blind exception: Exception
(BLE001)
1154-1154: subprocess call: check for execution of untrusted input
(S603)
1155-1155: Starting a process with a partial executable path
(S607)
1165-1166: try-except-pass detected, consider logging the exception
(S110)
1165-1165: Do not catch blind exception: Exception
(BLE001)
1172-1172: Do not catch blind exception: Exception
(BLE001)
1229-1229: Possible binding to all interfaces
(S104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: e2e-test-operator
- GitHub Check: deploy-kind
- GitHub Check: tests
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: lint-go
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-release-0-7
| PODS_TEMPLATE = """<!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>Pod Status - Jumpstarter</title> | ||
| <link href="https://jumpstarter.dev/_static/favicon.png" rel="shortcut icon"> | ||
| <link rel="stylesheet" href="/static/styles.css"> | ||
| <style> | ||
| /* Pods page specific overrides */ | ||
| .container { | ||
| max-width: 1400px; | ||
| } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <div class="container"> | ||
| <div class="banner"> | ||
| <div class="banner-text">Jumpstarter Controller Community Edition • Powered by MicroShift</div> | ||
| <div class="logos"> | ||
| <a href="https://jumpstarter.dev/" target="_blank" rel="noopener noreferrer" class="logo-link" title="Jumpstarter - Remote device management and testing"> | ||
| <img src="https://jumpstarter.dev/main/_static/logo-light-theme.svg" alt="Jumpstarter" class="jumpstarter-logo" /> | ||
| </a> | ||
| <a href="https://microshift.io/" target="_blank" rel="noopener noreferrer" class="logo-link" title="MicroShift - Optimized OpenShift for the device edge"> | ||
| <img src="https://microshift.io/images/microshift_logo_white.png" alt="MicroShift" class="microshift-logo" /> | ||
| </a> | ||
| </div> | ||
| </div> | ||
| <div class="header"> | ||
| <div> | ||
| <h1>Pod Status Monitor</h1> | ||
| <div class="refresh-info">Auto-refreshing every 5 seconds</div> | ||
| </div> | ||
| <a href="/" class="back-btn">← Back to Configuration</a> | ||
| </div> | ||
|
|
||
| <div id="error-container"></div> | ||
| <div id="pod-count" class="pod-count"></div> | ||
|
|
||
| <div class="table-wrapper"> | ||
| <table id="pods-table"> | ||
| <thead> | ||
| <tr> | ||
| <th>Namespace</th> | ||
| <th>Name</th> | ||
| <th>Ready</th> | ||
| <th>Status</th> | ||
| <th>Restarts</th> | ||
| <th>Age</th> | ||
| <th>Node</th> | ||
| </tr> | ||
| </thead> | ||
| <tbody id="pods-body"> | ||
| <tr> | ||
| <td colspan="7" class="loading">Loading pods...</td> | ||
| </tr> | ||
| </tbody> | ||
| </table> | ||
| </div> | ||
| </div> | ||
|
|
||
| <script> | ||
| function getStatusClass(status) { | ||
| const statusLower = status.toLowerCase(); | ||
| if (statusLower === 'running') return 'status-running'; | ||
| if (statusLower === 'pending') return 'status-pending'; | ||
| if (statusLower === 'failed' || statusLower === 'error') return 'status-failed'; | ||
| if (statusLower === 'succeeded' || statusLower === 'completed') return 'status-succeeded'; | ||
| if (statusLower.includes('crashloop')) return 'status-crashloopbackoff'; | ||
| return 'status-unknown'; | ||
| } | ||
|
|
||
| function updatePods() { | ||
| fetch('/api/pods') | ||
| .then(response => response.json()) | ||
| .then(data => { | ||
| const errorContainer = document.getElementById('error-container'); | ||
| const podCount = document.getElementById('pod-count'); | ||
| const tbody = document.getElementById('pods-body'); | ||
|
|
||
| if (data.error) { | ||
| errorContainer.innerHTML = '<div class="error">' + data.error + '</div>'; | ||
| tbody.innerHTML = '<tr><td colspan="7" class="loading">Failed to load pods</td></tr>'; | ||
| return; | ||
| } | ||
|
|
||
| errorContainer.innerHTML = ''; | ||
| podCount.textContent = 'Total pods: ' + data.pods.length; | ||
|
|
||
| if (data.pods.length === 0) { | ||
| tbody.innerHTML = '<tr><td colspan="7" class="loading">No pods found</td></tr>'; | ||
| return; | ||
| } | ||
|
|
||
| tbody.innerHTML = data.pods.map(pod => ` | ||
| <tr> | ||
| <td>${pod.namespace}</td> | ||
| <td>${pod.name}</td> | ||
| <td>${pod.ready}</td> | ||
| <td><span class="status-badge ${getStatusClass(pod.status)}">${pod.status}</span></td> | ||
| <td>${pod.restarts}</td> | ||
| <td>${pod.age}</td> | ||
| <td>${pod.node}</td> | ||
| </tr> | ||
| `).join(''); | ||
| }) | ||
| .catch(error => { | ||
| console.error('Error fetching pods:', error); | ||
| document.getElementById('error-container').innerHTML = | ||
| '<div class="error">Failed to fetch pod data: ' + error.message + '</div>'; | ||
| }); | ||
| } | ||
|
|
||
| // Initial load | ||
| updatePods(); | ||
|
|
||
| // Refresh every 5 seconds | ||
| setInterval(updatePods, 5000); | ||
| </script> | ||
| </body> | ||
| </html>""" |
There was a problem hiding this comment.
DOM XSS on /pods: untrusted pod fields and data.error are injected via innerHTML / template literals.
Pod names/namespaces/status can be attacker-controlled by any workload author in-cluster; rendering them via HTML strings is an XSS vector. Build DOM nodes and set textContent (and avoid innerHTML for errors).
- errorContainer.innerHTML = '<div class="error">' + data.error + '</div>';
+ const err = document.createElement('div');
+ err.className = 'error';
+ err.textContent = data.error;
+ errorContainer.replaceChildren(err);
...
- tbody.innerHTML = data.pods.map(pod => `...`).join('');
+ tbody.replaceChildren(...data.pods.map(pod => {
+ const tr = document.createElement('tr');
+ const tdNs = document.createElement('td'); tdNs.textContent = pod.namespace;
+ const tdName = document.createElement('td'); tdName.textContent = pod.name;
+ const tdReady = document.createElement('td'); tdReady.textContent = pod.ready;
+ const tdStatus = document.createElement('td');
+ const badge = document.createElement('span');
+ badge.className = `status-badge ${getStatusClass(pod.status)}`;
+ badge.textContent = pod.status;
+ tdStatus.appendChild(badge);
+ const tdRestarts = document.createElement('td'); tdRestarts.textContent = String(pod.restarts);
+ const tdAge = document.createElement('td'); tdAge.textContent = pod.age;
+ const tdNode = document.createElement('td'); tdNode.textContent = pod.node;
+ tr.append(tdNs, tdName, tdReady, tdStatus, tdRestarts, tdAge, tdNode);
+ return tr;
+ }));🤖 Prompt for AI Agents
In deploy/microshift-bootc/config-svc/app.py around lines 130 to 250, the client
HTML currently injects untrusted pod fields and data.error via
innerHTML/template literals causing DOM XSS; replace all innerHTML-based
rendering for errors and table rows with DOM node creation and textContent
assignments. Specifically: when data.error is present, set
errorContainer.textContent (or create a div element and set its textContent)
instead of injecting HTML; when rendering pods, clear tbody and for each pod
create a tr with td elements, assigning
pod.namespace/name/ready/status/restarts/age/node via textContent, create the
status badge span and add classes with classList.add rather than injecting HTML;
for loading/no-pods messages create a tr>td and set td.textContent; remove all
uses of innerHTML for untrusted content and only use innerHTML for static/safe
templates if needed.
| **Output example:** | ||
| ``` | ||
| MicroShift is running in a bootc container | ||
| Hostname: jumpstarter.10.0.2.2.nip.io | ||
| Container: jumpstarter-microshift-okd | ||
| LVM disk: /var/lib/microshift-okd/lvmdisk.image | ||
| VG name: myvg1 | ||
| Ports: HTTP:80, HTTPS:443, Config Service:8880 | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Add languages to fenced code blocks to satisfy markdownlint (MD040).
At least the “Output example” and the architecture diagram blocks should specify a language (text is fine).
-```
+```text
MicroShift is running in a bootc container
......
- +text
┌─────────────────────────────────────────────┐
...
Also applies to: 184-195
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In deploy/microshift-bootc/README.md around lines 55 to 64 and also around lines
184 to 195, the fenced code blocks currently lack a language identifier which
triggers markdownlint MD040; update each triple-backtick fence for the “Output
example” block and the architecture diagram block to include a language (use
"text") so the opening fences become ```text, leaving the block contents
unchanged.
121a1f3 to
87c6824
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (15)
deploy/microshift-bootc/README.md (2)
55-64: Add language specifier to fenced code blocks.This was flagged in a previous review. The output example block should specify
textas the language.
184-195: Add language specifier to the architecture diagram code block.This was flagged in a previous review. The diagram block should specify
textas the language.deploy/microshift-bootc/run-microshift.sh (4)
86-86: Hostname is hardcoded instead of using the host's default route IP.This was flagged in a previous review. The README implies the hostname should use the host's IP address, but
127-0-0-1is hardcoded.
66-78: Missingpvcreatebeforevgcreateon raw disk image.As noted in a previous review,
vgcreatetypically requires the device to be initialized as a physical volume first. On a fresh truncated disk, this will likely fail.# Find available loop device LOOP_DEV=\$(losetup --find --show --nooverlap '${container_lvm_disk}') echo \"Created loop device: \${LOOP_DEV}\" + # Initialize physical volume + pvcreate -ff -y \"\${LOOP_DEV}\" + # Create volume group vgcreate -f -y '${vg_name}' \"\${LOOP_DEV}\"
116-124: Add timeout to kubeconfig wait loop to prevent indefinite hangs.This was flagged in a previous review. If MicroShift fails to start, this loop will hang forever.
echo "Waiting for MicroShift to start" local -r kubeconfig="/var/lib/microshift/resources/kubeadmin/kubeconfig" + local -r deadline=$((SECONDS + 180)) while true ; do if podman exec "${container_name}" /bin/test -f "${kubeconfig}" &>/dev/null ; then break fi + if (( SECONDS > deadline )); then + echo "ERROR: Timed out waiting for kubeconfig" >&2 + return 1 + fi sleep 1 done
157-157: Config Service URL will not resolve.
${HOSTNAME%%.*}strips the domain, resulting inhttp://jumpstarter:8880which won't resolve externally. Consider usinghttp://localhost:${CONFIG_SVC_PORT}only, or use the full hostname.-echo " - Config Service: http://${HOSTNAME%%.*}:${CONFIG_SVC_PORT} or http://localhost:${CONFIG_SVC_PORT}" +echo " - Config Service: http://localhost:${CONFIG_SVC_PORT}"deploy/microshift-bootc/Makefile (4)
15-15: Typo: "buinding" should be "building".This was flagged in a previous review.
50-53:podman pushwill fail because image is in root's storage.Since
bootc-buildusessudo podman build, the push command should also usesudo podman push. This was flagged in a previous review.
86-89:podman rmiwill fail because image is in root's storage.Consistent with the build target, this should use
sudo podman rmi. This was flagged in a previous review.
68-68: Missing space before##breaks prerequisite.
bootc-stop##becomes a literal target name. This was flagged in a previous review.-bootc-rm: bootc-stop## Remove the MicroShift container +bootc-rm: bootc-stop ## Remove the MicroShift containerdeploy/microshift-bootc/config-svc/app.py (5)
27-50:sufallback authentication is unreliable without a TTY.The
sucommand typically requires a TTY for password input. Providing password via stdin often won't authenticate correctly, potentially locking users out if PAM isn't available. This was flagged in a previous review.
1154-1170: Validate hostname/baseDomain input.Arbitrary input to
hostnamectl set-hostnamecould cause issues. Consider validating against a hostname pattern. This was flagged in a previous review for the configure endpoint.+import re +HOSTNAME_RE = re.compile(r'^(?=.{1,253}$)(?!-)([a-zA-Z0-9-]{1,63}(?<!-)\.)*[a-zA-Z0-9-]{1,63}(?<!-)$') + def set_hostname(hostname): """Set the system hostname using hostnamectl.""" + if not HOSTNAME_RE.match(hostname): + return False, "Invalid hostname format" try:
1247-1258: Fix apiVersion for Jumpstarter CR.The CR uses
apiVersion: 'jumpstarter.dev/v1alpha1', but the Jumpstarter CRD is defined in theoperator.jumpstarter.devgroup. This will cause the CR to fail when applied. This was flagged in a previous review.cr = { - 'apiVersion': 'jumpstarter.dev/v1alpha1', + 'apiVersion': 'operator.jumpstarter.dev/v1alpha1', 'kind': 'Jumpstarter',
210-234: DOM XSS vulnerability viainnerHTMLwith pod data.Pod names, namespaces, and status values from the cluster are inserted via template literals into
innerHTML. Any workload author in the cluster could inject malicious scripts. This was flagged in a previous review.Use DOM APIs with
textContentinstead ofinnerHTML:// Replace template literal injection with safe DOM construction tbody.replaceChildren(...data.pods.map(pod => { const tr = document.createElement('tr'); [pod.namespace, pod.name, pod.ready].forEach(val => { const td = document.createElement('td'); td.textContent = val; tr.appendChild(td); }); // ... status badge with textContent, restarts, age, node return tr; }));
1173-1194: Validate password to preventchpasswdinjection.If the password contains
:or newline characters, it could break thechpasswdinput format or inject additional entries. This was flagged in a previous review.def set_root_password(password): """Set the root user password using chpasswd.""" + # Validate password to prevent chpasswd format injection + if any(c in password for c in (':', '\n', '\r')): + return False, "Password contains invalid characters (: or newlines not allowed)" + try: # Use chpasswd to set password (more reliable than passwd for scripting)
🧹 Nitpick comments (1)
deploy/microshift-bootc/Makefile (1)
1-1: Addcleanto.PHONYdeclaration.The
cleantarget at line 86 should be declared as phony to ensure it always runs.-.PHONY: help build bootc-build push bootc-push bootc-run bootc-stop bootc-sh bootc-rm +.PHONY: help build bootc-build push bootc-push bootc-run bootc-stop bootc-sh bootc-rm clean build-image bootc-reload-app
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
deploy/microshift-bootc/Containerfile(1 hunks)deploy/microshift-bootc/Makefile(1 hunks)deploy/microshift-bootc/README.md(1 hunks)deploy/microshift-bootc/config-svc/app.py(1 hunks)deploy/microshift-bootc/config-svc/config-svc.service(1 hunks)deploy/microshift-bootc/config-svc/update-banner.service(1 hunks)deploy/microshift-bootc/config-svc/update-banner.sh(1 hunks)deploy/microshift-bootc/config.toml(1 hunks)deploy/microshift-bootc/kustomization.yaml(1 hunks)deploy/microshift-bootc/run-microshift.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/microshift-bootc/Containerfile
- deploy/microshift-bootc/config-svc/config-svc.service
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-13T19:56:27.924Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml:23-26
Timestamp: 2025-05-13T19:56:27.924Z
Learning: In the jumpstarter-controller project, the router service uses the same ConfigMap as the controller service (controller-cm.yaml) even though it has been moved to its own separate deployment.
Applied to files:
deploy/microshift-bootc/run-microshift.sh
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Applied to files:
deploy/microshift-bootc/kustomization.yamldeploy/microshift-bootc/config-svc/app.py
🪛 checkmake (0.2.2)
deploy/microshift-bootc/Makefile
[warning] 86-86: Target body for "clean" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 LanguageTool
deploy/microshift-bootc/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # MicroShift Bootc Deployment This directory contains the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~35-~35: Ensure spelling is correct
Context: ...ended ## Quick Start ### 1. Build the Bootc Image bash make bootc-build Th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
deploy/microshift-bootc/README.md
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
184-184: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.8)
deploy/microshift-bootc/config-svc/app.py
40-40: subprocess call: check for execution of untrusted input
(S603)
41-41: Starting a process with a partial executable path
(S607)
46-46: Consider moving this statement to an else block
(TRY300)
47-47: Do not catch blind exception: Exception
(BLE001)
78-127: Possible hardcoded password assigned to: "PASSWORD_REQUIRED_TEMPLATE"
(S105)
899-899: subprocess call: check for execution of untrusted input
(S603)
900-900: Starting a process with a partial executable path
(S607)
930-930: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
935-935: Do not catch blind exception: Exception
(BLE001)
935-935: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
952-952: subprocess call: check for execution of untrusted input
(S603)
953-953: Starting a process with a partial executable path
(S607)
1009-1009: Do not catch blind exception: Exception
(BLE001)
1010-1010: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1060-1060: Do not catch blind exception: Exception
(BLE001)
1061-1061: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1091-1091: Do not catch blind exception: Exception
(BLE001)
1101-1101: Starting a process with a partial executable path
(S607)
1124-1124: subprocess call: check for execution of untrusted input
(S603)
1125-1125: Starting a process with a partial executable path
(S607)
1139-1139: Consider moving this statement to an else block
(TRY300)
1140-1140: Do not catch blind exception: Exception
(BLE001)
1149-1149: Do not catch blind exception: Exception
(BLE001)
1157-1157: subprocess call: check for execution of untrusted input
(S603)
1158-1158: Starting a process with a partial executable path
(S607)
1163-1163: Consider moving this statement to an else block
(TRY300)
1168-1168: Do not catch blind exception: Exception
(BLE001)
1178-1178: Starting a process with a partial executable path
(S607)
1184-1184: Unpacked variable stdout is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1191-1191: Consider moving this statement to an else block
(TRY300)
1192-1192: Do not catch blind exception: Exception
(BLE001)
1231-1231: Do not catch blind exception: Exception
(BLE001)
1268-1268: subprocess call: check for execution of untrusted input
(S603)
1269-1269: Starting a process with a partial executable path
(S607)
1279-1280: try-except-pass detected, consider logging the exception
(S110)
1279-1279: Do not catch blind exception: Exception
(BLE001)
1286-1286: Do not catch blind exception: Exception
(BLE001)
1343-1343: Possible binding to all interfaces
(S104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: lint-go
- GitHub Check: e2e-test-operator
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests-release-0-7
- GitHub Check: deploy-kind
- GitHub Check: tests
🔇 Additional comments (6)
deploy/microshift-bootc/kustomization.yaml (1)
1-11: LGTM!The kustomization file is well-structured with proper labels following Kubernetes conventions.
deploy/microshift-bootc/README.md (1)
1-420: Comprehensive and well-organized documentation.The README provides thorough coverage of setup, usage, troubleshooting, and architecture. The community edition disclaimer and security notes are particularly helpful.
deploy/microshift-bootc/config.toml (1)
1-38: Configuration looks appropriate for the bootc deployment.LVM setup with volume group
myvg1and XFS root filesystem follows sensible defaults for the MicroShift use case.deploy/microshift-bootc/config-svc/update-banner.service (1)
1-19: LGTM!The systemd service is properly configured as a oneshot with correct ordering dependencies to ensure the banner is updated before the login prompt appears.
deploy/microshift-bootc/config-svc/app.py (2)
1343-1343: Flask development server binds to all interfaces.
app.run(host='0.0.0.0', ...)exposes the service to all network interfaces. This is intentional for the config UI use case, but ensure the documentation reflects this security consideration (which it does in the README security notes).
1064-1093: Age calculation helper is well-implemented.Clean implementation with proper timezone handling and graceful fallback on errors.
mangelajo
force-pushed
the
microshift-image
branch
from
87c6824 to
0bc1fcc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
deploy/microshift-bootc/README.md (2)
55-64: Add language identifier to fenced code block.The output example code block is missing a language identifier, which triggers markdownlint MD040.
Apply this diff:
**Output example:** -``` +```text MicroShift is running in a bootc container Hostname: jumpstarter.10.0.2.2.nip.io
184-195: Add language identifier to architecture diagram code block.The architecture diagram code block is missing a language identifier, which triggers markdownlint MD040.
Apply this diff:
### Components -``` +```text ┌─────────────────────────────────────────────┐ │ Bootc Container / Image │deploy/microshift-bootc/Makefile (4)
14-17: Fix typo: "buinding" should be "building".Apply this diff:
bootc-build: ## Build the bootc image with MicroShift - @echo "Building bootc image: $(BOOTC_IMG): buinding as root to be on the container storage from root" + @echo "Building bootc image: $(BOOTC_IMG): building as root to be on the container storage from root" sudo podman build -t $(BOOTC_IMG) -f Containerfile ../..
50-53: Add sudo to podman push for consistency.Since
bootc-buildusessudo podman build, the image is in root's container storage. Thepodman pushcommand needssudoto access it.Apply this diff:
bootc-push: ## Push the bootc image to registry @echo "Pushing bootc image: $(BOOTC_IMG)" - podman push $(BOOTC_IMG) + sudo podman push $(BOOTC_IMG)
68-68: Fix missing space before comment marker.The lack of space before
##causesbootc-stop##to be treated as a literal prerequisite name, preventingbootc-stopfrom running.Apply this diff:
-bootc-rm: bootc-stop## Remove the MicroShift container +bootc-rm: bootc-stop ## Remove the MicroShift container
86-89: Add sudo to podman rmi for consistency.Since the image was built with
sudo podman build, it's in root's storage and requiressudoto remove.Apply this diff:
clean: ## Clean up local images and build artifacts @echo "Removing local images..." - -podman rmi $(BOOTC_IMG) + -sudo podman rmi $(BOOTC_IMG)deploy/microshift-bootc/config-svc/app.py (3)
260-260: DOM XSS vulnerability: untrusted data injected via innerHTML.User-controlled values from pod/route data and error messages are injected directly into the DOM via
innerHTML, creating an XSS vulnerability. Pod names and other fields can be controlled by workload authors.Replace innerHTML-based rendering with DOM node creation and textContent assignments to prevent XSS.
Also applies to: 273-284, 312-312, 325-336
884-971: Validate password input to prevent chpasswd injection.The password is passed to
chpasswdwithout validating for colon (:) or newline characters, which could cause command injection sincechpasswdexpectsuser:password\nformat.Add validation before calling
set_root_password():def change_password(): new_password = request.form.get('newPassword', '').strip() confirm_password = request.form.get('confirmPassword', '').strip() + + if any(c in new_password for c in (':', '\n', '\r')): + messages.append({'type': 'error', 'text': 'Password contains invalid characters'}) + # return error response
974-1021: Validate base domain as proper hostname.The
baseDomainparameter is passed toset_hostname()without validation, which could result in invalid hostnames or unexpected behavior.Add hostname validation:
+import re +HOSTNAME_RE = re.compile(r"^(?=.{1,253}$)(?!-)([a-zA-Z0-9-]{1,63}(?<!-)\.)*[a-zA-Z0-9-]{1,63}(?<!-)$") + def configure_jumpstarter(): base_domain = request.form.get('baseDomain', '').strip() image = request.form.get('image', '').strip() + if base_domain and not HOSTNAME_RE.match(base_domain): + messages.append({'type': 'error', 'text': 'Base domain is not a valid hostname'}) + # return error response
🧹 Nitpick comments (1)
deploy/microshift-bootc/Containerfile (1)
4-7: Pin python-pam version for reproducible builds.The
python-pampackage is installed via pip without version pinning, which can lead to non-reproducible builds and unexpected behavior if the package is updated.Consider applying this diff:
RUN dnf install -y epel-release && \ dnf install -y python3 iproute python3-flask python3-pip && \ - pip3 install python-pam && \ + pip3 install --no-cache-dir python-pam==2.0.2 && \ dnf clean allNote: Verify the appropriate version by checking PyPI or your project requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
deploy/microshift-bootc/Containerfile(1 hunks)deploy/microshift-bootc/Makefile(1 hunks)deploy/microshift-bootc/README.md(1 hunks)deploy/microshift-bootc/config-svc/app.py(1 hunks)deploy/microshift-bootc/config-svc/config-svc.service(1 hunks)deploy/microshift-bootc/config-svc/update-banner.service(1 hunks)deploy/microshift-bootc/config-svc/update-banner.sh(1 hunks)deploy/microshift-bootc/config.toml(1 hunks)deploy/microshift-bootc/kustomization.yaml(1 hunks)deploy/microshift-bootc/run-microshift.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- deploy/microshift-bootc/kustomization.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- deploy/microshift-bootc/config.toml
- deploy/microshift-bootc/run-microshift.sh
- deploy/microshift-bootc/config-svc/update-banner.sh
- deploy/microshift-bootc/config-svc/config-svc.service
- deploy/microshift-bootc/config-svc/update-banner.service
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Applied to files:
deploy/microshift-bootc/config-svc/app.py
🪛 checkmake (0.2.2)
deploy/microshift-bootc/Makefile
[warning] 86-86: Target body for "clean" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 LanguageTool
deploy/microshift-bootc/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # MicroShift Bootc Deployment This directory contains the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~35-~35: Ensure spelling is correct
Context: ...ended ## Quick Start ### 1. Build the Bootc Image bash make bootc-build Th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
deploy/microshift-bootc/README.md
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
184-184: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.8)
deploy/microshift-bootc/config-svc/app.py
40-40: subprocess call: check for execution of untrusted input
(S603)
41-41: Starting a process with a partial executable path
(S607)
46-46: Consider moving this statement to an else block
(TRY300)
47-47: Do not catch blind exception: Exception
(BLE001)
78-127: Possible hardcoded password assigned to: "PASSWORD_REQUIRED_TEMPLATE"
(S105)
1044-1044: subprocess call: check for execution of untrusted input
(S603)
1045-1045: Starting a process with a partial executable path
(S607)
1075-1075: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
1080-1080: Do not catch blind exception: Exception
(BLE001)
1080-1080: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
1097-1097: subprocess call: check for execution of untrusted input
(S603)
1098-1098: Starting a process with a partial executable path
(S607)
1154-1154: Do not catch blind exception: Exception
(BLE001)
1155-1155: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1171-1171: subprocess call: check for execution of untrusted input
(S603)
1172-1172: Starting a process with a partial executable path
(S607)
1234-1234: Do not catch blind exception: Exception
(BLE001)
1235-1235: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1253-1253: subprocess call: check for execution of untrusted input
(S603)
1254-1254: Starting a process with a partial executable path
(S607)
1267-1267: Do not catch blind exception: Exception
(BLE001)
1268-1268: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1275-1275: Do not catch blind exception: Exception
(BLE001)
1395-1395: Do not catch blind exception: Exception
(BLE001)
1396-1396: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1426-1426: Do not catch blind exception: Exception
(BLE001)
1436-1436: Starting a process with a partial executable path
(S607)
1459-1459: subprocess call: check for execution of untrusted input
(S603)
1460-1460: Starting a process with a partial executable path
(S607)
1474-1474: Consider moving this statement to an else block
(TRY300)
1475-1475: Do not catch blind exception: Exception
(BLE001)
1484-1484: Do not catch blind exception: Exception
(BLE001)
1492-1492: subprocess call: check for execution of untrusted input
(S603)
1493-1493: Starting a process with a partial executable path
(S607)
1498-1498: Consider moving this statement to an else block
(TRY300)
1503-1503: Do not catch blind exception: Exception
(BLE001)
1513-1513: Starting a process with a partial executable path
(S607)
1519-1519: Unpacked variable stdout is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1526-1526: Consider moving this statement to an else block
(TRY300)
1527-1527: Do not catch blind exception: Exception
(BLE001)
1566-1566: Do not catch blind exception: Exception
(BLE001)
1633-1633: subprocess call: check for execution of untrusted input
(S603)
1634-1634: Starting a process with a partial executable path
(S607)
1644-1645: try-except-pass detected, consider logging the exception
(S110)
1644-1644: Do not catch blind exception: Exception
(BLE001)
1651-1651: Do not catch blind exception: Exception
(BLE001)
1708-1708: Possible binding to all interfaces
(S104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: lint-go
- GitHub Check: e2e-test-operator
- GitHub Check: tests
- GitHub Check: deploy-kind
- GitHub Check: e2e-tests-release-0-7
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
🔇 Additional comments (5)
deploy/microshift-bootc/Containerfile (5)
1-1: LGTM: Base image properly pinned.The base image uses a specific version tag, which ensures reproducible builds.
9-12: LGTM: Manifest installation is correct.The manifests are properly copied to the MicroShift manifests directory with appropriate naming convention.
14-18: LGTM: Firewall configuration is appropriate.Correctly uses
firewall-offline-cmdduring the build phase, and the comment explains the rationale.
20-21: LGTM: Default password with forced change mechanism.The default password is appropriately documented in the README, and the web UI (config-svc/app.py) enforces a password change on first use.
23-30: LGTM: Config service installation is correct.All necessary components are copied, permissions are set, and systemd services are properly enabled.
mangelajo
force-pushed
the
microshift-image
branch
from
0bc1fcc to
57ed276
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
deploy/microshift-bootc/config-svc/config-svc.service (1)
6-6: Before= ordering may unnecessarily delay boot.The
[email protected] systemd-user-sessions.servicedirective forces this web UI service to start before login prompts appear. If the Flask app is slow to start or fails, it will delay all user logins. This ordering is typically reserved for essential early-boot services (like the update-banner script), not web UIs.Consider removing this line unless the service must complete before any user can log in:
After=network-online.target Wants=network-online.target -[email protected] systemd-user-sessions.servicedeploy/microshift-bootc/config-svc/app.py (1)
2031-2066: Prefer PyYAML over custom YAML serialization.The custom
json_to_yaml()andyaml_value()functions may not handle all YAML edge cases correctly (e.g., strings with quotes, newlines, unicode, or leading/trailing spaces). This could generate malformed YAML when applying the Jumpstarter CR.Use an established YAML library instead:
+import yaml + def json_to_yaml(obj, indent=0): - """Convert a JSON object to YAML format (simple implementation).""" - lines = [] - indent_str = ' ' * indent - - if isinstance(obj, dict): - for key, value in obj.items(): - if isinstance(value, (dict, list)): - lines.append(f"{indent_str}{key}:") - lines.append(json_to_yaml(value, indent + 1)) - else: - lines.append(f"{indent_str}{key}: {yaml_value(value)}") - elif isinstance(obj, list): - for item in obj: - if isinstance(item, (dict, list)): - lines.append(f"{indent_str}-") - lines.append(json_to_yaml(item, indent + 1)) - else: - lines.append(f"{indent_str}- {yaml_value(item)}") - - return '\n'.join(lines) - - -def yaml_value(value): - """Format a value for YAML output.""" - if value is None: - return 'null' - elif isinstance(value, bool): - return 'true' if value else 'false' - elif isinstance(value, str): - # Quote strings that contain special characters - if ':' in value or '#' in value or value.startswith('-'): - return f'"{value}"' - return value - else: - return str(value) + """Convert a JSON object to YAML format.""" + return yaml.dump(obj, default_flow_style=False, sort_keys=False)Then update the Containerfile to install PyYAML:
RUN pip install flask pyyaml
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
deploy/microshift-bootc/Containerfile(1 hunks)deploy/microshift-bootc/Makefile(1 hunks)deploy/microshift-bootc/README.md(1 hunks)deploy/microshift-bootc/config-svc/app.py(1 hunks)deploy/microshift-bootc/config-svc/config-svc.service(1 hunks)deploy/microshift-bootc/config-svc/update-banner.service(1 hunks)deploy/microshift-bootc/config-svc/update-banner.sh(1 hunks)deploy/microshift-bootc/config.toml(1 hunks)deploy/microshift-bootc/kustomization.yaml(1 hunks)deploy/microshift-bootc/run-microshift.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- deploy/microshift-bootc/config-svc/update-banner.sh
- deploy/microshift-bootc/run-microshift.sh
- deploy/microshift-bootc/Containerfile
- deploy/microshift-bootc/kustomization.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Applied to files:
deploy/microshift-bootc/config-svc/app.py
🪛 checkmake (0.2.2)
deploy/microshift-bootc/Makefile
[warning] 86-86: Target body for "clean" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 LanguageTool
deploy/microshift-bootc/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # MicroShift Bootc Deployment This directory contains the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~35-~35: Ensure spelling is correct
Context: ...ended ## Quick Start ### 1. Build the Bootc Image bash make bootc-build Th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
deploy/microshift-bootc/README.md
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
184-184: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.8)
deploy/microshift-bootc/config-svc/app.py
43-43: subprocess call: check for execution of untrusted input
(S603)
44-44: Starting a process with a partial executable path
(S607)
49-49: Consider moving this statement to an else block
(TRY300)
50-50: Do not catch blind exception: Exception
(BLE001)
81-130: Possible hardcoded password assigned to: "PASSWORD_REQUIRED_TEMPLATE"
(S105)
1211-1211: Starting a process with a partial executable path
(S607)
1225-1225: Starting a process with a partial executable path
(S607)
1230-1230: Starting a process with a partial executable path
(S607)
1244-1244: Starting a process with a partial executable path
(S607)
1248-1248: Starting a process with a partial executable path
(S607)
1267-1267: Starting a process with a partial executable path
(S607)
1273-1273: Starting a process with a partial executable path
(S607)
1277-1277: Starting a process with a partial executable path
(S607)
1290-1290: Starting a process with a partial executable path
(S607)
1324-1324: Do not catch blind exception: Exception
(BLE001)
1325-1325: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1341-1341: subprocess call: check for execution of untrusted input
(S603)
1342-1342: Starting a process with a partial executable path
(S607)
1372-1372: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
1377-1377: Do not catch blind exception: Exception
(BLE001)
1377-1377: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
1394-1394: subprocess call: check for execution of untrusted input
(S603)
1395-1395: Starting a process with a partial executable path
(S607)
1451-1451: Do not catch blind exception: Exception
(BLE001)
1452-1452: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1468-1468: subprocess call: check for execution of untrusted input
(S603)
1469-1469: Starting a process with a partial executable path
(S607)
1531-1531: Do not catch blind exception: Exception
(BLE001)
1532-1532: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1548-1548: subprocess call: check for execution of untrusted input
(S603)
1549-1549: Starting a process with a partial executable path
(S607)
1563-1563: Do not catch blind exception: Exception
(BLE001)
1564-1564: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1582-1582: subprocess call: check for execution of untrusted input
(S603)
1583-1583: Starting a process with a partial executable path
(S607)
1596-1596: Do not catch blind exception: Exception
(BLE001)
1597-1597: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1604-1604: Do not catch blind exception: Exception
(BLE001)
1724-1724: Do not catch blind exception: Exception
(BLE001)
1725-1725: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1755-1755: Do not catch blind exception: Exception
(BLE001)
1765-1765: Starting a process with a partial executable path
(S607)
1788-1788: subprocess call: check for execution of untrusted input
(S603)
1789-1789: Starting a process with a partial executable path
(S607)
1803-1803: Consider moving this statement to an else block
(TRY300)
1804-1804: Do not catch blind exception: Exception
(BLE001)
1813-1813: Do not catch blind exception: Exception
(BLE001)
1838-1838: subprocess call: check for execution of untrusted input
(S603)
1839-1839: Starting a process with a partial executable path
(S607)
1859-1859: Do not catch blind exception: Exception
(BLE001)
1867-1867: subprocess call: check for execution of untrusted input
(S603)
1868-1868: Starting a process with a partial executable path
(S607)
1873-1873: Consider moving this statement to an else block
(TRY300)
1878-1878: Do not catch blind exception: Exception
(BLE001)
1888-1888: Starting a process with a partial executable path
(S607)
1894-1894: Unpacked variable stdout is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1901-1901: Consider moving this statement to an else block
(TRY300)
1902-1902: Do not catch blind exception: Exception
(BLE001)
1941-1941: Do not catch blind exception: Exception
(BLE001)
2008-2008: subprocess call: check for execution of untrusted input
(S603)
2009-2009: Starting a process with a partial executable path
(S607)
2019-2020: try-except-pass detected, consider logging the exception
(S110)
2019-2019: Do not catch blind exception: Exception
(BLE001)
2026-2026: Do not catch blind exception: Exception
(BLE001)
2083-2083: Possible binding to all interfaces
(S104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: lint-go
- GitHub Check: tests
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-test-operator
- GitHub Check: e2e-tests-release-0-7
- GitHub Check: deploy-kind
mangelajo
force-pushed
the
microshift-image
branch
from
57ed276 to
5b50788
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
deploy/microshift-bootc/config-svc/app.py (1)
675-689: DOM XSS vulnerability: Pod fields are still injected via template literals.This is a duplicate of a previously raised concern. The code at line 675 uses
innerHTMLwith template literals to render pod data:tbody.innerHTML = data.pods.map(pod => `...${pod.name}...${pod.namespace}...`).join('');Pod names, namespaces, and other fields can be controlled by workload authors in-cluster and may contain malicious scripts. These values should be rendered using DOM APIs with
textContentinstead of template literals withinnerHTML.deploy/microshift-bootc/run-microshift.sh (2)
65-78: Missing pvcreate before vgcreate (duplicate of previous comment).This issue was previously flagged. Line 73 runs
vgcreatedirectly on the loop device without initializing it as a physical volume first:vgcreate -f -y '${vg_name}' "${LOOP_DEV}"While modern LVM may handle this, explicit
pvcreateis more robust.
85-86: Hardcoded hostname doesn't reflect actual host IP (duplicate).This issue was previously flagged. Line 86 hardcodes the hostname:
local -r hostname="jumpstarter.127-0-0-1.nip.io"This should detect the actual default route IP as suggested in the previous comment. The 127.0.0.1 address will not be accessible from outside the host.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
deploy/microshift-bootc/Containerfile(1 hunks)deploy/microshift-bootc/Makefile(1 hunks)deploy/microshift-bootc/README.md(1 hunks)deploy/microshift-bootc/config-svc/app.py(1 hunks)deploy/microshift-bootc/config-svc/config-svc.service(1 hunks)deploy/microshift-bootc/config-svc/update-banner.service(1 hunks)deploy/microshift-bootc/config-svc/update-banner.sh(1 hunks)deploy/microshift-bootc/config.toml(1 hunks)deploy/microshift-bootc/kustomization.yaml(1 hunks)deploy/microshift-bootc/run-microshift.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- deploy/microshift-bootc/config-svc/update-banner.sh
- deploy/microshift-bootc/config-svc/update-banner.service
- deploy/microshift-bootc/kustomization.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-13T19:56:27.924Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml:23-26
Timestamp: 2025-05-13T19:56:27.924Z
Learning: In the jumpstarter-controller project, the router service uses the same ConfigMap as the controller service (controller-cm.yaml) even though it has been moved to its own separate deployment.
Applied to files:
deploy/microshift-bootc/run-microshift.sh
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Applied to files:
deploy/microshift-bootc/config-svc/app.py
🪛 checkmake (0.2.2)
deploy/microshift-bootc/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 86-86: Target body for "clean" exceeds allowed length of 5 (6).
(maxbodylength)
🪛 LanguageTool
deploy/microshift-bootc/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # MicroShift Bootc Deployment This directory contains the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~35-~35: Ensure spelling is correct
Context: ...ended ## Quick Start ### 1. Build the Bootc Image bash make bootc-build Th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
deploy/microshift-bootc/README.md
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
184-184: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.8)
deploy/microshift-bootc/config-svc/app.py
123-123: subprocess call: check for execution of untrusted input
(S603)
124-124: Starting a process with a partial executable path
(S607)
129-129: Consider moving this statement to an else block
(TRY300)
130-130: Do not catch blind exception: Exception
(BLE001)
161-212: Possible hardcoded password assigned to: "PASSWORD_REQUIRED_TEMPLATE"
(S105)
1421-1421: Starting a process with a partial executable path
(S607)
1435-1435: Starting a process with a partial executable path
(S607)
1440-1440: Starting a process with a partial executable path
(S607)
1454-1454: Starting a process with a partial executable path
(S607)
1458-1458: Starting a process with a partial executable path
(S607)
1477-1477: Starting a process with a partial executable path
(S607)
1483-1483: Starting a process with a partial executable path
(S607)
1487-1487: Starting a process with a partial executable path
(S607)
1500-1500: Starting a process with a partial executable path
(S607)
1534-1534: Do not catch blind exception: Exception
(BLE001)
1535-1535: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1551-1551: subprocess call: check for execution of untrusted input
(S603)
1552-1552: Starting a process with a partial executable path
(S607)
1582-1582: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
1587-1587: Do not catch blind exception: Exception
(BLE001)
1587-1587: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
1604-1604: subprocess call: check for execution of untrusted input
(S603)
1605-1605: Starting a process with a partial executable path
(S607)
1665-1665: Do not catch blind exception: Exception
(BLE001)
1666-1666: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1682-1682: subprocess call: check for execution of untrusted input
(S603)
1683-1683: Starting a process with a partial executable path
(S607)
1745-1745: Do not catch blind exception: Exception
(BLE001)
1746-1746: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1762-1762: subprocess call: check for execution of untrusted input
(S603)
1763-1763: Starting a process with a partial executable path
(S607)
1777-1777: Do not catch blind exception: Exception
(BLE001)
1778-1778: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1796-1796: subprocess call: check for execution of untrusted input
(S603)
1797-1797: Starting a process with a partial executable path
(S607)
1810-1810: Do not catch blind exception: Exception
(BLE001)
1811-1811: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1818-1818: Do not catch blind exception: Exception
(BLE001)
1938-1938: Do not catch blind exception: Exception
(BLE001)
1939-1939: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1969-1969: Do not catch blind exception: Exception
(BLE001)
1979-1979: Starting a process with a partial executable path
(S607)
2002-2002: subprocess call: check for execution of untrusted input
(S603)
2003-2003: Starting a process with a partial executable path
(S607)
2017-2017: Consider moving this statement to an else block
(TRY300)
2018-2018: Do not catch blind exception: Exception
(BLE001)
2027-2027: Do not catch blind exception: Exception
(BLE001)
2052-2052: subprocess call: check for execution of untrusted input
(S603)
2053-2053: Starting a process with a partial executable path
(S607)
2073-2073: Do not catch blind exception: Exception
(BLE001)
2081-2081: subprocess call: check for execution of untrusted input
(S603)
2082-2082: Starting a process with a partial executable path
(S607)
2087-2087: Consider moving this statement to an else block
(TRY300)
2092-2092: Do not catch blind exception: Exception
(BLE001)
2102-2102: Starting a process with a partial executable path
(S607)
2108-2108: Unpacked variable stdout is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
2115-2115: Consider moving this statement to an else block
(TRY300)
2116-2116: Do not catch blind exception: Exception
(BLE001)
2155-2155: Do not catch blind exception: Exception
(BLE001)
2222-2222: subprocess call: check for execution of untrusted input
(S603)
2223-2223: Starting a process with a partial executable path
(S607)
2233-2234: try-except-pass detected, consider logging the exception
(S110)
2233-2233: Do not catch blind exception: Exception
(BLE001)
2240-2240: Do not catch blind exception: Exception
(BLE001)
2297-2297: Possible binding to all interfaces
(S104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-release-0-7
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: tests
- GitHub Check: deploy-kind
- GitHub Check: lint-go
- GitHub Check: e2e-test-operator
🔇 Additional comments (2)
deploy/microshift-bootc/config-svc/config-svc.service (1)
1-21: Service configuration looks appropriate.The systemd unit is well-structured with proper dependencies and ordering. The
[email protected]directive ensures the config service starts before console login, which aligns with the banner update mechanism that displays the web UI URL.deploy/microshift-bootc/run-microshift.sh (1)
116-139: Timeout implementation successfully addresses previous concern.The kubeconfig wait loop now includes a configurable timeout (lines 118-138) with proper error handling:
local -r max_wait=${MICROSHIFT_KUBECONFIG_TIMEOUT:-300} # ... time tracking ... if [ "${elapsed}" -ge "${max_wait}" ]; then echo "ERROR: Timeout waiting for MicroShift kubeconfig..." >&2 return 1 fiThis prevents indefinite hangs and provides clear error messages.
Creates a bootc based image based on MicroShift for evaluation and small labs.
mangelajo
force-pushed
the
microshift-image
branch
from
5b50788 to
99bf0a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
deploy/microshift-bootc/README.md (1)
55-64: Fix MD040: add languages to fenced code blocks.-``` +```text MicroShift is running in a bootc container Hostname: jumpstarter.10.0.2.2.nip.io Container: jumpstarter-microshift-okd LVM disk: /var/lib/microshift-okd/lvmdisk.image VG name: myvg1 Ports: HTTP:80, HTTPS:443, Config Service:8880@@
-+text
┌─────────────────────────────────────────────┐
│ Bootc Container / Image │
├─────────────────────────────────────────────┤
│ • Fedora CoreOS 9 base │
│ • MicroShift 4.20 (OKD) │
│ • Jumpstarter Operator │
│ • TopoLVM CSI (storage) │
│ • Configuration Service (Python/Flask) │
│ • Firewalld (ports 22, 80, 443, 8880) │
└─────────────────────────────────────────────┘Also applies to: 184-196 </blockquote></details> <details> <summary>deploy/microshift-bootc/config-svc/app.py (3)</summary><blockquote> `624-736`: **DOM XSS: pods/routes/error strings are injected via `innerHTML` / template literals.** Any workload author can influence pod/route fields; render with DOM nodes + `textContent` instead. ```diff - errorContainer.innerHTML = '<div class="error">' + data.error + '</div>'; + const err = document.createElement('div'); + err.className = 'error'; + err.textContent = data.error; + errorContainer.replaceChildren(err); @@ - tbody.innerHTML = data.pods.map(pod => ` - <tr> - <td>${pod.namespace}</td> - <td>${pod.name}</td> - <td>${pod.ready}</td> - <td><span class="status-badge ${getStatusClass(pod.status)}">${pod.status}</span></td> - <td>${pod.restarts}</td> - <td>${pod.age}</td> - <td>${pod.node}</td> - <td> - <a href="/logs/${pod.namespace}/${pod.name}" target="_blank" class="action-icon" title="View Logs">📋</a> - <a href="#" onclick="restartPod('${pod.namespace}', '${pod.name}'); return false;" class="action-icon" title="Restart Pod">🔄</a> - </td> - </tr> - `).join(''); + tbody.replaceChildren(...data.pods.map(pod => { + const tr = document.createElement('tr'); + const tdNs = document.createElement('td'); tdNs.textContent = pod.namespace; + const tdName = document.createElement('td'); tdName.textContent = pod.name; + const tdReady = document.createElement('td'); tdReady.textContent = pod.ready; + const tdStatus = document.createElement('td'); + const badge = document.createElement('span'); + badge.className = `status-badge ${getStatusClass(pod.status)}`; + badge.textContent = pod.status; + tdStatus.appendChild(badge); + const tdRestarts = document.createElement('td'); tdRestarts.textContent = String(pod.restarts); + const tdAge = document.createElement('td'); tdAge.textContent = pod.age; + const tdNode = document.createElement('td'); tdNode.textContent = pod.node; + const tdActions = document.createElement('td'); + const logs = document.createElement('a'); + logs.href = `/logs/${encodeURIComponent(pod.namespace)}/${encodeURIComponent(pod.name)}`; + logs.target = '_blank'; + logs.className = 'action-icon'; + logs.title = 'View Logs'; + logs.textContent = '📋'; + const restart = document.createElement('a'); + restart.href = '#'; + restart.className = 'action-icon'; + restart.title = 'Restart Pod'; + restart.textContent = '🔄'; + restart.addEventListener('click', (e) => { e.preventDefault(); restartPod(pod.namespace, pod.name); }); + tdActions.append(logs, restart); + tr.append(tdNs, tdName, tdReady, tdStatus, tdRestarts, tdAge, tdNode, tdActions); + return tr; + }));(Apply the same pattern to routes + the remaining
innerHTMLerror paths.)
523-603: Stored/reflective XSS risk in system stats rendering (innerHTMLtemplate literal).
At minimum, ensure user/cluster-controlled fields (e.g.,data.system.hostname, interface names/IPs) are inserted viatextContentor escaped.
110-133:sufallback auth is likely unreliable (TTY) and can lock out access.
Prefer: make PAM a hard dependency for the image, or fail closed with a clear error whenpamisn’t available (instead of attemptingsu).
🧹 Nitpick comments (3)
deploy/microshift-bootc/config-svc/update-banner.service (1)
1-19: WithDefaultDependencies=no, add explicitAfter=local-fs.target.
Ensures/etc/issueupdates aren’t racing filesystem readiness on boot.[Unit] Description=Update Jumpstarter Login Banner Documentation=https://github.com/jumpstarter-dev/jumpstarter-controller +After=local-fs.target After=network-online.target Wants=network-online.target [email protected] getty.target systemd-user-sessions.service DefaultDependencies=nodeploy/microshift-bootc/Makefile (1)
1-93: Use$(MAKE)+ expand.PHONYto avoid surprising no-ops.-.PHONY: help build bootc-build push bootc-push bootc-run bootc-stop bootc-sh bootc-rm +.PHONY: help build bootc-build push bootc-push bootc-run bootc-stop bootc-sh bootc-rm \ + bootc-reload-app build-image build-all push-all clean @@ build-image: bootc-build ## Build the bootc based qcow2 image @echo "Building image: output/qcow2/disk.qcow2" @@ sudo rm -f output/qcow2/disk.qcow2 - make output/qcow2/disk.qcow2 + $(MAKE) output/qcow2/disk.qcow2 @echo "Image built successfully in ./output/"deploy/microshift-bootc/config-svc/app.py (1)
2283-2298: Consider running behind a real WSGI server (systemd + Flask dev server).
Even for “community edition”, the dev server is a frequent source of robustness issues; considergunicorn/waitressand keep Flask as the app only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
deploy/microshift-bootc/Containerfile(1 hunks)deploy/microshift-bootc/Makefile(1 hunks)deploy/microshift-bootc/README.md(1 hunks)deploy/microshift-bootc/config-svc/app.py(1 hunks)deploy/microshift-bootc/config-svc/config-svc.service(1 hunks)deploy/microshift-bootc/config-svc/update-banner.service(1 hunks)deploy/microshift-bootc/config-svc/update-banner.sh(1 hunks)deploy/microshift-bootc/config.toml(1 hunks)deploy/microshift-bootc/kustomization.yaml(1 hunks)deploy/microshift-bootc/run-microshift.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- deploy/microshift-bootc/config.toml
- deploy/microshift-bootc/run-microshift.sh
- deploy/microshift-bootc/config-svc/update-banner.sh
- deploy/microshift-bootc/Containerfile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Applied to files:
deploy/microshift-bootc/kustomization.yamldeploy/microshift-bootc/config-svc/app.py
🪛 checkmake (0.2.2)
deploy/microshift-bootc/Makefile
[warning] 86-86: Target body for "clean" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 LanguageTool
deploy/microshift-bootc/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # MicroShift Bootc Deployment This directory contains the...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~35-~35: Ensure spelling is correct
Context: ...ended ## Quick Start ### 1. Build the Bootc Image bash make bootc-build Th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
deploy/microshift-bootc/README.md
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
184-184: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.8)
deploy/microshift-bootc/config-svc/app.py
123-123: subprocess call: check for execution of untrusted input
(S603)
124-124: Starting a process with a partial executable path
(S607)
129-129: Consider moving this statement to an else block
(TRY300)
130-130: Do not catch blind exception: Exception
(BLE001)
161-212: Possible hardcoded password assigned to: "PASSWORD_REQUIRED_TEMPLATE"
(S105)
1421-1421: Starting a process with a partial executable path
(S607)
1435-1435: Starting a process with a partial executable path
(S607)
1440-1440: Starting a process with a partial executable path
(S607)
1454-1454: Starting a process with a partial executable path
(S607)
1458-1458: Starting a process with a partial executable path
(S607)
1477-1477: Starting a process with a partial executable path
(S607)
1483-1483: Starting a process with a partial executable path
(S607)
1487-1487: Starting a process with a partial executable path
(S607)
1500-1500: Starting a process with a partial executable path
(S607)
1534-1534: Do not catch blind exception: Exception
(BLE001)
1535-1535: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1551-1551: subprocess call: check for execution of untrusted input
(S603)
1552-1552: Starting a process with a partial executable path
(S607)
1582-1582: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
1587-1587: Do not catch blind exception: Exception
(BLE001)
1587-1587: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
1604-1604: subprocess call: check for execution of untrusted input
(S603)
1605-1605: Starting a process with a partial executable path
(S607)
1665-1665: Do not catch blind exception: Exception
(BLE001)
1666-1666: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1682-1682: subprocess call: check for execution of untrusted input
(S603)
1683-1683: Starting a process with a partial executable path
(S607)
1745-1745: Do not catch blind exception: Exception
(BLE001)
1746-1746: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1762-1762: subprocess call: check for execution of untrusted input
(S603)
1763-1763: Starting a process with a partial executable path
(S607)
1777-1777: Do not catch blind exception: Exception
(BLE001)
1778-1778: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1796-1796: subprocess call: check for execution of untrusted input
(S603)
1797-1797: Starting a process with a partial executable path
(S607)
1810-1810: Do not catch blind exception: Exception
(BLE001)
1811-1811: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1818-1818: Do not catch blind exception: Exception
(BLE001)
1938-1938: Do not catch blind exception: Exception
(BLE001)
1939-1939: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1969-1969: Do not catch blind exception: Exception
(BLE001)
1979-1979: Starting a process with a partial executable path
(S607)
2002-2002: subprocess call: check for execution of untrusted input
(S603)
2003-2003: Starting a process with a partial executable path
(S607)
2017-2017: Consider moving this statement to an else block
(TRY300)
2018-2018: Do not catch blind exception: Exception
(BLE001)
2027-2027: Do not catch blind exception: Exception
(BLE001)
2052-2052: subprocess call: check for execution of untrusted input
(S603)
2053-2053: Starting a process with a partial executable path
(S607)
2073-2073: Do not catch blind exception: Exception
(BLE001)
2081-2081: subprocess call: check for execution of untrusted input
(S603)
2082-2082: Starting a process with a partial executable path
(S607)
2087-2087: Consider moving this statement to an else block
(TRY300)
2092-2092: Do not catch blind exception: Exception
(BLE001)
2102-2102: Starting a process with a partial executable path
(S607)
2108-2108: Unpacked variable stdout is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
2115-2115: Consider moving this statement to an else block
(TRY300)
2116-2116: Do not catch blind exception: Exception
(BLE001)
2155-2155: Do not catch blind exception: Exception
(BLE001)
2222-2222: subprocess call: check for execution of untrusted input
(S603)
2223-2223: Starting a process with a partial executable path
(S607)
2233-2234: try-except-pass detected, consider logging the exception
(S110)
2233-2233: Do not catch blind exception: Exception
(BLE001)
2240-2240: Do not catch blind exception: Exception
(BLE001)
2297-2297: Possible binding to all interfaces
(S104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests-release-0-7
- GitHub Check: lint-go
- GitHub Check: e2e-test-operator
- GitHub Check: deploy-kind
- GitHub Check: tests
🔇 Additional comments (2)
deploy/microshift-bootc/kustomization.yaml (1)
1-11: Kustomization looks correct and minimal.
resources+commonLabelsare sensible for consistent labeling across deployed objects.deploy/microshift-bootc/config-svc/config-svc.service (1)
1-21: Service unit is reasonable; restart + journald wiring looks good.
Given the service’s responsibilities (hostname/password changes), I wouldn’t add aggressive systemd sandboxing here without testing.
| def apply_jumpstarter_cr(base_domain, image, image_pull_policy='IfNotPresent'): | ||
| """Apply Jumpstarter Custom Resource using oc.""" | ||
| try: | ||
| # Path to MicroShift kubeconfig | ||
| kubeconfig_path = KUBECONFIG_PATH | ||
|
|
||
| # Check if kubeconfig exists | ||
| if not os.path.exists(kubeconfig_path): | ||
| return False, 'MicroShift kubeconfig not found. Is MicroShift running?' | ||
|
|
||
| # Build the CR YAML | ||
| cr = { | ||
| 'apiVersion': 'operator.jumpstarter.dev/v1alpha1', | ||
| 'kind': 'Jumpstarter', | ||
| 'metadata': { | ||
| 'name': 'jumpstarter', | ||
| 'namespace': 'default' | ||
| }, | ||
| 'spec': { | ||
| 'baseDomain': base_domain, | ||
| 'controller': { | ||
| 'grpc': { | ||
| 'endpoints': [ | ||
| { | ||
| 'address': f'grpc.{base_domain}', | ||
| 'route': { | ||
| 'enabled': True | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| 'image': image, | ||
| 'imagePullPolicy': image_pull_policy, | ||
| 'replicas': 1 | ||
| }, | ||
| 'routers': { | ||
| 'grpc': { | ||
| 'endpoints': [ | ||
| { | ||
| 'address': f'router.{base_domain}', | ||
| 'route': { | ||
| 'enabled': True | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| 'image': image, | ||
| 'imagePullPolicy': image_pull_policy, | ||
| 'replicas': 1 | ||
| }, | ||
| 'useCertManager': True | ||
| } | ||
| } | ||
|
|
||
| # Write CR to temporary file | ||
| with tempfile.NamedTemporaryFile(mode='w', suffix='.yaml', delete=False) as f: | ||
| yaml_content = json_to_yaml(cr) | ||
| f.write(yaml_content) | ||
| temp_file = f.name | ||
|
|
||
| try: | ||
| # Apply using oc with explicit kubeconfig | ||
| result = subprocess.run( | ||
| ['oc', '--kubeconfig', kubeconfig_path, 'apply', '-f', temp_file], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True | ||
| ) | ||
| return True, result.stdout.strip() | ||
| finally: | ||
| # Clean up temp file | ||
| try: | ||
| os.unlink(temp_file) | ||
| except Exception: | ||
| pass | ||
|
|
||
| except subprocess.CalledProcessError as e: | ||
| error_msg = e.stderr.strip() if e.stderr else str(e) | ||
| print(f"Error applying Jumpstarter CR: {error_msg}", file=sys.stderr) | ||
| return False, error_msg | ||
| except Exception as e: | ||
| print(f"Error applying Jumpstarter CR: {e}", file=sys.stderr) | ||
| return False, str(e) | ||
|
|
||
|
|
||
| def json_to_yaml(obj, indent=0): | ||
| """Convert a JSON object to YAML format (simple implementation).""" | ||
| lines = [] | ||
| indent_str = ' ' * indent | ||
|
|
||
| if isinstance(obj, dict): | ||
| for key, value in obj.items(): | ||
| if isinstance(value, (dict, list)): | ||
| lines.append(f"{indent_str}{key}:") | ||
| lines.append(json_to_yaml(value, indent + 1)) | ||
| else: | ||
| lines.append(f"{indent_str}{key}: {yaml_value(value)}") | ||
| elif isinstance(obj, list): | ||
| for item in obj: | ||
| if isinstance(item, (dict, list)): | ||
| lines.append(f"{indent_str}-") | ||
| lines.append(json_to_yaml(item, indent + 1)) | ||
| else: | ||
| lines.append(f"{indent_str}- {yaml_value(item)}") | ||
|
|
||
| return '\n'.join(lines) | ||
|
|
||
|
|
||
| def yaml_value(value): | ||
| """Format a value for YAML output.""" | ||
| if value is None: | ||
| return 'null' | ||
| elif isinstance(value, bool): | ||
| return 'true' if value else 'false' | ||
| elif isinstance(value, str): | ||
| # Quote strings that contain special characters | ||
| if ':' in value or '#' in value or value.startswith('-'): | ||
| return f'"{value}"' | ||
| return value | ||
| else: | ||
| return str(value) | ||
|
|
There was a problem hiding this comment.
Add json import and use JSON format instead of custom YAML conversion.
While json_to_yaml() produces technically valid YAML, using json.dumps() with JSON files is simpler and more reliable. The oc apply -f command accepts JSON manifests. However, the proposed change requires adding import json at the top of the file—ensure this import is included when applying the fix.
- with tempfile.NamedTemporaryFile(mode='w', suffix='.yaml', delete=False) as f:
- yaml_content = json_to_yaml(cr)
- f.write(yaml_content)
+ with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as f:
+ f.write(json.dumps(cr))
temp_file = f.nameCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
2222-2222: subprocess call: check for execution of untrusted input
(S603)
2223-2223: Starting a process with a partial executable path
(S607)
2233-2234: try-except-pass detected, consider logging the exception
(S110)
2233-2233: Do not catch blind exception: Exception
(BLE001)
2240-2240: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In deploy/microshift-bootc/config-svc/app.py around lines 2160 to 2281, replace
the custom JSON->YAML conversion with standard JSON output and add the missing
import: insert "import json" at the top of the file, change the temp file
creation to use suffix='.json' and replace yaml_content = json_to_yaml(cr) with
json_content = json.dumps(cr, indent=2) and write json_content to the temp file,
and optionally remove the now-unused json_to_yaml and yaml_value helper
functions (or leave them if you prefer) so oc apply -f uses a valid JSON
manifest.
There was a problem hiding this comment.
This is another option, probably better!
There was a problem hiding this comment.
@mangelajo Great! Using JSON is definitely simpler and more maintainable. Here's the updated implementation:
def apply_jumpstarter_cr(base_domain, image, image_pull_policy='IfNotPresent'):
"""Apply Jumpstarter Custom Resource using oc."""
try:
# Path to MicroShift kubeconfig
kubeconfig_path = KUBECONFIG_PATH
# Check if kubeconfig exists
if not os.path.exists(kubeconfig_path):
return False, 'MicroShift kubeconfig not found. Is MicroShift running?'
# Build the CR YAML
cr = {
'apiVersion': 'operator.jumpstarter.dev/v1alpha1',
'kind': 'Jumpstarter',
'metadata': {
'name': 'jumpstarter',
'namespace': 'default'
},
'spec': {
'baseDomain': base_domain,
'controller': {
'grpc': {
'endpoints': [
{
'address': f'grpc.{base_domain}',
'route': {
'enabled': True
}
}
]
},
'image': image,
'imagePullPolicy': image_pull_policy,
'replicas': 1
},
'routers': {
'grpc': {
'endpoints': [
{
'address': f'router.{base_domain}',
'route': {
'enabled': True
}
}
]
},
'image': image,
'imagePullPolicy': image_pull_policy,
'replicas': 1
},
'useCertManager': True
}
}
# Write CR to temporary file
with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as f:
f.write(json.dumps(cr, indent=2))
temp_file = f.name
try:
# Apply using oc with explicit kubeconfig
result = subprocess.run(
['oc', '--kubeconfig', kubeconfig_path, 'apply', '-f', temp_file],
capture_output=True,
text=True,
check=True
)
return True, result.stdout.strip()
finally:
# Clean up temp file
try:
os.unlink(temp_file)
except Exception:
pass
except subprocess.CalledProcessError as e:
error_msg = e.stderr.strip() if e.stderr else str(e)
print(f"Error applying Jumpstarter CR: {error_msg}", file=sys.stderr)
return False, error_msg
except Exception as e:
print(f"Error applying Jumpstarter CR: {e}", file=sys.stderr)
return False, str(e)Note: The import json statement is already present at the top of the file (line 10), so no additional import is needed.
Optional cleanup: If the json_to_yaml() and yaml_value() helper functions aren't used elsewhere in the codebase, you can remove them to keep the code cleaner.
mangelajo
force-pushed
the
microshift-image
branch
2 times, most recently
from
5b50788 to
275d81f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deploy/microshift-bootc/Makefile (1)
15-15: Verify wording fix for the echo message on line 15.The past review noted this line should use "use container storage" instead of "be on the container storage," and change the colon to a dash. The current code still shows the original phrasing. Please confirm if this was addressed or needs correction.
- @echo "Building bootc image: $(BOOTC_IMG): building as root to be on the container storage from root" + @echo "Building bootc image: $(BOOTC_IMG) - building as root to use container storage from root"
🧹 Nitpick comments (1)
deploy/microshift-bootc/Makefile (1)
101-101: Potential fragility in losetup cleanup command.The nested
losetup -j ... | cut -d: -f1assumes a single loop device. If multiple loop devices are attached to the same image, only the first one will be detached. Consider a more robust approach:- -sudo losetup -d $$(sudo losetup -j /var/lib/microshift-okd/lvmdisk.image | cut -d: -f1) 2>/dev/null || true + -sudo losetup -j /var/lib/microshift-okd/lvmdisk.image 2>/dev/null | cut -d: -f1 | xargs -r sudo losetup -d
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deploy/microshift-bootc/Makefile(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
deploy/microshift-bootc/Makefile
[warning] 114-114: Target body for "clean" exceeds allowed length of 5 (8).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: deploy-kind
- GitHub Check: e2e-test-operator
- GitHub Check: lint-go
- GitHub Check: tests
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-tests-release-0-7
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
🔇 Additional comments (1)
deploy/microshift-bootc/Makefile (1)
84-86: Clarify intent ofbuild-allandpush-alltargets.Based on the PR description mentioning a complete build-and-push workflow, these targets appear incomplete. Currently,
build-allonly builds the bootc image but doesn't build the QCOW2 or ISO images (which are built separately viabuild-imageandbuild-iso). Similarly,push-allonly pushes the bootc image. Should these targets orchestrate the full build pipeline?-build-all: bootc-build ## Build bootc image +build-all: bootc-build build-image build-iso ## Build all images (bootc, QCOW2, ISO)
|
|
||
|
|
||
| # HTML template for forced password change | ||
| PASSWORD_REQUIRED_TEMPLATE = """<!DOCTYPE html> |
There was a problem hiding this comment.
There are ways to do this in pure python: https://mesop-dev.github.io/mesop/, a lot more maintainable.
There was a problem hiding this comment.
Very cool, but too new perhaps, and not sure how I would control styling properly there.
I know how to do flash + css + html, and Claude 4.5 seems to know too.. In fact I should add the Co-Authored flag to credit claude ':D
I did a lot of direction on api structure, functionality and design, but code is mostly Claude's.
What I think I should do, is really structure this app in sub-modules, and have it installed by uv on the system. instead of a big monolytic py.
| #!/bin/bash | ||
| # Update login banner with Jumpstarter web UI URL | ||
|
|
||
| python3 << 'EOF' |
There was a problem hiding this comment.
Use python directly in shebang?
There was a problem hiding this comment.
horrible, yes, I need to improve this
| firewall-offline-cmd --add-port=8880/tcp | ||
|
|
||
| # Set root password | ||
| RUN echo "root:jumpstarter" | chpasswd |
There was a problem hiding this comment.
Can't this be set in config.toml?
There was a problem hiding this comment.
Then it doesn't work when ran as a container :D
There was a problem hiding this comment.
Mor detail, we have make bootc-run that starts microshift+jumpstarter in a bootc container, it's quick and nice for iterating on the development before creating an image and flashing something.
| build-iso: bootc-build ## Build the bootc based ISO image | ||
| @echo "Building ISO image: output/iso/disk.iso" | ||
| @echo "Cleaning up any existing LVM resources to avoid conflicts..." | ||
| -sudo vgs --noheadings -o vg_name,vg_uuid | grep myvg1 | while read vg uuid; do sudo vgremove -f --select vg_uuid=$$uuid 2>/dev/null || true; done |
There was a problem hiding this comment.
I don't remember doing this when building bootc images, it lvm necessary?
There was a problem hiding this comment.
this is something that needs to be cleaned up, if you ran the container locally the run-microshift.sh stolen from their repo and modified... creates a loop back device to mount the PVs.
| try: | ||
| # Try using PAM authentication first | ||
| import pam | ||
| p = pam.pam() | ||
| return p.authenticate(username, password) | ||
| except ImportError: | ||
| # Fallback: use subprocess to authenticate via su | ||
| try: |
There was a problem hiding this comment.
pam works, we should remove the fallbacks
|
|
||
|
|
||
| # HTML template for forced password change | ||
| PASSWORD_REQUIRED_TEMPLATE = """<!DOCTYPE html> |
There was a problem hiding this comment.
Very cool, but too new perhaps, and not sure how I would control styling properly there.
I know how to do flash + css + html, and Claude 4.5 seems to know too.. In fact I should add the Co-Authored flag to credit claude ':D
I did a lot of direction on api structure, functionality and design, but code is mostly Claude's.
What I think I should do, is really structure this app in sub-modules, and have it installed by uv on the system. instead of a big monolytic py.
| 5. **Production Use:** Consider additional hardening for production deployments | ||
|
|
There was a problem hiding this comment.
| 5. **Production Use:** Consider additional hardening for production deployments |
| # For bare metal | ||
| dd if=output/qcow2/disk.qcow2 of=/dev/sdX bs=4M status=progress |
There was a problem hiding this comment.
you cannot flash a qcow2 to disk!
We need a section at the start explaining how to install into a baremetal device, by creating a raw and flashing into storage, or by using an iso.
| 4. **First boot:** | ||
| - Console login will require password change from default `jumpstarter` | ||
| - Access web UI at `http://<host-ip>:8880` and set new password | ||
|
|
There was a problem hiding this comment.
| 4. **First boot:** | |
| - Console login will require password change from default `jumpstarter` | |
| - Access web UI at `http://<host-ip>:8880` and set new password |
This is explained somewhere else..
We probably need good documentation (on the website side) about how to install into host and run..
| ## Support | ||
|
|
||
| For issues and questions: | ||
| - File issues on the Jumpstarter GitHub repository | ||
| - Check container logs: `sudo podman logs jumpstarter-microshift-okd` | ||
| - Review systemd journals: `make bootc-sh` then `journalctl -xe` | ||
|
|
There was a problem hiding this comment.
| ## Support | |
| For issues and questions: | |
| - File issues on the Jumpstarter GitHub repository | |
| - Check container logs: `sudo podman logs jumpstarter-microshift-okd` | |
| - Review systemd journals: `make bootc-sh` then `journalctl -xe` |
| @@ -0,0 +1,179 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Add a header that explains this script is used to run/test the bootc container as a container instead of installing it into a host.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deploy/microshift-bootc/Containerfile(1 hunks)deploy/microshift-bootc/Makefile(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 201
File: deploy/microshift-bootc/Makefile:24-24
Timestamp: 2025-12-16T20:28:48.388Z
Learning: The bootc-image-builder tool's official documentation recommends using `-it` flags with podman run commands when building images, as seen in deploy/microshift-bootc/Makefile.
📚 Learning: 2025-12-16T20:28:48.388Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 201
File: deploy/microshift-bootc/Makefile:24-24
Timestamp: 2025-12-16T20:28:48.388Z
Learning: The bootc-image-builder tool's official documentation recommends using `-it` flags with podman run commands when building images, as seen in deploy/microshift-bootc/Makefile.
Applied to files:
deploy/microshift-bootc/Containerfile
📚 Learning: 2025-12-16T20:28:48.388Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 201
File: deploy/microshift-bootc/Makefile:24-24
Timestamp: 2025-12-16T20:28:48.388Z
Learning: In deploy/microshift-bootc/Makefile, follow the bootc-image-builder docs and use -it with podman run when building images. This ensures an interactive TTY is allocated for prompts during image build steps. Apply this consideration to similar Makefiles that invoke podman run for image builds, verifying consistency with the official documentation.
Applied to files:
deploy/microshift-bootc/Makefile
📚 Learning: 2025-10-10T13:32:54.936Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 169
File: deploy/operator/Makefile:62-67
Timestamp: 2025-10-10T13:32:54.936Z
Learning: Podman now supports the buildx command, making it compatible with Docker's buildx syntax for multi-platform builds. No special gating or conditional logic is needed when using buildx with CONTAINER_TOOL=podman in Makefiles.
Applied to files:
deploy/microshift-bootc/Makefile
🪛 checkmake (0.2.2)
deploy/microshift-bootc/Makefile
[warning] 145-145: Target body for "clean" exceeds allowed length of 5 (8).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: e2e-test-operator
- GitHub Check: lint-go
- GitHub Check: e2e-tests-release-0-7
- GitHub Check: tests
- GitHub Check: deploy-kind
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
🔇 Additional comments (6)
deploy/microshift-bootc/Containerfile (3)
9-11: LGTM!The manifest installation correctly uses paths relative to the build context (repository root), which aligns with the
podman buildcommand in the Makefile that sets the context to../...
13-17: LGTM!Using
firewall-offline-cmdis the correct approach for configuring firewall rules in a container image build where firewalld isn't running. The comment clearly explains the rationale.
23-24: > Likely an incorrect or invalid review comment.deploy/microshift-bootc/Makefile (3)
57-74: LGTM!The ISO build target correctly mirrors the QCOW2 build pattern with appropriate volume mounts and the
-itflags as recommended by bootc-image-builder documentation.Based on learnings, the
-itflags are intentional per bootc-image-builder documentation.
76-83: LGTM!The LVM cleanup before building is a good defensive practice to avoid conflicts from previous builds. The use of
rm -fcorrectly handles the case when the file doesn't exist.
139-143: Nice dev utility!The
bootc-reload-apptarget provides a quick development workflow by hot-reloading the config service without rebuilding the entire image. This is a good developer experience improvement.
| RUN dnf install -y epel-release && \ | ||
| dnf install -y python3 iproute python3-flask python3-pip && \ | ||
| pip3 install python-pam && \ | ||
| dnf clean all |
There was a problem hiding this comment.
Use dnf for python-pam instead of pip3.
The package python3-pam is available in EPEL and can be installed via dnf. Using system packages avoids version inconsistencies and missing dependencies. Apply this change:
RUN dnf install -y epel-release && \
- dnf install -y python3 iproute python3-flask python3-pip && \
- pip3 install python-pam && \
+ dnf install -y python3 iproute python3-flask python3-pam && \
dnf clean all📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN dnf install -y epel-release && \ | |
| dnf install -y python3 iproute python3-flask python3-pip && \ | |
| pip3 install python-pam && \ | |
| dnf clean all | |
| RUN dnf install -y epel-release && \ | |
| dnf install -y python3 iproute python3-flask python3-pam && \ | |
| dnf clean all |
🤖 Prompt for AI Agents
In deploy/microshift-bootc/Containerfile around lines 3 to 6, the Dockerfile
installs python-pam via pip3 which should instead use the EPEL/system package;
replace the pip3 install python-pam step with the dnf package name (python3-pam)
so dnf installs it along with other system deps, ensuring the package list and
cleanup commands remain in the same RUN chain.
| bootc-build-multi: ## Build the bootc image for multiple architectures (amd64, arm64) | ||
| @echo "Building multiarch bootc image: $(BOOTC_IMG)" | ||
| @echo "This will build for linux/amd64 and linux/arm64" | ||
| @# Remove existing manifest if it exists | ||
| -podman manifest rm $(BOOTC_IMG) 2>/dev/null || true | ||
| @# Create a new manifest | ||
| podman manifest create $(BOOTC_IMG) | ||
| @# Build for amd64 | ||
| @echo "Building for linux/amd64..." | ||
| podman build --platform linux/amd64 -t $(BOOTC_IMG)-amd64 -f Containerfile ../.. | ||
| @# Build for arm64 | ||
| @echo "Building for linux/arm64..." | ||
| podman build --platform linux/arm64 -t $(BOOTC_IMG)-arm64 -f Containerfile ../.. | ||
| @# Add both images to the manifest | ||
| podman manifest add $(BOOTC_IMG) $(BOOTC_IMG)-amd64 | ||
| podman manifest add $(BOOTC_IMG) $(BOOTC_IMG)-arm64 | ||
| @echo "Multiarch manifest created successfully!" | ||
| @echo "To inspect: podman manifest inspect $(BOOTC_IMG)" | ||
| @echo "To push: make bootc-push-multi" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent sudo usage in multi-arch build.
The bootc-build-multi target uses podman without sudo (lines 22, 24, 27, 30, 32-33), while bootc-build uses sudo podman (line 16). This places images in different container storage locations (user vs. root), which can cause:
- The manifest won't include the base image if it was built with
bootc-buildfirst - The
bootc-push-multitarget (line 105) won't find the images in root's storage
Apply this diff to ensure consistency:
bootc-build-multi: ## Build the bootc image for multiple architectures (amd64, arm64)
@echo "Building multiarch bootc image: $(BOOTC_IMG)"
@echo "This will build for linux/amd64 and linux/arm64"
@# Remove existing manifest if it exists
- -podman manifest rm $(BOOTC_IMG) 2>/dev/null || true
+ -sudo podman manifest rm $(BOOTC_IMG) 2>/dev/null || true
@# Create a new manifest
- podman manifest create $(BOOTC_IMG)
+ sudo podman manifest create $(BOOTC_IMG)
@# Build for amd64
@echo "Building for linux/amd64..."
- podman build --platform linux/amd64 -t $(BOOTC_IMG)-amd64 -f Containerfile ../..
+ sudo podman build --platform linux/amd64 -t $(BOOTC_IMG)-amd64 -f Containerfile ../..
@# Build for arm64
@echo "Building for linux/arm64..."
- podman build --platform linux/arm64 -t $(BOOTC_IMG)-arm64 -f Containerfile ../..
+ sudo podman build --platform linux/arm64 -t $(BOOTC_IMG)-arm64 -f Containerfile ../..
@# Add both images to the manifest
- podman manifest add $(BOOTC_IMG) $(BOOTC_IMG)-amd64
- podman manifest add $(BOOTC_IMG) $(BOOTC_IMG)-arm64
+ sudo podman manifest add $(BOOTC_IMG) $(BOOTC_IMG)-amd64
+ sudo podman manifest add $(BOOTC_IMG) $(BOOTC_IMG)-arm64📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bootc-build-multi: ## Build the bootc image for multiple architectures (amd64, arm64) | |
| @echo "Building multiarch bootc image: $(BOOTC_IMG)" | |
| @echo "This will build for linux/amd64 and linux/arm64" | |
| @# Remove existing manifest if it exists | |
| -podman manifest rm $(BOOTC_IMG) 2>/dev/null || true | |
| @# Create a new manifest | |
| podman manifest create $(BOOTC_IMG) | |
| @# Build for amd64 | |
| @echo "Building for linux/amd64..." | |
| podman build --platform linux/amd64 -t $(BOOTC_IMG)-amd64 -f Containerfile ../.. | |
| @# Build for arm64 | |
| @echo "Building for linux/arm64..." | |
| podman build --platform linux/arm64 -t $(BOOTC_IMG)-arm64 -f Containerfile ../.. | |
| @# Add both images to the manifest | |
| podman manifest add $(BOOTC_IMG) $(BOOTC_IMG)-amd64 | |
| podman manifest add $(BOOTC_IMG) $(BOOTC_IMG)-arm64 | |
| @echo "Multiarch manifest created successfully!" | |
| @echo "To inspect: podman manifest inspect $(BOOTC_IMG)" | |
| @echo "To push: make bootc-push-multi" | |
| bootc-build-multi: ## Build the bootc image for multiple architectures (amd64, arm64) | |
| @echo "Building multiarch bootc image: $(BOOTC_IMG)" | |
| @echo "This will build for linux/amd64 and linux/arm64" | |
| @# Remove existing manifest if it exists | |
| -sudo podman manifest rm $(BOOTC_IMG) 2>/dev/null || true | |
| @# Create a new manifest | |
| sudo podman manifest create $(BOOTC_IMG) | |
| @# Build for amd64 | |
| @echo "Building for linux/amd64..." | |
| sudo podman build --platform linux/amd64 -t $(BOOTC_IMG)-amd64 -f Containerfile ../.. | |
| @# Build for arm64 | |
| @echo "Building for linux/arm64..." | |
| sudo podman build --platform linux/arm64 -t $(BOOTC_IMG)-arm64 -f Containerfile ../.. | |
| @# Add both images to the manifest | |
| sudo podman manifest add $(BOOTC_IMG) $(BOOTC_IMG)-amd64 | |
| sudo podman manifest add $(BOOTC_IMG) $(BOOTC_IMG)-arm64 | |
| @echo "Multiarch manifest created successfully!" | |
| @echo "To inspect: podman manifest inspect $(BOOTC_IMG)" | |
| @echo "To push: make bootc-push-multi" |
🤖 Prompt for AI Agents
In deploy/microshift-bootc/Makefile around lines 18 to 36, the bootc-build-multi
target mixes non-root podman invocations while bootc-build uses sudo, causing
images to be placed in different container storages; update every podman
invocation in this target (manifest rm/create, build, and manifest add) to use
sudo (e.g., change podman ... to sudo podman ...) so all builds/manifests run as
root and reside in the same storage as bootc-build.
| @echo "To push: make bootc-push-multi" | ||
|
|
||
| output/qcow2/disk.qcow2: ## Build a bootable QCOW2 image from the bootc image | ||
| @echo "Building QCOW2 image from: $(BOOTC_IMG)"a |
There was a problem hiding this comment.
Fix typo: remove trailing 'a'.
Line 39 has a stray character at the end.
Apply this diff:
- @echo "Building QCOW2 image from: $(BOOTC_IMG)"a
+ @echo "Building QCOW2 image from: $(BOOTC_IMG)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @echo "Building QCOW2 image from: $(BOOTC_IMG)"a | |
| @echo "Building QCOW2 image from: $(BOOTC_IMG)" |
🤖 Prompt for AI Agents
In deploy/microshift-bootc/Makefile around line 39, there's a stray trailing 'a'
at the end of the echo command; remove the extraneous character so the line
reads the intended echo statement without the trailing 'a'.
| bootc-push-multi: ## Push the multiarch manifest to registry | ||
| @echo "Pushing multiarch manifest: $(BOOTC_IMG)" | ||
| @echo "This will push the manifest list with amd64 and arm64 images" | ||
| podman manifest push $(BOOTC_IMG) $(BOOTC_IMG) | ||
| @echo "Multiarch manifest pushed successfully!" | ||
| @echo "Images available for linux/amd64 and linux/arm64" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
bootc-push-multi needs sudo for consistency.
Since bootc-build-multi should use sudo (per previous comment) to match where bootc-build stores images, this push target must also use sudo.
Apply this diff:
bootc-push-multi: ## Push the multiarch manifest to registry
@echo "Pushing multiarch manifest: $(BOOTC_IMG)"
@echo "This will push the manifest list with amd64 and arm64 images"
- podman manifest push $(BOOTC_IMG) $(BOOTC_IMG)
+ sudo podman manifest push $(BOOTC_IMG) $(BOOTC_IMG)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bootc-push-multi: ## Push the multiarch manifest to registry | |
| @echo "Pushing multiarch manifest: $(BOOTC_IMG)" | |
| @echo "This will push the manifest list with amd64 and arm64 images" | |
| podman manifest push $(BOOTC_IMG) $(BOOTC_IMG) | |
| @echo "Multiarch manifest pushed successfully!" | |
| @echo "Images available for linux/amd64 and linux/arm64" | |
| bootc-push-multi: ## Push the multiarch manifest to registry | |
| @echo "Pushing multiarch manifest: $(BOOTC_IMG)" | |
| @echo "This will push the manifest list with amd64 and arm64 images" | |
| sudo podman manifest push $(BOOTC_IMG) $(BOOTC_IMG) | |
| @echo "Multiarch manifest pushed successfully!" | |
| @echo "Images available for linux/amd64 and linux/arm64" |
🤖 Prompt for AI Agents
In deploy/microshift-bootc/Makefile around lines 102 to 107, the podman manifest
push is missing sudo which makes its behavior inconsistent with
bootc-build-multi; update the target to run podman under sudo (e.g., prefix the
podman manifest push command with sudo and keep the leading @) so the push
operates with the same root-owned image store as the build step.
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.