Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crd: can customize nifi container environment variables #394

Merged
merged 2 commits into from
May 6, 2024

Conversation

mheers
Copy link
Contributor

@mheers mheers commented Mar 1, 2024

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

  • Implementation tested
  • Error handling code meets the guideline
  • Logging code meets the guideline
  • User guide and development docs updated (if needed)
  • Append changelog with changes

@mh013370
Copy link
Member

mh013370 commented Mar 1, 2024

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
Copy link
Contributor Author

mheers commented Apr 9, 2024

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
Copy link
Contributor

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
Copy link
Member

mh013370 commented Apr 17, 2024

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.

…itional env variables that will only be embed in the nifi container
@mheers mheers force-pushed the feature/nifi-container-spec branch from 649a4d8 to 385f97a Compare April 19, 2024 08:39
@mheers
Copy link
Contributor Author

mheers commented Apr 19, 2024

Implemented AdditionalNifiEnvs and force pushed.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@mh013370 mh013370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mheers
Copy link
Contributor Author

mheers commented May 6, 2024

@juldrixx do you need anything to merge?

@juldrixx juldrixx changed the title crd: can customize nifi container spec crd: can customize nifi container environment variables May 6, 2024
@juldrixx juldrixx merged commit 38e0768 into konpyutaika:master May 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants