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

SNOW-1492090 Snowpipe streaming file master key id rotation #786

Conversation

sfc-gh-bmikaili
Copy link
Contributor

@sfc-gh-bmikaili sfc-gh-bmikaili commented Jun 27, 2024

This PR implements encryption key refresh from the server. If the register blob endpoint responds with encryption keys as null, the encryption key from the open channel response is used. If the register blob endpoint responds with non-empty list of encryption keys, these keys are used for encryption of subsequent BDEC files.

@sfc-gh-bmikaili sfc-gh-bmikaili self-assigned this Jun 27, 2024
@sfc-gh-bmikaili sfc-gh-bmikaili marked this pull request as ready for review June 28, 2024 11:56
@sfc-gh-bmikaili sfc-gh-bmikaili requested review from sfc-gh-tzhang and a team as code owners June 28, 2024 11:56
@sfc-gh-bmikaili sfc-gh-bmikaili marked this pull request as draft June 28, 2024 17:28
@sfc-gh-bmikaili sfc-gh-bmikaili marked this pull request as ready for review June 28, 2024 17:52
@sfc-gh-bmikaili sfc-gh-bmikaili force-pushed the bmikaili-SNOW-1492090-snowpipe-streaming-file-master-key-id-rotation branch 2 times, most recently from 12d3fff to 4b00b0a Compare July 9, 2024 10:20
@sfc-gh-bmikaili sfc-gh-bmikaili force-pushed the bmikaili-SNOW-1492090-snowpipe-streaming-file-master-key-id-rotation branch from 7c8f5ed to 7147d15 Compare August 15, 2024 14:09
@sfc-gh-bmikaili sfc-gh-bmikaili force-pushed the bmikaili-SNOW-1492090-snowpipe-streaming-file-master-key-id-rotation branch from 7147d15 to 77e4da5 Compare August 15, 2024 14:57
@sfc-gh-bmikaili sfc-gh-bmikaili force-pushed the bmikaili-SNOW-1492090-snowpipe-streaming-file-master-key-id-rotation branch from 42a23ca to 200034b Compare August 15, 2024 16:00
Copy link
Contributor

@sfc-gh-lsembera sfc-gh-lsembera left a comment

Choose a reason for hiding this comment

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

Thanks, @sfc-gh-bmikaili , as we discuss IRL, let's keep the old code path around, and only trigger the new code path when the encryption keys array from the server is not-null.

@sfc-gh-bmikaili sfc-gh-bmikaili force-pushed the bmikaili-SNOW-1492090-snowpipe-streaming-file-master-key-id-rotation branch from f74b749 to 34968af Compare September 4, 2024 19:46
@sfc-gh-xhuang
Copy link
Contributor

@sfc-gh-bmikaili to resolve merge conflicts
@sfc-gh-lsembera to review again so this can be merged

@sfc-gh-lsembera
Copy link
Contributor

@sfc-gh-tzhang @sfc-gh-hmadan @sfc-gh-alhuang could you review this PR, please?

@sfc-gh-lsembera
Copy link
Contributor

cc @sfc-gh-azagrebin

@sfc-gh-lsembera
Copy link
Contributor

Long running tests succeeded, with one exception that failed due to flaky unrelated test.

}

public String getFullyQualifiedName() {
return String.format("%s.%s.%s", databaseName, schemaName, tableName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be replace with Utils.getFullyQualifiedTableName(databaseName, schemaName, tableName).

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

@@ -404,6 +409,17 @@ public SnowflakeStreamingIngestChannelInternal<?> openChannel(OpenChannelRequest
this.storageManager.registerTable(
new TableRef(response.getDBName(), response.getSchemaName(), response.getTableName()));

// Add encryption key to the client map for the table
this.encryptionKeysPerTable.put(
Copy link
Contributor

Choose a reason for hiding this comment

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

This might not be necessary? getOrDefault with the same default value as here is called in BlobBuilder.java line 90.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, removed.

@sfc-gh-lsembera sfc-gh-lsembera merged commit d8cdedf into master Nov 8, 2024
47 checks passed
@sfc-gh-lsembera sfc-gh-lsembera deleted the bmikaili-SNOW-1492090-snowpipe-streaming-file-master-key-id-rotation branch November 8, 2024 14:04
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.

4 participants