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

Add support for credential password prompting on job launch #9028

Conversation

mabashian
Copy link
Member

SUMMARY

link #7015

The main problem that I ran in to while adding support for this feature is the fact that each time formik's validation runs, errors for fields that are not shown get wiped. To get around this, I added a method to each step that will re-apply errors whenever a user navigates to a particular step. Each step is also responsible for determining whether or not that step has an error.

The scope of these changes will require us to re-test all of the launch prompting scenarios. As far as credentials go, there are 3 scenarios where credential passwords.

  1. A JT can have default credentials that require passwords on launch but the JT itself doesn't prompt for credentials.
  2. A JT can have default credentials that require passwords on launch and the JT can also prompt for credentials. This allows users to select different credentials.
  3. A JT can not have default credentials but prompt for credentials. This would allow a user to select credentials that prompt for passwords.

Note that this issue: #8200 is still not implemented.

#8921 is also not implemented.

Both of these issues may be encountered during testing.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • UI

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Copy link
Member

@AlexSCorey AlexSCorey left a comment

Choose a reason for hiding this comment

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

My gut tells me that the validation on useCredentialPasswordStep can be improved. Let me know if you want to discuss.

label={
credId === ''
? i18n._(t`Vault password`)
: i18n._(t`Vault password - ${credId}`)
Copy link
Member

Choose a reason for hiding this comment

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

In this PR #8738 we are using | to separate the credential name from the vault ID. I realize this is not exactly the same use case/situation, but thought I'd share. We may want to have some consistency with the naming pattern with these credentials.

) {
launchConfig.passwords_needed_to_start.forEach(password => {
if (password === 'ssh_password') {
initialValues.credentialPasswordSsh = '';
Copy link
Member

Choose a reason for hiding this comment

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

Why are we added they keys here, only to switch them back to keys that the api recognizes in https://github.com/ansible/awx/pull/9028/files#diff-b4b7e4b3882a0650f7db64d9166a39cc1c7ecae225a73f41efaaa9bc0334286fR1? It seems that if we didn't switch keys a lot of the validation could be consolidated into a forEach loop making this code more concise and easier to read.

@@ -0,0 +1,29 @@
export default function getCredentialPasswords(values) {
Copy link
Member

Choose a reason for hiding this comment

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

This could possibly be shortened or maybe even eliminated if we can avoid created keys in the formik values object that the api doesn't recognize.

@tiagodread
Copy link
Contributor

tiagodread commented Jan 19, 2021

@mabashian @one-t

  • Trying to relaunch a JT with credentials prompts enabled and JT credentials prompt enabled:
    image

Full Flow:
flowprompt

Note that, when I launch the JT for the first time, the Private key passphrase * is required on wizard but on relaunch this field isn't displayed.

  • Should this icon on step 2 (with error) be at the same line with the text?

image

@mabashian mabashian force-pushed the 7015-prompt-cred-passwords-v2 branch from 44c68c2 to fb62e0e Compare January 19, 2021 14:59
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Copy link
Contributor

@tiagodread tiagodread left a comment

Choose a reason for hiding this comment

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

Verified that:

  • POL regression
  • The user can prompt credentials without POL on credential
  • The user can prompt credentials with POL on credential
  • The user can prompt using different credentials like machine and vault
  • The user can relaunch a job and prompt credentials fields

PR with stubs and a test https://github.com/ansible/tower-qa/pull/5871

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Copy link
Member

@AlexSCorey AlexSCorey left a comment

Choose a reason for hiding this comment

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

I think these changes are an improvement to reduce some of the complexity in the validation function. I think this is a big step to get to feature parity for all of the prompt on launch features. Well done!

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 8a7c714 into ansible:devel Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:ui type:feature prioritized on a feature board
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants