Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renamed master key to principal key #228

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Conversation

dutow
Copy link
Collaborator

@dutow dutow commented Jun 25, 2024

This commit contains lots of changes, but it's just a repeated
execution of find <...> -exec sed <...>, so everything should
work as before.

Note: PR also contains changes from the SMGR branch merge, as it is based on that, please only check the latest commit: 9c08d95

@dutow dutow force-pushed the masterrename branch 2 times, most recently from dd15e3b to 6ee4ea4 Compare June 25, 2024 17:50
This commit contains lots of changes, but it's just a repeated
execution of find <...> -exec sed <...>, so everything should
work as before.
@dutow dutow marked this pull request as ready for review June 26, 2024 16:03
Copy link
Member

@dAdAbird dAdAbird left a comment

Choose a reason for hiding this comment

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

I made a couple of comments marking not renamed entities. But there are quite a bit of them.
I think patterns MasterKey and masterKey were omitted.
Plus there is a variable mastere_key_info (a misspelling and a snake case) in load_latest_versioned_key_name() (src/catalog/tde_principal_key.c)

@@ -63,7 +63,7 @@
typedef struct TDEFileHeader
{
int32 file_version;
TDEMasterKeyInfo master_key_info;
TDEMasterKeyInfo principal_key_info;
Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to rename the type as well

XLogRelKey xlrec;

master_key = GetMasterKey(newrlocator->dbOid, newrlocator->spcOid, NULL);
if (master_key == NULL)
principal_key = GetMasterKey(newrlocator->dbOid, newrlocator->spcOid, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

and the function

@@ -260,12 +260,12 @@ tde_encrypt_rel_key(TDEMasterKey *master_key, RelKeyData *rel_key_data, const Re
* Decrypts a given key and returns the decrypted one.
*/
RelKeyData *
tde_decrypt_rel_key(TDEMasterKey *master_key, RelKeyData *enc_rel_key_data, const RelFileLocator *rlocator)
tde_decrypt_rel_key(TDEMasterKey *principal_key, RelKeyData *enc_rel_key_data, const RelFileLocator *rlocator)
Copy link
Member

Choose a reason for hiding this comment

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

TDEMasterKey

@@ -215,7 +215,7 @@ init_gl_catalog_keys(void)
}

static TDEMasterKey *
create_master_key(const char *key_name, GenericKeyring * keyring,
create_principal_key(const char *key_name, GenericKeyring * keyring,
Oid dbOid, Oid spcOid, bool ensure_new_key)
{
TDEMasterKey *masterKey;
Copy link
Member

Choose a reason for hiding this comment

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

The variable name (along with the type)

/* parameter for the master key info shared hash */
static dshash_parameters master_key_dsh_params = {
/* parameter for the principal key info shared hash */
static dshash_parameters principal_key_dsh_params = {
sizeof(Oid),
sizeof(TDEMasterKey),
dshash_memcmp, /* TODO use int compare instead */
dshash_memhash};

TdeMasterKeylocalState masterKeyLocalState;
Copy link
Member

Choose a reason for hiding this comment

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

Both the type and variable

@dutow
Copy link
Collaborator Author

dutow commented Jun 26, 2024

@dAdAbird yes, I only searched for master.key, not [mM]aster[k]Key. Fixed now.

Copy link
Member

@dAdAbird dAdAbird left a comment

Choose a reason for hiding this comment

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

Found a couple more

@@ -1021,12 +1021,12 @@ pg_tde_get_key_from_file(const RelFileLocator *rlocator, GenericKeyring *keyring
LWLockAcquire(lock_files, LW_SHARED);

/* Get/generate a master, create the key for relation and get the encrypted key with bytes to write */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* Get/generate a master, create the key for relation and get the encrypted key with bytes to write */
/* Get/generate a principal key, create the key for relation and get the encrypted key with bytes to write */

* If ensure_new_key is true, then we will keep on incrementing the version number
* till we get a key name that is not present in the keyring
*/
keyInfo *
load_latest_versioned_key_name(TDEMasterKeyInfo *mastere_key_info, GenericKeyring *keyring, bool ensure_new_key)
load_latest_versioned_key_name(TDEPrincipalKeyInfo *mastere_key_info, GenericKeyring *keyring, bool ensure_new_key)
Copy link
Member

Choose a reason for hiding this comment

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

mastere_key_info here and all over the func body + in the function prototype, in src/include/catalog/tde_principal_key.h

@dutow dutow requested a review from dAdAbird June 26, 2024 20:50
@dutow dutow merged commit 2a4f0d8 into Percona-Lab:main Jun 27, 2024
8 checks passed
@dutow dutow deleted the masterrename branch June 27, 2024 11:32
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.

None yet

2 participants