[FLINK-21700][security] Add an option to disable credential retrieval on a secure cluster by zuston · Pull Request #15131 · apache/flink

zuston · 2021-03-10T06:03:37Z

Linked to ISSUE.

Why

I want to support Flink Batch Action on Oozie.
As we know, Oozie will obtain HDFS/HBase delegation token using Hadoop proxy user mechanism before starting Flink submitter cli. If not, Flink will obtain delegation token again, this will cause exception.

Actually, Spark support disable fetching delegation token on Spark client, related Spark doc.

So i think Flink should allow to disable fetching Hadoop delegation token on Yarn.

flinkbot · 2021-03-10T06:05:59Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 3c7129c (Sat Aug 28 11:21:03 UTC 2021)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

Details

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2021-03-10T06:27:15Z

CI report:

3c7129c Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

zuston · 2021-03-11T02:27:46Z

Hi @lirui-apache , @XComp @rmetzger , could you help to review this PR?
Thanks

KarmaGYZ

Thank for the PR. I'm still trying to understand this change. Left some minor comments. Also, you should change the commit message/title following the format "[FLINK-21700][yarn] xxxx".

KarmaGYZ · 2021-03-12T09:31:45Z

                            "A comma-separated list of additional Kerberos-secured Hadoop filesystems Flink is going to access. For example, yarn.security.kerberos.additionalFileSystems=hdfs://namenode2:9002,hdfs://namenode3:9003. The client submitting to YARN needs to have access to these file systems to retrieve the security tokens.");

+    public static final ConfigOption<Boolean> YARN_SECURITY_ENABLED =
+            key("yarn.security.kerberos.fetch.delegationToken.enabled")


We also need to generate the doc with mvn clean package -Dgenerate-config-docs -pl flink-docs -am -nsu -DskipTests -Dcheckstyle.skip

KarmaGYZ · 2021-03-12T09:32:58Z

@@ -1081,13 +1081,22 @@ private ApplicationReport startAppMaster(
        if (UserGroupInformation.isSecurityEnabled()) {
            // set HDFS delegation tokens when security is enabled
            LOG.info("Adding delegation token to the AM container.");


If yarnFetchDelegationTokenEnabled == false, we should not print this log.

I verified it: The log message is still valid since Utils.setTokensFor also sets user-related tokens. We only disable HDFS and HBase when SecurityOptions.KERBEROS_FETCH_DELEGATION_TOKEN is disabled.

KarmaGYZ · 2021-03-12T09:36:57Z

+            final Text alias = new Text(token.getService());
+            LOG.info("Adding user token " + alias + " with " + token);
+            credentials.addToken(alias, token);


Could you help me to understand why we need this and what the different between token.getService() and token.getIdentifier(). Thanks

I found that the identifier is not the only uniq id of the token.
So when getting existed token from UserGroupInformation.getCurrentUser().getTokens and these token may be having the same identifier, this is will cause overwriting tokens by using credentials.addToken(identifier, token);

Acutally, i think we should use service as the alias.

More detailed info has been replied in email to you.

Looks like the token fetching for HBase is also relying on the service name instead of the identifier to add the token. I don't understand your argument, though: In what situation would the identifier not be unique. ...considering that you called the identifier not being the only unique id. Could you elaborate a bit more.

According to digging into Hadoop source code, i found that alias is just as the key as internal Credentials token HashMap.

In what situation would the identifier not be unique

In our production env, i found that identifier is the same in active and standby namenode HDFS delegation token. But i dont find any detailed Hadoop doc to support it.

Are there different delegation tokens for the different namenodes Flink has to deal with? I would have had assumed that there's only a single token for Flink accessing the Yarn cluster. Could you point me to the Hadoop source code you're referring to.

Sorry if it's a bit tedious. I haven't worked with Yarn/Kerberos, yet

Refer to https://issues.apache.org/jira/browse/HDFS-9276.
May not be very relevant.

zuston · 2021-03-12T09:45:04Z

Thank for the PR. I'm still trying to understand this change. Left some minor comments. Also, you should change the commit message/title following the format "[FLINK-21700][yarn] xxxx".

Ok. i will force push it to correct commit log later.

KarmaGYZ · 2021-03-15T01:52:24Z

@zuston Could you squash your PR into one commit? Also the CI failed because of the style check, you could execute mvn com.diffplug.spotless:spotless-maven-plugin:apply to resolve it.

zuston · 2021-03-15T02:27:59Z

@zuston Could you squash your PR into one commit? Also the CI failed because of the style check, you could execute mvn com.diffplug.spotless:spotless-maven-plugin:apply to resolve it.

Done. Thanks

XComp

Hi @zuston and thanks for your contribution.
I'm wondering whether we can test that Flink is behaving in the right way when disabling the newly introduced flag. Right now, no test is covering this specific behavior.

XComp · 2021-03-15T07:46:27Z

+            <td><h5>yarn.security.kerberos.fetch.delegationToken.enabled</h5></td>
+            <td style="word-wrap: break-word;">true</td>
+            <td>Boolean</td>
+            <td>When this is true Flink will fetch HDFS/HBase delegation token injected into AM container.</td>


We might want to elaborate a bit more on what's necessary to do when this flag is disabled.

Why do we have the "kerberos" in the config key? Does it mean the delegation token only be obtained when "kerberos" is enabled?

I think the answer is YES. The delegation token mechanism is indeed an extension of Kerberos.

XComp · 2021-03-15T08:01:30Z

+            final Text alias = new Text(token.getService());
+            LOG.info("Adding user token " + alias + " with " + token);
+            credentials.addToken(alias, token);


Looks like the token fetching for HBase is also relying on the service name instead of the identifier to add the token. I don't understand your argument, though: In what situation would the identifier not be unique. ...considering that you called the identifier not being the only unique id. Could you elaborate a bit more.

zuston · 2021-03-16T01:52:55Z

Hi @zuston and thanks for your contribution.
I'm wondering whether we can test that Flink is behaving in the right way when disabling the newly introduced flag. Right now, no test is covering this specific behavior.

It only works when credentials existed in UGI and no keytab is specified. I think It's hard to test, could you give me some ideas on it? @XComp

XComp · 2021-03-16T09:57:23Z

Hi @zuston and thanks for your contribution.
I'm wondering whether we can test that Flink is behaving in the right way when disabling the newly introduced flag. Right now, no test is covering this specific behavior.

It only works when credentials existed in UGI and no keytab is specified. I think It's hard to test, could you give me some ideas on it? @XComp

That's a good questions. @rmetzger How did we deal with these kind of problems in the past? Was it sufficient enough to have a manual test with the external system to see that it worked properly?

zuston · 2021-03-16T13:34:42Z

Do i need to submit another PR about changing identify to service in credentials?
Actually there is no relationship between this PR and identify changes.
@XComp

XComp · 2021-03-17T07:03:55Z

Do i need to submit another PR about changing identify to service in credentials?
Actually there is no relationship between this PR and identify changes.
@XComp

Putting it in a separate commit within this PR is good enough if we go with this change.

XComp · 2021-03-17T07:05:27Z

Hi @zuston and thanks for your contribution.
I'm wondering whether we can test that Flink is behaving in the right way when disabling the newly introduced flag. Right now, no test is covering this specific behavior.

It only works when credentials existed in UGI and no keytab is specified. I think It's hard to test, could you give me some ideas on it? @XComp

That's a good questions. @rmetzger How did we deal with these kind of problems in the past? Was it sufficient enough to have a manual test with the external system to see that it worked properly?

Could you come up with a manual test which is documented properly in the PR to make it reproducible?

zuston · 2021-03-17T07:10:02Z

Thanks @XComp.
Ok, i will attach a detailed manual test in this PR later.

XComp

Could you visualize how the Oozie -> Flink -> Kerberos -> HDFS/Hbase communication works in contrast to the default one? That might help me understand the change in a better way.

XComp · 2021-03-17T07:08:30Z

-        // for HBase
-        obtainTokenForHBase(credentials, conf);
+
+        if (yarnFetchDelegationEnabled) {


I think it's also worth it to add an info log message here for both cases making it explicit in the logs that the delegation fetching is enabled or disabled.

wangyang0918

Thanks for creating this ticket. After reading some implementation of Spark, I think it is a correct direction to disable fetching the delegation tokens. I left some comments and please have a look.

wangyang0918 · 2021-03-17T12:23:21Z

+            <td><h5>yarn.security.kerberos.fetch.delegationToken.enabled</h5></td>
+            <td style="word-wrap: break-word;">true</td>
+            <td>Boolean</td>
+            <td>When this is true Flink will fetch HDFS/HBase delegation token injected into AM container.</td>


Why do we have the "kerberos" in the config key? Does it mean the delegation token only be obtained when "kerberos" is enabled?

wangyang0918 · 2021-03-17T12:24:29Z

+            ContainerLaunchContext amContainer,
+            List<Path> paths,
+            Configuration conf,
+            boolean yarnFetchDelegationEnabled)


Suggested change

boolean yarnFetchDelegationEnabled)

boolean obtainingDelegationTokens)

wangyang0918 · 2021-03-17T12:25:52Z

-            final Text id = new Text(token.getIdentifier());
-            LOG.info("Adding user token " + id + " with " + token);
-            credentials.addToken(id, token);
+            final Text alias = new Text(token.getService());


I think using the token.getService() makes sense here. However, I would suggest to factor out these changes into a separate commit.

wangyang0918 · 2021-03-17T12:30:35Z

-            List<Path> yarnAccessList =
-                    ConfigUtils.decodeListFromConfig(
-                            configuration, YarnConfigOptions.YARN_ACCESS, Path::new);
+            List<Path> yarnAccessList = new ArrayList<>();


I would suggest to document that yarn.security.kerberos.additionalFileSystems should be unset when obtaining delegation tokens are disabled.

Maybe we could also add a precondition check instead of process it silently.

You mean that when YARN_ACCESS and YARN_SECURITY_ENABLED are existed together, Flink client should fast fail through throwing exception or exit Non-zero code.
Right? @wangyang0918

If we disable obtaining the delegation tokens(HDFS/HBase), then yarn.security.kerberos.additionalFileSystems is expected to be unset. Right?
If this is the case, I think throwing exception here makes sense.

Yes.
Agree with you, i will add precheck.

Done. Please review it.

lirui-apache · 2021-03-18T02:53:29Z

                    .withDescription(
                            "A comma-separated list of additional Kerberos-secured Hadoop filesystems Flink is going to access. For example, yarn.security.kerberos.additionalFileSystems=hdfs://namenode2:9002,hdfs://namenode3:9003. The client submitting to YARN needs to have access to these file systems to retrieve the security tokens.");

+    public static final ConfigOption<Boolean> YARN_SECURITY_ENABLED =


I think whether to fetch delegation token and whether "security is enabled" are different things. So I would suggest rename this option to something like KERBEROS_USE_DELEGATION_TOKEN.

Besides, delegation token is not specific to yarn. I think spark supports delegation token for both yarn and mesos. So I also suggest move this option to SecurityOptions in flink-core, alongside with other kerberos configurations.

Yes. Rename is ok. But KERBEROS_USE_DELEGATION_TOKEN maybe not suitable.
Actually, this option indicates Flink whether to fetch delegation token actively, not no use delegation token.

Hmm, could you please explain in which situation would a user want to fetch delegation tokens but not to use them? Or what a user need to do to really use delegation tokens?

Hadoop delegation token is used to access to HDFS/HBase/Hive service. So when you want to be interactive with HDFS, you will need it.
Two options can be used to access above services.
First is to fetch delegation token using Keytab.
Second is to get it from current ugi, if current ugi has the delegation token which is already existing and can be reused.

Refer to: https://blog.cloudera.com/hadoop-delegation-tokens-explained/

If this option only controls whether we fetch delegation tokens, then maybe we can call it KERBEROS_FETCH_DELEGATION_TOKEN ?

+1. Got it.

+1. Got it.

After rechecking the delegation token usage scope, i think it's only specific to Hadoop and related framework, like Hive/HBase. Right?
If not, can you give me some doc or issue link on Spark mesos delegation token? Thanks.

@lirui-apache

Delegation tokens are usually used for a distributed job to authenticate with a Hadoop-based service like Hive or HBase. But it should be orthogonal to the resource management framework like yarn, mesos, or k8s. Spark doc indicates that delegation tokens are supported for yarn and mesos. And its delegation token implementations (e.g. HadoopDelegationTokenManager, HadoopDelegationTokenProvider) are in spark-core.

Actually other projects may also implement their own delegation token mechanisms like what kafka does. But I guess that exceeds the scope of this ticket.

Done. Please review it.

zuston · 2021-03-25T07:38:12Z

Update commits. Please check it. @XComp @lirui-apache @wangyang0918 @KarmaGYZ

lirui-apache · 2021-03-26T09:56:53Z

+                    .defaultValue(true)
+                    .withDescription(
+                            "Indicates whether to fetch delegation token. If true, Flink will fetch "
+                                    + "HDFS/HBase delegation tokens and inject into AM container."


I think we should have a more generic description for this option, maybe something like "whether to fetch delegation tokens for external services the Flink job needs to contact". We can mention only HDFS and HBase are supported, and only works for flink-yarn at the moment.

Done. Please take a look. Thanks.

lirui-apache · 2021-03-30T03:47:18Z

+                                    + "Only HDFS and HBase are supported, and only works for flink-yarn at the moment. "
+                                    + "If true, Flink will fetch HDFS/HBase delegation tokens and inject into Yarn AM container. "
+                                    + "If false, Flink will assume that the job has delegation tokens and will not fetch them. "
+                                    + "This applies to submission mechanisms like Oozie, which will obtain delegation tokens "


Could you please elaborate what it means by saying "This applies to submission mechanisms like Oozie"? IIUC, Flink can't control how Oozie submits jobs, right?

AFAIU, Oozie schedules Flink job submissions and utilizes Hadoop's ProxyUser feature to obtain an delegation token to access the components (in our case HDFS and Hbase). As far as I understand it, the delegation token request usually triggered by Flink would fail if Apache Oozie handles the delegation tokens as it uses it's own Proxy User that impersonates the actual users.

IMHO, This applies to submission mechanisms like Oozie seems to imply that Oozie also respects this config option. Alternatively, maybe we can say something like: You may want to disable this option, if you rely on submission mechanisms, e.g. Apache Oozie, to handle delegation tokens.

Sorry, the description is not clear.

ou may want to disable this option, if you rely on submission mechanisms, e.g. Apache Oozie, to handle delegation tokens.

This is good.

XComp

Hi @zuston, sorry for the late reply. I didn't have time to look into the delegation token topic till now.

I'm still not comfortable with this change considering that it's not tested. Have you had the chance to check whether it works? Could you provide this manual test in a reproducible fashion (e.g. docker) or is this too much of an effort?

Based on what I read about it, the issue is that Apache Oozie utilizes Apache Hadoop's ProxyUser which impersonates the actual user which has access to the actual data. I still don't understand why the delegation token fetching causes an error. Is it because the Flink job would be still submitted under the "normal" user instead of the Oozie user?

XComp · 2021-04-05T17:43:20Z

+                                    + "Only HDFS and HBase are supported, and only works for flink-yarn at the moment. "
+                                    + "If true, Flink will fetch HDFS/HBase delegation tokens and inject into Yarn AM container. "
+                                    + "If false, Flink will assume that the job has delegation tokens and will not fetch them. "
+                                    + "This applies to submission mechanisms like Oozie, which will obtain delegation tokens "


AFAIU, Oozie schedules Flink job submissions and utilizes Hadoop's ProxyUser feature to obtain an delegation token to access the components (in our case HDFS and Hbase). As far as I understand it, the delegation token request usually triggered by Flink would fail if Apache Oozie handles the delegation tokens as it uses it's own Proxy User that impersonates the actual users.

XComp · 2021-04-05T17:47:02Z

+                            "Indicates whether to fetch delegation token for external services the Flink job needs to contact. "
+                                    + "Only HDFS and HBase are supported, and only works for flink-yarn at the moment. "
+                                    + "If true, Flink will fetch HDFS/HBase delegation tokens and inject into Yarn AM container. "
+                                    + "If false, Flink will assume that the job has delegation tokens and will not fetch them. "
+                                    + "This applies to submission mechanisms like Oozie, which will obtain delegation tokens "


Suggested change

"Indicates whether to fetch delegation token for external services the Flink job needs to contact. "

+ "Only HDFS and HBase are supported, and only works for flink-yarn at the moment. "

+ "If true, Flink will fetch HDFS/HBase delegation tokens and inject into Yarn AM container. "

+ "If false, Flink will assume that the job has delegation tokens and will not fetch them. "

+ "This applies to submission mechanisms like Oozie, which will obtain delegation tokens "

"Indicates whether to fetch the delegation tokens for external services the Flink job needs to contact. "

+ "Only HDFS and HBase are supported. It is used in YARN deployments. "

+ "If true, Flink will fetch HDFS and HBase delegation tokens and inject them into Yarn AM containers. "

+ "If false, Flink will assume that the delegation tokens are managed outside of Flink. As a consequence, it will not fetch delegation tokens for HDFS and HBase."

+ "This applies to submission mechanisms like Apache Oozie which will obtain delegation tokens "

Here's a proposal to make the docs more explicit.

It looks like the documentation has not updated, yet.

XComp · 2021-04-05T17:48:41Z

-        // for HBase
-        obtainTokenForHBase(credentials, conf);
+
+        if (obtainingDelegationTokens) {


Would it make sense to have separate parameters for each of the services similar to how Spark is implementing this feature? That's just an idea I came up with after browsing the Spark sources where it's separated like that.

Maybe it should submit another issue to separate it.

I guess it's not necessary for now considering that we don't have a use-case where we want to disable them individually. So, I'm fine with keeping it like that considering that it makes configuration easier.

XComp · 2021-04-05T19:46:57Z

+            boolean yarnAccessFSEnabled =
+                    flinkConfiguration.get(YarnConfigOptions.YARN_ACCESS) != null;
+            if (!kerberosFetchDTEnabled && yarnAccessFSEnabled) {
+                throw new RuntimeException(


Wouldn't be a warning sufficient enough? The YARN_ACCESS setting does not harm the system. It would be just ignored if delegation tokens are disabled.

YARN_ACCESS and KERBEROS_FETCH_DELEGATION_TOKEN are exclusive, it's better to fast fail.
Because if it’s ignored, it will make users confused whether YARN_ACCESS really works.

Refer to: https://github.com/apache/flink/pull/15131/files/0500d84f78d8fdb967b077fb024fda0cd4ba2d6a#r595970681

True, I missed the thread. Thanks for pointing to it.

XComp · 2021-04-05T20:00:09Z

+            Boolean kerberosFetchDelegationTokenEnabled =
+                    configuration.getBoolean(SecurityOptions.KERBEROS_FETCH_DELEGATION_TOKEN);
+
+            if (kerberosFetchDelegationTokenEnabled) {
+                // set HDFS delegation tokens when security is enabled
+                LOG.info("Adding delegation token to the AM container.");
+                yarnAccessList =
+                        ConfigUtils.decodeListFromConfig(
+                                configuration, YarnConfigOptions.YARN_ACCESS, Path::new);
+            }
+
            Utils.setTokensFor(
                    amContainer,
                    ListUtils.union(yarnAccessList, fileUploader.getRemotePaths()),
-                    yarnConfiguration);
+                    yarnConfiguration,
+                    kerberosFetchDelegationTokenEnabled);


I'm wondering whether this change is actually necessary? Is there a specific reason for it? Aren't we implicitly ignoring the YARN_ACCESS setting in Utils.setTokensFor? Or am I missing something?

YARN_ACCESS and KERBEROS_FETCH_DELEGATION_TOKEN are exclusive.

It's kerberos mechanism.
Please check spark related doc: https://spark.apache.org/docs/latest/running-on-yarn.html#launching-your-application-with-apache-oozie

zuston · 2021-04-13T08:42:54Z

@XComp Thanks for your reply

I'm still not comfortable with this change considering that it's not tested. Have you had the chance to check whether it works? Could you provide this manual test in a reproducible fashion (e.g. docker) or is this too much of an effort?

We have applied and tested this PR in our production. It looks fine.

Sorry, I am not very familiar with the Flink code. Can you provide a test case link about with kerberos HDFS? I want to refer it to add test case.

How to reproduce it?
I think you can fetch HDFS delegation token before submitting Flink job on Yarn, it will throw exception.

Based on what I read about it, the issue is that Apache Oozie utilizes Apache Hadoop's ProxyUser which impersonates the actual user which has access to the actual data. I still don't understand why the delegation token fetching causes an error. Is it because the Flink job would be still submitted under the "normal" user instead of the Oozie user?

No. Oozie will submit Flink job without keytab and only rely on delegation token to access HDFS.
So when using Oozie to submit job, flink fetching delegation token without keytab by itself will throw exception.

And why cause these exception?
Limited by kerberos mechanism.

Actually there is no need for Flink to fetch token, Flink can use the token Oozie has fetched directly.

XComp · 2021-04-28T06:30:45Z

@XComp Thanks for your reply.
Docs is updated and latest commit is uploaded. And this commit has made the following changes compared to the last one.

Update the security.kerberos.fetch.delegation-token conf description

Directly use token.getService as credential alias, instead of creating a new object, like Text alias = new Text(token.getService).

Rename some variables.

Some minor changes

Thanks for the update @zuston . I'm just wondering why you squashed the two commits into one again. Considering that the two changes (service name as alias and new parameter) are independent of each other I would favor @wangyang0918 's proposal of keeping the two commits separate.

zuston · 2021-04-28T07:19:20Z

@XComp Thanks for your reply.
Done.

zuston · 2021-04-29T08:45:16Z

Gentle ping @XComp

XComp

Thanks for recreating the split between the two commits. Two things slipped through under my radar in the last review which needs to be addressed still.

XComp · 2021-04-29T09:41:56Z

+                throw new IllegalConfigurationException(
+                        "When security.kerberos.fetch.delegation-token is set, "
+                                + "yarn.security.kerberos.additionalFileSystems must be unset.");


Suggested change

throw new IllegalConfigurationException(

"When security.kerberos.fetch.delegation-token is set, "

+ "yarn.security.kerberos.additionalFileSystems must be unset.");

throw new IllegalConfigurationException(

String.format(

"When %s is disabled, %s must be disabled as well.",

SecurityOptions.KERBEROS_FETCH_DELEGATION_TOKEN.key(),

YarnConfigOptions.YARN_ACCESS.key()));

The message was wrong, wasn't it? Additionally, it's better to use the references to the parameters instead of plain text.

XComp · 2021-04-29T09:53:54Z

@@ -1081,13 +1081,22 @@ private ApplicationReport startAppMaster(
        if (UserGroupInformation.isSecurityEnabled()) {
            // set HDFS delegation tokens when security is enabled
            LOG.info("Adding delegation token to the AM container.");


I verified it: The log message is still valid since Utils.setTokensFor also sets user-related tokens. We only disable HDFS and HBase when SecurityOptions.KERBEROS_FETCH_DELEGATION_TOKEN is disabled.

XComp · 2021-04-29T09:57:46Z

-                    amContainer,
-                    ListUtils.union(yarnAccessList, fileUploader.getRemotePaths()),
-                    yarnConfiguration);
+            List<Path> pathsToObtainToken = null;


Suggested change

List<Path> pathsToObtainToken = null;

List<Path> pathsToObtainToken = Collections.emptyList();

I'm not that comfortable to pass null into Utils.setTokensFor as it's not a @Nullable parameter. It does not harm the execution because it's only used in Utils.setTokensFor if SecurityOptions.KERBEROS_FETCH_DELEGATION_TOKEN is enabled (and in that case we overwrite pathsToObtainToken). But still, I would vote for switching to an empty list instead.

zuston · 2021-04-29T12:10:03Z

@XComp Thanks~. All resolved

XComp

Thanks for responding so quickly, @zuston. I did some code analysis on the getIdentifier/getService issue which makes me wonder whether we should do it in a dedicated ticket instead of sneaking it in here as part of FLINK-21700. This solution does not require the change from getIdentifier to getService, does it?
See my comments below.

XComp · 2021-04-29T12:44:29Z

-            final Text id = new Text(token.getIdentifier());
-            LOG.info("Adding user token " + id + " with " + token);
-            credentials.addToken(id, token);
+            LOG.info("Adding user token " + token.getService() + " with " + token);


Iterating over it once more, I realized that HadoopModule uses the identifier as well. In contrast, the HBase delegation token always used getService. Do we have to align that? Moreover, I'm wondering whether we should make this a dedicated ticket to align it as it's not necessary for this feature. That would make this issue more transparent. Do you have any objections against that, @zuston ?

Can't agree more. It will make more clear when seperating PR.
I will submit new one and we could talk more details on it.

IIUC, we will move the second commit to a separate ticket. Right?

XComp · 2021-04-29T12:45:56Z

+                                    + "If false, Flink will assume that the delegation tokens are managed outside of Flink. "
+                                    + "As a consequence, it will not fetch delegation tokens for HDFS and HBase. "
+                                    + "You may need to disable this option, if you rely on submission mechanisms, e.g. Apache Oozie, "
+                                    + "to handle delegation tokens. ");


Suggested change

+ "to handle delegation tokens. ");

+ "to handle delegation tokens.");

a minor thing

XComp · 2021-04-29T13:09:07Z

+            TokenCache.obtainTokensForNamenodes(credentials, paths.toArray(new Path[0]), conf);
+            // for HBase
+            obtainTokenForHBase(credentials, conf);
+        }


Have we discussed adding an else branch containing an info log message pointing out that delegation token retrieval for HDFS and HBase is disabled?

Sorry, i missed.
So message like: LOG.info("Delegation token retrieval for HDFS and HBase is disabled.");

zuston · 2021-04-29T13:46:21Z

@XComp. So i need to remove the token alias commit? Right?
And i will submit new one to solve it. link: #15810

zuston · 2021-04-29T13:52:31Z

@XComp Done.

XComp

Thanks for splitting it up, @zuston . I mentioned one minor issue below. But other than that, I guess, we can finalize it. @wangyang0918 would you be fine with confirming this PR?

zuston · 2021-04-30T11:48:15Z

@XComp Thanks for your review. All done.

zuston · 2021-05-06T09:55:59Z

@XComp Gently ping. Could you help merge it?

XComp · 2021-05-06T13:11:02Z

Sure, I will initiate it as soon as @wangyang0918 gave it another pass. He has it on his ToDo list and will do it soon'ish

wangyang0918

@zuston Thanks for updating this PR. It looks really good to me. I just left some minor comments. Please have a look.

BTW, we have unrelated documentation changes in the second commit. And if we agree to move the second commit to a separate ticket, then this PR needs to be refined again.

wangyang0918 · 2021-05-07T03:37:53Z

                                + "does not have Kerberos credentials or delegation tokens!");
            }
+
+            boolean fetchToken =


nit: fetchToken and yarnAccessFSEnabled could be final.

wangyang0918 · 2021-05-07T03:41:44Z

+
+            boolean fetchToken =
+                    flinkConfiguration.getBoolean(SecurityOptions.KERBEROS_FETCH_DELEGATION_TOKEN);
+            boolean yarnAccessFSEnabled =


We could use CollectionUtil.isNullOrEmpty to check whether YARN_ACCESS is not set.

wangyang0918 · 2021-05-07T03:47:08Z

-                    amContainer,
-                    ListUtils.union(yarnAccessList, fileUploader.getRemotePaths()),
-                    yarnConfiguration);
+            List<Path> pathsToObtainToken = Collections.emptyList();


nit: I would like to create a final list and add them explicitly.

This could avoid pathsToObtainToken is changed unexpectedly. And also the IDE complains the Uncheck assignment when using ListUtils.union.

final List<Path> pathsToObtainToken = new ArrayList<>(); ... pathsToObtainToken.addAll(yarnAccessList); pathsToObtainToken.addAll(fileUploader.getRemotePaths());

wangyang0918 · 2021-05-07T03:51:42Z

-            final Text id = new Text(token.getIdentifier());
-            LOG.info("Adding user token " + id + " with " + token);
-            credentials.addToken(id, token);
+            LOG.info("Adding user token " + token.getService() + " with " + token);


IIUC, we will move the second commit to a separate ticket. Right?

… on a secure cluster

zuston · 2021-05-07T05:45:28Z

@wangyang0918 All done. Thanks

wangyang0918

+1 for merging.

XComp · 2021-05-07T15:08:13Z

Thanks @wangyang0918 for giving it another pass. @rmetzger may you take over and merge the PR, please?

XComp · 2021-05-10T14:10:56Z

@knaufk

knaufk · 2021-05-12T07:08:49Z

Thanks for the contribution and the reviews. Merging.

rmetzger added the review=description? label Mar 10, 2021

rmetzger added the component=Deployment/YARN label Mar 10, 2021

zuston closed this Mar 11, 2021

zuston reopened this Mar 11, 2021

KarmaGYZ requested changes Mar 12, 2021

View reviewed changes

zuston force-pushed the lmaster branch from 5df56aa to 808fe7e Compare March 15, 2021 02:26

XComp requested changes Mar 15, 2021

View reviewed changes

XComp requested changes Mar 17, 2021

View reviewed changes

wangyang0918 requested changes Mar 17, 2021

View reviewed changes

lirui-apache reviewed Mar 18, 2021

View reviewed changes

zuston force-pushed the lmaster branch from 808fe7e to 3e3702a Compare March 25, 2021 07:35

lirui-apache reviewed Mar 26, 2021

View reviewed changes

zuston force-pushed the lmaster branch from 3e3702a to 0500d84 Compare March 29, 2021 11:50

KarmaGYZ approved these changes Mar 30, 2021

View reviewed changes

lirui-apache reviewed Mar 30, 2021

View reviewed changes

XComp requested changes Apr 5, 2021

View reviewed changes

zuston changed the title ~~[FLINK-21700][yarn]Allow to disable fetching Hadoop delegation token on Yarn~~ [FLINK-21700][security] Add an option to disable credential retrieval on a secure cluster Apr 27, 2021

zuston force-pushed the lmaster branch 3 times, most recently from 6988932 to c5b8d96 Compare April 27, 2021 11:03

zuston force-pushed the lmaster branch from c5b8d96 to d0b3fcc Compare April 28, 2021 07:17

XComp requested changes Apr 29, 2021

View reviewed changes

zuston force-pushed the lmaster branch from d0b3fcc to e024401 Compare April 29, 2021 12:07

XComp requested changes Apr 29, 2021

View reviewed changes

zuston force-pushed the lmaster branch from e024401 to 60812bb Compare April 29, 2021 13:51

XComp approved these changes Apr 30, 2021

View reviewed changes

Comment thread flink-core/src/main/java/org/apache/flink/configuration/SecurityOptions.java Outdated

wangyang0918 reviewed May 7, 2021

View reviewed changes

[FLINK-21700][security] Add an option to disable credential retrieval…

3c7129c

… on a secure cluster

zuston force-pushed the lmaster branch from 8cb12de to 3c7129c Compare May 7, 2021 05:43

wangyang0918 approved these changes May 7, 2021

View reviewed changes

zuston mentioned this pull request May 12, 2021

[FLINK-22534] Set delegation token's service name as credential alias #15810

Closed

knaufk merged commit 3a1b2e1 into apache:master May 12, 2021

	boolean yarnFetchDelegationEnabled)
	boolean obtainingDelegationTokens)

	List<Path> pathsToObtainToken = null;
	List<Path> pathsToObtainToken = Collections.emptyList();

	+ "to handle delegation tokens. ");
	+ "to handle delegation tokens.");

Uh oh!

Conversation

zuston commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Uh oh!

flinkbot commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automated Checks

Review Progress

Uh oh!

flinkbot commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

zuston commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KarmaGYZ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuston Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuston Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuston commented Mar 12, 2021

Uh oh!

KarmaGYZ commented Mar 15, 2021

Uh oh!

zuston commented Mar 15, 2021

Uh oh!

XComp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuston commented Mar 16, 2021

Uh oh!

XComp commented Mar 16, 2021

Uh oh!

zuston commented Mar 16, 2021

Uh oh!

XComp commented Mar 17, 2021

Uh oh!

XComp commented Mar 17, 2021

Uh oh!

zuston commented Mar 17, 2021

Uh oh!

zuston commented Mar 10, 2021 •

edited

Loading

flinkbot commented Mar 10, 2021 •

edited

Loading

flinkbot commented Mar 10, 2021 •

edited

Loading

zuston commented Mar 11, 2021 •

edited

Loading

zuston Mar 16, 2021 •

edited

Loading

zuston Mar 16, 2021 •

edited

Loading