frontend: Added displaying of imported environment variables per container.

[vlasov-y]

Displaying of environment variables per container in pod is the main feature my developers used in kubernetes-dashboard. I have added it here as well.
Also, I have found an error in browser console and applied a small fix to TooltipLight file.

Features

• 1. Values from secrets are hidden by default
• 2. Secret values can be revealed on click
• 3. If configmap or secret is younger than container start time (or pod creation) the warning is shown
• 4. Variable name and value are copied to the clipboard on click
• 5. Names of configmaps and secrets are valid links to the respective resources
• 6. Handing of optional, resourceFieldRef and fieldRef variables is implemented

[vlasov-y]

Hi @illume @skoeva ✋
Could you please take a look?

[gecube]

Oh... in lens there is an incorrect behaviour. TLDR: you should SHOW NOT THE current ENVs from secret and / or configmap, but CURRENT IN THE POD (which is effectively impossible).

[vlasov-y]

Oh... in lens there is an incorrect behaviour. TLDR: you should SHOW NOT THE current ENVs from secret and / or configmap, but CURRENT IN THE POD (which is effectively impossible).

Agree, that would be much better, but I could not find a way to do that.

[gecube]

@vlasov-y it is mandatory, otherwise this feature will not work properly. My developer asked me about "why application is not working, the env variables are shown properly in Lens"... it took for me 1h to explain him, that lens shown not reality, but it's own hallucinations...

[vlasov-y]

@vlasov-y it is mandatory, otherwise this feature will not work properly. My developer asked me about "why application is not working, the env variables are shown properly in Lens"... it took for me 1h to explain him, that lens shown not reality, but it's own hallucinations...

I had same situations. But it is better than nothing and as far as I know, env variables (if they are overriden) will differ per process in container, are not they? Some containers may not have a shell as well.
On the current project, developers use these values to get database credentials or URLs.

[gecube]

@vlasov-y it could lead to improper expectations and even more debug than necessary.

But it is better than nothing and as far as I know, env variables (if they are overriden) will differ per process in container, are not they? Some containers may not have a shell as well.

yes, but we are talking about container envs that is equal to PID1 process envs.

Again. Situation is very simple. You run a pod. You show envs in dashboard. Then somebody changed configmap or secret. What would be shown - the new values from CM or secret or actual values from pod? I know the answer and it is not satisfactory.

[vlasov-y]

@vlasov-y it could lead to improper expectations and even more debug than necessary.

But it is better than nothing and as far as I know, env variables (if they are overriden) will differ per process in container, are not they? Some containers may not have a shell as well.

yes, but we are talking about container envs that is equal to PID1 process envs.

Again. Situation is very simple. You run a pod. You show envs in dashboard. Then somebody changed configmap or secret. What would be shown - the new values from CM or secret or actual values from pod? I know the answer and it is not satisfactory.

New value will be shown of course, but reloader will save us from issues with that.
Anyway, I 100% agree with you, @gecube, I just have done what I could and I believe that is better than nothing.

[gecube]

@vlasov-y you should not mention the stakater here as it is some design flaw not related to it.

Let's discuss all possible options

• not to implement this feature at all. I think it is counter-productive as it would be useful for many users
• so we need somehow to fix the inconsistency or stale data I explained. The first option could be to check pod start time and the data of configmap change and then show warning that data is inaccurate (or could be inaccurate!!!!)
• or even we can just go directly to pod and print the PID1 env variables and match them back to UI (then the question arises could they be changed in some way after pod was running?) Or go directly to CRI (container engine) and ask it about envs that we injected during pod startup (but it would be, of course, CRI-specific and dependant on its' API)
• and ANYWAY we should show warning if secret/configmap was removed and pod using envs from it is still running (so we can't retrieve the env values from k8s api)....

Any ideas / suggestions here?

[vlasov-y]

@vlasov-y you should not mention the stakater here as it is some design flaw not related to it.

Let's discuss all possible options

• not to implement this feature at all. I think it is counter-productive as it would be useful for many users
• so we need somehow to fix the inconsistency or stale data I explained. The first option could be to check pod start time and the data of configmap change and then show warning that data is inaccurate (or could be inaccurate!!!!)
• or even we can just go directly to pod and print the PID1 env variables and match them back to UI (then the question arises could they be changed in some way after pod was running?) Or go directly to CRI (container engine) and ask it about envs that we injected during pod startup (but it would be, of course, CRI-specific and dependant on its' API)
• and ANYWAY we should show warning if secret/configmap was removed and pod using envs from it is still running (so we can't retrieve the env values from k8s api)....

Any ideas / suggestions here?

p.1 - agree
p.2 - that is definitely possible and I can do that, I suggest to start with that at least
p.3 - headlamp frontend uses custom library to interact with Kubernetes, I am not sure it can make pod execs. I assume you suggest to make pod exec and print PID 1 env from /proc/1/env or similar? What is pod won't have shell inside?
p.4 - I believe it is implemented already. Error message will be printed instead of the configmap/secret value in case of any access error, including not found.

[vlasov-y]

FYI @gecube

That is what I have, but there is an issue. I compare configmap/secret creationTimestamp with Pod's creation timestamp. There is no updateTimestamp in configmap, while I can change its data without changing the creationTimestamp. Do you have any ideas?

[gecube]

@vlasov-y no idea to be honest....

[gecube]

p.3 - headlamp frontend uses custom library to interact with Kubernetes, I am not sure it can make pod execs. I assume you suggest to make pod exec and print PID 1 env from /proc/1/env or similar? What is pod won't have shell inside?

then we may somehow get data from CRI. The thesis about absence of shell is totally correct, but it happens only for a small subset of images.

[vlasov-y]

p.3 - headlamp frontend uses custom library to interact with Kubernetes, I am not sure it can make pod execs. I assume you suggest to make pod exec and print PID 1 env from /proc/1/env or similar? What is pod won't have shell inside?

then we may somehow get data from CRI. The thesis about absence of shell is totally correct, but it happens only for a small subset of images.

UPD I have understood that comparing with metadata.creationTimestamp is not always correct because containers in the pod can be restarted on failure and internet says that they will consume newer version of configmap. Hence, I have used .status.containerStatuses[container by name].status.running.startedAt and only if it is not available I am falling back to creation timestamp.

Regarding a small subset of images: maybe 1/3 of images in my clusters do not have shell. Most kubernets Go operator does not have one, since they are launched in google's distroless image. So that will be an often case.

Honestly, I think that this disclaimer will be enough :)

[joaquimrocha]

I have read the thread (thanks for the feature and the discussion, @vlasov-y and @gecube ). I think I understand the issue (the UI may hint to an instance of a secret/config-map that's not the one being used by the pod and thus the values may differ).
IMO the hint is good enough as a way to mitigate the problem. And in the future we may add more ways if possible (like checking the shell if needed, but there's a performance impact we should evaluate AFAIU)
@gecube Are you fine moving forward with this solution? (we are yet to review the code though)

[joaquimrocha]

Left a few comments but would like @sniok to take a look as well.
There's also a Merge branch that should be removed from the PR (by doing git rebase), and please check our git guidelines for the commit messages: https://headlamp.dev/docs/latest/contributing#2-follow-commit-guidelines

It's an awesome feature, @vlasov-y . Let's work together to land it.

[joaquimrocha]

I think you should use the SecretField component here, instead of recreating it. That will simplify not only this code but also the logic of handling revealed secrets, which should rather be inside the component that displays them IMO.
I know our current SecretField doesn't handle copying but we could add that in a separate commit.

[vlasov-y]

I have applied that here #2728 (comment)
Also, I have copying implemented there as well. If you want, I can move copy functionality inside of SecretField code.

[joaquimrocha]

I don't understand what optional is doing or where it is being assigned. Can you add more context to the commit message?

[vlasov-y]

It is part of Kubernetes API. Some configMaps/secrets may be absent, but be optional in the pod manifest at the same time. In that case, if we receive 404 - that is not an issues.
https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#optional-configmaps

[joaquimrocha]

We have similar logic in https://github.com/headlamp-k8s/headlamp/blob/f21da73a9def367461680227659befbe5ff2806f/frontend/src/lib/units.ts . Can you check if these are helpful to simplify the logic here?

[vlasov-y]

Hmm.. That is close, but my logic supports more cases. I would suggest to keep mine. WDYT?

[sniok]

It wouldn't be a good idea to keep two separate implementations of a pretty much the same thing. I'd suggest to either extend the existing logic to support more cases or reuse that logic as-is

[vlasov-y]

Done
e5452ff

[vlasov-y]

Demo

[sniok]

This is a really great feature and thank you for taking time to implement it!

My biggest concern is that current implementation breaks the rules of hooks. I think this can be refactored into a bit different flow.

Instead of processing values and putting them into a variables map you could display values by using components like <ValueFromConfigMap configMapName={} configMapKey={} /> that can calls hooks and directly render those values. This will simplify things a bit and also avoid breaking the rules of hooks.

[vlasov-y]

This is a really great feature and thank you for taking time to implement it!

My biggest concern is that current implementation breaks the rules of hooks. I think this can be refactored into a bit different flow.

Instead of processing values and putting them into a variables map you could display values by using components like <ValueFromConfigMap configMapName={} configMapKey={} /> that can calls hooks and directly render those values. This will simplify things a bit and also avoid breaking the rules of hooks.

That is awesome! I did not know you have such component.
Thank you for comments @sniok @joaquimrocha I will do my best to apply your recommendations shortly.
P.S. I am not a frontend developer, I am a DevOps, so frontend development is kind of unusual for me 😃
P.S.2. I will push new commits and once all our conversations are resolved I will squash them with rebase and push with force, so only one commit will be left

[vlasov-y]

@sniok I will need you help.
#2728 (comment)
#2728 (review)
I have not followed the idea. I got that current architecture with calls of process functions is a bad approach, but I could not figure out how do you see it.
I try to stay with an idea that it must be one table with all environment variables. If move that process functions to separate components, they will produce separate InnerTables, won't they?
Also we have to consider that envrionment variables may override each other, because we may have variables with the same name in the container's .env and in the conrfigmap, for example.
Anyway, please, give me more information, or/and we can arrange a quick call so you would quickly example me what you want and I will be able to implement that.

[vlasov-y]

@sniok @joaquimrocha Most of conversations have been updated. I have pushed many commits, but do not worry, I will squash them at the very end of the feature development. Please, resolve conversations that can be considered done.

[sniok]

@sniok I will need you help. #2728 (comment) #2728 (review) I have not followed the idea. I got that current architecture with calls of process functions is a bad approach, but I could not figure out how do you see it. I try to stay with an idea that it must be one table with all environment variables. If move that process functions to separate components, they will produce separate InnerTables, won't they? Also we have to consider that envrionment variables may override each other, because we may have variables with the same name in the container's .env and in the conrfigmap, for example. Anyway, please, give me more information, or/and we can arrange a quick call so you would quickly example me what you want and I will be able to implement that.

I thought a bit more about it and it might be simpler to stick to the current way. Let's name all the functions that use hooks inside to have prefix use, to make sure it's clear that they are hooks. processValueFromSecret, processValueFromConfigMap, processAllSecretKeys, processAllConfigMapKeys -> useValueFromSecret, useValueFromConfigMap, .... I know that if you're not familiar with react this change might seem to be pointless but it is a very important convention. And processEnvVariables function is immediately called after it's defined, so maybe let's just inline it. Now the issue is that there are hooks that are called in a loop, but since there's no way to add or remove env variables they'll stay the same (correct me if I'm wrong here though but I'm pretty sure you can't add/remove env variables on an existing container) and there won't be an issue with hooks here.

Some useful doc pages on this
https://react.dev/reference/rules/rules-of-hooks#only-call-hooks-at-the-top-level
https://react.dev/reference/rules/rules-of-hooks#only-call-hooks-from-react-functions

[vlasov-y]

FYI @sniok

Let's name all the functions that use hooks inside to have prefix use

Sure, resolved in d87ca5a

correct me if I'm wrong here though but I'm pretty sure you can't add/remove env variables on an existing container

Yes. In order to apply changed environment variables in manifest you have to recreate the pod. Does not matter whether you alter variables in configmap/secret or inline variables.

[vlasov-y]

@sniok @joaquimrocha Hi ✋
Kindly pushing this one forward 😃
Can we merge it finally? (after you say "OK" - I will do a rebase to squash commits and we can merge, or you can squash during the merge in GitHub, as you wish)

[joaquimrocha]

Thanks for iterating on this @vlasov-y . I left only a comment.
@sniok Can you review it as well?
And maybe @skoeva you could test it since you were looking into metrics even without being able to reproduce the problem you were chasing?

[sniok]

Hi, thanks for the changes. Here's some things I've noticed while testing

1. Clicking on show (eye) button also copies the value
   
   

I think the intention here was to copy the value when user clicks on the value right? But there's no indication that it's going to happen, I'd remove it or have an explicit "copy" button. I wouldn't want to lose my clipboard value unexpectedly

---

2. Hidden value is implemented as <input type=password>

Chrome complains about this in the console

[DOM] Password field is not contained in a form: (More info: https://goo.gl/9p2vKq)

Which I mostly agree with, replacing input with plain text •••••••• would work here

---

Code wise I think it's okay, documentation for some functions would be nice but they're small enough that it's not a big deal. I'd also ask you cleanup commits to follow the atomic commits guidelines (tldr: one commit per small change, for example refactoring units would be in it's own commit) so we can review commit messages and content too

[sniok]

Great addition, just a couple of issues to address that I mentioned in the comment above

[vlasov-y]

@sniok
Thanks for the review!

1. Added copy buttons instead of implicit copy

2. Changed secret field input type to text, since it is replaced with dots anyway.
3. Squashed commits.

[sniok]

With all the copy buttons UI looks a bit busy now. What if the copy button was only next to the value and not the name? I think copying value is more common than the name. What do you think?

And please split the commit into smaller ones, for example unit refactoring should be in a separate commit. We also have a convention that commit messages start with a category, like frontend: Added displaying of imported environment variables per container

[vlasov-y]

With all the copy buttons UI looks a bit busy now. What if the copy button was only next to the value and not the name? I think copying value is more common than the name. What do you think?

Okay.

And please split the commit into smaller ones, for example unit refactoring should be in a separate commit. We also have a convention that commit messages start with a category, like frontend: Added displaying of imported environment variables per container

I do not think it is possible to split commits in my case :( I have already squashed. It will be hard for me to separate changes. Is it possible to merge one big commit?
Will add frontend: prefix.

[vlasov-y]

@sniok can you advice me, please, how to update snapshots to match new code?

[joaquimrocha]

Hi @vlasov-y . We can make an exception to accept your PR with one commit. However, we shouldn't have merge commits in it.
Can you rebase your branch, so the merge goes away? git pull --rebase origin main

[vlasov-y]

Hi @vlasov-y . We can make an exception to accept your PR with one commit. However, we shouldn't have merge commits in it. Can you rebase your branch, so the merge goes away? git pull --rebase origin main

Done

[sniok]

@sniok can you advice me, please, how to update snapshots to match new code?

To update snapshots you can run

npm run test -- -u

[illume]

I don't want us to merge this as is.

I'd prefer we take on the work of breaking it up into smaller commits and finishing off anything else.

[vlasov-y]

@sniok can you advice me, please, how to update snapshots to match new code?

To update snapshots you can run

npm run test -- -u

Thanks, done that.

[vlasov-y]

I don't want us to merge this as is.

I'd prefer we take on the work of breaking it up into smaller commits and finishing off anything else.

Feel free to make change. Do you I need to grant you some permissions to the fork?

[illume]

Do you I need to grant you some permissions to the fork?

No, we don't need permissions.
I can merge this PR into a separate branch, and then continue the work from there.