[feat][fn] PIP-257: Support mounting k8s ServiceAccount for OIDC auth

[michaeljmarshall]

PIP: #19771

Motivation

In order to make OIDC work with functions, we must give them a way to authenticate with the broker using tokens that are able to be validated by an using an Authorization Server. This PR introduces the KubernetesServiceAccountAuthProvider.

Modifications

• Create an KubernetesServiceAccountAuthProvider implementation. It adds a service account token volume projection as defined in the k8s docs here. The implementation provides a way to specify the expiration time that the token will receive.

• Instead of creating a secret with the broker's trusted ca.crt in it, this new KubernetesServiceAccountAuthProvider expects a secret to already exist with the ca.crt. The major advantage for this implementation is that when the ca.crt is rotated, we can refresh it (assuming the client is configured to observe the updated file).

• Add support for specifying the token's expiration and audience.

• One point of divergence from the KubernetesSecretsTokenAuthProvider implementation is that I did not provide a way for functions to authenticate as the anonymous role. It seems like a stretch that functions would use such authentication because it will not be multi-tenant. However, if that is a concern, we can add the support.

• The feature will be configured with the following yaml:

functionRuntimeFactoryConfigs:
  kubernetesFunctionAuthProviderConfig:
    brokerClientTrustCertsSecretName: "secret-name"
    serviceAccountTokenExpirationSeconds: 3600
    serviceAccountTokenAudience: "pulsar-cluster-audience"

Verifying this change

I verified the correctness of the code with unit tests. I'll verify the integration with k8s once we've determined this PR's design is correct.

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

This adds new configuration options to the function worker.

Documentation

• doc-required

Matching PR in forked repository

PR in forked repository: michaeljmarshall#36

[eolivelli]

this method is not meant to be called, I suggest to throw an exception here

[michaeljmarshall]

The Pulsar Function Worker does not call this method. We could probably deprecate now and remove it later, if we want. It is used in another implementation here:

pulsar/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/auth/KubernetesFunctionAuthProvider.java

Lines 40 to 45 in 01a4992

	default void initialize(CoreV1Api coreClient, byte[] caBytes,
	java.util.function.Function<Function.FunctionDetails, String> namespaceCustomizerFunc) {
	setCaBytes(caBytes);
	setNamespaceProviderFunc(namespaceCustomizerFunc);
	initialize(coreClient);
	}

From an API design perspective, it seems like we only need the initialize method and could deprecate the setCaBytes and setNamespaceProviderFunc methods as well since they are only called in the initialize method and never outside of the class.

[eolivelli]

is there any expectations about the "format" of the file ?
should we have an entry which describes the format used to create the file ?

[michaeljmarshall]

Great question. First, I think I might have needed to use ca.pem. Would that clear things up?

Second, the current design reads the bytes from the file at this path:

pulsar/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java

Lines 788 to 799 in 9bb3a21

	public byte[] getTlsTrustChainBytes() {
	if (StringUtils.isNotEmpty(getBrokerClientTrustCertsFilePath())
	&& Files.exists(Paths.get(getBrokerClientTrustCertsFilePath()))) {
	try {
	return Files.readAllBytes(Paths.get(getBrokerClientTrustCertsFilePath()));
	} catch (IOException e) {
	throw new IllegalStateException("Failed to read CA bytes", e);
	}
	} else {
	return null;
	}
	}

and that relies on:

pulsar/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java

Lines 615 to 629 in 9bb3a21

	@FieldContext(
	category = CATEGORY_CLIENT_SECURITY,
	doc = "The path to trusted certificates used by the Pulsar client to authenticate with Pulsar brokers"
	)
	private String brokerClientTrustCertsFilePath;
	public String getBrokerClientTrustCertsFilePath() {
	// for compatible, if user do not define brokerClientTrustCertsFilePath, we will use tlsTrustCertsFilePath,
	// otherwise we will use brokerClientTrustCertsFilePath
	if (StringUtils.isNotBlank(brokerClientTrustCertsFilePath)) {
	return brokerClientTrustCertsFilePath;
	} else {
	return tlsTrustCertsFilePath;
	}
	}

So we do not allow for configuration at the moment, and we expect users to provide the right format already. We can, of course, improve on that design.

[lhotari]

Good stuff!

[michaeljmarshall]

/pulsarbot rerun-failure-checks

[codecov-commenter]

Codecov Report

Merging #19888 (f585cb3) into master (32e677d) will increase coverage by 1.15%.
The diff coverage is 73.60%.

@@             Coverage Diff              @@
##             master   #19888      +/-   ##
============================================
+ Coverage     71.70%   72.85%   +1.15%     
- Complexity    31368    31611     +243     
============================================
  Files          1848     1862      +14     
  Lines        137151   137453     +302     
  Branches      15111    15133      +22     
============================================
+ Hits          98338   100138    +1800     
+ Misses        30904    29357    -1547     
- Partials       7909     7958      +49     

Flag	Coverage Δ
inttests	24.31% <4.80%> (?)
systests	24.85% <4.80%> (?)
unittests	72.13% <73.60%> (+0.43%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ker/loadbalance/extensions/BrokerRegistryImpl.java	83.33% <ø> (ø)
...a/org/apache/pulsar/proxy/server/ProxyService.java	79.36% <60.86%> (-0.82%)	⬇️
...uth/KubernetesServiceAccountTokenAuthProvider.java	77.27% <77.27%> (ø)
...rg/apache/pulsar/broker/web/PulsarWebResource.java	64.03% <100.00%> (+0.08%)	⬆️
...functions/auth/KubernetesFunctionAuthProvider.java	80.00% <100.00%> (+5.00%)	⬆️
...s/runtime/kubernetes/KubernetesRuntimeFactory.java	80.42% <100.00%> (+0.35%)	⬆️
...ime/kubernetes/KubernetesRuntimeFactoryConfig.java	97.36% <100.00%> (+0.14%)	⬆️

... and 239 files with indirect coverage changes

[Anonymitaet]

Hi @michaeljmarshall thanks for bringing this feature!

I see this PR is labeled with doc-required, for these new configurations, do we need to add some instructions and usage examples to function docs?

If so, feel free to add docs and ping me review. Thank you!

[michaeljmarshall]

Docs added here: apache/pulsar-site#570