Repository navigation
feat: support a remote Liqo Snapshot control plane - #16
Merged
Merged
Conversation
Keep Snapshot metadata on the consumer while a provider agent validates reflected Pod identities and executes against its local runtime. Delegate artifact cleanup explicitly so the consumer cannot acknowledge deletion from an unrelated filesystem. Scope the initial path to one namespace mapping and physical node. Add capture reservation, restore revalidation, cleanup guards, chart configuration and regression tests. Leave the feature disabled by default. Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
A restore Pod with an unknown or unrecognized phase may still be using its artifact. Only treat Succeeded and Failed Pods as terminal, and continue blocking cleanup while a restore protection finalizer remains. Cover terminal, active and unknown phases on both metadata and provider APIs, including protected terminal Pods. Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
The credential volume exposed only the kubeconfig key, so referenced token and certificate files were unavailable. Project the dedicated Secret in full into the agent without subPath mounts, preserving file-based token rotation. Document the token-file configuration and cover the complete, read-only agent projection and exclusion from PageBroker in a Helm regression test. Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Keep consumer-owned Snapshot metadata without requiring a modified Liqo reflector. Stamp actual consumer Pod UIDs with optimistic preconditions and validate new provider ShadowPods against fresh metadata and the configured reflector identity. Prevent updates from adopting or rebinding an existing ShadowPod while allowing cleanup after source loss. Share binding checks between the agent and webhook, deploy admission as a separate process with read-only credentials and provisioned TLS, and keep the new components disabled by default. Document fresh-namespace migration and fail-closed availability requirements. Cover stamping and admission with unit tests, two-apiserver TLS integration tests, and Helm configuration guards. Real Liqo/GPU qualification remains a separate deployment gate. Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
|
Running ultrareview automatically — this PR appears to change authentication, authorization, or input validation, where a missed bug can expose data or grant the wrong access, so a deeper multi-pass review is worth running. I'll post findings when complete. |
rst0git
added
the
skip-issue-check
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview ran automatically — this PR appears to change authentication, authorization, or input validation, where a missed bug can expose data or grant the wrong access, so a deeper multi-pass review is worth running.
1 issue found across 43 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="agent/pkg/provenance/client.go">
<violation number="1" location="agent/pkg/provenance/client.go:51">
P2: This client-wide timeout also applies to `w.dynClient`, closing each long-lived PodSnapshotContent informer watch after 30 seconds. Leave it unset and bound individual API calls with request contexts.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| if !strings.HasPrefix(cfg.Host, "https://") || cfg.Insecure { | ||
| return nil, fmt.Errorf("metadata API requires HTTPS with certificate verification") | ||
| } | ||
| cfg.Timeout = 30 * time.Second |
There was a problem hiding this comment.
P2: This client-wide timeout also applies to w.dynClient, closing each long-lived PodSnapshotContent informer watch after 30 seconds. Leave it unset and bound individual API calls with request contexts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At agent/pkg/provenance/client.go, line 51:
<comment>This client-wide timeout also applies to `w.dynClient`, closing each long-lived PodSnapshotContent informer watch after 30 seconds. Leave it unset and bound individual API calls with request contexts.</comment>
<file context>
@@ -0,0 +1,116 @@
+ if !strings.HasPrefix(cfg.Host, "https://") || cfg.Insecure {
+ return nil, fmt.Errorf("metadata API requires HTTPS with certificate verification")
+ }
+ cfg.Timeout = 30 * time.Second
+ return cfg, nil
+}
</file context>
Capture: the agent now names itself in a remote capture reservation and remembers which content it handed to the checkpoint. A retry by the same process may resume a reservation it never executed, so a transient API or runtime error in the checks after the reservation, or a reservation whose reply was lost, no longer fails the capture as CaptureOutcomeUnknown. A reservation from another process, or one already executed, still fails closed. Restore: resolve now requires the queued (or CRI) Pod's Snapshot request annotations to equal the fresh, authoritative ones, so restore-from, the container map and the compatibility bypass can no longer come from a stale copy. A runtime that cannot report a container's Pod is retried as pending; only a runtime naming another Pod stays terminal. Cleanup: any Pod carrying the restore protection finalizer defers artifact deletion, whatever its restore-from annotation now says. Operator: pending content on a node later listed in RemoteAgentNodes gets its cleanup delegation backfilled instead of staying Pending; terminal content never changes cleanup owner. Agent client: the content informer uses a copy of the remote config without the 30s client timeout, which ended every watch. Binding validation reports fields in a fixed order. Chart: the agent reads ShadowPods through a Role in the provider namespace instead of a cluster-wide rule; the provenance name is truncated to a valid label value; the webhook rolls when its binding changes. Docs require restarting the webhook, not only the agent, after credential rotation. Each behaviour change has a regression test that fails on the previous code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Member
Author
|
Addressed all 11 review findings in 613f6dd, each |
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
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.
Box agent with separate Curie and box API clients, pod-identity stamping and an admission webhook, explicit cleanup handoff. Unmodified Liqo. Design: doublewordai/internal#4677