Opaque secrets#3674
Conversation
✅ Deploy Preview for golemcloud canceled.
|
|
📖 Docs preview: https://docs-7ckcyiiso-golem-cloud.vercel.app Built from commit |
| Ok(None) => Err(SecretRevealError::VersionNotFound( | ||
| entry.pinned_revision.get(), | ||
| )), | ||
| Err(error) => Err(SecretRevealError::Internal(error.to_string())), |
There was a problem hiding this comment.
Is it okay to persist here potentially transient errors? Can these be retried later?
There was a problem hiding this comment.
it should be retried with the active retry policy and eventually fail permanently. let me check if this is the case.
| Ok(results) | ||
| } | ||
|
|
||
| async fn get_revision( |
There was a problem hiding this comment.
Is it intentional (probably yes), that this does not filter on deleted apps / envs (AFAIR we usually check those too)?
If yes, maybe it worth mentioning this, or check what naming pattern we use in similar cases where we allow seeing deleted entities.
There was a problem hiding this comment.
Yes because we are not storing any secrets now in the oplog - so we have to be able to resolve arbitrary old ones during replay.
| @@ -0,0 +1,68 @@ | |||
| import fs from 'node:fs'; | |||
There was a problem hiding this comment.
Why do we need patching? Is this a wasm-rquickjs missing things, or something else?
There was a problem hiding this comment.
i have no idea what this is and definitely should not exist 😱
| JOIN accounts a ON a.account_id = ap.account_id | ||
| JOIN agent_secret_revisions secr ON secr.agent_secret_id = sec.agent_secret_id AND secr.revision_id = $4 | ||
| WHERE sec.environment_id = $1 AND sec.agent_secret_id = $2 AND sec.path = $3 AND secr.deleted = FALSE | ||
| AND ($5 OR (sec.deleted_at IS NULL AND e.deleted_at IS NULL AND ap.deleted_at IS NULL AND a.deleted_at IS NULL)) |
There was a problem hiding this comment.
in general we switched from this style to use two static selects, or a query builder, to avoid accidental planner complexities
noise64
left a comment
noise64
left a comment
There was a problem hiding this comment.
added minor comment on the sql fix, otherwise looks good
2 participants
Resolves #3529