camunda
feat: make the secret provider optionally process definition id aware
Description
This PR makes the secret provider for outbound secrets aware for the process definition id.
There are no changes to the actual behaviour as the default secret provider are not sensitive to this.
The reason is that the behaviour cannot be unified across inbound and outbound connectors.
Context: https://camunda.slack.com/archives/C02JLRNQQ05/p1762355301979309
Related issues
closes #
Checklist
- [x] PR has a milestone or the
no milestonelabel. - [ ] Backport labels are added if these code changes should be backported. No backport label is added to the latest release, as this branch will be rebased onto main before the next release.
The process definition Id is known at the time of constructing the inbound connector, isn't it? Because we are parsing the Process Definition to extract the inbound connector definitions. So with sufficient refactoring, we should be able to carry that forward into the InboundSecretManager. Maybe someone else in the team can back me up or correct me here
If I understand correctly, the main concern with inbound is that there is a theoretical possibility of supporting cross-process deduplication in the future (not supported now, but our model does not contradict this).
As already mentioned in the slack thread, it's unlikely that this will be implemented, and it also probably brings more challenges than benefits. Process definition seems to be the main unit of granularity in Camunda, e.g. when it comes to authorization. With this in mind, I would prefer a simpler interface over theoretical extensibility, and simply do this:
public record SecretContext(String tenantId, String bpmnProcessId) {}
If we introduce a sealed interface, secret provider implementations that care about bpmnProcesId are forced to handle extra complexity (is it an inbound or outbound context?) with no obvious reason
@vringar @chillleader thank you for the feedback.
In adjusted the PR to reflect these assumptions. While I also got some good insights in slack, I am confident that the feature will work as expected.
A question going forward: Should the "EnvironmentSecretProvider" have a "processDefinitionIdAware" flag and implement this awareness, just like with the tenants?
A question going forward: Should the "EnvironmentSecretProvider" have a "processDefinitionIdAware" flag and implement this awareness, just like with the tenants?
@johnBgood any opinion on this?
@johnBgood any opinion on this?
Sorry I missed this.
Can't theEnvironmentSecretProvider act depending on what's in the SecretContext? Or do we see an issue if, for example, we provide a tenantId and it shouldn't be taken into account by the provider? If not, we could remove the old tenantIdAware flag, and use the SecretContext directly.
- I think you update the PR description as we don't have any differences between inbound/outbound now
@johnBgood the tenantIdAware is still relevant to stay backwards-compatible as the original impl of the EnvironmentSecretProvider did not have prefix and tenant-awareness.
In fact, I will add another flag processDefinitionIdAware to the EnvironmentSecretProvider to control whether it will be sensitive to this.
@sbuettner thank you for flagging me here. I created tests to verify the secret provider is working as expected.
Regarding the tests on the InboundConnectorDetails, I am not 100% sure how and where I would test this as the only thing to assert here would be the data mapping.
@jonathanlukas Alright, secret handling works the same for inbound Connectors: https://github.com/camunda/connectors/blob/e63d9dfb40c88edd4aa9973a8dab6da30f29e980/connector-runtime/connector-runtime-core/src/main/java/io/camunda/connector/runtime/core/inbound/InboundConnectorContextImpl.java#L344