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

Server UI shows "undefined" for environment variable populated from a secret #3973

Closed
ebr opened this issue Sep 9, 2020 · 12 comments
Closed
Assignees
Labels

Comments

@ebr
Copy link

ebr commented Sep 9, 2020

Summary

When an environment variable is populated from a secret, the server UI shows undefined for its value. This is highly confusing because the user's first reaction is that the secret value isn't being correctly passed. This messaging could be improved by showing secret or hidden or something to that effect, to indicate that the value does exist, but is not displayed.


Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.

@ebr ebr added the type/feature Feature request label Sep 9, 2020
@alexec alexec added type/bug ui good first issue Good for newcomers and removed type/feature Feature request labels Sep 9, 2020
@alexec
Copy link
Contributor

alexec commented Sep 10, 2020

Thank you for the bug report. Would you like to submit a PR to fix this?

@ebr
Copy link
Author

ebr commented Sep 10, 2020

I would like to, if I can get to it in the next couple of weeks! (i only have basic familiarity with either Go or React, but look forward to figuring it out)

@alexec
Copy link
Contributor

alexec commented Sep 10, 2020

You probably don't need to know Go to do this. I think you need to change: ui/src/app/workflows/components/workflow-node-info/workflow-node-info.tsx

@alexec alexec added hacktoberfest and removed ui labels Sep 28, 2020
@aletepe
Copy link
Contributor

aletepe commented Oct 20, 2020

I'm working on this bug for Hacktoberfest. @alexec I'd like to not only fix the bug but maybe tweak it a bit to differentiate not available values from secrets. I was thinking having the value show as [redacted] (or something similar) when the value is a secret, and showing not available for other reasons. Where would you say is the best place to put that logic?

@alexec
Copy link
Contributor

alexec commented Oct 20, 2020

@aletepe I don't think the actual value will be returned by the API call. So I don't think you'll need to redact it. What will be useful is showing it is from a secret (e.g. using an FontAwesome icon, e.g. <i className='fa fa-key'/>), the name of the secret and key within it. This allows the user to then go and look it up themselves if they need to.

@aletepe
Copy link
Contributor

aletepe commented Oct 20, 2020

@alexec I meant writing literally redacted, so as to make users understand there was actually a value behind it and not simply that there is no value available, and thus avoiding the confusion which led to raising this bug.
I like the idea of showing a more details about the secret, like the icon you mention 👍

PS: could you please assign this issue to me? Thanks before hand!

@alexec
Copy link
Contributor

alexec commented Oct 20, 2020

sure or maybe "secret"?

@aletepe
Copy link
Contributor

aletepe commented Oct 29, 2020

@ebr could you please add to the Issue description a screenshot of the UI showing undefined? And maybe steps to reproduce it (an example yaml file or similar)?

@aletepe
Copy link
Contributor

aletepe commented Oct 29, 2020

I've used the example in the docs here: https://github.com/argoproj/argo/blob/master/examples/secrets.yaml

Without changes the behaviour is the following:
image

With my proposed changes it would look something like this (injected two secrets to show multiline behaviour):
image

It would still show undefined like in the first screenshot for undefined values not coming from secrets.

@alexec @ebr how is this looking?

@alexec
Copy link
Contributor

alexec commented Oct 29, 2020

Looks great!
Can I ask that when you hover your mouse of the key icon you get some explanatory text?

@aletepe
Copy link
Contributor

aletepe commented Oct 29, 2020

Something like this?

The value of this environment variable has been hidden for security reasons because it comes from a kubernetes secret.

@alexec
Copy link
Contributor

alexec commented Oct 29, 2020

perfect

aletepe added a commit to aletepe/argo that referenced this issue Nov 27, 2020
Signed-off-by: Alejandro Tejera <aletepe@gmail.com>
@alexec alexec closed this as completed in 454f3ae Dec 2, 2020
alexcapras pushed a commit to alexcapras/argo that referenced this issue Dec 2, 2020
Signed-off-by: github@finnesand.no <github@finnesand.no>

feat(ui): Add Template/Cron workflow filter to workflow page. Closes argoproj#4532 (argoproj#4543)

Signed-off-by: Tianchu Zhao <evantczhao@gmail.com>

feat(executor): Auto create s3 bucket if not present.

Signed-off-by: Alex Capras <alexcapras@gmail.com>

Apply codegen

Signed-off-by: Alex Capras <alexcapras@gmail.com>

Add argo-e2e label to test wf

Signed-off-by: Alex Capras <alexcapras@gmail.com>

chore: Updated stress test YAML (argoproj#4569)

Signed-off-by: Alex Collins <alex_collins@intuit.com>

docs: Updated kubectl apply command in manifests README (argoproj#4577)

Signed-off-by: Stefan Gloutnikov <stefan@gloutnikov.com>

feat(controller): Make MAX_OPERATION_TIME configurable. Close argoproj#4239 (argoproj#4562)

Signed-off-by: Alex Collins <alex_collins@intuit.com>

docs: Fix a typo in example (argoproj#4590)

Signed-off-by: Takayoshi Nishida <takayoshi.nishida@gmail.com>

feat(controller): Retry transient offload errors. Resolves argoproj#4464 (argoproj#4482)

Signed-off-by: Alex Collins <alex_collins@intuit.com>

fix(server): use the correct name when downloading artifacts (argoproj#4579)

Signed-off-by: Daniel Herman <dherman@factset.com>

fix(server): serve artifacts directly from disk to support large artifacts (argoproj#4589)

Signed-off-by: Daniel Herman <dherman@factset.com>

fix(executor): Handle sidecar killing in a process-namespace-shared pod (argoproj#4575)

Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com>

docs: Add JSON schema for IDE validation (argoproj#4581)

Signed-off-by: Paul Brabban <paul.brabban@gmail.com>

refactor: Use polling model for workflow phase metric (argoproj#4557)

Signed-off-by: Simon Behar <simbeh7@gmail.com>

Addressing reviewers comments

Signed-off-by: Alex Capras <alexcapras@gmail.com>

Addressing reviewers comments

docs: Minor typo fix (argoproj#4610)

Signed-off-by: Paavo Pokkinen <paavo.pokkinen@vaimo.com>

fix(controller): Prevent tasks with names starting with digit to use either 'depends' or 'dependencies' (argoproj#4598)

Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>

fix(docs): Bring minio chart instructions up to date (argoproj#4586)

Signed-off-by: Ranga Krishnan <ranga@bei.re>

fix(executor): Fixed waitMainContainerStart returning prematurely. Closes argoproj#4599 (argoproj#4601)

Signed-off-by: fsiegmund <siegmund@slb.com>

feat(controller): Enhanced artifact repository ref. See argoproj#3184 (argoproj#4458)

Signed-off-by: Alex Collins <alex_collins@intuit.com>

fix: Null check pagination variable (argoproj#4617)

Signed-off-by: Simon Behar <simbeh7@gmail.com>

fix: Perform fields filtering server side (argoproj#4595)

Signed-off-by: Simon Behar <simbeh7@gmail.com>

fix(server): Correct webhook event payload marshalling. Fixes argoproj#4572 (argoproj#4594)

Signed-off-by: Alex Collins <alex_collins@intuit.com>

feat(ui): Add columns--narrower-height to AttributeRow (argoproj#4371)

fix: Fix TestCleanFieldsExclude (argoproj#4625)

Signed-off-by: Simon Behar <simbeh7@gmail.com>

fix(argo-server): fix global variable validation error with reversed dag.tasks (argoproj#4369)

Signed-off-by: chenyu.zheng <chenyu.zheng@hulu.com>

fix: derive jsonschema and fix up issues, validate examples dir… (argoproj#4611)

Signed-off-by: Paul Brabban <paul.brabban@gmail.com>

fix(ui): Reference secrets in EnvVars. Fixes argoproj#3973  (argoproj#4419)

Signed-off-by: Alejandro Tejera <aletepe@gmail.com>

fix(ui): Fix Snyk issues (argoproj#4631)

Signed-off-by: Alex Collins <alex_collins@intuit.com>

feat(executor): More informative log when executors do not support output param from base image layer (argoproj#4620)

Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>

Codegen patch. Signed off by alexcapras@gmail.com

Codegen patch. Signed off by alexcapras@gmail.com

Delete test.patch
alexcapras pushed a commit to alexcapras/argo that referenced this issue Dec 2, 2020
Signed-off-by: github@finnesand.no <github@finnesand.no>

feat(ui): Add Template/Cron workflow filter to workflow page. Closes argoproj#4532 (argoproj#4543)

Signed-off-by: Tianchu Zhao <evantczhao@gmail.com>

feat(executor): Auto create s3 bucket if not present.

Signed-off-by: Alex Capras <alexcapras@gmail.com>

Apply codegen

Signed-off-by: Alex Capras <alexcapras@gmail.com>

Add argo-e2e label to test wf

Signed-off-by: Alex Capras <alexcapras@gmail.com>

chore: Updated stress test YAML (argoproj#4569)

Signed-off-by: Alex Collins <alex_collins@intuit.com>

docs: Updated kubectl apply command in manifests README (argoproj#4577)

Signed-off-by: Stefan Gloutnikov <stefan@gloutnikov.com>

feat(controller): Make MAX_OPERATION_TIME configurable. Close argoproj#4239 (argoproj#4562)

Signed-off-by: Alex Collins <alex_collins@intuit.com>

docs: Fix a typo in example (argoproj#4590)

Signed-off-by: Takayoshi Nishida <takayoshi.nishida@gmail.com>

feat(controller): Retry transient offload errors. Resolves argoproj#4464 (argoproj#4482)

Signed-off-by: Alex Collins <alex_collins@intuit.com>

fix(server): use the correct name when downloading artifacts (argoproj#4579)

Signed-off-by: Daniel Herman <dherman@factset.com>

fix(server): serve artifacts directly from disk to support large artifacts (argoproj#4589)

Signed-off-by: Daniel Herman <dherman@factset.com>

fix(executor): Handle sidecar killing in a process-namespace-shared pod (argoproj#4575)

Signed-off-by: Daisuke Taniwaki <daisuketaniwaki@gmail.com>

docs: Add JSON schema for IDE validation (argoproj#4581)

Signed-off-by: Paul Brabban <paul.brabban@gmail.com>

refactor: Use polling model for workflow phase metric (argoproj#4557)

Signed-off-by: Simon Behar <simbeh7@gmail.com>

Addressing reviewers comments

Signed-off-by: Alex Capras <alexcapras@gmail.com>

Addressing reviewers comments

docs: Minor typo fix (argoproj#4610)

Signed-off-by: Paavo Pokkinen <paavo.pokkinen@vaimo.com>

fix(controller): Prevent tasks with names starting with digit to use either 'depends' or 'dependencies' (argoproj#4598)

Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>

fix(docs): Bring minio chart instructions up to date (argoproj#4586)

Signed-off-by: Ranga Krishnan <ranga@bei.re>

fix(executor): Fixed waitMainContainerStart returning prematurely. Closes argoproj#4599 (argoproj#4601)

Signed-off-by: fsiegmund <siegmund@slb.com>

feat(controller): Enhanced artifact repository ref. See argoproj#3184 (argoproj#4458)

Signed-off-by: Alex Collins <alex_collins@intuit.com>

fix: Null check pagination variable (argoproj#4617)

Signed-off-by: Simon Behar <simbeh7@gmail.com>

fix: Perform fields filtering server side (argoproj#4595)

Signed-off-by: Simon Behar <simbeh7@gmail.com>

fix(server): Correct webhook event payload marshalling. Fixes argoproj#4572 (argoproj#4594)

Signed-off-by: Alex Collins <alex_collins@intuit.com>

feat(ui): Add columns--narrower-height to AttributeRow (argoproj#4371)

fix: Fix TestCleanFieldsExclude (argoproj#4625)

Signed-off-by: Simon Behar <simbeh7@gmail.com>

fix(argo-server): fix global variable validation error with reversed dag.tasks (argoproj#4369)

Signed-off-by: chenyu.zheng <chenyu.zheng@hulu.com>

fix: derive jsonschema and fix up issues, validate examples dir… (argoproj#4611)

Signed-off-by: Paul Brabban <paul.brabban@gmail.com>

fix(ui): Reference secrets in EnvVars. Fixes argoproj#3973  (argoproj#4419)

Signed-off-by: Alejandro Tejera <aletepe@gmail.com>

fix(ui): Fix Snyk issues (argoproj#4631)

Signed-off-by: Alex Collins <alex_collins@intuit.com>

feat(executor): More informative log when executors do not support output param from base image layer (argoproj#4620)

Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>

Codegen patch. Signed off by alexcapras@gmail.com

Codegen patch. Signed off by alexcapras@gmail.com

Delete test.patch

Signed-off-by: Alex Capras <alexcapras@gmail.com>
simster7 pushed a commit that referenced this issue Dec 10, 2020
Signed-off-by: Alejandro Tejera <aletepe@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants