Added CMEK for Secret auto managed

[luigi-bitonti]

Users can choose a custom KMS key for Secret Manager secret replicated automatically.

Checklist

I applicable, I acknowledge that I have:

• Read the contributing guide
• Ran terraform fmt on all modified files
• Regenerated the relevant README.md files using tools/tfdoc.py
• Made sure all relevant tests pass

[juliocc]

The changes introduced in this PR make the module's use ambiguous. If I understand correctly, you're assuming that if the user specifies a global key, then it will be used for automatic replication.

This opens a few questions:

• How do I enable customer managed replication with a global key?
• How do I disable automatic replication with a global key?
• What happens if the user provides global and regional keys at the same time?

At the very least, I think there should be a way for the user to specify what type of replication to use.

[luigi-bitonti]

The changes introduced in this PR make the module's use ambiguous. If I understand correctly, you're assuming that if the user specifies a global key, then it will be used for automatic replication.

This opens a few questions:

• How do I enable customer managed replication with a global key?
• How do I disable automatic replication with a global key?
• What happens if the user provides global and regional keys at the same time?

At the very least, I think there should be a way for the user to specify what type of replication to use.

Hi Julio
your assumption it's correct.

• I haven't changed this part, so if you wanted to use a global key to encrypt a secret in europe-west1, you would have to insert in encryption key variable: europe-west1: global_key_selflink;
• If you don't put null in secrets variable, you won't use automatic replication, so the global key will not be used. The same happen in customer managed replication: if your secret it's in europe-west1 but in encryption key variable there is not the key europe-west1, the secret will be crypted with Google default key.
• Customer managed replication secrets will be crypted with regional key, automatic replication secrets will be crypted with global key.
• If you put null as value in secrets variable, the secret will have automatic replication. If you put a region, the secret will have a custom replication.

[luigi-bitonti]

@juliocc Is there anything else I need to do to keep PR going?

[ludoo]

If we start supporting a new feature which we did not previously support, we need to implement it fully. The replication block supports auto and user management settings, please refactor this to properly support both.

[luigi-bitonti]

If we start supporting a new feature which we did not previously support, we need to implement it fully. The replication block supports auto and user management settings, please refactor this to properly support both.

Both user_managed replication and auto replication are supported. With this PR, I want to introduce CMEK feature on auto replication. Currently, CMEK can be used only for user_managed replication.

[ludoo]

Replication should be controlled via its own variable, and the CMEK key should be an attribute in it, not a top-level variable.

[luigi-bitonti]

Replication should be controlled via its own variable, and the CMEK key should be an attribute in it, not a top-level variable.

So you suggest that the entire logic of this part should be refactored, also the As Is?

[ludoo]

Replication should be controlled via its own variable, and the CMEK key should be an attribute in it, not a top-level variable.

So you suggest that the entire logic of this part should be refactored, also the As Is?

yep, and sorry for doing this in pieces, if we were face to face it would have been a 5m discussion :) I think we should make replication a top-level variable now, and expose auto/user managed/key in its structure

I can give it a shot if you like

[luigi-bitonti]

Replication should be controlled via its own variable, and the CMEK key should be an attribute in it, not a top-level variable.

So you suggest that the entire logic of this part should be refactored, also the As Is?

yep, and sorry for doing this in pieces, if we were face to face it would have been a 5m discussion :) I think we should make replication a top-level variable now, and expose auto/user managed/key in its structure

I can give it a shot if you like

I was thinking something like this:

But OK, if you want you can do it :)

[luigi-bitonti]

@ludoo I tried to implement your suggest by refactoring the logic of secrets variable.

[luigi-bitonti]

Hi @wiktorn, is there anything else I can do for this PR?

As discussed, moving forward with this PR

[wiktorn]

Thank you for your contribution @luigi-bitonti and sorry to keep you waiting.