Fix Kerberos ticket refresh #16680

hashhar · 2023-03-23T06:09:17Z

The Hadoop UGI class handles ticket refresh only if the Subject is not provided externally. For external Subject UGI expects the refresh will be handled by the creator of the Subject which in our case we did not do.

Because of this before this change any Trino query which ran longer than the ticket_lifetime failed with errors like

GSS initiate failed [Caused by GSSException: No valid credentials
provided (Mechanism level: Failed to find any Kerberos tgt)].

In Hadoop code the UGI instance also gets re-used in some places (e.g. DFSClient) which means we cannot just create a new UGI with refreshed credentials and return that since other parts of code will keep using the old UGI with expired credentials. So the fix is to create a new UGI, extract the credentials from it and update the existing UGI's credentials with them so that all users of the existing UGI also observe the new valid credentials.

Release notes

(x) Release notes are required, with the following suggested text:

# Hive
* Fix possible query failure with a Kerberized Hive connector when the query executes longer than the Kerberos ticket lifetime. ({issue}`16680`)

kokosing · 2023-03-23T09:34:37Z

Is it still draft?

hashhar · 2023-03-23T09:36:47Z

draft only because I want to remember to remove the stress test commit

kokosing

looks good

...sts-launcher/src/main/resources/docker/presto-product-tests/common/hadoop-kerberos/krb5.conf

hashhar · 2023-03-24T12:52:38Z

Added a better test, it fails without the fix and passes only with the fix applied.

...duct-tests/src/main/java/io/trino/tests/product/hive/TestHiveConnectorKerberosSmokeTest.java

hashhar · 2023-03-26T08:56:54Z

...sts-launcher/src/main/resources/docker/presto-product-tests/common/hadoop-kerberos/krb5.conf

+ forwardable = true
+ allow_weak_crypto = true
+ # low ticket_lifetime to make sure refresh code in Trino is tested
+ ticket_lifetime = 30s


Note that I had to remove renew_lifetime from here to avoid hitting #11251 (comment).

We can add it back if we have trinodb/docker-images#162 but I don't see any reason to have it since our code doesn't care whether tickets are renewable or not since it always performs a "relogin" instead of renewal.

reminder to self, trinodb/docker-images#162 is moving forward so this comment can soon be addressed.

hashhar · 2023-03-27T04:49:23Z

@electrum @Praveen2112 this is ready for another round of review, I've simplified the test and fixed the failures due to having renew_lifetime in the krb5.conf.

...roduct-tests-launcher/src/main/java/io/trino/tests/product/launcher/suite/suites/Suite2.java

...duct-tests/src/main/java/io/trino/tests/product/hive/TestHiveConnectorKerberosSmokeTest.java

Praveen2112 · 2023-03-27T05:50:18Z

...duct-tests/src/main/java/io/trino/tests/product/hive/TestHiveConnectorKerberosSmokeTest.java

+        onTrino().executeQuery("SET SESSION scale_writers = false");
+        onTrino().executeQuery("SET SESSION task_scale_writers_enabled = false");


Will this session be reflected across all the usage of onTrino

When onTrino is closed it closes the connection it was using thus resetting session properties.

I'll need to verify whether the session is shared even across test classes.

cc: @findepi if he knows the answer. I also see lots of other tests use SET SESSION directly without resetting or any form of isolation.

So do we close the connection once the a test is completed ?

Actually yes, anything we get from TestContext (e.g. testContext().getDependency(QueryExecutor.class, config)) gets closed after each test. This is done by tempto, see https://github.com/trinodb/tempto/blob/a2f7ef3b914db2aeb3480cd845929b1ab26103ef/tempto-core/src/main/java/io/trino/tempto/internal/context/GuiceTestContext.java#L135

Thanks @kokosing for explaining how this magic works.

When onTrino is closed it closes the connection it was using thus resetting session properties.

yes

I'll need to verify whether the session is shared even across test classes.

product tests are single-threaded

(i think the test class context holds the query executor, so they likely wouldn't be shared anyway)

The Hadoop UGI class handles ticket refresh only if the Subject is not provided externally. For external Subject UGI expects the refresh will be handled by the creator of the Subject which in our case we did not do. Because of this before this change any Trino query which ran longer than the ticket_lifetime failed with errors like GSS initiate failed [Caused by GSSException: No valid credentials provided (Mechanism level: Failed to find any Kerberos tgt)]. In Hadoop code the UGI instance also gets re-used in some places (e.g. DFSClient) which means we cannot just create a new UGI with refreshed credentials and return that since other parts of code will keep using the old UGI with expired credentials. So the fix is to create a new UGI, extract the credentials from it and update the existing UGI's credentials with them so that all users of the existing UGI also observe the new valid credentials.

hashhar · 2023-03-27T10:09:40Z

AC @Praveen2112. PTAL again.

Praveen2112

Should the commit message mention re-login as we don't exactly do a refresh but an explicit login

hashhar · 2023-03-27T10:22:46Z

refresh != renewal. I kept refresh because existing code uses getNextRefreshTime for example.

I do agree however that relogin makes more sense. In a follow-up I can rename the methods as well to use relogin to clarify what the code actually does. Would that be fine?

Praveen2112 · 2023-03-27T10:23:57Z

Yeah

kokosing · 2023-03-27T10:24:50Z

...duct-tests/src/main/java/io/trino/tests/product/hive/TestHiveConnectorKerberosSmokeTest.java

+
+        // 2x of ticket_lifetime as configured in hadoop-kerberos krb5.conf, sufficient to cause at-least 1 ticket expiry
+        SECONDS.sleep(60L);
+        cancelQueryIfRunning(sql);


do we need to cancel the query? Why not simply wait?

It runs very very long. See some reasons to not wait in #16680 (comment)

In short because we can't very reliably control how long a query runs. If query finishes quickly then refresh won't get tested, if it runs too long it adds time on CI + possibility of timeout.

hashhar added the tests:all Run all tests label Mar 23, 2023

cla-bot bot added the cla-signed label Mar 23, 2023

hashhar requested review from Praveen2112 and kokosing March 23, 2023 09:29

kokosing requested a review from electrum March 23, 2023 09:35

hashhar marked this pull request as ready for review March 23, 2023 09:36

kokosing reviewed Mar 23, 2023

View reviewed changes

Praveen2112 reviewed Mar 23, 2023

View reviewed changes

...sts-launcher/src/main/resources/docker/presto-product-tests/common/hadoop-kerberos/krb5.conf Outdated Show resolved Hide resolved

hashhar force-pushed the hashhar/kerberos-renewal-fix branch from 2c01e00 to e708bab Compare March 23, 2023 10:15

hashhar commented Mar 24, 2023

View reviewed changes

...duct-tests/src/main/java/io/trino/tests/product/hive/TestHiveConnectorKerberosSmokeTest.java Outdated Show resolved Hide resolved

github-actions bot added the tests:hive label Mar 24, 2023

hashhar force-pushed the hashhar/kerberos-renewal-fix branch from 1b37cdd to 3c55768 Compare March 26, 2023 08:55

hashhar commented Mar 26, 2023

View reviewed changes

hashhar requested a review from Praveen2112 March 26, 2023 08:57

hashhar force-pushed the hashhar/kerberos-renewal-fix branch from 3c55768 to 50057b7 Compare March 26, 2023 08:58

Praveen2112 approved these changes Mar 27, 2023

View reviewed changes

hashhar force-pushed the hashhar/kerberos-renewal-fix branch from 50057b7 to 214bca5 Compare March 27, 2023 10:09

Praveen2112 approved these changes Mar 27, 2023

View reviewed changes

kokosing reviewed Mar 27, 2023

View reviewed changes

kokosing approved these changes Mar 27, 2023

View reviewed changes

empty for ci - seems stuck

c3e5eba

hashhar merged commit f3c721d into trinodb:master Mar 28, 2023

hashhar deleted the hashhar/kerberos-renewal-fix branch March 28, 2023 05:33

hashhar mentioned this pull request Mar 28, 2023

Release notes for 411 #16453

Closed

github-actions bot added this to the 411 milestone Mar 28, 2023

colebow mentioned this pull request Mar 29, 2023

Add Trino 411 release notes #16552

Merged

hashhar mentioned this pull request Sep 13, 2023

Lower KRB ticket lifetime in EnvMultinodeTlsKerberosDelegation #19024

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Kerberos ticket refresh #16680

Fix Kerberos ticket refresh #16680

hashhar commented Mar 23, 2023 •

edited

Loading

kokosing commented Mar 23, 2023

hashhar commented Mar 23, 2023

kokosing left a comment

hashhar commented Mar 24, 2023

hashhar Mar 26, 2023

hashhar Sep 7, 2023 •

edited

Loading

hashhar commented Mar 27, 2023

Praveen2112 Mar 27, 2023

hashhar Mar 27, 2023

Praveen2112 Mar 27, 2023

hashhar Mar 27, 2023

findepi Mar 27, 2023

hashhar commented Mar 27, 2023

Praveen2112 left a comment

hashhar commented Mar 27, 2023

Praveen2112 commented Mar 27, 2023

kokosing Mar 27, 2023

hashhar Mar 27, 2023

hashhar Mar 27, 2023

		onTrino().executeQuery("SET SESSION scale_writers = false");
		onTrino().executeQuery("SET SESSION task_scale_writers_enabled = false");

Fix Kerberos ticket refresh #16680

Fix Kerberos ticket refresh #16680

Conversation

hashhar commented Mar 23, 2023 • edited Loading

Release notes

kokosing commented Mar 23, 2023

hashhar commented Mar 23, 2023

kokosing left a comment

Choose a reason for hiding this comment

hashhar commented Mar 24, 2023

Choose a reason for hiding this comment

hashhar Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

hashhar commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar commented Mar 27, 2023

Praveen2112 left a comment

Choose a reason for hiding this comment

hashhar commented Mar 27, 2023

Praveen2112 commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar commented Mar 23, 2023 •

edited

Loading

hashhar Sep 7, 2023 •

edited

Loading