Skip to content

fix: classification banner working for SPAs#2710

Open
joelmccoy wants to merge 2 commits into
mainfrom
fix-classification-banner
Open

fix: classification banner working for SPAs#2710
joelmccoy wants to merge 2 commits into
mainfrom
fix-classification-banner

Conversation

@joelmccoy
Copy link
Copy Markdown
Contributor

@joelmccoy joelmccoy commented May 29, 2026

Description

The classification banner was noticed to block traffic to Grafana in uds-identity-config. See: defenseunicorns/uds-identity-config#870. In our envoy filter response_handle:body() buffers the entire body and blocks until end-of-stream and on Grafana 13's streamed shell (no Content-Length) this hung.

This PR:

  • Fixes the classification-banner EnvoyFilter so it injects without hanging: buffer responses with a Content-Length, stream-inject (bodyChunks) those without one, and skip non-200s (redirects).
  • Pins the banner font (Arial, Helvetica, sans-serif, 16px) so it's identical on every app instead of inheriting each app's font.
  • Reserves viewport space so the banner doesn't overlap apps whose nav is position: fixed (SPAs).
  • Enables the banner on sso/portal/grafana in the k3d-standard bundle and adds a Playwright test to confirm working classification banner.

NOTE: People will see the UNKNOWN classification banner for demo bundle deploys. If we feel this is weird I would be open to changing this. I also set this to UNKNOWN instead of UNCLASSIFIED as I didn't want to assume classification for where people are deploying the demo bundle locally.

image

Related Issue

Fixes CORE-590

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Steps to Validate

  • uds run and check the banners on portal, sso, and grafana

Checklist before merging

Copilot AI review requested due to automatic review settings May 29, 2026 20:56
@joelmccoy joelmccoy requested a review from a team as a code owner May 29, 2026 20:56
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Istio classification-banner EnvoyFilter to avoid hanging on streamed HTML responses (notably Grafana 13’s streamed shell), standardizes banner font styling, reserves viewport space to prevent overlap in SPAs, and enables/validates the banner in the k3d-standard demo bundle via a new Playwright test.

Changes:

  • Update the classification-banner Lua filter to inject safely for both bounded (Content-Length) and streamed (chunked/no Content-Length) HTML responses, and skip non-200 responses.
  • Adjust injected banner styling and add client-side logic to reserve space so fixed-position SPA chrome doesn’t overlap the banner.
  • Enable the banner on demo bundle hosts (sso/portal/grafana) and add a Playwright test to verify banner injection.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
test/playwright/classification-banner.test.ts Adds an end-to-end Playwright check that the banner is injected on sso/grafana (and portal when present).
src/istio/common/chart/templates/classification-banner.yaml Updates Envoy Lua injection logic to avoid blocking on streamed responses and improves banner layout/styling behavior.
bundles/k3d-standard/uds-bundle.yaml Enables the banner on selected demo hosts and sets demo text to UNKNOWN.

Comment thread src/istio/common/chart/templates/classification-banner.yaml
Comment thread src/istio/common/chart/templates/classification-banner.yaml Outdated
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 29, 2026

Greptile Summary

This PR fixes the classification-banner EnvoyFilter which previously hung the gateway on Grafana 13 because response_handle:body() blocks until end-of-stream and Grafana's HTML response has no Content-Length. The fix gates buffering on the presence of a Content-Length header and uses the non-blocking bodyChunks() iterator otherwise, while also skipping non-200 responses (e.g., 3xx redirects with no body).

  • EnvoyFilter Lua: Extracts an inject_banner helper, picks buffered vs. streaming injection based on content-length header, restricts injection to HTTP 200 only, pins banner font/size, and replaces the paddingTop/paddingBottom script with a borderTop/borderBottom + translateZ(0) transform technique that keeps SPA fixed-position navbars from overlapping the banner.
  • k3d-standard bundle: Enables the banner with UNKNOWN classification on sso, portal, and grafana for dev bundle deploys.
  • Tests: Adds Playwright tests for all three enabled hosts; the grafana test implicitly exercises the 302 redirect non-hang path.

Confidence Score: 4/5

The change is safe to merge — it fixes a real gateway hang and the new code paths degrade gracefully when tag boundaries straddle chunk edges.

The buffered and streaming injection logic is correct: inject_banner uses gsub-with-limit-1 so it cannot double-inject, the injected flag correctly tracks completion, and the early-return on non-200 status closes the original redirect hang. The only open items are cosmetic: a redundant response_handle:body() call and a tostring() wrapper that the Envoy Lua runtime makes a no-op.

No files require special attention; the Lua logic in classification-banner.yaml is the most complex part and reads correctly after careful review.

Important Files Changed

Filename Overview
src/istio/common/chart/templates/classification-banner.yaml Core EnvoyFilter Lua refactor: adds content-length-gated buffered vs. streaming injection, skips non-200 responses, extracts inject_banner helper, pins font styles, and updates the JS padding script to use border/transform trick for SPA compat.
bundles/k3d-standard/uds-bundle.yaml Adds classificationBanner Helm overrides (text: UNKNOWN, enabledHosts: sso/portal/grafana) under the istio-controlplane package for the k3d-standard dev bundle.
test/playwright/classification-banner.test.ts New Playwright tests asserting the classification banner div is visible with correct text on sso, grafana, and portal. Grafana test implicitly validates the 302-redirect hang fix.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[envoy_on_response] --> B{status == 200\nAND text/html\nAND enabled host?}
    B -- No --> Z[return — pass through untouched]
    B -- Yes --> C{content-length\nheader present?}

    C -- Yes --> D[response_handle:body\nbuffer full response]
    D --> E[inject_banner on full body]
    E --> F[body:setBytes patched body]

    C -- No --> G[response_handle:bodyChunks\nstreaming iterator]
    G --> H{chunk non-empty\nAND not yet injected?}
    H -- No --> I[pass chunk through]
    I --> H
    H -- Yes --> J[inject_banner on chunk]
    J --> K[chunk:setBytes patched chunk]
    K --> L{classification-banner-top\nfound in patched data?}
    L -- Yes --> M[injected = true\npass remaining chunks through]
    L -- No --> H

    subgraph inject_banner
        N[gsub body tag → insert header div]
        N --> O[gsub head tag → insert padding script]
    end
Loading

Reviews (1): Last reviewed commit: "fix: classification banner working for S..." | Re-trigger Greptile

Comment thread src/istio/common/chart/templates/classification-banner.yaml Outdated
Comment thread src/istio/common/chart/templates/classification-banner.yaml Outdated
Comment thread src/istio/common/chart/templates/classification-banner.yaml
Copy link
Copy Markdown
Contributor

@chance-coleman chance-coleman left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants