Alexey Serbin has posted comments on this change. Change subject: [security] load/store public TSK in the system table ......................................................................
Patch Set 5: (38 comments) http://gerrit.cloudera.org:8080/#/c/5935/5/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: Line 192: if (s.IsNetworkError() || s.IsServiceUnavailable()) { > Probably doesn't belong in this patch, right? I think you put this in a dif Right, this part has been removed from this patch and is committed as a part of some other patch. http://gerrit.cloudera.org:8080/#/c/5935/5/src/kudu/integration-tests/token_signer-itest.cc File src/kudu/integration-tests/token_signer-itest.cc: Line 42: using std::shared_ptr; > warning: using decl 'shared_ptr' is unused [misc-unused-using-decls] Done Line 85: SignedTokenPB MakeToken() { > Declare as static? Done Line 125: gscoped_ptr<MiniCluster> cluster_; > unique_ptr Done Line 141: SleepFor(MonoDelta::FromSeconds(1)); > Why is this needed? Done PS5, Line 145: have > has Done PS5, Line 145: folower > follower Done http://gerrit.cloudera.org:8080/#/c/5935/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 204: DEFINE_int64(catalog_manager_sys_table_propagation_seconds, 60, > Not really sure what this means. This has been updated and moved. Line 208: // TODO(PKI): add flag tags > Indeed. Done Line 211: using std::remove_if; > warning: using decl 'remove_if' is unused [misc-unused-using-decls] Done Line 342: CHECK(token_signer_); > could use CHECK_NOTNULL in the initializer list above. Done Line 345: // The entry_id corresponds to the sequence number of the TSK key. > But we're ignoring the entry_id, so why is this comment useful? removed Line 359: LOG(INFO) << "Imported TSPK " << tspk.key_seq_num() << " from system table"; > This may be too noisy. How many non-expired entries would we typically expe Usually not more than 1K. Agree -- that would be too much. Will change to VLOG(1) instead. Line 365: const set<int64_t>& loaded_key_seq_numbers() const { > We only ever get the first number, so change this method to expose that rat Luckily, this is no longer needed. PS5, Line 371: // Epoch time > What does this mean? I tried to add a description saying key_exp_time_seconds_ is absolute time, in seconds, representing Epoch time. The value is to define cut-off time for TSK entries being loaded: entries with expiration time less than the specified value are not imported into TokenSigner. I fill add this comment into the code. PS5, Line 469: firther > further Done Line 485: // TODO Add tests for this in the revision that makes > warning: missing username/bug in TODO [google-readability-todo] Done PS5, Line 489: firther > further Done Line 497: catalog_manager_->LoadTskEntries(&last_seen_tsk_entry_id); > This runs every second. Isn't that super wasteful? This is removed. Line 500: } while (false); > Why this change? We can have an extra scope (for lock acquisition/release) That's to handle those 'brake' statements when it's found the instance is no longer a leader master. Simply calling 'continue' would not work here -- it's necessary to go to that Wait() directive in the end of the cycle. Using 'goto' was not considered as an option. PS5, Line 3269: There's a bit of subtlety here: > Yeah, this is complicated. I'll take another look at this later; maybe ask yup, this is simplified now. Line 3290: RETURN_NOT_OK(signer->GenerateSigningKey(&next_tsk_)); > Why are we writing to next_tsk_ before persisting, then clearing it on fail This is gone. Line 3296: const Status s = sys_catalog_->AddTskEntries({sys_entry}); > Can we use security::TokenSigningPublicKeyPB in the interface to this funct Done Line 3320: TskEntryLoader loader(signer, WallTime_Now()); > A certain amount of imprecision is OK here, right? Masters' clocks may not That's fine -- the propagation interval and other parameters are supposed to have bigger margins. Line 3322: const set<int64_t> seq_numbers(loader.loaded_key_seq_numbers()); > Could be a cref. Good catch. This has changed, though. http://gerrit.cloudera.org:8080/#/c/5935/5/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: Line 565: //Status LoadTokenSigningInfo(std::vector<SysTskEntryPB>* new_entries); > Why is this commented out? It seems it's no longer needed. Line 627: Status LoadTskEntries(std::string* tsk_entry_id); > warning: function 'kudu::master::CatalogManager::LoadTskEntries' has a defi Done PS5, Line 773: follower master > follower masters this part is gone. PS5, Line 773: pubic > public ugh http://gerrit.cloudera.org:8080/#/c/5935/5/src/kudu/master/master.proto File src/kudu/master/master.proto: Line 200: // The on-disk entry in the sys.catalog table ("metadata" column) to represent > This isn't used (and is commented out); can it be removed? I think it'd be Done Line 204: // be single entry of this type in the sys.catalog table. > Trailing whitespace here. Done http://gerrit.cloudera.org:8080/#/c/5935/5/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: Line 478: // TSK-related methods > This isn't using the same format as the other boundaries, plus it's splitti Done Line 490: void SysCatalogTable::ReqAddTskEntry(WriteRequestPB* req, > Could be inlined into AddTskEntries(), since it's just used in that one pla That's a good idea, thanks. Line 511: void SysCatalogTable::ReqDeleteTskEntry(WriteRequestPB* req, > Not used? It seems this patch is missing the part which removes expired TSK entries from the system table. I'll add that missed part in the next revision. Line 520: CHECK_OK(row.SetStringNoCopy( > Is NoCopy() correct? Doesn't the string from TskSeqNumberToEntryId() go out Good catch -- it should be replaced with SetString(). Line 568: unique_ptr<Arena> range_arena; > Can't reuse the arena below? I was not sure whether it would be legal to do so. Anyway, this part is going away since we are going to read all the TSK entries from the system table during our scans. Line 576: const string key_upper_boundary = key_low_boundary + "."; > Confused by this; if we want to scan from key_low_boundary and up, shouldn' For some reason I didn't see it was possible to pass nullptr as the condition for the range (I assumed it would be possible and tried to look at that). Probably, I ended up looking into some wrong place or something else. Thank you for pointing at this -- at least now I can see that passing nullptr either for upper or lower boundary is acceptable. http://gerrit.cloudera.org:8080/#/c/5935/5/src/kudu/master/sys_catalog.h File src/kudu/master/sys_catalog.h: PS5, Line 148: If the 'low_entry_id' : // is not empty, it is used as the scan's exclusive low boundary for the : // 'entry_id' column. > This feels premature. The number of TSK rows is pretty low, right? Can't we Good idea. Todd is advocating for this simpler approach as well. I'll simplify it, thanks. -- To view, visit http://gerrit.cloudera.org:8080/5935 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie91d4129bda0ca49e81988c28385895a2abcd201 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
