Awx: passwords_needed_to_start isn't properly updated after replacing default vault credential on launch

[mabashian]

ISSUE TYPE

• Bug Report

COMPONENT NAME

• API

SUMMARY

1) Create a vault credential with vault_id: foo and check the box to ask password on launch. (lets say this is ID: 1)
2) Create a second vault credential with vault_id: bar and also check the box to ask password on launch. (lets say this is ID: 2)
3) Create a job template and select the vault credential from step one as a default credential.
4) Click the checkbox to prompt for credentials on job template launch and save.

If we launched this job without any credential changes, the payload would look like:

{
    "credentials": [
        1
    ],
    "credential_passwords": {
        "vault_password.foo": "somepasswordhere"
    }
}

If we wanted to replace the default credential (id: 1) with the other vault credential (id: 2) then I would expect the payload to look like:

{
    "credentials": [
        2
    ],
    "credential_passwords": {
        "vault_password.bar": "somepasswordhere"
    }
}

but when that gets POST'd to the launch endpoint we get back a 400 error with the following:

{passwords_needed_to_start: ["vault_password.foo", "vault_password.bar"]}

Link https://github.com/ansible/awx/issues/352

[ryanpetrello]

@mabashian I think this is a bug introduced by @AlanCoding's workflow prompted credential support. Specifically, it looks like when you specify {'credentials': [2]}, it's _always_ using the "merge with what's on the JT" behavior that was added for workflow prompting:

https://github.com/ansible/awx/blame/devel/awx/main/models/unified_jobs.py#L385

[ryanpetrello]

@mabashian yea, this is indeed a larger issue unrelated to password prompting, but instead related to how credentials are set at JT launch time. This test summarizes the problem:

def test_job_launch_with_explicit_credentials(vault_credential, deploy_jobtemplate, get, post, admin):
    vault_cred = Credential(
        name='Vault #1',
        credential_type=vault_credential.credential_type,
        inputs={
            'vault_password': 'abc-pass',
            'vault_id': 'abc'
        }
    )
    vault_cred.save()
    deploy_jobtemplate.ask_credential_on_launch = True
    deploy_jobtemplate.save()

    url = reverse('api:job_template_launch', kwargs={'pk': deploy_jobtemplate.pk})
    launch_kwargs = {
        'credentials': [vault_cred.pk],
    }
    resp = post(url, launch_kwargs, admin, expect=201)
    assert Job.objects.get(pk=resp.data['id']).credentials.count() == 1

assert Job.objects.get(pk=resp.data['id']).credentials.count() == 1
E   assert 2 == 1

[ryanpetrello]

I've thought on it some more, and I think I understand _why_ Alan’s merge behavior works this way; the point of the credentials merge is that it allows you “add or modify” what’s on the base JT. For example:

• If you have a JT with zero credentials, specifying a machine credential at launch time will use it.

• If you have a JT with a machine credential, specifying a _different_ machine credential at launch time will use that _instead_.

• If you have a JT with a vault credential, specifying a different vault credential _with the same Vault ID_ will use that _instead_.

The merge logic applies to vault credentials, but it’s keyed on the vault ID; in @mabashian’s example, the vault credential he specified at launch time is interpreted as “add to the existing list”, not replace, because its vault ID is _different_ than the one already on the JT.

All of this begs the question: with this merging algorithm, how do you _remove_ a credential at launch time for a vanilla JT? I’m not sure it really _is_ supported, unfortunately, and I think this whole exercise probably suggests that this pattern is a little hard to understand. When @alancoding returns, we should probably revisit this and think on a better approach. I _think_ the most reasonable solution here would be to change (non workflow) JT launch credentials to be an explicit list you always specify (which is what my PR does).

[wenottingham]

There was a long conversation I remember having with @AlanCoding about this; if I recall correctly, having it be a explicit list creates a problem for a) workflow nodes b) JT relaunch where the template can change after you create the node with the explicit list.

[ryanpetrello]

@wenottingham I think the behavior makes sense for credentials you specify on workflow nodes - you must provide a diff in that scenario for the reasons you stated. I think what’s confusing is that manual, vanilla Job Template launches also follow this pattern, and that creates a few problems:

• The API isn’t immediately obvious because credentials is a list of "additions/changes" (a diff), not an explicit list. @mabashian's title for this ticket (passwords_needed_to_start isn't properly updated after replacing default vault credential on launch) illustrates the confusion perfectly; the POST payload he specified in the ticket description _doesn't_ replace the default vault credential. Rather, it just performs a diff and adds the second vault credential to the list (because it has a different Vault ID). The first vault credential is still there, and needs a prompted password, which is why he's seeing an HTTP 400.

• It creates a scenario where you can’t exclude/exempt a default credential at launch time. The merge pattern takes what's on the base Job Template and _adds_ what's in the POST payload, replacing like types. So if you have a Job Template with SSH-1 and you specify credentials=SSH-2, Vault-1 in the POST payload, the resulting job will haveSSH-2andVault-1`. There _is_ no removal, so it's impossible to do what @mabashian is attempting in this ticket (which I think is a valid use case).

For workflow node launches, I don't have any better solutions than the merging pattern, but I really feel like this pattern is not intuitive for API users who just want to manually launch a Job Template with an explicit set of manual-launch-time-specified credentials.

I'm not sure what @mabashian has in mind, but terms of the UI, I'd imagine the most intuitive way to illustrate this for users would be:

• When you manually launch a job template (i.e., not via a workflow node), you see the list of credentials on the job template, and you can edit that list (within the limitations of kind validation). When you launch, _exactly_ that list is used.

• When you edit a workflow node, you have the ability to specify "extra" credentials to apply to the job template, and when the node runs, the merge pattern takes effect, adding to or modifying the original list of credentials on the Job Template.

[AlanCoding]

If WFJT nodes and direct launch doesn't follow the same logic for prompts (the merge logic here in this discussion), then it's not truly a saved launch configuration.

So if you have a Job Template with SSH-1 and you specify credentials=SSH-2, Vault-1 in the POST payload, the resulting job will haveSSH-2andVault-1`. There is no removal, so it's impossible to do what @mabashian is attempting in this ticket (which I think is a valid use case).

slightly tangential to this issue - We do need to support the case where SSH-1 prompts for password, and you launch without providing that password. I can't remember if I specifically addressed that, but I should have, and it does need QE attention.

When you edit a workflow node, you have the ability to specify "extra" credentials to apply to the job template, and when the node runs, the merge pattern takes effect, adding to or modifying the original list of credentials on the Job Template.

Again, this means that the UI flow is different for launch and the WFJT nodes. From the start, making that interaction the same from the API perspective was my goal. I anticipate eventually adding a new endpoint for WFJT nodes like /api/v2/workflow_job_template_nodes/N/apply_prompts/ where you give a set of prompts _as if_ you were launching the JT, and then it saves that to the node. The point is to allow the UI use the exact format as launch. We can totally reuse the launch serializer for this too.

With different credential combination logic for both, this is completely out the window. Although the merge logic is hard to communicate, you need to weigh its negatives against the negatives of this option. Also, making manual launch and WFJT node launch different doesn't fully fix the hard-to-communicate problem, because we still have to communicate it for WFJT nodes, and the UI still needs to build the same wonky merge-based dialog.

[ryanpetrello]

I can appreciate wanting this to all work the same way, and I don't think there's anything inherently _wrong_ with this approach as long as everybody understands it and has buy-in with the limitations. Probably my biggest concern is that (in my opinion) this _is_ a breaking API change for /api/v2/job_templates/N/launch/ behavior (or at the very least, how we previously described credentials=[...] would work at launch time).

@AlanCoding under the merge pattern, is there a way to do what @mabashian is attempting in this issue (launch a JT, but unset one of the default credentials)?

If not, @wenottingham, is that okay (you cannot _remove_ existing credentials from a JT at manual launch time; you can only _replace_ existing ones with like types - i.e., replace an SSH with a _different_ SSH - or append new ones to the list)? This is a fairly notable departure from how launch time credentials work today, but we _are_ supporting an entirely new feature with this change.

[ryanpetrello]

After speaking in depth with @AlanCoding and @wenottingham about this issue, it sounds like the answer to:

under the merge pattern, is there a way to do what @mabashian is attempting in this issue (launch a JT, but unset one of the default credentials)?

...is no, and that's okay. Our plan is to implement launch-time credentials as they currently exist in devel (meaning, they always subscribe to the merging pattern; you can't remove a credential at launch time - you can only add to the list or replace like types).