feat(operator): pod management#345
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ) | ||
| core.delete_namespaced_config_map( | ||
| name=configmap.metadata.name, namespace=workload_namespace | ||
| ) |
There was a problem hiding this comment.
Orphaned Deployments after upgrade
High Severity
The operator no longer deletes previously created Deployment workloads, and deployment RBAC was removed. On upgrade, old Deployments keep running their Pods while the new path also creates managed Pods for the same pipeline, so consumers can run twice and double-process traffic.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 50b3b68. Configure here.
| namespace=namespace, | ||
| field_manager=FIELD_MANAGER, | ||
| force_conflicts=True, | ||
| ) |
There was a problem hiding this comment.
Missing workload ownership checks
Medium Severity
apply_pod and save_generations apply with force_conflicts and never verify streams.sentry.io/owner-uid. Unlike _apply for ConfigMaps, a second StreamingPipeline that renders the same base name can overwrite another pipeline’s Pods or generation ledger instead of failing permanently.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 50b3b68. Configure here.
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d2ee769. Configure here.
d2ee769 to
50b3b68
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 399b877. Configure here.
| ) | ||
|
|
||
|
|
||
| def _prune_stale_resources( |
There was a problem hiding this comment.
Stale canary status after removal
Medium Severity
Status is written with a JSON merge patch, and pods.sets is a nested object. When a workload set such as canary is removed, that key is omitted rather than cleared, so the old sets entry can remain in status even though the Pods were deleted.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 399b877. Configure here.
| @kopf.daemon(GROUP, VERSION, PLURAL) | ||
| async def reconcile_pipeline_daemon( | ||
| stopped: kopf.DaemonStopped, | ||
| spec: kopf.Spec, | ||
| name: str, | ||
| namespace: str | None, | ||
| uid: str, | ||
| patch: kopf.Patch, | ||
| memo: kopf.Memo, | ||
| logger: Logger, | ||
| **_: Any, | ||
| ) -> None: | ||
| assert namespace is not None | ||
| workload_namespace = _workload_namespace() | ||
| if namespace is None: | ||
| raise kopf.PermanentError("Missing namespace!") | ||
|
|
||
| consumer = from_crd_spec(dict(spec), name=name) | ||
| scheduler = _scheduler(memo) | ||
| event = scheduler.register(uid) | ||
| try: | ||
| validate(consumer) | ||
| result = render(consumer) | ||
| except Exception as e: | ||
| patch.status["conditions"] = [_condition("Rendered", False, type(e).__name__, str(e))] | ||
| raise kopf.PermanentError(f"StreamingPipeline {namespace}/{name} failed to render: {e}") | ||
|
|
||
| manifests = [result["configmap"], result["deployment"]] | ||
| if "canary_deployment" in result: | ||
| manifests.append(result["canary_deployment"]) | ||
|
|
||
| dyn = dynamic.DynamicClient(client.ApiClient()) | ||
| for manifest in manifests: | ||
| _prepare_manifest( | ||
| manifest, | ||
| workload_namespace=workload_namespace, | ||
| owner_uid=uid, | ||
| owner_name=name, | ||
| owner_namespace=namespace, | ||
| ) | ||
| _apply( | ||
| dyn, | ||
| manifest, | ||
| workload_namespace=workload_namespace, | ||
| owner_uid=uid, | ||
| ) | ||
| while not stopped: | ||
| event.clear() | ||
| timeout = await _reconcile_once( | ||
| spec=spec, | ||
| name=name, | ||
| namespace=namespace, | ||
| uid=uid, | ||
| logger=logger, | ||
| scheduler=scheduler, | ||
| stopped=stopped, | ||
| ) | ||
| if stopped: | ||
| break | ||
| await _wait_for_reconcile(event, stopped, timeout) | ||
| finally: | ||
| scheduler.unregister(uid, event) | ||
|
|
||
| _prune_stale_resources( | ||
| workload_namespace=workload_namespace, | ||
| owner_uid=uid, | ||
| desired_deployments={ | ||
| manifest["metadata"]["name"] | ||
| for manifest in manifests | ||
| if manifest["kind"] == "Deployment" | ||
| }, | ||
| desired_configmaps={ | ||
| manifest["metadata"]["name"] | ||
| for manifest in manifests | ||
| if manifest["kind"] == "ConfigMap" | ||
| }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Pod and generation-ledger applies omit cross-pipeline ownership checks
The new reconcile path applies Pods and generation-ledger ConfigMaps in the shared workload namespace without the owner-uid guard used for other resources, so a StreamingPipeline whose rendered base name collides can overwrite another pipeline's ledger (and race on Pod names). Reuse the existing owner-uid check from _apply before apply_pod/save_generations, or fail closed on collision.
Evidence
reconcile_pipeline_daemon(this hunk) calls_reconcile_once→reconcile_pipeline, which creates Pods viaapply_podand generation ConfigMaps viasave_generations._applyinreconcile.pyrefuses to mutate an existing object whenstreams.sentry.io/owner-uidbelongs to another pipeline;apply_podandsave_generationsperformserver_side_apply(..., force_conflicts=True)with no equivalent check.- Workload object names are derived from CR-controlled
service_name/pipeline_name/segment_id(andemergency_patchcan override the deployment name), and all pipelines shareWORKLOAD_NAMESPACE. - A colliding CR can therefore overwrite another pipeline's
{base_name}-generationsConfigMap data and owner labels in the workload namespace; Pod name collisions are also unguarded.
Also found at 2 additional locations
sentry_streams_k8s/sentry_streams_k8s/operator/pod_resources.py:55-62sentry_streams_k8s/sentry_streams_k8s/operator/reconcile.py:261-350
Identified by Warden security-review · 66Z-LS4
1 participant
Adds Pod management abilities to the operator. Instead of generating a Deployment, the operator will generate and manage Pods. Uses a ConfigMap per CR to track the Pod generations. Watches Pod events and uses a health classifier to determine how to respond to them and reconcile.
This is a different implementation than the original test we did and is based on Kopf's official suggestion for how to reconcile: https://docs.kopf.dev/en/latest/reconciliation/#level-based-triggering
Our old implementation runs reconcile on every event. This is really bad especially when there are lots of events and many CRs and can easily overwhelm the k8s API. We instead use a level-based reconcile loop (one per CR) via a Kopf daemon. CR changes and relevant k8s events trigger the loop. This also ensures that reconciles are one at a time and there aren't any race conditions from multiple reconciles.