Vault refactor to support workflowOwner as owner of a secret#2068
Conversation
…common into vault-owner-refactor-common
✅ API Diff Results -
|
| reserved 2, 3; | ||
| reserved "org_id", "workflow_owner"; |
There was a problem hiding this comment.
Did you consider this method of deprecation?
| reserved 2, 3; | |
| reserved "org_id", "workflow_owner"; | |
| string org_id = 2 [deprecated = true]; | |
| string workflow_owner = 3 [deprecated = true]; |
The advantage being that the generated code will remain compatible.
There was a problem hiding this comment.
I actually revisited this, and decided to delete entirely.
These fields were never used on prod or even staging. They are not not needed as per design changes. So best is to just delete them, as we know it is safe
There was a problem hiding this comment.
These fields are currently referenced from chainlink/v2 though. Please do not remove them in a breaking manner.
There was a problem hiding this comment.
Removed them all here: smartcontractkit/chainlink#22525
There was a problem hiding this comment.
That's great but the historical references are still there, so the incompatibility may still come up, e.g. when backporting to a release patch, or when working in chainlink-deployments or a chain integrations-tests/ module.
There was a problem hiding this comment.
Ok, these fields were just added recently and never used/shipped so the only risk is when someone is backporting to an older release branch.
So bringing the fields back with deprecated tag.
…common into vault-owner-refactor-common
| VaultOrgIdAsSecretOwnerEnabled Setting[bool] | ||
| VaultOrgIdAsSecretOwnerEnabled Setting[bool] // Deprecated | ||
| PropagateOrgIDInRequestMetadata Setting[bool] | ||
| TenantID Setting[uint64] |
There was a problem hiding this comment.
Is there any more we could add to this name? Because it may be confused with the settings package notion of tenant. I notice you already removed the Vault prefix.
There was a problem hiding this comment.
removed this, and added to job-specs
| GatewayIncomingPayloadSizeLimit: Size(1 * config.MByte), | ||
| GatewayVaultManagementEnabled: Bool(true), | ||
| VaultJWTAuthEnabled: Bool(false), | ||
| // Deprecated: retained for backwards compatibility; workflow owner identifies secret ownership. |
There was a problem hiding this comment.
One thing to note here is that we never shipped a CLI version that used org owned secrets so we do not need to maintain backwards compatibility. This can be safely deprecated
There was a problem hiding this comment.
Thanks, yes agreed.
I initially deleted it, but @jmank88 asked to keep it deprecated.
I think it is just an artifact of this field existing in cld repo on some env, or if an older release branch needs to bump chainlink-common.
4 participants
Supports smartcontractkit/chainlink#22525.
Design: Vault DON design doc (new) — JWT Vault tenants and related behavior.
Tenant for the Vault JWT path is configured in Chainlink job specs (
auth0.tenantID, alongside issuer and audience; see chainlink#22525). Thischainlink-commonchange keeps cresettings aligned with that model.Coordination
chainlink-commonrevision that contains this change.