[linstor-gui] Add package for LINBIT linstor-gui web UI - #2382

packages/system/linstor-gui/Makefile (1)

9-32: Add .PHONY declarations for targets.

The image, image-linstor-gui, and test targets are not declared as phony. While this works, declaring them prevents issues if files with these names are ever created.

♻️ Proposed fix to add PHONY declarations

 include ../../../hack/package.mk

 LINSTOR_GUI_VERSION ?= 2.3.0

+.PHONY: image image-linstor-gui test
+
 image: image-linstor-gui

 image-linstor-gui:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/linstor-gui/Makefile` around lines 9 - 32, The Makefile is
missing .PHONY declarations for the targets which can cause unexpected behavior
if files named "image", "image-linstor-gui", or "test" appear; add a .PHONY line
listing image, image-linstor-gui, and test (e.g. .PHONY: image image-linstor-gui
test) near the top of the Makefile before the rules so the Makefile treats these
as phony targets rather than files.

packages/system/linstor-gui/images/linstor-gui/Dockerfile (1)

9-13: Consider adding checksum verification for supply chain security.

The tarball is downloaded from an external source without integrity verification. Adding a checksum validation step would strengthen the supply chain security of the build.

🛡️ Proposed approach for checksum verification

 ARG LINSTOR_GUI_VERSION
+ARG LINSTOR_GUI_SHA256
 USER root
 RUN apk add --no-cache curl tar && \
     curl -fsSL "https://pkg.linbit.com/downloads/linstor/linstor-gui-${LINSTOR_GUI_VERSION}.tar.gz" -o /tmp/linstor-gui.tar.gz && \
+    echo "${LINSTOR_GUI_SHA256}  /tmp/linstor-gui.tar.gz" | sha256sum -c - && \
     mkdir -p /usr/share/nginx/html && \

The checksum should be maintained alongside LINSTOR_GUI_VERSION in the Makefile and passed as a build arg.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/linstor-gui/images/linstor-gui/Dockerfile` around lines 9 -
13, Add SHA256 checksum verification for the downloaded tarball: introduce a
build-arg (e.g., LINSTOR_GUI_SHA256) alongside LINSTOR_GUI_VERSION (maintained
in the Makefile), pass it into the Docker build, and in the Dockerfile's RUN
sequence (the lines that curl the tarball and extract it) compute the tarball's
sha256 and compare it to LINSTOR_GUI_SHA256 (fail/exit non-zero on mismatch)
before extracting; ensure the comparison uses a reliable tool (sha256sum or
sha256sum -c via an echo "checksum  filename" | sha256sum -c -) and remove the
tarball on failure to avoid continuing with an unverified artifact.

packages/system/linstor-gui/templates/deployment.yaml (1)

28-28: Consider lowering priorityClassName for a non-critical UI component.

system-cluster-critical is typically reserved for core cluster components whose failure would impact cluster operation. The LINSTOR GUI is an optional management interface - operators can still use the CLI if the GUI is unavailable. Consider using a lower priority or removing this setting to allow proper pod eviction during resource pressure.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/linstor-gui/templates/deployment.yaml` at line 28, The
deployment template sets priorityClassName to the heavyweight
"system-cluster-critical" for the LINSTOR GUI; remove that setting or replace
"priorityClassName: system-cluster-critical" with a lower/non-critical class (or
omit the priorityClassName key entirely) in the Deployment template so the GUI
pods can be evicted under pressure; update the templates/deployment.yaml
manifest where the priorityClassName key appears in the LINSTOR GUI Deployment
spec.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/system/linstor-gui/Makefile (1)

9-32: Consider adding .PHONY declarations for non-file targets.

The image, image-linstor-gui, and test targets don't produce files with those names, so declaring them as .PHONY prevents issues if files with those names are ever created and ensures they always run.

🔧 Proposed fix

+.PHONY: image image-linstor-gui test
+
 image: image-linstor-gui

 image-linstor-gui:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/linstor-gui/Makefile` around lines 9 - 32, Add a .PHONY
declaration for the non-file Makefile targets to ensure they always run: declare
.PHONY for the targets image, image-linstor-gui, and test in the Makefile (e.g.,
add a line like ".PHONY: image image-linstor-gui test" near the top or before
the target definitions) so these names are treated as phony targets rather than
real files.

packages/system/linstor-gui/templates/configmap-nginx.yaml (1)

50-61: Consider extracting repeated mTLS proxy directives into a shared snippet.

The /v1 and /metrics locations duplicate the same 8 proxy_ssl_* directives. While functional, this increases maintenance burden if TLS settings need adjustment.

Also applies to: 63-74

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/linstor-gui/templates/configmap-nginx.yaml` around lines 50 -
61, The nginx config duplicates the same mTLS proxy directives in the location
blocks (e.g., location /v1 and location /metrics) — extract the repeated
proxy_ssl_* lines (proxy_ssl_certificate, proxy_ssl_certificate_key,
proxy_ssl_trusted_certificate, proxy_ssl_verify, proxy_ssl_verify_depth,
proxy_ssl_server_name, proxy_ssl_name, etc.) into a reusable template/snippet
(e.g., define a Helm template or nginx include named something like
"linstor-mtls-proxy") and replace the repeated blocks with a single call to that
snippet, preserving interpolation of {{ $endpointHost }} and any existing
proxy_pass/headers in the location blocks so TLS settings are managed in one
place.

packages/system/linstor-gui/templates/deployment.yaml (1)

7-8: Redundant config reload mechanisms.

Both reloader.stakater.com/auto: "true" (line 8) and checksum/nginx-config (line 22) trigger pod restarts on ConfigMap changes. The reloader annotation automatically watches mounted ConfigMaps, making the manual checksum annotation unnecessary when Stakater Reloader is deployed.

Consider keeping only one approach for clarity. The Reloader annotation is simpler but requires the operator to be installed; the checksum is self-contained.

Also applies to: 21-22

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/linstor-gui/templates/deployment.yaml` around lines 7 - 8,
The deployment currently uses two restart mechanisms: the annotation
reloader.stakater.com/auto: "true" on the Deployment and the manual
checksum/nginx-config Pod annotation; remove the redundant approach by choosing
one strategy—prefer keeping reloader.stakater.com/auto and delete the
checksum/nginx-config annotation (or vice versa if you require self-contained
behavior) so only a single ConfigMap-watching restart mechanism remains; update
the templates/deployment.yaml to remove the unused checksum/nginx-config entry
(or remove the reloader annotation) and ensure any references to checksum-based
rolling restarts (e.g., label or annotation generation code that sets
checksum/nginx-config) are also cleaned up.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/system/linstor-gui/templates/configmap-nginx.yaml (1)

72-76: ⚠️ Potential issue | 🟡 Minor

Content-Type header is not applied in /healthz due to return ordering.

On Line 74, return finalizes the response; Line 75 add_header is ineffective in this block.

🔧 Proposed fix

         location = /healthz {
           access_log off;
+          default_type text/plain;
           return 200 'ok';
-          add_header Content-Type text/plain;
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/linstor-gui/templates/configmap-nginx.yaml` around lines 72 -
76, The Content-Type header in the location = /healthz block is not applied
because the return directive finalizes the response before add_header runs; move
the add_header directive (use add_header Content-Type text/plain always;) above
the return in the location = /healthz block (refer to the location = /healthz
block to find and update the lines), keeping access_log off and the same return
body.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/system/linstor-gui/templates/deployment.yaml (1)

1-102: 🛠️ Refactor suggestion | 🟠 Major

Use umbrella + vendored-chart structure for this system package.

This package still owns the primary workload manifest directly in templates/, which conflicts with the repository rule for packages/system/**.

As per coding guidelines: packages/system/**: "Use Helm Chart umbrella pattern with vendored upstream charts in charts/ directory".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/system/linstor-gui/templates/deployment.yaml` around lines 1 - 102,
The deployment manifest (Deployment named linstor-gui, container name
linstor-gui, etc.) lives directly under templates/ which violates the
packages/system umbrella+vendored-chart rule; fix by converting this package
into an umbrella chart that vendors the workload as a subchart: create a
charts/linstor-gui subchart (Chart.yaml, values.yaml, templates/) and move the
existing deployment.yaml contents into that subchart's templates, update the
umbrella Chart.yaml to declare the linstor-gui chart as a dependency and mirror
any values used (.Values.image, .Values.replicas, .Values.linstor.clientSecret)
into the umbrella values.yaml, then remove the original
templates/deployment.yaml so the vendored subchart owns the Deployment manifest.