Skip to content

LG-10222 Retire old MultiRegionKmsClient#8696

Merged
jmhooper merged 7 commits intomainfrom
jmhooper-retire-multi-region-kms
Jun 30, 2023
Merged

LG-10222 Retire old MultiRegionKmsClient#8696
jmhooper merged 7 commits intomainfrom
jmhooper-retire-multi-region-kms

Conversation

@jmhooper
Copy link
Contributor

We implemented a version of a multi-region KMS client. It was intended to encrypt across multiple regions so that if a region was unavailable a ciphertext could be decrypted in a different region. This code was never enabled due to complications with configuring the KMS clients.

Since this implementation was put together AWS has released KMS instances that are multi-region. This means we no longer need to use this approach to encrypt/decrypt across regions. As a result, this code can be removed.

We implemented a version of a multi-region KMS client. It was intended to encrypt across multiple regions so that if a region was unavailable a ciphertext could be decrypted in a different region. This code was never enabled due to complications with configuring the KMS clients.

Since this implementation was put together AWS has released KMS instances that are multi-region. This means we no longer need to use this approach to encrypt/decrypt across regions. As a result, this code can be removed.

[skip changelog]
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the diff is a little funky on this, just to confirm, this is basically taking encrypt_legacy from the MultiRegionKmsClient and promoting that to be the default/only implementation?

@jmhooper
Copy link
Contributor Author

jmhooper commented Jun 29, 2023

the diff is a little funky on this, just to confirm, this is basically taking encrypt_legacy from the MultiRegionKmsClient and promoting that to be the default/only implementation?

That is correct. The one change here that is a little different is how the connection pool is managed. The connection pools look like they were added after the multi-region change. This gets rid of the hash of pools by region and switches to a single pool for the current region. If you want to give that some extra attention I'd appreciate it!

@zachmargolis
Copy link
Contributor

That is correct. The one change here that is a little different is how the connection pool is managed. The connection pools look like they were added after the multi-region change. This gets rid of the hash of pools by region and switches to a single pool for the current region. If you want to give that some extra attention I'd appreciate it!

I think it looks good! The connection pools as constants can sometimes contribute to test pollution (since the stubs outlive the specs associated with them), I would port over the stub_const stuff we had in the old specs:

    stub_const(
      'Encryption::MultiRegionKmsClient::KMS_REGION_CLIENT_POOL',
      Hash.new do |h, region|
        h[region] = FakeConnectionPool.new { Aws::KMS::Client.new(region: region) }
      end,
    )

(but simplify it to be single region)

@jmhooper
Copy link
Contributor Author

good catch, I just pushed a commit to make the stub_const call

@jmhooper jmhooper changed the title Retire old MultiRegionKmsClient LG-10222 Retire old MultiRegionKmsClient Jun 29, 2023
@jmhooper jmhooper merged commit f0f3733 into main Jun 30, 2023
@jmhooper jmhooper deleted the jmhooper-retire-multi-region-kms branch June 30, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants