mirror of https://github.com/pulumi/pulumi.git
275ab73ba7
1. Remove unused error returns when constructing an `Encrypter` or `Decrypter` from a `SecretManager`. 2. Remove passing an `Encrypter` into the deserialization methods. ## Design rational 1. We were calling encrypt within deserialize whenever the source was plaintext, but would just discard the result if the decrypter wasn't an instance of `cachingCrypter`. Therefore the first optimisation we can make is to only encrypt after we've established that we've got a `cachingCrypter` to write to. 3. When the decrypter is a `cachingCrypter`, it must have been created by the `cachingManager` and so we know that the encrypter also came from the `cachingManager`. However, the `cachingCrypter` was instantiated twice - once with only an encrypter and once with only a decrypter. If we make the encrypter available within the same instance of the `cachingCrypter`, then we can use the encrypter from the instance instead of requiring the encrypter to be passed in as a separate argument. 4. If the `cachingCrypter` class is just implementing the Encrypter and Decrypter interfaces with the values from the `cachingSecretsManager`, we could just implement these interfaces directly on the `cachingSecretsManager` instead to avoid copying references to the cache into the `cachingCrypter`. This makes it much easier to follow how the cache gets reused between the decrypter and encrypter instances as they're now the same object. Adding the crypter interface onto the `cachingSecretsManager` makes it easiest to fetch the underlying encrypter and decrypter instances within the constructor which opens a couple of questions: ### Is both Encrypter and Decrypter always available? - Passphrase, service, cloud and base64 secret managers only have a single crypter object returned from both Encrypter and Decrypter methods. - The cachingSecretsManager just calls through to the provided Encrypter or Decrypter. - Only the MockSecretsManager could have only one of the encrypter or decrypter set up. However, this panics rather than returns an error. ### In what situations are we returned an error when asking the SecretManager for an Encrypter or Decrypter? - Never. Therefore, we can just delete the error returns from all `SecretManager` `Encrypter()` and `Decrypter()` methods. Finally, having moved the `EncryptValue` call to the `cachingSecretsManager`, we no longer use the encrypter on any code paths for Snapshot deserialization and can remove all unused instances of this argument. Commits are factored to separate out these logical steps for review. ## Pending questions > Why do we call encrypt at all during deserialization? It appears the purpose it to prime the cache so that all existing secrets are contained in the cache after deserialization so we're able to re-serialize it very quickly, if unchanged. > Do we always need these cached values? Not always. The cache is only _required_ when round-tripping a deployment, which could in theory be done using a NoopCrypter which never really deserializes the secrets but just sets the plaintexts to the ciphertext then reverses the process during serialization. Therefore we can't entirely remove the process of injecting the secret values into the cache, but this should allow us to simplify further with the new abstractions being built for bulk serialization in https://github.com/pulumi/pulumi/pull/18576 |
||
|---|---|---|
| .. | ||
| manager.go | ||
| provider.go | ||