frontend: Added displaying of imported environment variables per container. #2728
Conversation
dosubot
bot
added
the
size:L
Signed-off-by: Yurii Vlasov <[email protected]>
|
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. |
|
@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. |
|
@vlasov-y it could lead to improper expectations and even more debug than necessary.
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. |
|
@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
Any ideas / suggestions here? |
p.1 - agree |
|
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? |
…ewer than pod. Signed-off-by: Yurii Vlasov <[email protected]>
|
@vlasov-y no idea to be honest.... |
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 :) |
…amp. Signed-off-by: Yurii Vlasov <[email protected]>
Signed-off-by: Yurii Vlasov <[email protected]>
Signed-off-by: Yurii Vlasov <[email protected]>
vlasov-y
changed the title
Signed-off-by: Yurii Vlasov <[email protected]>
dosubot
bot
added
size:XL
|
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). |
There was a problem hiding this comment.
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.
| @@ -1,6 +1,7 @@ | |||
| import { Icon } from '@iconify/react'; | |||
| import Editor from '@monaco-editor/react'; | |||
| import { InputLabel } from '@mui/material'; | |||
| import { Visibility, VisibilityOff } from '@mui/icons-material'; // Material icons for eye | |||
There was a problem hiding this comment.
We use iconify instead of MUI icons in the UI. The equivalent icon is mdi:eye and mdi:eye-off, but I'd rather use the SecretField we have in this file.
There was a problem hiding this comment.
Resolved in ed991a3
Please review and resolve this conversation if it is fine.
| isSecret: boolean; | ||
| }; | ||
|
|
||
| export function GetEnvironmentVariables(props: EnvironmentVariablesProps) { |
There was a problem hiding this comment.
You could call this ContainerEnvironmentVariables as it sounds more in line with the rest of the components.
Also, I am not sure we should export this for now. Maybe we keep it unexported for now.
There was a problem hiding this comment.
Resolved in 3ac20cb
Please review and resolve this conversation if it is fine.
P.S. 'Clean code' says that function name must contain a verb, but I do not mind renaming the function as you have suggested.
| <Box display="flex" alignItems="center"> | ||
| {data.isError && ( | ||
| <Box aria-label="hidden" display="flex" alignItems="center" px={1}> | ||
| <Icon icon="mdi:alert-outline" width="1.3rem" height="1.3rem" aria-label="error" /> | ||
| <Typography color="error" sx={{ marginLeft: 1 }}> | ||
| {displayValue} | ||
| </Typography> | ||
| </Box> | ||
| )} | ||
| {!data.isError && ( | ||
| <Box onClick={() => handleCopy(data.value)}> | ||
| <StatusLabel status="" sx={{ fontFamily: 'monospace' }}> | ||
| {displayValue} | ||
| </StatusLabel> | ||
| <Snackbar open={copied} message="Copied!" autoHideDuration={2000} /> | ||
| </Box> | ||
| )} | ||
| {!data.isError && data.isSecret && ( | ||
| <IconButton onClick={() => toggleSecret(data.key)} sx={{ marginLeft: 1 }}> | ||
| {isRevealed ? <VisibilityOff /> : <Visibility />} | ||
| </IconButton> | ||
| )} | ||
| </Box> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| props?.env?.forEach(item => { | ||
| if (item.value) { | ||
| variables.set(item.name, { | ||
| value: item.value, | ||
| from: 'manifest', | ||
| isSecret: false, | ||
| isError: false, | ||
| }); | ||
| } else if (item.valueFrom) { | ||
| const ref = item.valueFrom; | ||
| if (ref.secretKeyRef) { | ||
| processValueFromSecret(item.name, ref.secretKeyRef); | ||
| } else if (ref.configMapKeyRef) { | ||
| processValueFromConfigMap(item.name, ref.configMapKeyRef); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // Process env variables from envFrom (configMap or secret references) | ||
| props?.envFrom?.forEach(item => { | ||
| if (item.secretRef) { | ||
| processAllSecretKeys(item.secretRef.name, prefix); | ||
| } else if (item.configMapRef) { | ||
| processAllConfigMapKeys(item.configMapRef.name, prefix); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I am not sure this logic will not end up violating hooks' rules. Maybe @sniok can help review it.
There was a problem hiding this comment.
You can only call hooks inside components or inside other hooks, these processX functions should be components: processValueFromSecret, processValueFromConfigMap, processAllSecretKeys, processAllConfigMapKeys, processValueFromFieldRef, processValueFromResourceFieldRef.
There was a problem hiding this comment.
I am not sure I have followed... I understand it like I have to move this functionality to separate functions that return React components and then use that components independently in ContainerEnvironmentVariables function. I think I can do that.
| return data.from; | ||
| } | ||
| return ( | ||
| <Link routeName={kind} params={{ name, namespace }}> |
There was a problem hiding this comment.
Usually we have done linking to resources by getting the details link from the respective class. See the makeOwnerReferences from https://github.com/headlamp-k8s/headlamp/blob/58c51a58932408d361ee725b1af27bcfc52df7ff/frontend/src/components/common/Resource/MetadataDisplay.tsx .
There was a problem hiding this comment.
Resolved in 495f183
Review and resolve this conversation, please, if everything is alright.
| const processValueFromSecret = (name: string, secretRef: { name: string; key: string }) => { | ||
| const processValueFromSecret = ( | ||
| name: string, | ||
| secretKeyRef: { name: string; key: string; optional?: boolean } |
There was a problem hiding this comment.
I don't understand what optional is doing or where it is being assigned. Can you add more context to the commit message?
There was a problem hiding this comment.
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
| case 'metadata.name': | ||
| value = pod.metadata.name || ''; | ||
| break; | ||
| case 'metadata.namespace': | ||
| value = pod.metadata.namespace || ''; | ||
| break; | ||
| case 'spec.nodeName': | ||
| value = pod.spec.nodeName || ''; | ||
| break; | ||
| case 'spec.serviceAccountName': | ||
| value = pod.spec.serviceAccountName || ''; | ||
| break; | ||
| case 'status.hostIP': | ||
| value = pod.status.hostIP || ''; | ||
| break; | ||
| case 'status.podIP': | ||
| value = pod.status.podIP || ''; | ||
| break; |
There was a problem hiding this comment.
Looks like this could be automated by checking whether we can split by . and checking 1st and 2nd split results from pod.
But probably a better solution is to use JSONPath to try to match the fieldPath.
There was a problem hiding this comment.
Resolved in 3ef217d
Review and resolve this conversation, please, if everything is alright.
| const resourceType = resourceFieldRef.resource; | ||
| const divisor = resourceFieldRef.divisor || '1'; | ||
|
|
||
| const BINARY_SUFFIXES = new Map([ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Hmm.. That is close, but my logic supports more cases. I would suggest to keep mine. WDYT?
There was a problem hiding this comment.
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
| ) => { | ||
| const [secret, setSecret] = React.useState<Secret | null>(null); | ||
| const [error, setError] = React.useState<ApiError | null>(null); | ||
| ResourceClasses.Secret.useApiGet(setSecret, secretKeyRef.name, namespace, setError); |
There was a problem hiding this comment.
we have a better way to do this now
const [secret, error] = Secret.useGet(secretKeyRef.name, namespace)There was a problem hiding this comment.
Resolved in 9da9ae2
Review and resolve this conversation, please, if everything is alright.
| let value = ''; | ||
| let isError = false; | ||
|
|
||
| try { |
There was a problem hiding this comment.
you can use JSONPath here, we're already using it in other places too, for example https://github.com/headlamp-k8s/headlamp/blob/main/frontend/src/components/crd/CustomResourceDetails.tsx#L79C7-L79C14
There was a problem hiding this comment.
Resolved in 3ef217d
Review and resolve this conversation, please, if everything is alright.
There was a problem hiding this comment.
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. |
Signed-off-by: Yurii Vlasov <[email protected]>
Signed-off-by: Yurii Vlasov <[email protected]>
Signed-off-by: Yurii Vlasov <[email protected]>
Signed-off-by: Yurii Vlasov <[email protected]>
Signed-off-by: Yurii Vlasov <[email protected]>
Signed-off-by: Yurii Vlasov <[email protected]>
Signed-off-by: Yurii Vlasov <[email protected]>
Signed-off-by: Yurii Vlasov <[email protected]>
|
@sniok I will need you help. |
|
@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. |
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 Some useful doc pages on this |
Signed-off-by: Yurii Vlasov <[email protected]>
dosubot
bot
added
size:L
|
FYI @sniok
Sure, resolved in d87ca5a
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. |
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