Fix save vCenter credentials functionality

[pcbailey]

This PR fixes an issue where vCenter credentials are not saved unless the create VM wizard is completed, regardless of success or failure. These changes cause the kubevirt.io/temporary label to be removed from the secret if the "Remember vCenter credentials" box is checked and adds the label with a value of true if the "Remember vCenter credentials" box is unchecked; however, these changes are only made if the connection check is successful.

Bug: https://bugzilla.redhat.com/1714004

Depends on: kubevirt/web-ui#399

[coveralls]

Pull Request Test Coverage Report for Build 2035

• 26 of 44 (59.09%) changed or added relevant lines in 7 files are covered.
• 53 unchanged lines in 5 files lost coverage.
• Overall coverage decreased (-1.3%) to 82.807%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/k8s/requests/v2v/createV2VvmwareObject.js	0	1	0.0%
src/k8s/requests/v2v/importVmware.js	0	4	0.0%
src/components/Wizard/CreateVmWizard/stateUpdate/providers/vmWareStateUpdate.js	9	22	40.91%

Files with Coverage Reduction	New Missed Lines	%
src/k8s/util/utils.js	2	60.0%
src/k8s/objects/v2v/vmware/vmWareSecret.js	4	9.09%
src/k8s/request.js	5	80.97%
src/k8s/requests/v2v/utils.js	8	5.0%
src/k8s/requests/v2v/importVmware.js	34	9.59%

Totals
Change from base Build 1744:	-1.3%
Covered Lines:	3982
Relevant Lines:	4607

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 1735

• 10 of 30 (33.33%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.3%) to 83.796%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/k8s/requests/v2v/createV2VvmwareObject.js	0	1	0.0%
src/utils/patches.js	2	10	20.0%
src/components/Wizard/CreateVmWizard/stateUpdate/providers/vmWareStateUpdate.js	5	16	31.25%

Totals
Change from base Build 1724:	-0.3%
Covered Lines:	4036
Relevant Lines:	4611

---

💛 - Coveralls

[mareklibra]

I am not sure if this is the right way to go.
Intentionally, the secret was persisted (from user's perspective) once the wizard is finished only.
Until then (i.e. the user hits cancel), the system stays untouched.

As long as we call the existing button as Check, I would not overload it with the storing functionality (again: meant from user's POV).
If the persisted (long-term) secret is supposed to be created even at the early phase of the wizard, I would redesign the dialog, call the button like Create Connection and the check button rename to i.e. Persistent Connection. Potentially add a new input box for the name (so remove it's auto-generating).

But I do not think this is the right direction. With this small-side-management approach, concerns of the wizard will become huge very soon. It would even make more sense to me to manage "persistent" connection is a separate part of the UI.

Anyway for the BZ 1714004, I think it's enough to keep behaviour as it is now, except changing the last step fired in request.js - let's remove the persistent secret from the rollback if VM creation fails. In that case, the persistent secret will be created

• once wizard finishes only
• independently on VM creation success/failure

@pcbailey, @andybraren and others, what do you think?

PS: if nothing else, the flow around request.js would need to be updated anyway if we go with the approach proposed originally by this PR.

[pcbailey]

As long as we call the existing button as Check, I would not overload it with the storing functionality (again: meant from user's POV).
If the persisted (long-term) secret is supposed to be created even at the early phase of the wizard, I would redesign the dialog, call the button like Create Connection and the check button rename to i.e. Persistent Connection. Potentially add a new input box for the name (so remove it's auto-generating).

This is a good point. If we move forward with the proposed functionality, the button name should be changed to better reflect what it's actually doing. Letting the user decide the name is a good idea, as well.

But I do not think this is the right direction. With this small-side-management approach, concerns of the wizard will become huge very soon. It would even make more sense to me to manage "persistent" connection is a separate part of the UI.

I agree that things could easily get out of control. In terms of managing the connection as a separate part of the UI, what exactly are you thinking?

Anyway for the BZ 1714004, I think it's enough to keep behaviour as it is now, except changing the last step fired in request.js - let's remove the persistent secret from the rollback if VM creation fails. In that case, the persistent secret will be created

• once wizard finishes only
• independently on VM creation success/failure

I haven't tested it, but it's my understanding that this is how it works currently. I did intend to remove the creation of the additional secret at the end of the flow for this PR, but obviously forgot it.

@pcbailey, @andybraren and others, what do you think?

I think it makes sense to only persist the connection once the wizard is completed. I don't expect any changes I make in a wizard to become permanent until I get to the end.

[andybraren]

But I do not think this is the right direction. With this small-side-management approach, concerns of the wizard will become huge very soon. It would even make more sense to me to manage "persistent" connection is a separate part of the UI.

I agree @mareklibra, we definitely don't want to overload this wizard with side-tasks. Allowing users to validate and visibly see a new vCenter connection "saving" inside of Step 1 probably makes sense from a UX perspective though. I can see why this was filed as a bug.

Thankfully I think there's a way to improve this that (hopefully) won't require significant changes.

Here's a screenshot from Marek's recent video (which is super useful by the way, thank you!).

What do we think of this flow?:

1. We relabel "Remember vCenter credentials" to "Save as new vCenter Instance"
2. We make "Save as new vCenter Instance" checked by default
   • Users would probably prefer this most of the time, right?
3. The "Check" button becomes "Check and save"
   • If the user unchecks "Save as new vCenter Instance", this button's label changes to just "Check" like it is today
4. If the credentials are correct and "Save as new vCenter Instance" is checked, the Hostname/Username/Password fields are collapsed, the newly-saved vCenter instance appears in the "vCenter Instance" field, and the "Loading data" spinner appears like this:

FYI @matthewcarleton @lblanchard @yfrimanm @Ranelim

[andybraren]

I agree that things could easily get out of control. In terms of managing the connection as a separate part of the UI, what exactly are you thinking?

I agree 100% that including a mini Create Secret Wizard within a Create VM wizard is a little odd, but until a separate UI is developed (not sure by whom, or where it would live in the console) including this mini-wizard within Create VM is still much better than letting the user figure out how to create the secret correctly on their own.

Hopefully this is the only mini-wizard like this we'll need for Create VM.

I think it makes sense to only persist the connection once the wizard is completed. I don't expect any changes I make in a wizard to become permanent until I get to the end.

@pcbailey Curious to hear what your thoughts are on the proposed flow above.

[pcbailey]

I agree that things could easily get out of control. In terms of managing the connection as a separate part of the UI, what exactly are you thinking?

I agree 100% that including a mini Create Secret Wizard within a Create VM wizard is a little odd, but until a separate UI is developed (not sure by whom, or where it would live in the console) including this mini-wizard within Create VM is still much better than letting the user figure out how to create the secret correctly on their own.

I was wondering about the UX of having the user configure the secret first and then using the wizard. That's why I was asking for more details. I agree that a mini-wizard (I like the term, btw) is a viable option for the time being, unless technical limitations dictate otherwise. I'd like to hear @mareklibra's thoughts on that.

Hopefully this is the only mini-wizard like this we'll need for Create VM.

Agreed!

I think it makes sense to only persist the connection once the wizard is completed. I don't expect any changes I make in a wizard to become permanent until I get to the end.

@pcbailey Curious to hear what your thoughts are on the proposed flow above.

With the proposed changes, I think it's much clearer what's happening and is an acceptable way to handle the issue until/unless a better approach can be developed.

Do you think we should provide any information to the user about how to manage the secret in case they decide after creating/saving it that they really didn't want to store the information or details need to be changed? I don't know that it's going to be obvious to them that it's being stored as a secret.

Also, do you have an opinion one way or another on user-selected naming vs name generation?

[andybraren]

Do you think we should provide any information to the user about how to manage the secret in case they decide after creating/saving it that they really didn't want to store the information or details need to be changed? I don't know that it's going to be obvious to them that it's being stored as a secret.

Great point. Maybe descriptive text underneath the checkbox with an external link to documentation on managing Secrets. Something short like this? (wording is tricky):

[X] Save as new vCenter Instance
These credentials will be stored within a secret. Learn more (external link icon)

Also, do you have an opinion one way or another on user-selected naming vs name generation?

I know @matthewcarleton tested this at Summit and IIRC participants liked the idea of autogenerated names. Matt has been working on a PF4-ified version of this wizard over in the OpenShift Design repo that positions the Name & Description fields at the bottom of Step 1 to support this, but there are some remaining questions about whether/how users should be able to specify their own naming patterns.

[pcbailey]

Great point. Maybe descriptive text underneath the checkbox with an external link to documentation on managing Secrets. Something short like this? (wording is tricky):

[X] Save as new vCenter Instance
These credentials will be stored within a secret. Learn more (external link icon)

That would work nicely. =) The wording does feel a bit off, but I can't think of anything better off the top of my head, either.

Also, do you have an opinion one way or another on user-selected naming vs name generation?

I know @matthewcarleton tested this at Summit and IIRC participants liked the idea of autogenerated names. Matt has been working on a PF4-ified version of this wizard over in the OpenShift Design repo that positions the Name & Description fields at the bottom of Step 1 to support this, but there are some remaining questions about whether/how users should be able to specify their own naming patterns.

Cool. I know users would appreciate that feature once they're importing and creating VMs in bulk. It would also be pretty easy to provide both as an option if we found that enough users wanted to be able to provide custom names.

@matthewcarleton The PF4 mockups look great!

[jelkosz]

> Great point. Maybe descriptive text underneath the checkbox with an external link to documentation on managing Secrets. Something short like this? (wording is tricky):

[X] Save as new vCenter Instance
These credentials will be stored within a secret. Learn more (external link icon)

That would work nicely. =) The wording does feel a bit off, but I can't think of anything better off the top of my head, either.

I don't think the users need to understand that the underlying structure is a kubernetes secret. In this whole VM dialog we are trying to make everything very easy, not trying to explain the underlying concepts. For example when we create a new disk, we are not explaining that we are using DataVolumes / CDI for that. It is just a disk of a specified size from the user's PoV. Here it should just be the connection parameters.

But other than that I really like the small changes proposed here, they will make it very clear what is happening.

[pcbailey]

I don't think the users need to understand that the underlying structure is a kubernetes secret. In this whole VM dialog we are trying to make everything very easy, not trying to explain the underlying concepts. For example when we create a new disk, we are not explaining that we are using DataVolumes / CDI for that. It is just a disk of a specified size from the user's PoV. Here it should just be the connection parameters.

I was looking at it from the perspective that we offer the user the option to store the credentials and they may change their mind later and want to delete them. It's like when I use a credit card online. Sometimes I save the card while processing a transaction and decide afterward that I really don't want to leave that information stored there, so I go back into my account and remove it.

Also, is it not possible that the username or password may change and require updating? I'm not really sure how these things are managed and used in real life, so I'm thinking about it from the way I manage my own data.

[jelkosz]

I was looking at it from the perspective that we offer the user the option to store the credentials and they may change their mind later and want to delete them. It's like when I use a credit card online. Sometimes I save the card while processing a transaction and decide afterward that I really don't want to leave that information stored there, so I go back into my account and remove it.

That is actually a good point. But it than brings a new question: how will the user know which of the secrets in the system is this particular thing stored in?

Im not saying to not to solve this issue, but not in this particular PR.

[pcbailey]

That is actually a good point. But it than brings a new question: how will the user know which of the secrets in the system is this particular thing stored in?

I guess that's where naming comes into play. If the user will be able to define their own naming patterns as @andybraren mentioned previously, then they should be able to identify it. Being able to set their own names at the time the instance is created would also help, if users actually want that option.

Im not saying to not to solve this issue, but not in this particular PR.

Understood. I don't know how important management of these credentials are to our user base and how many instances can be expected in a normal scenario. If it's very important and/or many instances is the norm, then it's probably worth it to look into creating functionality that allows the user to manage them within the more specific context of vCenter credentials instead of the more generic secret context. Just my two cents worth.

[jelkosz]

Understood. I don't know how important management of these credentials are to our user base and how many instances can be expected in a normal scenario. If it's very important and/or many instances is the norm, then it's probably worth it to look into creating functionality that allows the user to manage them within the more specific context of vCenter credentials instead of the more generic secret context. Just my two cents worth.

AFAIK you will typically have one vcenter instance from which you will be migrating the VMs.

[pcbailey]

AFAIK you will typically have one vcenter instance from which you will be migrating the VMs.

Gotcha. Then it's probably not worth investing much time and effort into a separate UI for managing them. As long as the name shows in the list and the user knows that it's stored as a secret, they can search the secrets list by name to find it. My only concerns are whether or not the typical user will know that it's stored as a secret and do we want to tell them about it. I'll leave that up to the UX folks to decide. =) @andybraren Thoughts?

[matthewcarleton]

+1 on what Andy is proposing here.
I agree with what @jelkosz is saying about exposing secrets. Do we need to overload the user with that info in the case where nothing has gone wrong? If there is a connection issue we could prompt them to go fix the credentials via the error text. This requires them to leave the wizard, but I think they would have to do this anyway to proceed.

We then wouldn't need to advise them initially about the secret. I'd also prefer to not send them away from the wizard to learn more about something unless it's to resolve something that's broken.

An alternative to this would be something similar to what Andy is suggesting when they need to update they could do it in the same way they are adding the credentials.

[pcbailey]

+1 on what Andy is proposing here.
I agree with what @jelkosz is saying about exposing secrets. Do we need to overload the user with that info in the case where nothing has gone wrong? If there is a connection issue we could prompt them to go fix the credentials via the error text. This requires them to leave the wizard, but I think they would have to do this anyway to proceed.

I'm ok with this if everyone else is, but I would like for us to at least have something in the documentation that tells a user how to update credentials without having to use the wizard. If I were a user that wanted to update a set of credentials, I would find it odd to have to pretend to import a VM just to make that update. That being said, I also don't know enough about the use case(s) to know how likely this scenario is in the real world. So, if you guys think that it isn't realistic, then I acquiesce. =)

[matthewcarleton]

what if we tweak the text @andybraren is suggesting from
Save as new vCenter Instance
to
Save vCenter credentials in secrets
Not sure if that is exactly right, but maybe hinting at the secrets in the checkbox label could be useful.

[jelkosz]

what if we tweak the text @andybraren is suggesting from
Save as new vCenter Instance
to
Save vCenter credentials in secrets
Not sure if that is exactly right, but maybe hinting at the secrets in the checkbox label could be useful.

yes, that sounds great! Maybe just one correction, not "secrets" but "secret" (we store this credentials in one secret only)

[pcbailey]

My apologies, guys. I think this whole conversation may be moot. I didn't notice that there's already an info icon next to the checkbox with the following text:
If checked, new secret keeping connection details will be created for later use.

If everyone agrees, I'm going to leave the info icon, but improve the wording a tiny bit and change it to the following:
If checked, a new secret containing the connection details will be created for future use.

[jelkosz]

My apologies, guys. I think this whole conversation may be moot. I didn't notice that there's already an info icon next to the checkbox with the following text:
If checked, new secret keeping connection details will be created for later use.

If everyone agrees, I'm going to leave the info icon, but improve the wording a tiny bit and change it to the following:
If checked, a new secret containing the connection details will be created for future use.

+1, sounds good

[matthewcarleton]

@pcbailey just to confirm are we not adding any further functionality to this feature now (no check and save, check and update actions)? I just want to ensure I cover everything in the new designs too :)

[pcbailey]

@matthewcarleton I'll be pushing an update to this PR today that will add parts 1-3 of @andybraren's proposed flow:

1. We relabel "Remember vCenter credentials" to "Save as new vCenter Instance"

2. We make "Save as new vCenter Instance" checked by default

   • Users would probably prefer this most of the time, right?

3. The "Check" button becomes "Check and save"

   • If the user unchecks "Save as new vCenter Instance", this button's label changes to just "Check" like it is today

I'm running into an issue I haven't been able to fix while trying to implement part 4, but we plan to add that in a future update. We just want to get the primary fix into tomorrow's build. Correct me if I'm wrong @jelkosz.

4. If the credentials are correct and "Save as new vCenter Instance" is checked, the Hostname/Username/Password fields are collapsed, the newly-saved vCenter instance appears in the "vCenter Instance" field, and the "Loading data" spinner appears like this:

[matthewcarleton]

ok great! What do we think about the ability to add "check and update" as well. Seems like a nice feature rather than sending the user to go update things and come back. Is there any technical issues with this approach?

[pcbailey]

I like the idea. The only technical concern I can think of off the top of my head would be determining whether the check failed because the credentials are incorrect or from some other issue such as a network timeout. I'm not sure how much information we get when a check fails. I'd also want @mareklibra or @suomiy to weigh in, as they're far more knowledgeable than I am.

[matthewcarleton]

ah good point. In that case we should investigate wording a bit more around different types of errors I suppose. If there is an error hopefully we can provide some meaningful assistance.

…

On Thu, Jun 6, 2019 at 5:22 PM Phillip Bailey ***@***.***> wrote: I like the idea. The only technical concern I can think of off the top of my head would be determining whether the check failed because the credentials are incorrect or from some other issue such as a network timeout. I'm not sure how much information we get when a check fails. I'd also want @mareklibra <https://github.com/mareklibra> or @suomiy <https://github.com/suomiy> to weigh in, as they're far more knowledgeable than I am. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#489?email_source=notifications&email_token=AACGJCBNGSU5GBDOF2DYWADPZFWYHA5CNFSM4HSEAU32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXEBRCI#issuecomment-499652745>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACGJCC55J33BGWICQ7AYZLPZFWYHANCNFSM4HSEAU3Q> .

-- Matthew Carleton Senior Interaction Designer User Experience Design Team Red Hat, Inc. mcarleton@redhat.com <mcarleto@redhat.com>

[jelkosz]

I like the idea. The only technical concern I can think of off the top of my head would be determining whether the check failed because the credentials are incorrect or from some other issue such as a network timeout. I'm not sure how much information we get when a check fails.

well, its our code on the backend, it can return us anything we need. I would just be careful implementing it now. We are not doing any development on master until we move everything to upstream openshift and we are not adding any new features to the stable branch.

In general its worth a little design work, but not really in this PR we need to get in asap :)

[mareklibra]

The only technical concern I can think of off the top of my head would be determining whether the check failed because the credentials are incorrect or from some other issue such as a network timeout. I'm not sure how much information we get when a check fails

We can pass through whatever details needed, the backend is ours. But for 2.0.0 there is no time to do that. Recently just the info about failing connection is provided.

Relevant links to backend:

• https://github.com/oVirt/v2v-conversion-host/blob/b97d946bbbe14f84a1390a65f4f6cba936c64ecf/kubevirt-vmware/pkg/controller/v2vvmware/v2vvmware_controller.go#L21
• https://github.com/oVirt/v2v-conversion-host/blob/b97d946bbbe14f84a1390a65f4f6cba936c64ecf/kubevirt-vmware/pkg/controller/v2vvmware/actions.go#L47

[mareklibra]

LGTM, I think recent PR can work, including garbage-collector implemented in the backend.

Please just remove

• web-ui-components/src/k8s/requests/v2v/importVmware.js

  Line 239 in 9db34ed

  createProviderSecret,

as we moved the logic from last step to the earlier phase.

[matthewcarleton]

@pcbailey @jtomasek @mareklibra that sounds good to me. I'll add it to our list of design updates to follow up on.