feat(webui): Add log-ingestor and S3 auth config to deployment templates.#2196
Open
junhaoliao wants to merge 5 commits intomainfrom
Open
feat(webui): Add log-ingestor and S3 auth config to deployment templates.#2196junhaoliao wants to merge 5 commits intomainfrom
junhaoliao wants to merge 5 commits intomainfrom
Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughConditionally configures LogIngestor host/port and S3-based logs input AWS auth/profile in generated WebUI settings, adds environment variables for S3 credentials, exposes them via Fastify config, propagates them through Docker Compose and Helm templates, and adds container-transform behavior for LogIngestor. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
junhaoliao
commented
Apr 10, 2026
| minimum: 1, | ||
| }, | ||
|
|
||
| // S3 Logs Input |
Member
Author
There was a problem hiding this comment.
Suggested change
| // S3 Logs Input | |
| // S3 |
components/webui/server/.env
Outdated
| # Security | ||
| RATE_LIMIT=1000 | ||
|
|
||
| # S3 Logs Input |
Member
Author
There was a problem hiding this comment.
Suggested change
| # S3 Logs Input | |
| # S3 |
Comment on lines
306
to
307
| {{- end }} | ||
| {{- end }} |
Member
Author
There was a problem hiding this comment.
add {{/* */}} comments for these two lines
| description: "A Helm chart for CLP's (Compressed Log Processor) package deployment" | ||
| type: "application" | ||
| appVersion: "0.10.1-dev" | ||
| appVersion: "0.11.1-dev" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/clp-package-utils/clp_package_utils/controller.py`:
- Around line 797-807: The code is writing LogIngestorHost/Port whenever
log_ingestor exists even if log ingestion was disabled by
_set_up_env_for_log_ingestor(); update the conditional to only set
server_settings_json_updates["LogIngestorHost"] and ["LogIngestorPort"] when a
log_ingestor is configured AND the logs input type is S3 (i.e. check
container_clp_config.logs_input.type == StorageType.S3 or reuse the same enabled
condition used in _set_up_env_for_log_ingestor()); otherwise set both values to
None so the WebUI won't receive a non-functional endpoint. Ensure you reference
self._clp_config.log_ingestor, container_clp_config.log_ingestor,
container_clp_config.logs_input.type and StorageType.S3 in the new condition.
- Around line 1012-1023: The code that builds Docker Compose env vars in
controller.py drops AWS session credentials; when
self._clp_config.logs_input.aws_authentication.type == AwsAuthType.credentials
also check for logs_input_aws_auth.credentials.session_token and add it to
env_vars (e.g., "CLP_LOGS_INPUT_AWS_SESSION_TOKEN":
logs_input_aws_auth.credentials.session_token) alongside the access key and
secret; then propagate this new variable through the corresponding places
mentioned (components/webui/server/.env,
components/webui/server/src/plugins/external/env.ts, and
tools/deployment/package/docker-compose-all.yaml) so the WebUI receives the STS
session token.
In `@components/clp-py-utils/clp_py_utils/clp_config.py`:
- Around line 783-785: transform_for_container currently only rewrites the
LogIngestor host (in method transform_for_container) but leaves an overridden
log_ingestor.port visible to WebUI; update transform_for_container to also reset
the container's port to the container-default by assigning the LogIngestor port
to the constant (e.g., LOG_INGESTOR_PORT) or the literal default (3002) so that
self.port is set to the container listening port when you set self.host =
LOG_INGESTOR_COMPONENT_NAME.
In `@tools/deployment/package-helm/templates/configmap.yaml`:
- Around line 286-292: The LogIngestorHost/Port block is currently shown
whenever .Values.clpConfig.log_ingestor is true even for non-S3 log backends;
change the Helm conditional for the LogIngestorHost and LogIngestorPort
rendering so it only renders when log_ingestor is enabled AND logs_input.type ==
"s3" (use Helm's and and eq functions), i.e. replace the existing {{- if
.Values.clpConfig.log_ingestor }} test around the
"LogIngestorHost"/"LogIngestorPort" keys with {{- if and
.Values.clpConfig.log_ingestor (eq .Values.logs_input.type "s3") }} so the WebUI
won't advertise an ingestor endpoint for non-S3 deployments.
In `@tools/deployment/package-helm/templates/webui-deployment.yaml`:
- Around line 67-75: The template omits the AWS session token so STS temporary
credentials fail; update the helm template block for
.Values.clpConfig.logs_input (the if that checks .type "s3" and
.aws_authentication.type "credentials") to also inject the session token
environment variable (e.g., CLP_LOGS_INPUT_AWS_SESSION_TOKEN) by reading
.aws_authentication.credentials.session_token and quoting it, using the same
conditional scope as the existing CLP_LOGS_INPUT_AWS_ACCESS_KEY_ID and
CLP_LOGS_INPUT_AWS_SECRET_ACCESS_KEY entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b62b054c-dd62-416b-a970-78af6c2b5956
📒 Files selected for processing (8)
components/clp-package-utils/clp_package_utils/controller.pycomponents/clp-py-utils/clp_py_utils/clp_config.pycomponents/webui/server/.envcomponents/webui/server/src/plugins/external/env.tstools/deployment/package-helm/Chart.yamltools/deployment/package-helm/templates/configmap.yamltools/deployment/package-helm/templates/webui-deployment.yamltools/deployment/package/docker-compose-all.yaml
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tools/deployment/package-helm/templates/configmap.yaml (1)
286-292:⚠️ Potential issue | 🟠 MajorGate
LogIngestor*template fields on S3 logs input.At Line [286], the condition still checks only
log_ingestor. This can render an ingestor endpoint in non-S3 deployments.Proposed fix
- {{- if .Values.clpConfig.log_ingestor }} + {{- if and .Values.clpConfig.log_ingestor (eq .Values.clpConfig.logs_input.type "s3") }} "LogIngestorHost": "{{ include "clp.fullname" . }}-log-ingestor", "LogIngestorPort": 3002, {{- else }} "LogIngestorHost": null, "LogIngestorPort": null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/deployment/package-helm/templates/configmap.yaml` around lines 286 - 292, The LogIngestor template currently gates "LogIngestorHost" and "LogIngestorPort" only on .Values.clpConfig.log_ingestor, which can emit ingestor endpoints even when logs aren't using S3; update the conditional around the "LogIngestorHost"/"LogIngestorPort" block so it requires both .Values.clpConfig.log_ingestor and the S3 logs input flag (e.g. .Values.logs.input == "s3" or the project’s equivalent .Values.logs.s3.enabled) before rendering those fields; modify the if in the template that surrounds the "LogIngestorHost"/"LogIngestorPort" entries to check both conditions.components/clp-package-utils/clp_package_utils/controller.py (2)
797-807:⚠️ Potential issue | 🟠 MajorGate
LogIngestorHost/LogIngestorPorton S3 logs input too.At Line [797], the block still publishes an ingestor endpoint whenever
log_ingestorexists, even iflogs_input.typeis not S3. That advertises a non-existent service path in non-S3 deployments.Proposed fix
- if self._clp_config.log_ingestor is not None: + if ( + self._clp_config.log_ingestor is not None + and self._clp_config.logs_input.type == StorageType.S3 + ): server_settings_json_updates["LogIngestorHost"] = ( container_clp_config.log_ingestor.host ) server_settings_json_updates["LogIngestorPort"] = ( container_clp_config.log_ingestor.port ) else: server_settings_json_updates["LogIngestorHost"] = None server_settings_json_updates["LogIngestorPort"] = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/clp-package-utils/clp_package_utils/controller.py` around lines 797 - 807, The code currently sets server_settings_json_updates["LogIngestorHost"/"LogIngestorPort"] whenever self._clp_config.log_ingestor is present, but it should only advertise those values for S3-backed logs; update the conditional around the block that writes to server_settings_json_updates so it checks both that container_clp_config.log_ingestor is not None and that container_clp_config.logs_input.type (or equivalent logs_input property) equals "S3" (or the S3 enum/value used in your config) and only then populate LogIngestorHost/Port, otherwise set them to None; ensure you reference container_clp_config.log_ingestor and container_clp_config.logs_input.type when implementing the check and leave server_settings_json_updates as the target for updates.
1019-1029:⚠️ Potential issue | 🟠 MajorDo not drop temporary AWS session credentials for logs input.
At Line [1019], credential export still omits
session_token. STS-based configs will fail because WebUI won’t receive complete credentials.Proposed fix
if self._clp_config.logs_input.type == StorageType.S3: logs_input_aws_auth = self._clp_config.logs_input.aws_authentication if logs_input_aws_auth.type == AwsAuthType.credentials: env_vars |= { "CLP_LOGS_INPUT_AWS_ACCESS_KEY_ID": ( logs_input_aws_auth.credentials.access_key_id ), "CLP_LOGS_INPUT_AWS_SECRET_ACCESS_KEY": ( logs_input_aws_auth.credentials.secret_access_key ), } + if logs_input_aws_auth.credentials.session_token is not None: + env_vars["CLP_LOGS_INPUT_AWS_SESSION_TOKEN"] = ( + logs_input_aws_auth.credentials.session_token + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/clp-package-utils/clp_package_utils/controller.py` around lines 1019 - 1029, The code handling S3 logs input credentials omits temporary STS session tokens, causing failures for AwsAuthType.credentials with session-based creds; update the block that builds env_vars from self._clp_config.logs_input.aws_authentication (when StorageType.S3 and AwsAuthType.credentials) to also export the session token if present by adding the environment variable key (e.g., "CLP_LOGS_INPUT_AWS_SESSION_TOKEN") populated from logs_input_aws_auth.credentials.session_token so temporary credentials are passed through to the WebUI alongside access_key_id and secret_access_key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/clp-package-utils/clp_package_utils/controller.py`:
- Around line 797-807: The code currently sets
server_settings_json_updates["LogIngestorHost"/"LogIngestorPort"] whenever
self._clp_config.log_ingestor is present, but it should only advertise those
values for S3-backed logs; update the conditional around the block that writes
to server_settings_json_updates so it checks both that
container_clp_config.log_ingestor is not None and that
container_clp_config.logs_input.type (or equivalent logs_input property) equals
"S3" (or the S3 enum/value used in your config) and only then populate
LogIngestorHost/Port, otherwise set them to None; ensure you reference
container_clp_config.log_ingestor and container_clp_config.logs_input.type when
implementing the check and leave server_settings_json_updates as the target for
updates.
- Around line 1019-1029: The code handling S3 logs input credentials omits
temporary STS session tokens, causing failures for AwsAuthType.credentials with
session-based creds; update the block that builds env_vars from
self._clp_config.logs_input.aws_authentication (when StorageType.S3 and
AwsAuthType.credentials) to also export the session token if present by adding
the environment variable key (e.g., "CLP_LOGS_INPUT_AWS_SESSION_TOKEN")
populated from logs_input_aws_auth.credentials.session_token so temporary
credentials are passed through to the WebUI alongside access_key_id and
secret_access_key.
In `@tools/deployment/package-helm/templates/configmap.yaml`:
- Around line 286-292: The LogIngestor template currently gates
"LogIngestorHost" and "LogIngestorPort" only on .Values.clpConfig.log_ingestor,
which can emit ingestor endpoints even when logs aren't using S3; update the
conditional around the "LogIngestorHost"/"LogIngestorPort" block so it requires
both .Values.clpConfig.log_ingestor and the S3 logs input flag (e.g.
.Values.logs.input == "s3" or the project’s equivalent .Values.logs.s3.enabled)
before rendering those fields; modify the if in the template that surrounds the
"LogIngestorHost"/"LogIngestorPort" entries to check both conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60975dec-2c4d-4858-9ea9-34b8fb27ce59
📒 Files selected for processing (4)
components/clp-package-utils/clp_package_utils/controller.pycomponents/webui/server/.envcomponents/webui/server/src/plugins/external/env.tstools/deployment/package-helm/templates/configmap.yaml
- controller.py + configmap.yaml: Only publish LogIngestorHost/Port when both log_ingestor is configured AND logs_input.type is S3. - clp_config.py: Add DEFAULT_PORT ClassVar to LogIngestor; reset port in transform_for_container().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Extracted from the WIP prototype in #2169.
Adds deployment configuration for the log-ingestor service and S3 logs-input AWS credentials across
all deployment targets (Docker Compose, Helm, and the CLP package controller). This is independent
infrastructure that downstream PRs will consume when wiring up S3 ingestion job submission.
Key changes:
LogIngestorHost,LogIngestorPort,LogsInputS3AwsAuthType, andLogsInputS3AwsProfileintosettings.json. PassesCLP_LOGS_INPUT_AWS_*credentials ascontainer env vars when logs-input uses S3 with credential-based auth.
LogIngestorHost/Portare only published when both
log_ingestoris configured ANDlogs_input.typeis S3.LogIngestor.transform_for_container()so the log-ingestor hostnameresolves to the container service name and port resets to
DEFAULT_PORT. AddsDEFAULT_PORT: ClassVar[int] = 3002following the pattern of other model classes.LogIngestorHost,LogIngestorPort,LogsInputS3AwsAuthType, andLogsInputS3AwsProfileto the webui server settings template.LogIngestor*values are gatedon both
log_ingestorbeing configured andlogs_input.typebeings3.logs_inputcredentials conditional block (mirrors theexisting
stream_outputblock) forCLP_LOGS_INPUT_AWS_*env vars.CLP_LOGS_INPUT_AWS_ACCESS_KEY_IDandCLP_LOGS_INPUT_AWS_SECRET_ACCESS_KEYto the webui service environment.0.2.1-dev.7.Checklist
breaking change.
Validation performed
Scenario 1: Python syntax check
Task: Verify the modified Python files have no syntax errors.
Command:
Output:
Scenario 2: TypeScript build
Task: Verify the new env vars compile correctly in the Fastify env plugin.
Command:
Output:
Scenario 3: Helm template rendering
Task: Verify the updated Helm templates render correctly with S3 logs-input configuration.
Command:
Output:
Explanation: The Helm template correctly renders the
CLP_LOGS_INPUT_AWS_*env vars from thevalues configuration, confirming the conditional block works as expected.
Scenario 4: LogIngestor settings gated on S3 input
Task: Verify LogIngestorHost/Port are null when logs_input.type is not S3.
Command:
Output:
Explanation: With
logs_input.type=fs, the configmap correctly renders null values forLogIngestor settings even if
log_ingestoris configured, preventing the WebUI from advertisingan endpoint for an S3-only flow.
Summary by CodeRabbit
New Features
Chores