Sailesh Mukil has posted comments on this change. Change subject: KUDU-1845: Kerberos client keytab should be periodically renewed ......................................................................
Patch Set 10: (11 comments) http://gerrit.cloudera.org:8080/#/c/5820/9/src/kudu/security/init.cc File src/kudu/security/init.cc: PS9, Line 69: : // Acquires a new Ticket Granting Ticket (TGT). > is this still accurate? If this method isn't renewing a ticket, but rather Whoops sorry, I forgot to revert the comment. It does renewal again now. Line 83: // Returns a value based on 'time_remaining' that increases exponentially with > please rename to include units (sleep_interval_secs()) or have it return a Renamed to sleep_interval_secs() PS9, Line 95: > nit: name g_kinit_ctx Done Line 100: > I think non-POD global data is a no-no. Better to use a pointer and explici Done PS9, Line 120: entation > I don't think the d.length check is necessary, since memcmp with 0 length i Done Line 176: > did we determine that the underlying get_init_creds_keytab held the appropr It doesn't use internal locks, but it looks like it retries once if it doesn't go through the first time but that won't be enough, so I'll wrap the lock around it. PS9, Line 197: // This follows the same format as is_local_tgt() from krb5:src/clients/klist/klist.c : if (creds.server->length != 2 || : data_eq(creds.server->data[1], principal_->realm) == 0 || > isn't this already set when we initialized opts_? Oh yea, this is redundant. Removed it. PS9, Line 203: } : : time_t now = time(nullptr); : time_t ticket_expiry = creds.times.endtime; : time_t renew_till = creds.times.renew_till; : time_t renew_deadline = renew_till - 30; : : krb5_creds new_creds; > wrap this in a block so that the log message below isn't holding the lock Done PS9, Line 252: KRB5_RETURN_NOT_OK_PREPEND(krb5_cc_store_cred(krb5_ctx_, ccache_, &new_creds), : "Failed to store credentials in ccache"); : } : LOG(INFO) << "Successfully renewed kerberos TGT"; : } : ticket_end_timestamp_ = new_creds.times.endtime; : break; : } > I'm worried that, since we're just setting the interval to wae up just befo Added a backoff mechanism on failures, and also this is recalculated after every ticket renewal. http://gerrit.cloudera.org:8080/#/c/5820/9/src/kudu/security/init.h File src/kudu/security/init.h: Line 20: > can just use a forward decl Done Line 29: Status InitKerberosForServer(); > I think it's worth documenting this further in the header. Something like: Done -- To view, visit http://gerrit.cloudera.org:8080/5820 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic4c072c1210216369e60eac88be4a20d9b166b2d Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
