[FLINK-20664][k8s] Support setting service account for TaskManager pod. #14447

blublinsky · 2020-12-21T14:47:33Z

What is the purpose of the change

Currently, we only set the service account for JobManager. The TaskManager is using the default service account. Before the KubernetesHAService is introduced, it works because the TaskManager does not need to access the K8s resource(e.g. ConfigMap) directly. But now the TaskManager needs to watch ConfigMap and retrieve leader address. So if the default service account does not have enough permission, users could not specify a valid service account for TaskManager.

Brief change log

New configuration parameter TASK_MANAGER_SERVICE_ACCOUNT created
Method getServiceAccount added to KubernetesTaskManagerParameters
Service account added to the pod creation in InitTaskManagerDecorator
InitTaskManagerDecoratorTest is extended to incorporate service account test

Verifying this change

This change added tests and can be verified by running unit tests

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no) no
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no) no
The serializers: (yes / no / don't know) no
The runtime per-record code paths (performance sensitive): (yes / no / don't know) no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: (yes / no / don't know) yes
The S3 file system connector: (yes / no / don't know) no

Documentation

Does this pull request introduce a new feature? (yes / no) yes
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented) not documented

flinkbot · 2020-12-21T14:49:33Z

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 d07b609 (Mon Dec 21 14:49:33 UTC 2020)

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.

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 · 2020-12-21T15:04:53Z

CI report:

b1b5c50 Azure: CANCELED
1581e2c Azure: PENDING

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

wangyang0918

@blublinsky Thanks for creating this PR. Generally, it looks good to me. I left some comments and let's check it before merging.

Moreover, I have verified this change in minikube with K8s HA configured. It works well.

...ernetes/src/main/java/org/apache/flink/kubernetes/configuration/KubernetesConfigOptions.java

...est/java/org/apache/flink/kubernetes/kubeclient/decorators/InitTaskManagerDecoratorTest.java

wangyang0918 · 2020-12-22T07:43:38Z

...ernetes/src/main/java/org/apache/flink/kubernetes/configuration/KubernetesConfigOptions.java

@@ -60,6 +60,13 @@
 		.withDescription("Service account that is used by jobmanager within kubernetes cluster. " +
 			"The job manager uses this service account when requesting taskmanager pods from the API server.");

+	public static final ConfigOption<String> TASK_MANAGER_SERVICE_ACCOUNT =
+		key("kubernetes.taskmanager.service-account")


I believe we also need to update the native-kubernetes.md#RBAC section. At least, we need to tell users they could set the service account for TaskManager via kubernetes.taskmanager.service-account. And it is necessary only Kubernetes HA mode,.

wangyang0918 · 2020-12-22T07:58:37Z

The commit message also needs to be refined. I think it could be [FLINK-20664][k8s] Support setting service account for TaskManager pod.

wangyang0918 · 2020-12-22T12:20:20Z

@tillrohrmann is suggesting to introduce a common config option kubernetes.service-account as per default. @blublinsky would you mind to integrate this suggestion?

blublinsky · 2020-12-22T15:02:02Z

Fixed requested. Not sure about Till's request

wangyang0918 · 2020-12-23T04:43:17Z

@blublinsky I think what till means is to introduce a common config option for service account. It could be kubernetes.service-account. It should only take effect when the jobmanager/taskmanager specific config option is not configured.

Then the KubernetesJobManagerParameters#getServiceAccount() could be like following. Same is for KubernetesTaskManagerParameters.

public String getServiceAccount() {
    return flinkConfig.getOptional(KubernetesConfigOptions.JOB_MANAGER_SERVICE_ACCOUNT)
	.orElse(flinkConfig.getString(KubernetesConfigOptions.SERVICE_ACCOUNT));
}

Moreover, we need a test for such fallback logics.

blublinsky · 2020-12-23T15:00:18Z

Even with this approach the sequence for job manager and task manager should be different. otherwise, if a job manager account is defined it will always take precedence for both

wangyang0918 · 2020-12-24T03:07:35Z

@blublinsky Why the sequence for job manager and task manager should be different? In my opinion, the specific config option for jobmanager/taskmanager should always have the precedence.

blublinsky · 2020-12-24T03:10:04Z

If I want to define tm and jm accounts differently, then sequence definitely matters.

xintongsong · 2020-12-24T03:18:44Z

@blublinsky
I think what @tillrohrmann and @wangyang0918 mean is to have a general config option in addition to the jm/tm specific options.

There are three options in total:

kubernetes.service-account
kubernetes.service-account.jobmanager
kubernetes.service-account.taskmanager

JM uses the account defined by kubernetes.service-account.jobmanager, and if that is not defined fallback to kubernetes.service-account.
TM uses the account defined by kubernetes.service-account.taskmanager, and if that is not defined fallback to kubernetes.service-account.

In this way, a user can choose to either define a common account for JM/TM via kubernetes.service-account, or separate accounts via kubernetes.service-account.jobmanager and kubernetes.service-account.taskmanager.

xintongsong · 2020-12-24T03:22:05Z

You may take a look at how the following config options work as an example.

env.java.opts
env.java.opts.jobmanager
env.java.opts.taskmanager
env.java.opts.historyserver
env.java.opts.client

blublinsky · 2020-12-24T03:22:15Z

Oh, thats not what the code snipet reads. Thats fine

blublinsky · 2020-12-25T00:00:16Z

Introduced a common config option for service account and added unit tests

xintongsong

Thanks @blublinsky for preparing and updating the PR, and thank @wangyang0918 for the review.
Then changes LGTM. I have only a few minor comments, which I'll address myself during merging.

xintongsong · 2020-12-25T02:00:12Z

...ernetes/src/main/java/org/apache/flink/kubernetes/configuration/KubernetesConfigOptions.java

+		.stringType()
+		.defaultValue("default")
+		.withDescription("Service account that is used by jobmanager and taskmanger within kubernetes cluster. " +
+			"if specific JOB_MANAGER_SERVICE_ACCOUNT and/or TASK_MANAGER_SERVICE_ACCOUNT are not defined.");


It might be more user friendly to mention the configuration keys rather than the java field names.

And I believe we should also mention in descriptions of JOB_MANAGER_SERVICE_ACCOUNT and TASK_MANAGER_SERVICE_ACCOUNT that they will by default fallback to KUBERNETES_SERVICE_ACCOUNT.

xintongsong · 2020-12-25T02:46:33Z

...a/org/apache/flink/kubernetes/kubeclient/decorators/InitTaskManagerDecoratorAccountTest.java

+import static org.junit.Assert.assertThat;
+
+/**
+ * General tests for the {@link InitJobManagerDecorator}.


Inconsistent JavaDoc.

xintongsong · 2020-12-25T02:46:51Z

...va/org/apache/flink/kubernetes/kubeclient/decorators/InitJobManagerDecoratorAccountTest.java

+import static org.junit.Assert.assertEquals;
+
+/**
+ * General tests for the {@link InitJobManagerDecorator}.


Inconsistent JavaDoc.

xintongsong · 2020-12-25T02:47:00Z

...va/org/apache/flink/kubernetes/kubeclient/decorators/InitJobManagerDecoratorAccountTest.java

+	private static final String JOB_MANGER_SERVICE_ACCOUNT_NAME = "jm-service-test";
+
+	private Pod resultPod;
+	private Container resultMainContainer;


xintongsong · 2020-12-25T02:47:09Z

...a/org/apache/flink/kubernetes/kubeclient/decorators/InitTaskManagerDecoratorAccountTest.java

+	private static final String TASK_MANAGER_SERVICE_ACCOUNT_NAME = "tm-service-test";
+
+	private Pod resultPod;
+	private Container resultMainContainer;


xintongsong · 2020-12-25T02:54:36Z

...a/org/apache/flink/kubernetes/kubeclient/decorators/InitTaskManagerDecoratorAccountTest.java

+
+	@Test
+	public void testPodServiceAccountName() {
+		assertThat(TASK_MANAGER_SERVICE_ACCOUNT_NAME, is(this.resultPod.getSpec().getServiceAccountName()));


Suggested change

assertThat(TASK_MANAGER_SERVICE_ACCOUNT_NAME, is(this.resultPod.getSpec().getServiceAccountName()));

assertThat(this.resultPod.getSpec().getServiceAccountName(), is(TASK_MANAGER_SERVICE_ACCOUNT_NAME));

The util method is defined as follows. Out-of-ordered arguments could lead to improper error messages.

public static <T> void assertThat(T actual, Matcher<? super T> matcher)

xintongsong · 2020-12-25T02:55:27Z

...va/org/apache/flink/kubernetes/kubeclient/decorators/InitJobManagerDecoratorAccountTest.java

+
+	@Test
+	public void testPodServiceAccountName() {
+		assertEquals(JOB_MANGER_SERVICE_ACCOUNT_NAME, this.resultPod.getSpec().getServiceAccountName());


assertThat(actual, is(expected)) is preferred.

wangyang0918

LGTM.

wangyang0918 · 2020-12-25T02:37:41Z

...ernetes/src/main/java/org/apache/flink/kubernetes/configuration/KubernetesConfigOptions.java

+		key("kubernetes.service-account")
+		.stringType()
+		.defaultValue("default")
+		.withDescription("Service account that is used by jobmanager and taskmanger within kubernetes cluster. " +


typo taskmanger -> taskmanager

wangyang0918 · 2020-12-25T03:11:48Z

...ernetes/src/main/java/org/apache/flink/kubernetes/configuration/KubernetesConfigOptions.java

+			.defaultValue("default")
+			.withDescription("Service account that is used by jobmanager and taskmanger within kubernetes cluster. " +
+				"Notice that this can be overwritten by config options '" + JOB_MANAGER_SERVICE_ACCOUNT.key() +
+				"' and '" + TASK_MANAGER_SERVICE_ACCOUNT.key() + "' for  jobmanager and taskmanager respectively.");


Suggested change

"' and '" + TASK_MANAGER_SERVICE_ACCOUNT.key() + "' for jobmanager and taskmanager respectively.");

"' and '" + TASK_MANAGER_SERVICE_ACCOUNT.key() + "' for jobmanager and taskmanager respectively.");

A useless space here.

wangyang0918 · 2020-12-25T03:13:34Z

...est/java/org/apache/flink/kubernetes/kubeclient/decorators/InitTaskManagerDecoratorTest.java

@@ -187,6 +189,11 @@ public void testPodAnnotations() {
 		assertThat(resultAnnotations, is(equalTo(ANNOTATIONS)));
 	}

+	@Test
+	public void testPodServiceAccountName() {
+		assertThat(SERVICE_ACCOUNT_NAME, is(this.resultPod.getSpec().getServiceAccountName()));


Suggested change

assertThat(SERVICE_ACCOUNT_NAME, is(this.resultPod.getSpec().getServiceAccountName()));

assertThat(this.resultPod.getSpec().getServiceAccountName(), is(SERVICE_ACCOUNT_NAME));

blublinsky · 2020-12-25T03:27:20Z

Thanks guys, can we do the same with other PRs?

xintongsong · 2020-12-25T03:31:23Z

@blublinsky
Which other PRs do you mean?

blublinsky · 2020-12-25T03:33:39Z

FLINK-20359,
FLINK-20324,
FLINK-15649

wangyang0918 · 2020-12-25T03:45:09Z

@blublinsky For FLINK-20359, I will have a look in this week. And the other two tickets, let's do the pod template first.

xintongsong · 2020-12-25T03:52:24Z

FLINK-20359,
FLINK-20324,
FLINK-15649

@blublinsky

I'm sure we can continue with the discussions and code reviews for these issues, in the corresponding JIRA tickets and pull requests.

I also noticed that, for the three PRs, non of the corresponding JIRA tickets are assigned. I'd like to kindly remind that, the Apache Flink community suggests to reaching consensus on the approach and getting assigned to the ticket by a committer before start working on the implementation. Please take a close look at How to Contribute Code.

…ment This closes #14447

…ment This closes apache#14447

rmetzger added the review=description? label Dec 21, 2020

rmetzger added the component=Deployment/Kubernetes label Dec 21, 2020

wangyang0918 requested changes Dec 22, 2020

View reviewed changes

blublinsky changed the title ~~[FLINK-20664][k8s] Support setting service account for TaskManager pod~~ [FLINK-20664][k8s] Support setting service account for TaskManager pod. Dec 22, 2020

xintongsong approved these changes Dec 25, 2020

View reviewed changes

[FLINK-20664][k8s] Support setting service account for TaskManager pod.

0efb1f9

wangyang0918 approved these changes Dec 25, 2020

View reviewed changes

xintongsong added 2 commits December 25, 2020 11:21

[follow-up] update config option descriptions

77373d9

[follow-up] update tests

9de69b3

[hotfix][doc] Update RBAC section in native kubernetes document

1581e2c

xintongsong closed this in 4e5448c Dec 25, 2020

xintongsong pushed a commit that referenced this pull request Dec 25, 2020

[FLINK-20664][k8s][doc] Update RBAC section in native kubernetes docu…

d3ec48b

…ment This closes #14447

meijies pushed a commit to meijies/flink that referenced this pull request Dec 28, 2020

[FLINK-20664][k8s][doc] Update RBAC section in native kubernetes docu…

9244b03

…ment This closes apache#14447

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-20664][k8s] Support setting service account for TaskManager pod. #14447

[FLINK-20664][k8s] Support setting service account for TaskManager pod. #14447

blublinsky commented Dec 21, 2020

flinkbot commented Dec 21, 2020

flinkbot commented Dec 21, 2020 •

edited

Loading

wangyang0918 left a comment

wangyang0918 Dec 22, 2020

wangyang0918 commented Dec 22, 2020

wangyang0918 commented Dec 22, 2020

blublinsky commented Dec 22, 2020

wangyang0918 commented Dec 23, 2020

blublinsky commented Dec 23, 2020

wangyang0918 commented Dec 24, 2020

blublinsky commented Dec 24, 2020

xintongsong commented Dec 24, 2020

xintongsong commented Dec 24, 2020

blublinsky commented Dec 24, 2020

blublinsky commented Dec 25, 2020

xintongsong left a comment

xintongsong Dec 25, 2020

xintongsong Dec 25, 2020

xintongsong Dec 25, 2020

xintongsong Dec 25, 2020

xintongsong Dec 25, 2020

xintongsong Dec 25, 2020

xintongsong Dec 25, 2020

wangyang0918 left a comment

wangyang0918 Dec 25, 2020

wangyang0918 Dec 25, 2020

wangyang0918 Dec 25, 2020

blublinsky commented Dec 25, 2020

xintongsong commented Dec 25, 2020

blublinsky commented Dec 25, 2020

wangyang0918 commented Dec 25, 2020

xintongsong commented Dec 25, 2020 •

edited

Loading

	assertThat(TASK_MANAGER_SERVICE_ACCOUNT_NAME, is(this.resultPod.getSpec().getServiceAccountName()));
	assertThat(this.resultPod.getSpec().getServiceAccountName(), is(TASK_MANAGER_SERVICE_ACCOUNT_NAME));

	"' and '" + TASK_MANAGER_SERVICE_ACCOUNT.key() + "' for jobmanager and taskmanager respectively.");
	"' and '" + TASK_MANAGER_SERVICE_ACCOUNT.key() + "' for jobmanager and taskmanager respectively.");

	assertThat(SERVICE_ACCOUNT_NAME, is(this.resultPod.getSpec().getServiceAccountName()));
	assertThat(this.resultPod.getSpec().getServiceAccountName(), is(SERVICE_ACCOUNT_NAME));

[FLINK-20664][k8s] Support setting service account for TaskManager pod. #14447

[FLINK-20664][k8s] Support setting service account for TaskManager pod. #14447

Conversation

blublinsky commented Dec 21, 2020

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Dec 21, 2020

Automated Checks

Review Progress

flinkbot commented Dec 21, 2020 • edited Loading

CI report:

wangyang0918 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangyang0918 commented Dec 22, 2020

wangyang0918 commented Dec 22, 2020

blublinsky commented Dec 22, 2020

wangyang0918 commented Dec 23, 2020

blublinsky commented Dec 23, 2020

wangyang0918 commented Dec 24, 2020

blublinsky commented Dec 24, 2020

xintongsong commented Dec 24, 2020

xintongsong commented Dec 24, 2020

blublinsky commented Dec 24, 2020

blublinsky commented Dec 25, 2020

xintongsong left a 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

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

wangyang0918 left a 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

blublinsky commented Dec 25, 2020

xintongsong commented Dec 25, 2020

blublinsky commented Dec 25, 2020

wangyang0918 commented Dec 25, 2020

xintongsong commented Dec 25, 2020 • edited Loading

flinkbot commented Dec 21, 2020 •

edited

Loading

xintongsong commented Dec 25, 2020 •

edited

Loading