Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds FIPS build and test paths across CI and runtime: dynamic build matrix including FIPS variants, optional DockerHub FIPS login, new FIPS compose file, FIPS Dockerfile stages and runtime preload modules, E2E workflow changes to pull/start -fips images, and new FIPS E2E jobs and orchestration. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions (CI)
participant Matrix as Matrix Generator
participant Builder as build-docker (buildx bake)
participant DockerHub as DockerHub (optional FIPS login)
participant Registry as GHCR / Registry
participant E2E as ci-test-e2e workflow
participant Compose as Docker Compose (COMPOSE_FILES)
participant Services as App Services
CI->>Matrix: generate matrix (arches × services × types incl. fips)
Matrix-->>CI: matrix output
CI->>Builder: start build with matrix
alt TEMP_DOCKERHUB_FIPS_* provided for fips
Builder->>DockerHub: login with TEMP_DOCKERHUB_FIPS_*
end
Builder->>Builder: assemble compose_files, run buildx bake (retry loop)
Builder->>Registry: push images (include -fips suffix for fips builds)
CI->>E2E: trigger e2e workflows with release=fips
E2E->>Registry: pull images (including -fips)
E2E->>Compose: up using COMPOSE_FILES
Compose->>Services: start services (FIPS stages load ./src/fips.js)
Services-->>E2E: readiness/health signals
E2E->>CI: report test results (FIPS jobs included)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39324 +/- ##
===========================================
- Coverage 70.55% 70.53% -0.03%
===========================================
Files 3270 3270
Lines 116769 116769
Branches 21065 21034 -31
===========================================
- Hits 82391 82359 -32
- Misses 32319 32352 +33
+ Partials 2059 2058 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
cardoso
force-pushed
the
fips-ddp-streamer
branch
from
c43868a to
6461cbe
Compare
|
/jira FIPS-11 |
cardoso
force-pushed
the
fips-ddp-streamer
branch
2 times, most recently
from
994e9db to
4e907e2
Compare
coderabbitai
bot
added
the
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:651">
P2: The new `release: fips` E2E jobs won’t download the FIPS ddp-streamer image in fork PRs because the artifact download pattern only matches `*-coverage`. Update the E2E workflow to include `*-fips` artifacts when `inputs.release == 'fips'` so the FIPS jobs can load the image locally.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/build-docker/action.yml:
- Around line 51-57: The GitHub Actions step named "Login to DockerHub for FIPS
base images" currently runs whenever inputs.type == 'fips' and fails if
TEMP_DOCKERHUB_FIPS_USER or TEMP_DOCKERHUB_FIPS_PASS are empty; update the
step's if condition (currently if: inputs.type == 'fips') to also verify both
inputs are non-empty (e.g., check inputs.TEMP_DOCKERHUB_FIPS_USER != '' &&
inputs.TEMP_DOCKERHUB_FIPS_PASS != '') so the docker/login-action@v3 step only
runs when FIPS credentials are provided.
In @.github/workflows/ci-test-e2e.yml:
- Line 76: The artifact download pattern is not accounting for the FIPS suffix
set by DOCKER_TAG_SUFFIX_DDP_STREAMER, so non-published FIPS runs can miss the
FIPS images; update the artifact selection logic used in the download step to
include the FIPS suffix when inputs.release == 'fips' (use the same
DOCKER_TAG_SUFFIX_DDP_STREAMER value or inputs.release check) so the pattern
matches artifacts that end with the '-fips' variant as well as the regular ones
(e.g., incorporate the suffix into the wildcard used for selecting
coverage/non-CE artifacts).
In @.github/workflows/ci.yml:
- Around line 447-448: The publish step still resolves the source image by using
${service} as a compose key (in the docker-image-publish step), which fails for
the special-case service ddp-streamer-service-fips; update the
docker-image-publish logic to mirror the earlier mapping: when service ==
ddp-streamer-service-fips, resolve the compose key as ddp-streamer-service and
append -fips (or simply reuse the previously computed IMAGE env var), ensure
IMAGE is exported/available to the docker-image-publish step, and keep
docker-compose-ci.yml as the source for non-fips services.
In `@ee/apps/ddp-streamer/src/fips.ts`:
- Around line 3-13: Current file only logs FIPS-related state but must fail fast
when prerequisites aren't met: add an early validation after computing
OPENSSL_CONFIG_PATH, hasOpenSSLConfigFlag, hasOpenSSLSharedConfigFlag and
crypto.getFips()/crypto.getCiphers() that enforces required conditions (e.g.,
crypto.getFips() === 1 and at least expected cipher count and presence of
--openssl-config or --openssl-shared-config as your policy requires); if any
prerequisite is missing, emit a clear processLogger.error/console.error
referencing OPENSSL_CONFIG_PATH and the flag variables and terminate immediately
with process.exit(1) (or throw an Error) so startup does not continue. Ensure
this check runs at module initialization before any other startup logic that
depends on FIPS being enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12a00ee3-034c-4ee9-819a-7c7917888bc6
📒 Files selected for processing (7)
.github/actions/build-docker/action.yml.github/workflows/ci-test-e2e.yml.github/workflows/ci.ymldocker-compose-ci.ymlee/apps/ddp-streamer/Dockerfileee/apps/ddp-streamer/openssl-ddp-streamer-fips.cnfee/apps/ddp-streamer/src/fips.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/apps/ddp-streamer/src/fips.ts
🧠 Learnings (2)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
ee/apps/ddp-streamer/src/fips.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
ee/apps/ddp-streamer/src/fips.ts
🔇 Additional comments (9)
ee/apps/ddp-streamer/openssl-ddp-streamer-fips.cnf (1)
1-14: OpenSSL provider configuration looks consistent for the intended FIPS stage wiring.The section layout and provider activation match the Docker FIPS entrypoint usage.
ee/apps/ddp-streamer/Dockerfile (2)
94-94:release-standardstage aliasing is clear and improves target selection.
121-135: FIPS release stage wiring is clean and explicit.The stage base image, config copy, and Node startup flags are aligned for the dedicated FIPS path.
.github/actions/build-docker/action.yml (1)
8-13: FIPS inputs/type docs and suffix routing are consistent with the new lane.Also applies to: 36-36, 137-137
docker-compose-ci.yml (1)
111-111: Build target and image suffix parameterization are correctly aligned with FIPS/non-FIPS flows.Also applies to: 118-118
.github/workflows/ci-test-e2e.yml (1)
191-191: EE/FIPS conditional expansion for container startup, readiness, and IS_EE flags is coherent.Also applies to: 202-202, 220-220, 236-236, 251-251
.github/workflows/ci.yml (3)
295-299: FIPS lane matrix and build env wiring are consistent across all image build invocations.Also applies to: 321-327, 339-345, 358-364, 377-383
644-707: Dedicated FIPS E2E jobs are wired cleanly and mirror existing CE/EE job structure.
858-858:tests-doneaggregation correctly includes the new FIPS job gates.Also applies to: 895-905
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
483-484:⚠️ Potential issue | 🔴 CriticalDockerHub publish path still misses the FIPS service mapping.
You mapped
ddp-streamer-service-fipsfor GHCR manifest creation here, but the DockerHub publish step still resolves source image with${service}directly (Line 1100). Forddp-streamer-service-fips, that compose key is absent, so publish can fail.🔧 Proposed follow-up patch (`docker-image-publish` step)
- SRC=$(docker compose -f docker-compose-ci.yml config --format json 2>/dev/null | jq -r --arg s "${service}" '.services[$s].image') + if [[ "${service}" == 'ddp-streamer-service-fips' ]]; then + SRC=$(docker compose -f docker-compose-ci.yml config --format json 2>/dev/null | jq -r --arg s "ddp-streamer-service" '.services[$s].image')-fips + else + SRC=$(docker compose -f docker-compose-ci.yml config --format json 2>/dev/null | jq -r --arg s "${service}" '.services[$s].image') + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 483 - 484, The DockerHub publish step fails for the ddp-streamer-service-fips mapping because IMAGE is resolved from the compose key using ${service} but docker-compose has no ddp-streamer-service-fips key; update the docker-image-publish logic to translate that service name to the correct source image (same mapping used earlier where ddp-streamer-service-fips sets IMAGE from ddp-streamer-service and appends -fips). Concretely, in the docker-image-publish step ensure the branch that computes IMAGE handles "ddp-streamer-service-fips" by looking up ".services[\"ddp-streamer-service\"].image" and appending "-fips" (the same behavior as the earlier block that sets IMAGE), so the publish step uses the correct source image string.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
317-318: FIPS env branches inbuild-gh-dockerare currently unreachable.
matrix.typein this job is production/coverage, so thesematrix.type == 'fips'expressions never evaluate true. Consider removing them from this job (or addingfipsto matrix if intended) to reduce confusion.Also applies to: 335-336, 354-355, 373-374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 317 - 318, The FIPS conditional branches are unreachable because matrix.type for the build-gh-docker job only contains production/coverage; update the job to either remove the 'matrix.type == "fips"' checks or add "fips" to the job's matrix so the expressions can ever be true. Specifically, edit the environment entries DOCKER_TAG_SUFFIX_DDP_STREAMER and BUILD_TARGET (and the analogous entries at the other occurrences) to either drop the "&& matrix.type == 'fips'" clauses or expand the matrix.type values to include 'fips' so the conditional expressions evaluate as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 483-484: The DockerHub publish step fails for the
ddp-streamer-service-fips mapping because IMAGE is resolved from the compose key
using ${service} but docker-compose has no ddp-streamer-service-fips key; update
the docker-image-publish logic to translate that service name to the correct
source image (same mapping used earlier where ddp-streamer-service-fips sets
IMAGE from ddp-streamer-service and appends -fips). Concretely, in the
docker-image-publish step ensure the branch that computes IMAGE handles
"ddp-streamer-service-fips" by looking up
".services[\"ddp-streamer-service\"].image" and appending "-fips" (the same
behavior as the earlier block that sets IMAGE), so the publish step uses the
correct source image string.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 317-318: The FIPS conditional branches are unreachable because
matrix.type for the build-gh-docker job only contains production/coverage;
update the job to either remove the 'matrix.type == "fips"' checks or add "fips"
to the job's matrix so the expressions can ever be true. Specifically, edit the
environment entries DOCKER_TAG_SUFFIX_DDP_STREAMER and BUILD_TARGET (and the
analogous entries at the other occurrences) to either drop the "&& matrix.type
== 'fips'" clauses or expand the matrix.type values to include 'fips' so the
conditional expressions evaluate as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a51daa8-7270-4ff8-b6c3-2bc6c7747291
📒 Files selected for processing (2)
.github/workflows/ci-test-e2e.yml.github/workflows/ci.yml
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
🪛 actionlint (1.7.11)
.github/workflows/ci.yml
[error] 411-411: could not parse action metadata in "/home/jailuser/git/.github/actions/build-docker": line 4: unexpected key "type" for definition of input "service"
(action)
🔇 Additional comments (4)
.github/workflows/ci-test-e2e.yml (2)
145-145: FIPS artifact selection looks correctly wired now.The updated pattern covers FIPS image artifacts for fork/non-published runs and aligns with the new
-fipssuffix flow.
191-202: EE/FIPS conditional handling is consistent across startup and test env.Treating
release == 'fips'like EE for container startup, service readiness, andIS_EEkeeps the e2e execution path coherent.Also applies to: 220-220, 236-236, 251-251
.github/workflows/ci.yml (2)
680-743: FIPS test orchestration and final aggregation are wired correctly.The new FIPS API/livechat/UI jobs and
tests-donegating checks are consistently integrated.Also applies to: 894-895, 931-941
410-425: Thetypemetadata keys in inputs are non-standard but pose no build risk.The action.yml file contains
type: stringunder thedeno-versionandserviceinputs (lines 17 and 25), which is not a documented key in GitHub's official action metadata schema. However, GitHub Actions runtime silently ignores unknown input metadata keys rather than rejecting them—builds will proceed without failure. This is a code cleanliness issue, not a blocking error. Consider removing thetypekeys or consulting the GitHub Actions documentation if type validation is intended elsewhere.> Likely an incorrect or invalid review comment.
cardoso
force-pushed
the
fips-ddp-streamer
branch
8 times, most recently
from
505bd2c to
ab8e715
Compare
cardoso
changed the title
cardoso
force-pushed
the
fips-ddp-streamer
branch
2 times, most recently
from
7d45983 to
e203c3e
Compare
cardoso
force-pushed
the
fips-ddp-streamer
branch
2 times, most recently
from
0ee7456 to
c56b0fa
Compare
…r unsupported methods
… WebSocket handshake
…ion for WebSocket handshake
…to environment variables for DockerHub login
…s before proceeding
cardoso
force-pushed
the
fips-ddp-streamer
branch
from
ff751b8 to
2bc36e0
Compare
2 participants
Ensure micro-services are FIPS-compliant
Task: FIPS-11
Summary by CodeRabbit
New Features
Infrastructure