-
Notifications
You must be signed in to change notification settings - Fork 623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Secret replication #1775
base: main
Are you sure you want to change the base?
Secret replication #1775
Conversation
fb759c0
to
c671253
Compare
… and replication dal
178eb03
to
59044e7
Compare
DynamicSecretRevocation = "dynamic-secret-revocation" | ||
DynamicSecretRevocation = "dynamic-secret-revocation", | ||
SecretReplication = "secret-replication", | ||
SecretSync = "secret-sync" // parent queue to push integration sync, webhook, and secret replication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we update all direct calls of syncIntegration
to use this secretSync
queue instead?
// check if user has permission to import from target path | ||
ForbiddenError.from(permission).throwUnlessCan( | ||
ProjectPermissionActions.Create, | ||
subject(ProjectPermissionSub.Secrets, { | ||
environment: secretImportDoc.importEnv.slug, | ||
secretPath: secretImportDoc.importPath | ||
}) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait is this the same with checking if the user has sufficient permission to import from the source?
const findSecrets = async (filter: { folderId: string; secrets: { id: string; version: number }[] }, tx?: Knex) => { | ||
if (!filter.secrets) return []; | ||
|
||
const sqlRawDocs = await (tx || db)(TableName.SecretVersion) | ||
.where({ folderId: filter.folderId }) | ||
.andWhere((bd) => { | ||
filter.secrets.forEach((el) => { | ||
void bd.orWhere({ | ||
[`${TableName.SecretVersion}.secretId` as "secretId"]: el.id, | ||
[`${TableName.SecretVersion}.version` as "version"]: el.version | ||
}); | ||
}); | ||
}) | ||
.leftJoin<TSecretVersions>( | ||
(tx || db)(TableName.SecretVersion) | ||
.where("isReplicated", true) | ||
.groupBy("secretId") | ||
.max("version") | ||
.select("secretId") | ||
.as("latestVersion"), | ||
`${TableName.SecretVersion}.secretId`, | ||
"latestVersion.secretId" | ||
) | ||
.select(db.ref("max").withSchema("latestVersion").as("latestReplicatedVersion")) | ||
.select(selectAllTableCols(TableName.SecretVersion)); | ||
|
||
return sqlRawDocs; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this fetches secret versions right? maybe let's rename the method?
|
||
const replicatedSecretsGroupBySecretId = groupBy(replicatedSecrets, (i) => i.secretId); | ||
const lock = await keyStore.acquireLock( | ||
replicatedSecrets.map(({ id }) => id), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this have some sort of a prefix so that the lock is specific for replication purposes?
const hasJobCompleted = await keyStore.getItem( | ||
keystoreReplicationSuccessKey(job.id as string, secretImport.id), | ||
KeyStorePrefixes.SecretReplication | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! we can now add something like this to the integration sync flow
queueService.listen(QueueName.SecretReplication, "failed", (job, err) => { | ||
logger.error(err, "Failed to replicate secret", job?.data); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how are we informing users regarding failed replication jobs?
const handleResyncSecretReplication = async () => { | ||
if (resyncSecretReplication.isLoading) return; | ||
try { | ||
await resyncSecretReplication.mutateAsync({ | ||
id, | ||
environment, | ||
path: secretPath, | ||
projectId: currentWorkspace?.id || "" | ||
}); | ||
createNotification({ | ||
text: "Kindly refresh the board to see changes.", | ||
type: "success" | ||
}); | ||
} catch (error) { | ||
console.error(error); | ||
createNotification({ | ||
text: "Failed to resync replication", | ||
type: "error" | ||
}); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for this one, can we do the refetch behind the scenes instead of prompting to user to explicitly refresh?
Description 📣
Type ✨
Tests 🛠️
# Here's some code block to paste some code snippets