hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Xiao Chen (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (HDFS-10899) Add functionality to re-encrypt EDEKs
Date Tue, 22 Aug 2017 08:05:00 GMT

    [ https://issues.apache.org/jira/browse/HDFS-10899?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16136455#comment-16136455
] 

Xiao Chen edited comment on HDFS-10899 at 8/22/17 8:04 AM:
-----------------------------------------------------------

Thanks a lot for the reviews [~jojochuang], good comments!
Replying one by one, and attaching a patch in the end. Comments that's not mentioned are all
addressed.

{quote}
reencryptionHandler#reencryptEncryptionZone()
zoneId is obtained when holding FSDirectory read lock, release the lock, and then acquire
FSDirectory read lock again.
This assertion is only correct if there will be only one ReencryptionHandler running.
{quote}
There is only one {{ReencryptionHandler}}. Added texts to javadoc.
If the zone referred to by inodeid is changed (e.g. deleted/renamed) while the lock is not
held, {{checkZoneReady}} will throw. A similar test case would be {{TestReencryption#testZoneDeleteDuringReencrypt}}.

bq. ReencryptionStatus#updateZoneStatus() should check that zoneNode is an encryption zone.
For the 2 callers, {{FSD#addEZ}} is where the zoneId is added, so always true. {{FSDirXAttrOp#unprotectedSetXAttrs}}
is happening within the EZXattr, so also always true. (There's no 'disable encryption' command,
so zone node can only be deleted/renamed)

bq. Why is currentBatch a TreeMap?
Good question. Initially this was done to keep the element's ordering and using path as the
key. Now that it's changed to inode id based, we can just use a list. (Sorry didn't rebase
the inodeid patch here on 14...)

bq. Does ZoneReencryptionStatus#getLastProcessedFile return the relative path? or file name
only? or absolute path?
Absolute path - so we can restore in case of fail over.

bq. It Allocates a 2000-element map, copy it over, and then clear the map. That looks suboptimal.
Would it be feasible to wrap TreeMap and make a method that simply assigns the TreeMap reference
to another currentBatch?
Agreed, problem is {{currentBatch}} here is passed in from the very outside of the call stack.
Made it a member variable of {{ReencryptionHandler}} to address this. It's still safe with
the single-threaded handler model, but perhaps harder to read. Please share your thoughts.

bq. EDEKReencryptCallable ... retry ... if reencryptEdeks() returns numFailures > 0, call()
should not return a new ReencryptionTask object.
Initially talking with [~andrew.wang], we wanted to always retry things, so admin can just
fix the error, and continue (or cancel).
But since KMSCP already has the retry logic added by HADOOP-14521, and to trade off for maintainability,
we do not 'double retry' here, and only let KMSCP's retry policy to handle failures.
When -listReencryptionStatus, if numOfFailures > 0, a message is printed to ask admin to
examine failure and re-submit.
Implementation-wise, we still depend on the ReencryptionTask object to pass the failures to
the updater, so need that object. Updater handles failed tasks differently.



was (Author: xiaochen):
Thanks a lot for the reviews [~jojochuang], good comments!
Replying one by one, and attaching a patch in the end. Comments that's not mentioned are all
addressed.

{quote}
reencryptionHandler#reencryptEncryptionZone()
zoneId is obtained when holding FSDirectory read lock, release the lock, and then acquire
FSDirectory read lock again.
This assertion is only correct if there will be only one ReencryptionHandler running.
{quote}
There is only one {{ReencryptionHandler}}. Added texts to javadoc.
If the zone referred to by inodeid is changed (e.g. deleted/renamed) while the lock is not
held, {{checkZoneReady}} will throw. A similar test case would be {{TestReencryption#testZoneDeleteDuringReencrypt}}.

bq. ReencryptionStatus#updateZoneStatus() should check that zoneNode is an encryption zone.
For the 2 callers, {{FSD#addEZ}} is where the zoneId is added, so always true. {{FSDirXAttrOp#unprotectedSetXAttrs}}
is happening within the EZXattr, so also always true. (There's no 'disable encryption' command,
so zone node can only be deleted/renamed)

bq. Why is currentBatch a TreeMap?
Good question. Initially this was done to keep the element's ordering and using path as the
key. Now that it's changed to inode id based, we can just use a list. (Sorry didn't rebase
the inodeid patch here on 14...)

bq. Does ZoneReencryptionStatus#getLastProcessedFile return the relative path? or file name
only? or absolute path?
Absolute path - so we can restore in case of fail over.

bq. It Allocates a 2000-element map, copy it over, and then clear the map. That looks suboptimal.
Would it be feasible to wrap TreeMap and make a method that simply assigns the TreeMap reference
to another currentBatch?
Agreed, problem is {{currentBatch}} here is passed in from the very outside of the call stack.
Made it a member variable of {{ReencryptionHandler}} to address this. It's still safe with
the single-threaded handler model, but perhaps harder to read. Please share your thoughts.

bq. EDEKReencryptCallable ... retry ... if reencryptEdeks() returns numFailures > 0, call()
should not return a new ReencryptionTask object.
Initially talking with [~andrew.wang], we wanted to always retry things, so admin can just
fix the error, and continue (or cancel).
But since KMSCP already has the retry logic added by HADOOP-14521, and to trade off for maintainability,
we do not 'double retry' here, and only let KMSCP's retry policy to handle failures.
When -listReencryptionStatus, if numOfFailures > 0, a message is printed to ask admin to
examine failure and re-submit. 

> Add functionality to re-encrypt EDEKs
> -------------------------------------
>
>                 Key: HDFS-10899
>                 URL: https://issues.apache.org/jira/browse/HDFS-10899
>             Project: Hadoop HDFS
>          Issue Type: New Feature
>          Components: encryption, kms
>            Reporter: Xiao Chen
>            Assignee: Xiao Chen
>         Attachments: editsStored, HDFS-10899.01.patch, HDFS-10899.02.patch, HDFS-10899.03.patch,
HDFS-10899.04.patch, HDFS-10899.05.patch, HDFS-10899.06.patch, HDFS-10899.07.patch, HDFS-10899.08.patch,
HDFS-10899.09.patch, HDFS-10899.10.patch, HDFS-10899.10.wip.patch, HDFS-10899.11.patch, HDFS-10899.12.patch,
HDFS-10899.13.patch, HDFS-10899.14.patch, HDFS-10899.15.patch, HDFS-10899.wip.2.patch, HDFS-10899.wip.patch,
Re-encrypt edek design doc.pdf, Re-encrypt edek design doc V2.pdf
>
>
> Currently when an encryption zone (EZ) key is rotated, it only takes effect on new EDEKs.
We should provide a way to re-encrypt EDEKs after the EZ key rotation, for improved security.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-help@hadoop.apache.org


Mime
View raw message