[newrelic-logging] Adjust order of env list in daemonset to fix dependency reference

[nluedema]

Is this a new chart

No

What this PR does / why we need it:

When using persistentVolume mode, the env variable FB_DB references NODE_NAME. This reference is currently not resolved, because NODE_NAME is defined after FB_DB. Therefore, only a single database file named '$(NODE_NAME)-fb.db' is created in the persistent volume, instead of one file for each node. #1408 traced the problem down to this change, which moved the definition of NODE_NAME after the definition of FB_DB.

This PR fixes this, by moving the definition of NODE_NAME before the definition of FB_DB again.

The behavior of kubernetes for dependent environment variables is documented here.

Which issue this PR fixes

• fixes [newrelic-logging] BREAKING Bug in env values order #1408

Special notes for your reviewer:

I was thinking about a test that checks if NODE_NAME is defined before FB_DB, but I wasn't able to come up with a way to do this using helm-unittest.

Checklist

• Chart Version bumped
• Variables are documented in the README.md
• Title of the PR starts with chart name (e.g. [mychartname])

[CLAassistant]

All committers have signed the CLA.

[jsubirat]

Hi @nluedema ,

Thank you for identifying this issue. Indeed, I didn't realize that moving NODE_NAME would affect FB_DB when using persistent volumes due to ordering. The reason this is happening though is because that environment variable uses $(NODE_NAME) instead of ${NODE_NAME}, which means that it is Helm resolving the value of these variables rather than Fluent Bit when it starts.

To fix this, I would suggest to leave NODE_NAME untouched and instead fix FB_DB. We don't want environment variable definition to be dependent on order. To achieve this, could you please:

• Move NODE_NAME back to where it was
• Modify this:

- name: FB_DB
   value: "/db/$(NODE_NAME)-fb.db"

to this:

- name: FB_DB
 value: "/db/${NODE_NAME}-fb.db"

This will ensure that environment variable definitions stay order-insensitive and that it is actually the Fluent Bit process that resolves NODE_NAME upon start rather than Helm.

[nluedema]

Hi @jsubirat,

Thanks for the feedback! I didn't know that Fluent Bit is able to resolve environment variables. Doing it that way is of course a lot more elegant. I updated the PR according to your suggestions.

[nr-rkallempudi]

Hi @nluedema , We have validated your PR and found that Fluent Bit is not resolving the variable name ${NODE_NAME} as it doesn't support nested environment variable interpolation. So we agreed internally to do with your previous code changes for now even though it keeps order dependency.

Can you update code to previous one with curved braces and also add a comment mentioning the order dependency to have that variable resolved. So we can proceed with merging your PR.

[nr-rkallempudi]

Hi @nluedema , We have validated your PR and found that Fluent Bit is not resolving the variable name ${NODE_NAME} as it doesn't support nested environment variable interpolation. So we agreed internally to do with your previous code changes for now even though it keeps order dependency.

Can you update code to previous one with curved braces and also add a comment mentioning the order dependency to have that variable resolved. So we can proceed with merging your PR.

[nluedema]

@nr-rkallempudi @jsubirat
I updated the PR to only include the initial changes. Let me know what you think! A pity that nested environment variable interpolation doesn't work.

[jsubirat]

Thanks folks, and apologies for the confusion!