search

konpyutaika / nifikop

The NiFiKop NiFi Kubernetes operator makes it easy to run Apache NiFi on Kubernetes. Apache NiFI is a free, open-source solution that support powerful and scalable directed graphs of data routing, transformation, and system mediation logic.
https://konpyutaika.github.io/nifikop/
Apache License 2.0
122 stars 39 forks source link

crd: can customize nifi container environment variables #394

Closed mheers closed 1 month ago

mheers commented 4 months ago
Q A
Bug fix? no
New feature? yes
API breaks? no
Deprecations? no
License Apache 2.0

What's in this PR?

CRD / Controller: Can customize / overwrite the nifi container specification

Why?

When having custom certificates for the Java Truststore/Keystore this allows a flexible modification of the nifi container specification in the pods - where we can set the JAVA_TOOL_OPTIONS env var. This env var could also have been set using NifiClusterSpec.Node[*].ReadOnlyConfig.AdditionalSharedEnvs but this would share the maybe secret data between all containers in the pod and running a second Java container as a sidecar with a different JAVA_TOOL_OPTIONS configuration would not be possible.

Additional context

The implementation is inspired by the solution in vault operator: https://github.com/bank-vaults/vault-operator/blob/v1.22.0/pkg/controller/vault/vault_controller.go#L1336

Checklist

mh013370 commented 4 months ago

I understand the desire here, but nifikop generates the container specs so that it knows how they're laid out and many assumptions are made throughout the operator based on this. Allowing users to configure any part of the container spec means you could potentially break an assumption the operator is making. This might be a little too open to expose as an option.

Are just wanting to set environment variables in only the nifi container and not any other containers in the pod?

mheers commented 2 months ago

At first I'd be happy to set an Env only for the nifi container. I see, that the container spec is quite complex. With this PR I wanted to avoid to create too many new fields in the CRD to make it not too complex.

Would you suggest to have a AdditionalNifiContainerEnvs in the ReadOnlyConfig?

juldrixx commented 2 months ago

At first I'd be happy to set an Env only for the nifi container. I see, that the container spec is quite complex. With this PR I wanted to avoid to create too many new fields in the CRD to make it not too complex.

Would you suggest to have a AdditionalNifiContainerEnvs in the ReadOnlyConfig?

It could be a good idea, to add the possibility to add env variable only for the nifi container and not for all the containers. Don't you think @mh013370?

mh013370 commented 2 months ago

At first I'd be happy to set an Env only for the nifi container. I see, that the container spec is quite complex. With this PR I wanted to avoid to create too many new fields in the CRD to make it not too complex. Would you suggest to have a AdditionalNifiContainerEnvs in the ReadOnlyConfig?

It could be a good idea, to add the possibility to add env variable only for the nifi container and not for all the containers. Don't you think @mh013370?

Yeah I can understand the need. Perhaps an AdditionalNifiEnvs that are applied only to the NiFi container would be appropriate. The AdditionalSharedEnvs makes it clear the envs are applied to all containers in the pod. The AdditionalNifiEnvs would be applied only to the NiFi container. The docs should be updated as appropriate to clarify this.

mheers commented 2 months ago

Implemented AdditionalNifiEnvs and force pushed.

mheers commented 1 month ago

@juldrixx do you need anything to merge?