👍

Minor: Grant permissions to the ops-user before initializing the appliance

Minor: s/VMFolderSupportVersion/VCHFolderSupportVersion perhaps?

A short comment to describe this field for posterity would be nice to have.

Yeah I wondered whether it was a catch-all (VM) or exclusive to the VCH. I'll clarify.

I think the reason that this happens before is that there's no way to rollback opsuser.GrantOpsUserPerms if this fails.

To summarize this issue:

Today, configure (including upgrade) is organized in the following way:

1. A new configuration for the VCH is constructed based on the old configuration
   and requested changes.
2. The new configuration is validated.
3. The updated version of any ISO files being changed are uploaded.
4. Resource settings are updated, and a defer is registered to undo the change
   if subsequent steps fail.
5. A snapshot is taken.
6. The VCH is updated:
   a. If running, it is powered off.
   b. If volume stores are being added, those are created.
   c. The VCH itself is reconfigured.
   d. The VCH is powered on.
   e. We wait for the VCH to start and the service to be ready.
7. If the update is successful, we update the operations user.
8. If either the update or the operations user update is unsuccessful, we undo:
   a. We rollback to the snapshot we took.
   b. We delete uploaded images.
   c. We delete the snapshot we took.
   d. The defer undoes resource settings updates.

This process has a key characteristic: if a failure occurs, we cleanly return to
the initial state.

However, this process also has a key limitation: the process will fail at (6)(d)
or (6)(e) if the operations user requires additional privileges for the basic
operation of the VCH following the configuration change or upgrade.

A simple solution to this might be to reverse the order of steps 6 and 7: update
the operations user before updating the VCH. This will address the limitation,
but will weaken the guarantee around cleanly returning to the initial state when
a failure occurs; it's not clear how to undo the changes to the operations user.

The effects of this are twofold:

1. Following the rollback of a change where additional privileges were granted
   to the operations user, those additional privileges are left as "cruft".
2. Following the rollback of a change where privileges were revoked from the
   operations user, the VCH will not start.

Options to improve this might include:

1. Looking into ways to undo changes to the operations user, perhaps following a
   similar pattern to the resource settings. This requires reading the old state
   from the system before making changes, including both the role-privilege map
   and resource-role associations.
2. Moving the operations logic in between (6)(c) and (6)(d) instead of between
   (5) and (6). This reduces the cases where we will leave cruft behind.

I am not extremely familiar with this code. But I believe that we must determine what permissions the ops-user has before we begin to assemble the perms that are going to be needed in order to function. If we store that we could defere a configure back to those original permissions. @zjs why not make the ops-user changes before creating the volume-stores as well? They are created by vic-machine, but if we fail to assign the ops-user that will leave 1 less set of things to clean up.

In the event of a failure what is the downside to returning the supplied ops-user to it's original permissions set?

In the event of a failure what is the downside to returning the supplied ops-user to it's original permissions set?

Returning the operations user to the original permissions set is the desired behavior.

But I believe that we must determine what permissions the ops-user has before we begin to assemble the perms that are going to be needed in order to function. If we store that we could defere a configure back to those original permissions.

This is a possible approach, but actually extracting the "before" state is non-trivial. We need to track the changes being made to the privileges for each role as well as the roles being applied to each resource. Because of the way the vSphere APIs work, it's somewhat complicated to read all of that information. Because we use share the same set of roles for multiple operations users, safely rolling back in the presence of concurrent upgrade operations may not be straight-forward.

why not make the ops-user changes before creating the volume-stores as well? They are created by vic-machine, but if we fail to assign the ops-user that will leave 1 less set of things to clean up.

Currently, the operations user changes is the hardest thing to roll back (now tracked by #7814), so we do those as late in the process as possible.

Added #7814 (comment) to describe an alternate pattern, which is likely to be less work than tracking the old privileges/roles.

I'm not sure whether this is the behavior we want. (I'm also not sure that it isn't.)

By including o.GrantPermsLevel = n.GrantPermsLevel in this check, if an administrator uses vic-machine configure to change an operations user's password which previously had a GrantPermsLevel of AddPerms without explicitly including --ops-grant-perms in that command we'll actually downgrade GrantPermsLevel to "". I think that would be surprising behavior: change the password (e.g., due to expiration) and no longer have permissions be automatically managed.

It may be better to have a check here along the lines of clic.IsSet("ops-grant-perms") so that we only adjust GrantPermsLevel if --ops-grant-perms or --ops-grant-perms=false is included on the command line. (To do this, you'd just need to pass clic *cli.Context into the copyChangedConf method.) In that case, we only change the properties a user has asked us to change.

A downside of both the current implementation and this suggestion (and therefore an upside of the proposed change) is that if you change to a completely new operations user we won't clear the GrantPermsLevel. This could also be surprising: this is a case where the user might expect us to change something even though they haven't asked us to (because they may see --ops-grant-perms as being associated with the user, not the VCH). Unfortunately, the way configure is designed really doesn't give us a lot of options here (and leads to similar issues for other settings).

Good catch. There was no clean way to get the name of the option out of the cli flags, nor did there seem to be much reason to declare a constant for it all on its own, but if you find it irksome, I can pull it out as a constant anyway.

I think this can be Attempt To Disable DRS, can't it?

Could be, but it was basically a merge of your additions and @anchal-agrawal's additions, so I left both Attempt To Create Resource Pool and Attempt To Disable DRS in there for variety. Still trying to get granted ops-user perms work after upgrade to pass, so I'll clean it up as I go forward.

Ah, disabling DRS requires less privilege than creating a resource pool. Usually disabling DRS makes a good sanity check that we're not granting the operations user privileges they don't need.

In the specific case that we're using --affinity-vm-group, we currently have to grant the operations user the privilege that lets them disable DRS (as that's the same privilege that lets them manage affinity constructs), so we check creating a resource pool instead.

Missing newline at end of file.