fix: rm sys_ptrace from services, explicit user rather than root, set…

[kaixi-wang]

Rationale

• remove root requirement by setting user
• remove storageClass requirement by defaulting to false for redis persistence
• branched from feat(telemetry): integrate the new FOE telemetry sidecar #566

Review Priority

• high
• medium
• low

Changes

Checklist

• This PR maintains parity between Docker Compose and Helm

Testing

[coderabbitai]

Walkthrough

This pull request introduces FiftyOne Enterprise 2.19.0 with a comprehensive telemetry system, updating version references across all deployment platforms (Docker Compose, Kubernetes, Helm), adding Helm telemetry templates and test coverage, and hardening pod security contexts with explicit non-root defaults.

Changes

Telemetry Feature & Security Configuration

Layer / File(s)	Summary
Helm telemetry plumbing and helpers helm/fiftyone-teams-app/templates/_telemetry.tpl, helm/fiftyone-teams-app/templates/_helpers.tpl, helm/fiftyone-teams-app/templates/_do_targets.tpl	New _telemetry.tpl defines Redis URL resolution (external vs. in-cluster), sidecar container specs (standard and native variants with security context), RBAC role helpers, and socket/volume mounts. Updates to _helpers.tpl and _do_targets.tpl conditionally inject telemetry environment variables (FIFTYONE_TELEMETRY_REDIS_URL, TELEMETRY_SOCKET) into multiple workload containers when telemetry is enabled.
Helm values documentation and NOTES helm/fiftyone-teams-app/README.md, helm/fiftyone-teams-app/README.md.gotmpl, helm/fiftyone-teams-app/templates/NOTES.txt	Documents new telemetry values section with Redis, sidecar, and service account settings. Updates pod security context defaults (UID/GID 1000, runAsNonRoot) for apiSettings, appSettings, delegatedOperator, and pluginsSettings workloads. NOTES.txt now conditionally warns when telemetry Redis uses ephemeral emptyDir storage.
Helm unit tests for telemetry components tests/unit/helm/telemetry-redis_test.go, tests/unit/helm/telemetry-rolebinding_test.go, tests/unit/helm/telemetry-sidecar_test.go, tests/unit/helm/yaml_helpers.go	Adds comprehensive test suites validating telemetry Redis Deployment/Service/PVC rendering (with persistence/external-Redis behavior), Role/RoleBinding RBAC configuration, and sidecar injection with security context alignment and PID namespace sharing. Includes helper functions for YAML document splitting and disableTelemetry() value overrides.
Test harness modernization for telemetry-aware testing tests/unit/helm/api-deployment_test.go, tests/unit/helm/app-deployment_test.go, tests/unit/helm/plugins-deployment_test.go, tests/unit/helm/teams-app-deployment_test.go, tests/unit/helm/delegated-operator-instance-deployment_test.go, tests/unit/helm/delegated-operator-job-configmap_test.go	Updates all Helm deployment tests to disable telemetry by default via disableTelemetry() wrapper and revises pod security context assertions to expect concrete non-root values (FSGroup/RunAsGroup/RunAsUser=1000, RunAsNonRoot=true) instead of nil.
Docker Compose test updates for telemetry services tests/unit/compose/docker-compose-internal-auth_test.go, tests/unit/compose/docker-compose-legacy-auth_test.go	Extends test expectations to include telemetry services (telemetry-redis, telemetry sidecars), volumes (telemetry-redis-data, telemetry-socket), environment variables (FIFTYONE_TELEMETRY_REDIS_URL), and GPU profile support for delegated operations.
RBAC and API role tests for telemetry tests/unit/helm/api-role_test.go	Updates expected RBAC rules to include pods/log permission alongside existing rules and adds telemetry-disabled test case.
Test build tag configuration tests/unit/helm/common_test.go	Extends Go build tags to include telemetry-related unit test matrices (unitTelemetryRedis, unitTelemetryRoleBinding, unitTelemetrySidecar).

Version Bump 2.18.1 → 2.19.0

Layer / File(s)	Summary
Build and configuration version defaults Makefile, docs/custom-plugins.md, docs/orchestrators/configuring-databricks-orchestrator.md, docker/docs/configuring-gpu-workloads.md	Updates Makefile VERSION variable and Docker image tag examples from 2.18.1 to 2.19.0 across documentation and configuration examples.
Integration test version fixtures tests/fixtures/docker/integration_internal_auth.env, tests/fixtures/docker/integration_legacy_auth.env	Updates test environment version variables to 2.19.0rc11/2.19.0-rc.10 for integration test modes.
Helm upgrade and values documentation version updates helm/docs/upgrading.md	Updates Helm documentation with 2.19.0 references, including upgrade paths, SDK compatibility notes, license requirements, and version-specific upgrade steps across multiple migration scenarios.
Docker upgrade and configuration documentation version updates docker/README.md, docker/docs/upgrading.md	Updates Docker documentation to reference 2.19.0 in upgrade procedures, delegated operator examples, and SDK recommendations.

Telemetry Configuration Documentation

Layer / File(s)	Summary
Docker Compose telemetry configuration guide docker/docs/configuring-telemetry.md	Introduces new comprehensive guide covering default telemetry services, opt-out procedures via compose overrides, scaling guidance for delegated operators, environment variable configuration, resource limits, access control requirements, and Redis verification commands.
Docker environment template telemetry sections docker/internal-auth/env.template, docker/legacy-auth/env.template	Adds documented telemetry configuration sections describing optional Redis URL overrides, bundled Redis image/memory settings, namespace labeling, and sidecar target-process name matching variables.
Docker README telemetry section docker/README.md	Adds telemetry subsection under Advanced Configuration describing bundled sidecar + Redis backend, UI metrics visibility, and reference to full telemetry documentation.
Kubernetes telemetry sidecar configuration docs/orchestrators/configuring-kubernetes-orchestrator.md	Introduces new Telemetry Sidecar section and extends production template example with PID namespace sharing, telemetry sidecar initContainer, environment variables, and volume/mount configuration.
Helm upgrade documentation telemetry section helm/docs/upgrading.md	Adds v2.19+ Telemetry Sidecars upgrade section describing sidecar rollout scope, resource impact, cluster/POD security requirements (process namespace sharing, SYS_PTRACE), external Redis configuration, and opt-out via Helm values.
Fixture and validation script updates utils/bump-fixtures-helm.sh, utils/validate-docker-pulls.sh	Updates utility scripts: bump-fixtures-helm.sh now conditionally updates telemetry sidecar image tags; validate-docker-pulls.sh extends expected image allowlist with redis:7-alpine and voxel51/telemetry-sidecar with proper tag-handling logic.

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• voxel51/fiftyone-teams-app-deploy#568: Helm README template whitespace alignment on the same chart.valuesTable template call.
• voxel51/fiftyone-teams-app-deploy#564: Parallel version bump from 2.18.1 to 2.19.0 affecting Makefile and image tag references.

Suggested labels

priority-low

Suggested reviewers

• kevin-dimichel
• mo-getter

Poem

🐰 Telemetry lights twinkle bright,
Redis sidecars stand through the night,
Two-point-one-nine brings security tight,
Non-root pods and RBAC—all right! ✨

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch kacey/telemetry-including-redis-without-root-or-persistence

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

utils/validate-docker-pulls.sh (1)

105-116: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Current tag derivation is causing deterministic CI failures for unreleased image tags.

Using Chart.appVersion as the implied tag for all untagged voxel51/* images is failing this pipeline (no such manifest ...:v2.19.0). Please decouple pull validation from unreleased chart versions (for example via explicit per-image tags from values or a gated “release-only” validation mode).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@utils/validate-docker-pulls.sh` around lines 105 - 116, The script currently
derives tags from Chart.yaml.appVersion (variable expected_tag) and appends it
to untagged voxel51/* images (setting img_with_tag), which causes CI failures
for unreleased chart versions; change the tag resolution to prefer explicit
per-image tags from Helm values (e.g., read a values mapping like
.images.<name>.tag or a VALUES_IMAGE_TAGS env var) and only fall back to
Chart.appVersion in a gated "release-only" mode controlled by an env flag (e.g.,
RELEASE_VALIDATION=true); update the loop that sets img_with_tag (and references
EXPECTED_IMAGES and expected_tag) to lookup per-image tags first and skip or
mark as non-fatal when no published tag exists unless RELEASE_VALIDATION is
enabled.

tests/unit/helm/plugins-deployment_test.go (1)

2520-2536: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Align podSecurityContext non-root defaults across helm deployment tests (or document exceptions).

plugins-deployment_test.go asserts podSecurityContext.FSGroup=1000, RunAsGroup=1000, RunAsUser=1000, and RunAsNonRoot=true. Other deployments are not consistently asserting the same pod-level non-root defaults—e.g., cas-deployment_test.go (and at least one teams-app-deployment_test.go case) asserts podSecurityContext.RunAsNonRoot is nil. If this is intentional for those workloads, add a short rationale; otherwise make the test expectations/defaults consistent.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/helm/plugins-deployment_test.go` around lines 2520 - 2536, The
plugin deployment test case labeled "defaultValuesPluginsEnabled" asserts
podSecurityContext defaults (podSecurityContext.FSGroup, RunAsGroup, RunAsUser =
1000 and RunAsNonRoot = true) that are inconsistent with other tests (e.g.,
cases in cas-deployment_test.go and teams-app-deployment_test.go which expect
RunAsNonRoot == nil); either update those other tests to assert the same
pod-level non-root defaults or, if the difference is intentional, add a brief
rationale comment above the differing test cases explaining why
podSecurityContext.RunAsNonRoot (or other fields) are expected to be nil, and
ensure consistency by adjusting assertions for
podSecurityContext.FSGroup/RunAsGroup/RunAsUser/RunAsNonRoot accordingly.

🧹 Nitpick comments (1)

tests/unit/helm/app-deployment_test.go (1)

2094-2097: ⚡ Quick win

Guard pod security pointer fields before dereferencing.

Line 2094-Line 2097 dereference pointer fields directly; if rendering regresses to nil, this panics instead of producing a clear assertion failure.

Proposed hardening

 func(podSecurityContext *corev1.PodSecurityContext) {
-    s.Equal(int64(1000), *podSecurityContext.FSGroup, "fsGroup should be 1000 (image UID)")
-    s.Equal(int64(1000), *podSecurityContext.RunAsGroup, "runAsGroup should be 1000")
-    s.True(*podSecurityContext.RunAsNonRoot, "runAsNonRoot should be true")
-    s.Equal(int64(1000), *podSecurityContext.RunAsUser, "runAsUser should be 1000")
+    s.Require().NotNil(podSecurityContext, "pod security context should not be nil")
+    s.Require().NotNil(podSecurityContext.FSGroup, "fsGroup should be set")
+    s.Require().NotNil(podSecurityContext.RunAsGroup, "runAsGroup should be set")
+    s.Require().NotNil(podSecurityContext.RunAsNonRoot, "runAsNonRoot should be set")
+    s.Require().NotNil(podSecurityContext.RunAsUser, "runAsUser should be set")
+    s.Equal(int64(1000), *podSecurityContext.FSGroup, "fsGroup should be 1000 (image UID)")
+    s.Equal(int64(1000), *podSecurityContext.RunAsGroup, "runAsGroup should be 1000")
+    s.True(*podSecurityContext.RunAsNonRoot, "runAsNonRoot should be true")
+    s.Equal(int64(1000), *podSecurityContext.RunAsUser, "runAsUser should be 1000")
     s.Nil(podSecurityContext.FSGroupChangePolicy, "should be nil")
     s.Nil(podSecurityContext.SeccompProfile, "should be nil")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/helm/app-deployment_test.go` around lines 2094 - 2097, The test
dereferences podSecurityContext pointer fields (podSecurityContext.FSGroup,
.RunAsGroup, .RunAsNonRoot, .RunAsUser) directly which will panic if any are
nil; update the assertions to first guard each pointer with s.NotNil(...) (or
equivalent) referencing podSecurityContext and each field name, then dereference
them for the equality/true asserts so failures produce clear assertion errors
instead of panics.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docker/docs/configuring-telemetry.md`:
- Around line 70-71: Update the wording so it clarifies that the `SYS_PTRACE`
capability is required only for executor/delegated-operator telemetry sidecars
(the delegated-operator/executor path that uses py-spy) and not for service-mode
telemetry sidecars; modify the sentence around `SYS_PTRACE` and `py-spy` on the
line that currently implies every telemetry sidecar needs it, and ensure
`teams-plugins-telemetry` / `compose.dedicated-plugins.yaml` references remain
unchanged while adding a note that service-mode sidecars do not require
`SYS_PTRACE`.

In `@docker/docs/upgrading.md`:
- Around line 125-129: The docs overstate security requirements by implying
every sidecar needs the SYS_PTRACE capability; update the paragraph around
"PID-namespace sharing", "pid: \"service:<target>\"" and "SYS_PTRACE" to clarify
that only the telemetry-attaching sidecar/process that performs ptrace requires
SYS_PTRACE (or equivalent host PID namespace access), not all sidecars, and
provide a note that services removed from SYS_PTRACE per the PR should still
function when a dedicated attacher sidecar or host PID sharing is used as an
alternative to granting SYS_PTRACE broadly.

In `@helm/docs/upgrading.md`:
- Around line 170-181: The doc currently states the sidecar runs as root with
SYS_PTRACE, which conflicts with tests that validate non-root sidecars and only
grant SYS_PTRACE for executor/delegated-operator cases; update the paragraph to
reflect rendered manifests by stating that the telemetry sidecar runs as
non-root by default and that SYS_PTRACE is only added when using
executor/delegated-operator modes (or when explicitly enabled), and preserve the
guidance about PSA/admission by referencing the chart values telemetry.enabled
and namespace.name so users know to disable telemetry or relax policies when
they intentionally enable SYS_PTRACE.

In `@helm/fiftyone-teams-app/README.md`:
- Line 1044: Update the example pip install version in the README description to
match the documented recommended value: change the text that currently says "pip
install ... fiftyone==0.11.0" to use "pip install ... fiftyone==2.19.0" (or
interpolate the value referenced by
teamsAppSettings.env.FIFTYONE_APP_TEAMS_SDK_RECOMMENDED_VERSION) so the
displayed install command matches the recommended SDK version.

In `@tests/unit/helm/telemetry-redis_test.go`:
- Around line 121-129: The test TestBundledUrlWiresApiDeployment currently only
asserts the value when an env var named "FIFTYONE_TELEMETRY_REDIS_URL" is found,
so it can falsely pass if the variable is missing; update the test to assert the
env var is present for each container in
deployment.Spec.Template.Spec.Containers (e.g., set a found flag per container
while iterating container.Env and after the inner loop call s.True(found,
"container %s must set FIFTYONE_TELEMETRY_REDIS_URL") or use s.Require().True to
fail immediately), then keep the existing equality assertion of ev.Value ==
expectedURL when found. Ensure you reference the env variable name
"FIFTYONE_TELEMETRY_REDIS_URL" and the test function
TestBundledUrlWiresApiDeployment when making the change.

---

Outside diff comments:
In `@tests/unit/helm/plugins-deployment_test.go`:
- Around line 2520-2536: The plugin deployment test case labeled
"defaultValuesPluginsEnabled" asserts podSecurityContext defaults
(podSecurityContext.FSGroup, RunAsGroup, RunAsUser = 1000 and RunAsNonRoot =
true) that are inconsistent with other tests (e.g., cases in
cas-deployment_test.go and teams-app-deployment_test.go which expect
RunAsNonRoot == nil); either update those other tests to assert the same
pod-level non-root defaults or, if the difference is intentional, add a brief
rationale comment above the differing test cases explaining why
podSecurityContext.RunAsNonRoot (or other fields) are expected to be nil, and
ensure consistency by adjusting assertions for
podSecurityContext.FSGroup/RunAsGroup/RunAsUser/RunAsNonRoot accordingly.

In `@utils/validate-docker-pulls.sh`:
- Around line 105-116: The script currently derives tags from
Chart.yaml.appVersion (variable expected_tag) and appends it to untagged
voxel51/* images (setting img_with_tag), which causes CI failures for unreleased
chart versions; change the tag resolution to prefer explicit per-image tags from
Helm values (e.g., read a values mapping like .images.<name>.tag or a
VALUES_IMAGE_TAGS env var) and only fall back to Chart.appVersion in a gated
"release-only" mode controlled by an env flag (e.g., RELEASE_VALIDATION=true);
update the loop that sets img_with_tag (and references EXPECTED_IMAGES and
expected_tag) to lookup per-image tags first and skip or mark as non-fatal when
no published tag exists unless RELEASE_VALIDATION is enabled.

---

Nitpick comments:
In `@tests/unit/helm/app-deployment_test.go`:
- Around line 2094-2097: The test dereferences podSecurityContext pointer fields
(podSecurityContext.FSGroup, .RunAsGroup, .RunAsNonRoot, .RunAsUser) directly
which will panic if any are nil; update the assertions to first guard each
pointer with s.NotNil(...) (or equivalent) referencing podSecurityContext and
each field name, then dereference them for the equality/true asserts so failures
produce clear assertion errors instead of panics.

🪄 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: CHILL

Plan: Pro

Run ID: 0f8d243d-fa24-47bb-a7b9-0fbdc263f213

📥 Commits

Reviewing files that changed from the base of the PR and between 816edea and 23793fa.

⛔ Files ignored due to path filters (32)

• docker/common-services.yaml is excluded by !**/*.yaml
• docker/internal-auth/compose.dedicated-plugins.yaml is excluded by !**/*.yaml
• docker/internal-auth/compose.delegated-operators.gpu.yaml is excluded by !**/*.yaml
• docker/internal-auth/compose.delegated-operators.yaml is excluded by !**/*.yaml
• docker/internal-auth/compose.plugins.yaml is excluded by !**/*.yaml
• docker/internal-auth/compose.yaml is excluded by !**/*.yaml
• docker/legacy-auth/compose.dedicated-plugins.yaml is excluded by !**/*.yaml
• docker/legacy-auth/compose.delegated-operators.gpu.yaml is excluded by !**/*.yaml
• docker/legacy-auth/compose.delegated-operators.yaml is excluded by !**/*.yaml
• docker/legacy-auth/compose.plugins.yaml is excluded by !**/*.yaml
• docker/legacy-auth/compose.yaml is excluded by !**/*.yaml
• helm/fiftyone-teams-app/Chart.yaml is excluded by !**/*.yaml
• helm/fiftyone-teams-app/templates/api-deployment.yaml is excluded by !**/*.yaml
• helm/fiftyone-teams-app/templates/api-role.yaml is excluded by !**/*.yaml
• helm/fiftyone-teams-app/templates/app-deployment.yaml is excluded by !**/*.yaml
• helm/fiftyone-teams-app/templates/delegated-operator-instance-deployment.yaml is excluded by !**/*.yaml
• helm/fiftyone-teams-app/templates/delegated-operator-job-configmap.yaml is excluded by !**/*.yaml
• helm/fiftyone-teams-app/templates/plugins-deployment.yaml is excluded by !**/*.yaml
• helm/fiftyone-teams-app/templates/telemetry-redis.yaml is excluded by !**/*.yaml
• helm/fiftyone-teams-app/templates/telemetry-rolebinding.yaml is excluded by !**/*.yaml
• helm/fiftyone-teams-app/values.schema.json is excluded by !**/*.json
• helm/fiftyone-teams-app/values.yaml is excluded by !**/*.yaml
• helm/values.yaml is excluded by !**/*.yaml
• skaffold.yaml is excluded by !**/*.yaml
• tests/fixtures/docker/compose.override.mongodb.yaml is excluded by !**/*.yaml
• tests/fixtures/docker/compose.override.mongodb_do.yaml is excluded by !**/*.yaml
• tests/fixtures/docker/compose.override.mongodb_plugins.yaml is excluded by !**/*.yaml
• tests/fixtures/helm/integration_values.yaml is excluded by !**/*.yaml
• tests/unit/helm/test_data/delegated-operator-job-configmap_test/expected-cpu-default-override-template-values.yaml is excluded by !**/*.yaml
• tests/unit/helm/test_data/delegated-operator-job-configmap_test/expected-cpu-default.yaml is excluded by !**/*.yaml
• tests/unit/helm/test_data/delegated-operator-job-configmap_test/expected-override-example-cascading-template.yaml is excluded by !**/*.yaml
• tests/unit/helm/test_data/delegated-operator-job-configmap_test/expected-override-example-template.yaml is excluded by !**/*.yaml

📒 Files selected for processing (35)

• Makefile
• docker/README.md
• docker/docs/configuring-gpu-workloads.md
• docker/docs/configuring-telemetry.md
• docker/docs/upgrading.md
• docker/internal-auth/env.template
• docker/legacy-auth/env.template
• docs/custom-plugins.md
• docs/orchestrators/configuring-databricks-orchestrator.md
• docs/orchestrators/configuring-kubernetes-orchestrator.md
• helm/docs/upgrading.md
• helm/fiftyone-teams-app/README.md
• helm/fiftyone-teams-app/README.md.gotmpl
• helm/fiftyone-teams-app/templates/NOTES.txt
• helm/fiftyone-teams-app/templates/_do_targets.tpl
• helm/fiftyone-teams-app/templates/_helpers.tpl
• helm/fiftyone-teams-app/templates/_telemetry.tpl
• tests/fixtures/docker/integration_internal_auth.env
• tests/fixtures/docker/integration_legacy_auth.env
• tests/unit/compose/docker-compose-internal-auth_test.go
• tests/unit/compose/docker-compose-legacy-auth_test.go
• tests/unit/helm/api-deployment_test.go
• tests/unit/helm/api-role_test.go
• tests/unit/helm/app-deployment_test.go
• tests/unit/helm/common_test.go
• tests/unit/helm/delegated-operator-instance-deployment_test.go
• tests/unit/helm/delegated-operator-job-configmap_test.go
• tests/unit/helm/plugins-deployment_test.go
• tests/unit/helm/teams-app-deployment_test.go
• tests/unit/helm/telemetry-redis_test.go
• tests/unit/helm/telemetry-rolebinding_test.go
• tests/unit/helm/telemetry-sidecar_test.go
• tests/unit/helm/yaml_helpers.go
• utils/bump-fixtures-helm.sh
• utils/validate-docker-pulls.sh

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clarify SYS_PTRACE scope to delegated-operator sidecars only.

Line 70 currently reads as if every telemetry sidecar requires SYS_PTRACE. Based on this PR’s security direction and accompanying tests, that capability should be limited to executor/delegated-operator telemetry, not service-mode sidecars.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docker/docs/configuring-telemetry.md` around lines 70 - 71, Update the
wording so it clarifies that the `SYS_PTRACE` capability is required only for
executor/delegated-operator telemetry sidecars (the delegated-operator/executor
path that uses py-spy) and not for service-mode telemetry sidecars; modify the
sentence around `SYS_PTRACE` and `py-spy` on the line that currently implies
every telemetry sidecar needs it, and ensure `teams-plugins-telemetry` /
`compose.dedicated-plugins.yaml` references remain unchanged while adding a note
that service-mode sidecars do not require `SYS_PTRACE`.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Security requirement is overstated for all sidecars.

This section implies every telemetry sidecar must run with SYS_PTRACE. That conflicts with the PR’s “remove SYS_PTRACE from services” direction and may push users toward unnecessarily permissive runtime policy.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docker/docs/upgrading.md` around lines 125 - 129, The docs overstate security
requirements by implying every sidecar needs the SYS_PTRACE capability; update
the paragraph around "PID-namespace sharing", "pid: \"service:<target>\"" and
"SYS_PTRACE" to clarify that only the telemetry-attaching sidecar/process that
performs ptrace requires SYS_PTRACE (or equivalent host PID namespace access),
not all sidecars, and provide a note that services removed from SYS_PTRACE per
the PR should still function when a dedicated attacher sidecar or host PID
sharing is used as an alternative to granting SYS_PTRACE broadly.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

This security guidance conflicts with the chart behavior validated by tests.

Line 170 says the sidecar runs as root with SYS_PTRACE, but the telemetry sidecar tests in this PR validate non-root sidecars and limit SYS_PTRACE to executor/delegated-operator cases. Please align this section with actual rendered manifests.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@helm/docs/upgrading.md` around lines 170 - 181, The doc currently states the
sidecar runs as root with SYS_PTRACE, which conflicts with tests that validate
non-root sidecars and only grant SYS_PTRACE for executor/delegated-operator
cases; update the paragraph to reflect rendered manifests by stating that the
telemetry sidecar runs as non-root by default and that SYS_PTRACE is only added
when using executor/delegated-operator modes (or when explicitly enabled), and
preserve the guidance about PSA/admission by referencing the chart values
telemetry.enabled and namespace.name so users know to disable telemetry or relax
policies when they intentionally enable SYS_PTRACE.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the SDK install example version in the description.

Line 1044 still says pip install ... fiftyone==0.11.0, which conflicts with the newly documented recommended version 2.19.0 and can mislead users.

✏️ Suggested doc fix

-| teamsAppSettings.env.FIFTYONE_APP_TEAMS_SDK_RECOMMENDED_VERSION | string | `"2.19.0"` | The recommended fiftyone SDK version that will be displayed in the install modal (i.e. `pip install ... fiftyone==0.11.0`). |
+| teamsAppSettings.env.FIFTYONE_APP_TEAMS_SDK_RECOMMENDED_VERSION | string | `"2.19.0"` | The recommended fiftyone SDK version that will be displayed in the install modal (i.e. `pip install ... fiftyone==2.19.0`). |

📝 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.

Suggested change

	| teamsAppSettings.env.FIFTYONE_APP_TEAMS_SDK_RECOMMENDED_VERSION | string | `"2.19.0"` | The recommended fiftyone SDK version that will be displayed in the install modal (i.e. `pip install ... fiftyone==0.11.0`). |
	| teamsAppSettings.env.FIFTYONE_APP_TEAMS_SDK_RECOMMENDED_VERSION | string | `"2.19.0"` | The recommended fiftyone SDK version that will be displayed in the install modal (i.e. `pip install ... fiftyone==2.19.0`). |

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@helm/fiftyone-teams-app/README.md` at line 1044, Update the example pip
install version in the README description to match the documented recommended
value: change the text that currently says "pip install ... fiftyone==0.11.0" to
use "pip install ... fiftyone==2.19.0" (or interpolate the value referenced by
teamsAppSettings.env.FIFTYONE_APP_TEAMS_SDK_RECOMMENDED_VERSION) so the
displayed install command matches the recommended SDK version.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

TestBundledUrlWiresApiDeployment can pass when the env var is missing.

Line 121-Line 129 only validates value if FIFTYONE_TELEMETRY_REDIS_URL exists, so the test silently passes when the variable is absent.

Proposed fix

 expectedURL := fmt.Sprintf("redis://%s-telemetry-redis.%s.svc.cluster.local:6379",
 	s.releaseName, "fiftyone-teams")
 for _, container := range deployment.Spec.Template.Spec.Containers {
-	for _, ev := range container.Env {
-		if ev.Name == "FIFTYONE_TELEMETRY_REDIS_URL" {
-			s.Equal(expectedURL, ev.Value,
-				"FIFTYONE_TELEMETRY_REDIS_URL on %s should be release-scoped in-cluster URL",
-				container.Name)
-		}
-	}
+	var found *corev1.EnvVar
+	for i, ev := range container.Env {
+		if ev.Name == "FIFTYONE_TELEMETRY_REDIS_URL" {
+			found = &container.Env[i]
+			break
+		}
+	}
+	s.Require().NotNil(found,
+		"FIFTYONE_TELEMETRY_REDIS_URL should be set on %s container", container.Name)
+	s.Equal(expectedURL, found.Value,
+		"FIFTYONE_TELEMETRY_REDIS_URL on %s should be release-scoped in-cluster URL",
+		container.Name)
 }

📝 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.

Suggested change

	for _, container := range deployment.Spec.Template.Spec.Containers {
	for _, ev := range container.Env {
	if ev.Name == "FIFTYONE_TELEMETRY_REDIS_URL" {
	s.Equal(expectedURL, ev.Value,
	"FIFTYONE_TELEMETRY_REDIS_URL on %s should be release-scoped in-cluster URL",
	container.Name)
	}
	}
	}
	for _, container := range deployment.Spec.Template.Spec.Containers {
	var found *corev1.EnvVar
	for i, ev := range container.Env {
	if ev.Name == "FIFTYONE_TELEMETRY_REDIS_URL" {
	found = &container.Env[i]
	break
	}
	}
	s.Require().NotNil(found,
	"FIFTYONE_TELEMETRY_REDIS_URL should be set on %s container", container.Name)
	s.Equal(expectedURL, found.Value,
	"FIFTYONE_TELEMETRY_REDIS_URL on %s should be release-scoped in-cluster URL",
	container.Name)
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/helm/telemetry-redis_test.go` around lines 121 - 129, The test
TestBundledUrlWiresApiDeployment currently only asserts the value when an env
var named "FIFTYONE_TELEMETRY_REDIS_URL" is found, so it can falsely pass if the
variable is missing; update the test to assert the env var is present for each
container in deployment.Spec.Template.Spec.Containers (e.g., set a found flag
per container while iterating container.Env and after the inner loop call
s.True(found, "container %s must set FIFTYONE_TELEMETRY_REDIS_URL") or use
s.Require().True to fail immediately), then keep the existing equality assertion
of ev.Value == expectedURL when found. Ensure you reference the env variable
name "FIFTYONE_TELEMETRY_REDIS_URL" and the test function
TestBundledUrlWiresApiDeployment when making the change.

[kaixi-wang]

closing in favor of #566