From b38e08174a29bb4d5bb80b2587271203dae9ba74 Mon Sep 17 00:00:00 2001 From: Alan Rominger Date: Fri, 16 Sep 2022 09:14:16 -0400 Subject: [PATCH] Write logic to combing workflow labels, IGs with nodes Additionally, move the inventory-specific hacks of yesteryear into the prompts_dict method of the WorkflowJob model try to make it clear exactly what this is hacking and why Correctly summarize label prompts, and add missing EE Expand unit tests to apply more fields adding missing fields to preserve during copy to workflow.py Fix bug where empty workflow job vars blanked node vars (#12904) * Fix bug where empty workflow job vars blanked node vars * Fix bug where workflow job has no extra_vars, add test * Add empty workflow job extra vars to assure fix --- awx/api/serializers.py | 4 +- awx/api/views/__init__.py | 1 + awx/main/models/jobs.py | 11 ++- awx/main/models/unified_jobs.py | 2 +- awx/main/models/workflow.py | 98 +++++++++++-------- awx/main/tests/functional/api/test_job.py | 26 +++-- .../models/test_job_launch_config.py | 66 ++++++------- .../tests/functional/models/test_workflow.py | 62 ++++++++++++ 8 files changed, 183 insertions(+), 87 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index ce0d7ce4ca..23a3da9eae 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -3223,9 +3223,9 @@ class JobCreateScheduleSerializer(LabelsListMixin, BaseSerializer): ret[field_name] = self._summarize(field_name, ret[field_name]) for field_name, singular in (('credentials', 'credential'), ('instance_groups', 'instance_group')): if field_name in ret: - ret[field_name] = [self._summarize(singular, cred) for cred in ret[field_name]] + ret[field_name] = [self._summarize(singular, obj) for obj in ret[field_name]] if 'labels' in ret: - ret['labels'] = self._summary_field_labels(obj) + ret['labels'] = self._summary_field_labels(config) return ret except JobLaunchConfig.DoesNotExist: return {'all': _('Unknown, job may have been ran before launch configurations were saved.')} diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index 1b6e8eadde..9b34f0f794 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -3726,6 +3726,7 @@ class JobCreateSchedule(RetrieveAPIView): extra_data=config.extra_data, survey_passwords=config.survey_passwords, inventory=config.inventory, + execution_environment=config.execution_environment, char_prompts=config.char_prompts, credentials=set(config.credentials.all()), labels=set(config.labels.all()), diff --git a/awx/main/models/jobs.py b/awx/main/models/jobs.py index aa88cc533e..1713b6dc43 100644 --- a/awx/main/models/jobs.py +++ b/awx/main/models/jobs.py @@ -983,13 +983,16 @@ class LaunchTimeConfigBase(BaseModel): data[prompt_name] = prompt_values elif prompt_name == 'extra_vars': if self.extra_vars: + extra_vars = {} if display: - data[prompt_name] = self.display_extra_vars() + extra_vars = self.display_extra_vars() else: - data[prompt_name] = self.extra_vars + extra_vars = self.extra_vars # Depending on model, field type may save and return as string - if isinstance(data[prompt_name], str): - data[prompt_name] = parse_yaml_or_json(data[prompt_name]) + if isinstance(extra_vars, str): + extra_vars = parse_yaml_or_json(extra_vars) + if extra_vars: + data['extra_vars'] = extra_vars if self.survey_passwords and not display: data['survey_passwords'] = self.survey_passwords else: diff --git a/awx/main/models/unified_jobs.py b/awx/main/models/unified_jobs.py index 19ce50d986..d5aadfe72e 100644 --- a/awx/main/models/unified_jobs.py +++ b/awx/main/models/unified_jobs.py @@ -1007,7 +1007,7 @@ class UnifiedJob( # Here we are doing a loop to make sure we preserve order in case this is a Ordered field job_item = kwargs.get(field_name, []) if job_item: - parent_items = getattr(parent, field_name, []).all() + parent_items = list(getattr(parent, field_name, []).all()) for item in job_item: # Do not include this item in the config if its in the parent if item not in parent_items: diff --git a/awx/main/models/workflow.py b/awx/main/models/workflow.py index 8dcdb3d573..3bc3ee2ec5 100644 --- a/awx/main/models/workflow.py +++ b/awx/main/models/workflow.py @@ -166,6 +166,9 @@ class WorkflowJobTemplateNode(WorkflowNodeBase): 'char_prompts', 'all_parents_must_converge', 'identifier', + 'labels', + 'execution_environment', + 'instance_groups', ] REENCRYPTION_BLOCKLIST_AT_COPY = ['extra_data', 'survey_passwords'] @@ -288,19 +291,6 @@ class WorkflowJobNode(WorkflowNodeBase): def get_absolute_url(self, request=None): return reverse('api:workflow_job_node_detail', kwargs={'pk': self.pk}, request=request) - def prompts_dict(self, *args, **kwargs): - r = super(WorkflowJobNode, self).prompts_dict(*args, **kwargs) - # Explanation - WFJT extra_vars still break pattern, so they are not - # put through prompts processing, but inventory and others are only accepted - # if JT prompts for it, so it goes through this mechanism - if self.workflow_job: - if self.workflow_job.inventory_id: - # workflow job inventory takes precedence - r['inventory'] = self.workflow_job.inventory - if self.workflow_job.char_prompts: - r.update(self.workflow_job.char_prompts) - return r - def get_job_kwargs(self): """ In advance of creating a new unified job as part of a workflow, @@ -310,16 +300,38 @@ class WorkflowJobNode(WorkflowNodeBase): """ # reject/accept prompted fields data = {} + wj_special_vars = {} + wj_special_passwords = {} ujt_obj = self.unified_job_template if ujt_obj is not None: - # MERGE note: move this to prompts_dict method on node when merging - # with the workflow inventory branch - prompts_data = self.prompts_dict() - if isinstance(ujt_obj, WorkflowJobTemplate): - if self.workflow_job.extra_vars: - prompts_data.setdefault('extra_vars', {}) - prompts_data['extra_vars'].update(self.workflow_job.extra_vars_dict) - accepted_fields, ignored_fields, errors = ujt_obj._accept_or_ignore_job_kwargs(**prompts_data) + node_prompts_data = self.prompts_dict() + wj_prompts_data = self.workflow_job.prompts_dict() + # Explanation - special historical case + # WFJT extra_vars ignored JobTemplate.ask_variables_on_launch, bypassing _accept_or_ignore_job_kwargs + # inventory and others are only accepted if JT prompts for it with related ask_ field + # this is inconsistent, but maintained + if not isinstance(ujt_obj, WorkflowJobTemplate): + wj_special_vars = wj_prompts_data.pop('extra_vars', {}) + wj_special_passwords = wj_prompts_data.pop('survey_passwords', {}) + elif 'extra_vars' in node_prompts_data: + # Follow the vars combination rules + node_prompts_data['extra_vars'].update(wj_prompts_data.pop('extra_vars', {})) + elif 'survey_passwords' in node_prompts_data: + node_prompts_data['survey_passwords'].update(wj_prompts_data.pop('survey_passwords', {})) + + # Follow the credential combination rules + if ('credentials' in wj_prompts_data) and ('credentials' in node_prompts_data): + wj_pivoted_creds = Credential.unique_dict(wj_prompts_data['credentials']) + node_pivoted_creds = Credential.unique_dict(node_prompts_data['credentials']) + node_pivoted_creds.update(wj_pivoted_creds) + wj_prompts_data['credentials'] = [cred for cred in node_pivoted_creds.values()] + + # NOTE: no special rules for instance_groups, because they do not merge + # or labels, because they do not propogate WFJT-->node at all + + # Combine WFJT prompts with node here, WFJT at higher level + node_prompts_data.update(wj_prompts_data) + accepted_fields, ignored_fields, errors = ujt_obj._accept_or_ignore_job_kwargs(**node_prompts_data) if errors: logger.info( _('Bad launch configuration starting template {template_pk} as part of ' 'workflow {workflow_pk}. Errors:\n{error_text}').format( @@ -327,15 +339,6 @@ class WorkflowJobNode(WorkflowNodeBase): ) ) data.update(accepted_fields) # missing fields are handled in the scheduler - try: - # config saved on the workflow job itself - wj_config = self.workflow_job.launch_config - except ObjectDoesNotExist: - wj_config = None - if wj_config: - accepted_fields, ignored_fields, errors = ujt_obj._accept_or_ignore_job_kwargs(**wj_config.prompts_dict()) - accepted_fields.pop('extra_vars', None) # merge handled with other extra_vars later - data.update(accepted_fields) # build ancestor artifacts, save them to node model for later aa_dict = {} is_root_node = True @@ -348,15 +351,12 @@ class WorkflowJobNode(WorkflowNodeBase): self.ancestor_artifacts = aa_dict self.save(update_fields=['ancestor_artifacts']) # process password list - password_dict = {} + password_dict = data.get('survey_passwords', {}) if '_ansible_no_log' in aa_dict: for key in aa_dict: if key != '_ansible_no_log': password_dict[key] = REPLACE_STR - if self.workflow_job.survey_passwords: - password_dict.update(self.workflow_job.survey_passwords) - if self.survey_passwords: - password_dict.update(self.survey_passwords) + password_dict.update(wj_special_passwords) if password_dict: data['survey_passwords'] = password_dict # process extra_vars @@ -366,12 +366,12 @@ class WorkflowJobNode(WorkflowNodeBase): functional_aa_dict = copy(aa_dict) functional_aa_dict.pop('_ansible_no_log', None) extra_vars.update(functional_aa_dict) - if ujt_obj and isinstance(ujt_obj, JobTemplate): - # Workflow Job extra_vars higher precedence than ancestor artifacts - if self.workflow_job and self.workflow_job.extra_vars: - extra_vars.update(self.workflow_job.extra_vars_dict) + + # Workflow Job extra_vars higher precedence than ancestor artifacts + extra_vars.update(wj_special_vars) if extra_vars: data['extra_vars'] = extra_vars + # ensure that unified jobs created by WorkflowJobs are marked data['_eager_fields'] = {'launch_type': 'workflow'} if self.workflow_job and self.workflow_job.created_by: @@ -461,6 +461,7 @@ class WorkflowJobTemplate(UnifiedJobTemplate, WorkflowJobOptions, SurveyJobTempl 'survey_spec', 'skip_tags', 'job_tags', + 'execution_environment', ] class Meta: @@ -742,6 +743,25 @@ class WorkflowJob(UnifiedJob, WorkflowJobOptions, SurveyJobMixin, JobNotificatio artifacts.update(job.get_effective_artifacts(parents_set=new_parents_set)) return artifacts + def prompts_dict(self, *args, **kwargs): + if self.job_template_id: + # HACK: Exception for sliced jobs here, this is bad + # when sliced jobs were introduced, workflows did not have all the prompted JT fields + # so to support prompting with slicing, we abused the workflow job launch config + # these would be more properly saved on the workflow job, but it gets the wrong fields now + try: + wj_config = self.launch_config + r = wj_config.prompts_dict(*args, **kwargs) + except ObjectDoesNotExist: + r = {} + else: + r = super().prompts_dict(*args, **kwargs) + # Workflow labels and job labels are treated separately + # that means that they do not propogate from WFJT / workflow job to jobs in workflow + r.pop('labels', None) + + return r + def get_notification_templates(self): return self.workflow_job_template.notification_templates diff --git a/awx/main/tests/functional/api/test_job.py b/awx/main/tests/functional/api/test_job.py index 8c405c4e0e..53e31e5981 100644 --- a/awx/main/tests/functional/api/test_job.py +++ b/awx/main/tests/functional/api/test_job.py @@ -13,17 +13,11 @@ from django.utils import timezone # AWX from awx.api.versioning import reverse from awx.api.views import RelatedJobsPreventDeleteMixin, UnifiedJobDeletionMixin -from awx.main.models import ( - JobTemplate, - User, - Job, - AdHocCommand, - ProjectUpdate, -) +from awx.main.models import JobTemplate, User, Job, AdHocCommand, ProjectUpdate, InstanceGroup, Label, Organization @pytest.mark.django_db -def test_job_relaunch_permission_denied_response(post, get, inventory, project, credential, net_credential, machine_credential): +def test_job_relaunch_permission_denied_response(post, get, inventory, project, net_credential, machine_credential): jt = JobTemplate.objects.create(name='testjt', inventory=inventory, project=project, ask_credential_on_launch=True) jt.credentials.add(machine_credential) jt_user = User.objects.create(username='jobtemplateuser') @@ -39,6 +33,22 @@ def test_job_relaunch_permission_denied_response(post, get, inventory, project, job.launch_config.credentials.add(net_credential) r = post(reverse('api:job_relaunch', kwargs={'pk': job.pk}), {}, jt_user, expect=403) assert 'launched with prompted fields you do not have access to' in r.data['detail'] + job.launch_config.credentials.clear() + + # Job has prompted instance group that user cannot see + job.launch_config.instance_groups.add(InstanceGroup.objects.create()) + r = post(reverse('api:job_relaunch', kwargs={'pk': job.pk}), {}, jt_user, expect=403) + assert 'launched with prompted fields you do not have access to' in r.data['detail'] + job.launch_config.instance_groups.clear() + + # Job has prompted label that user cannot see + job.launch_config.labels.add(Label.objects.create(organization=Organization.objects.create())) + r = post(reverse('api:job_relaunch', kwargs={'pk': job.pk}), {}, jt_user, expect=403) + assert 'launched with prompted fields you do not have access to' in r.data['detail'] + job.launch_config.labels.clear() + + # without any of those prompts, user can launch + r = post(reverse('api:job_relaunch', kwargs={'pk': job.pk}), {}, jt_user, expect=201) @pytest.mark.django_db diff --git a/awx/main/tests/functional/models/test_job_launch_config.py b/awx/main/tests/functional/models/test_job_launch_config.py index 57a453768b..8f8e56522d 100644 --- a/awx/main/tests/functional/models/test_job_launch_config.py +++ b/awx/main/tests/functional/models/test_job_launch_config.py @@ -1,7 +1,7 @@ import pytest # AWX -from awx.main.models.jobs import JobTemplate, JobLaunchConfig, LaunchTimeConfigBase +from awx.main.models.jobs import JobTemplate, LaunchTimeConfigBase from awx.main.models.execution_environments import ExecutionEnvironment @@ -12,18 +12,6 @@ def full_jt(inventory, project, machine_credential): return jt -@pytest.fixture -def config_factory(full_jt): - def return_config(data): - job = full_jt.create_unified_job(**data) - try: - return job.launch_config - except JobLaunchConfig.DoesNotExist: - return None - - return return_config - - @pytest.mark.django_db class TestConfigCreation: """ @@ -41,39 +29,51 @@ class TestConfigCreation: assert config.limit == 'foobar' assert config.char_prompts == {'limit': 'foobar'} - def test_added_credential(self, full_jt, credential): - job = full_jt.create_unified_job(credentials=[credential]) + def test_added_related(self, full_jt, credential, default_instance_group, label): + job = full_jt.create_unified_job(credentials=[credential], instance_groups=[default_instance_group], labels=[label]) config = job.launch_config assert set(config.credentials.all()) == set([credential]) + assert set(config.labels.all()) == set([label]) + assert set(config.instance_groups.all()) == set([default_instance_group]) def test_survey_passwords_ignored(self, inventory_source): iu = inventory_source.create_unified_job(survey_passwords={'foo': '$encrypted$'}) assert iu.launch_config.prompts_dict() == {} +@pytest.fixture +def full_prompts_dict(inventory, credential, label, default_instance_group): + ee = ExecutionEnvironment.objects.create(name='test-ee', image='quay.io/foo/bar') + r = { + 'limit': 'foobar', + 'inventory': inventory, + 'credentials': [credential], + 'execution_environment': ee, + 'labels': [label], + 'instance_groups': [default_instance_group], + 'verbosity': 3, + 'scm_branch': 'non_dev', + 'diff_mode': True, + 'skip_tags': 'foobar', + 'job_tags': 'untagged', + 'forks': 26, + 'job_slice_count': 2, + 'timeout': 200, + 'extra_vars': {'prompted_key': 'prompted_val'}, + 'job_type': 'check', + } + assert set(JobTemplate.get_ask_mapping().keys()) - set(r.keys()) == set() # make fixture comprehensive + return r + + @pytest.mark.django_db -class TestConfigReversibility: +def test_config_reversibility(full_jt, full_prompts_dict): """ Checks that a blob of saved prompts will be re-created in the prompts_dict for launching new jobs """ - - def test_char_field_only(self, config_factory): - config = config_factory({'limit': 'foobar'}) - assert config.prompts_dict() == {'limit': 'foobar'} - - def test_related_objects(self, config_factory, inventory, credential, label, default_instance_group): - ee = ExecutionEnvironment.objects.create(name='test-ee', image='quay.io/foo/bar') - prompts = { - 'limit': 'foobar', - 'inventory': inventory, - 'credentials': [credential], - 'execution_environment': ee, - 'labels': [label], - 'instance_groups': [default_instance_group], - } - config = config_factory(prompts) - assert config.prompts_dict() == prompts + config = full_jt.create_unified_job(**full_prompts_dict).launch_config + assert config.prompts_dict() == full_prompts_dict @pytest.mark.django_db diff --git a/awx/main/tests/functional/models/test_workflow.py b/awx/main/tests/functional/models/test_workflow.py index b6df98fe59..a21fbaa73b 100644 --- a/awx/main/tests/functional/models/test_workflow.py +++ b/awx/main/tests/functional/models/test_workflow.py @@ -12,6 +12,9 @@ from awx.main.models.workflow import ( ) from awx.main.models.jobs import JobTemplate, Job from awx.main.models.projects import ProjectUpdate +from awx.main.models.credential import Credential, CredentialType +from awx.main.models.label import Label +from awx.main.models.ha import InstanceGroup from awx.main.scheduler.dag_workflow import WorkflowDAG from awx.api.versioning import reverse from awx.api.views import WorkflowJobTemplateNodeSuccessNodesList @@ -229,6 +232,65 @@ class TestWorkflowJob: assert queued_node.get_job_kwargs()['extra_vars'] == {'a': 42, 'b': 43} assert queued_node.ancestor_artifacts == {'a': 42, 'b': 43} + def test_combine_prompts_WFJT_to_node(self, project, inventory, organization): + """ + Test that complex prompts like variables, credentials, labels, etc + are properly combined from the workflow-level with the node-level + """ + jt = JobTemplate.objects.create( + project=project, + inventory=inventory, + ask_variables_on_launch=True, + ask_credential_on_launch=True, + ask_instance_groups_on_launch=True, + ask_labels_on_launch=True, + ask_limit_on_launch=True, + ) + wj = WorkflowJob.objects.create(name='test-wf-job', extra_vars='{}') + + common_ig = InstanceGroup.objects.create(name='common') + common_ct = CredentialType.objects.create(name='common') + + node = WorkflowJobNode.objects.create(workflow_job=wj, unified_job_template=jt, extra_vars={'node_key': 'node_val'}) + node.limit = 'node_limit' + node.save() + node_cred_unique = Credential.objects.create(credential_type=CredentialType.objects.create(name='node')) + node_cred_conflicting = Credential.objects.create(credential_type=common_ct) + node.credentials.add(node_cred_unique, node_cred_conflicting) + node_labels = [Label.objects.create(name='node1', organization=organization), Label.objects.create(name='node2', organization=organization)] + node.labels.add(*node_labels) + node_igs = [common_ig, InstanceGroup.objects.create(name='node')] + for ig in node_igs: + node.instance_groups.add(ig) + + # assertions for where node has prompts but workflow job does not + data = node.get_job_kwargs() + assert data['extra_vars'] == {'node_key': 'node_val'} + assert set(data['credentials']) == set([node_cred_conflicting, node_cred_unique]) + assert data['instance_groups'] == node_igs + assert set(data['labels']) == set(node_labels) + assert data['limit'] == 'node_limit' + + # add prompts to the WorkflowJob + wj.limit = 'wj_limit' + wj.extra_vars = {'wj_key': 'wj_val'} + wj.save() + wj_cred_unique = Credential.objects.create(credential_type=CredentialType.objects.create(name='wj')) + wj_cred_conflicting = Credential.objects.create(credential_type=common_ct) + wj.credentials.add(wj_cred_unique, wj_cred_conflicting) + wj.labels.add(Label.objects.create(name='wj1', organization=organization), Label.objects.create(name='wj2', organization=organization)) + wj_igs = [InstanceGroup.objects.create(name='wj'), common_ig] + for ig in wj_igs: + wj.instance_groups.add(ig) + + # assertions for behavior where node and workflow jobs have prompts + data = node.get_job_kwargs() + assert data['extra_vars'] == {'node_key': 'node_val', 'wj_key': 'wj_val'} + assert set(data['credentials']) == set([wj_cred_unique, wj_cred_conflicting, node_cred_unique]) + assert data['instance_groups'] == wj_igs + assert set(data['labels']) == set(node_labels) # as exception, WFJT labels not applied + assert data['limit'] == 'wj_limit' + @pytest.mark.django_db class TestWorkflowJobTemplate: