From 95e41e078c5f2528ab93ff4faebaeeec52f45814 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Wed, 15 Mar 2017 17:42:59 -0400 Subject: [PATCH 1/5] Handle can_start_without_user_input cornercase. --- awx/main/models/jobs.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/awx/main/models/jobs.py b/awx/main/models/jobs.py index 26969ffc32..b960b0fbd4 100644 --- a/awx/main/models/jobs.py +++ b/awx/main/models/jobs.py @@ -310,8 +310,10 @@ class JobTemplate(UnifiedJobTemplate, JobOptions, SurveyJobTemplateMixin, Resour elif self.variables_needed_to_start: variables_needed = True prompting_needed = False - for value in self._ask_for_vars_dict().values(): - if value: + for key, value in self._ask_for_vars_dict().iteritems(): + if value and not (key == 'extra_vars' + and callback_extra_vars is not None + and not variables_needed): prompting_needed = True return (not prompting_needed and not self.passwords_needed_to_start and @@ -1139,7 +1141,7 @@ class JobEvent(CreatedModifiedModel): # Save artifact data to parent job (if provided). if artifact_dict: if event_data and isinstance(event_data, dict): - # Note: Core has not added support for marking artifacts as + # Note: Core has not added support for marking artifacts as # sensitive yet. Going forward, core will not use # _ansible_no_log to denote sensitive set_stats calls. # Instead, they plan to add a flag outside of the traditional From 90bcc3d6ab1405f2ba4a691bb4667bbacba76ee3 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Thu, 16 Mar 2017 11:08:19 -0400 Subject: [PATCH 2/5] Unit test added. --- awx/main/models/jobs.py | 5 ++--- .../tests/unit/models/test_job_template_unit.py | 13 +++++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/awx/main/models/jobs.py b/awx/main/models/jobs.py index b960b0fbd4..992217ec33 100644 --- a/awx/main/models/jobs.py +++ b/awx/main/models/jobs.py @@ -311,9 +311,8 @@ class JobTemplate(UnifiedJobTemplate, JobOptions, SurveyJobTemplateMixin, Resour variables_needed = True prompting_needed = False for key, value in self._ask_for_vars_dict().iteritems(): - if value and not (key == 'extra_vars' - and callback_extra_vars is not None - and not variables_needed): + if value and not (key == 'extra_vars' and + callback_extra_vars is not None): prompting_needed = True return (not prompting_needed and not self.passwords_needed_to_start and diff --git a/awx/main/tests/unit/models/test_job_template_unit.py b/awx/main/tests/unit/models/test_job_template_unit.py index 194ce68cef..a6086b7b9d 100644 --- a/awx/main/tests/unit/models/test_job_template_unit.py +++ b/awx/main/tests/unit/models/test_job_template_unit.py @@ -115,3 +115,16 @@ def test_job_template_survey_mixin_length(job_template_factory): {'type':'password', 'variable':'my_other_variable'}]} kwargs = obj._update_unified_job_kwargs(extra_vars={'my_variable':'$encrypted$'}) assert kwargs['extra_vars'] == '{"my_variable": "my_default"}' + + +def test_job_template_can_start_with_callback_extra_vars_provided(job_template_factory): + objects = job_template_factory( + 'callback_extra_vars_test', + organization='org1', + inventory='inventory1', + credential='cred1', + persisted=False, + ) + obj = objects.job_template + obj.ask_variables_on_launch = True + assert obj.can_start_without_user_input(callback_extra_vars='{"foo": "bar"}') is True From 6083e9482e33282ff07252bffd4e11f4e2d23b92 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Fri, 17 Mar 2017 12:51:19 -0400 Subject: [PATCH 3/5] Refactor job template callback post to mimic the behavior of normal jt launch. --- awx/api/views.py | 10 +++++----- awx/main/models/jobs.py | 13 +++++++++---- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index 4b13a8b2e9..5d7891665d 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2727,14 +2727,14 @@ class JobTemplateCallback(GenericAPIView): return Response(data, status=status.HTTP_400_BAD_REQUEST) # Everything is fine; actually create the job. + kv = {"limit": limit, "launch_type": 'callback'} + if extra_vars is not None and job_template.ask_variables_on_launch: + kv['extra_vars'] = callback_filter_out_ansible_extra_vars(extra_vars) with transaction.atomic(): - job = job_template.create_job(limit=limit, launch_type='callback') + job = job_template.create_job(**kv) # Send a signal to celery that the job should be started. - kv = {"inventory_sources_already_updated": inventory_sources_already_updated} - if extra_vars is not None: - kv['extra_vars'] = callback_filter_out_ansible_extra_vars(extra_vars) - result = job.signal_start(**kv) + result = job.signal_start(inventory_sources_already_updated=inventory_sources_already_updated) if not result: data = dict(msg=_('Error starting job!')) return Response(data, status=status.HTTP_400_BAD_REQUEST) diff --git a/awx/main/models/jobs.py b/awx/main/models/jobs.py index 992217ec33..fd7718514b 100644 --- a/awx/main/models/jobs.py +++ b/awx/main/models/jobs.py @@ -300,6 +300,8 @@ class JobTemplate(UnifiedJobTemplate, JobOptions, SurveyJobTemplateMixin, Resour Return whether job template can be used to start a new job without requiring any user input. ''' + # It is worthwhile to find out if this function is now only used by + # provisioning callback. variables_needed = False if callback_extra_vars: extra_vars_dict = parse_yaml_or_json(callback_extra_vars) @@ -310,10 +312,13 @@ class JobTemplate(UnifiedJobTemplate, JobOptions, SurveyJobTemplateMixin, Resour elif self.variables_needed_to_start: variables_needed = True prompting_needed = False - for key, value in self._ask_for_vars_dict().iteritems(): - if value and not (key == 'extra_vars' and - callback_extra_vars is not None): - prompting_needed = True + # The behavior of provisioning callback should mimic + # that of job template launch, so prompting_needed should + # not block a provisioning callback from creating/launching jobs. + if callback_extra_vars is None: + for value in self._ask_for_vars_dict().values(): + if value: + prompting_needed = True return (not prompting_needed and not self.passwords_needed_to_start and not variables_needed) From 2edd4b338de56d7943730b01ac3ec5cb5143dfd3 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Fri, 17 Mar 2017 15:40:55 -0400 Subject: [PATCH 4/5] Add functional test to gurarantee consistent behavior of provisioning callback with jt launch. --- awx/api/views.py | 3 ++- .../functional/api/test_job_runtime_params.py | 27 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/awx/api/views.py b/awx/api/views.py index 5d7891665d..3c5d5b9dc9 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2678,7 +2678,8 @@ class JobTemplateCallback(GenericAPIView): def post(self, request, *args, **kwargs): extra_vars = None - if request.content_type == "application/json": + # Be careful here: content_type can look like '; charset=blar' + if request.content_type.startswith("application/json"): extra_vars = request.data.get("extra_vars", None) # Permission class should have already validated host_config_key. job_template = self.get_object() diff --git a/awx/main/tests/functional/api/test_job_runtime_params.py b/awx/main/tests/functional/api/test_job_runtime_params.py index af8bd659fd..4412325043 100644 --- a/awx/main/tests/functional/api/test_job_runtime_params.py +++ b/awx/main/tests/functional/api/test_job_runtime_params.py @@ -344,3 +344,30 @@ def test_job_launch_unprompted_vars_with_survey(mocker, survey_spec_factory, job # Check that the survey variable is accepted and the job variable isn't mock_job.signal_start.assert_called_once() + + +@pytest.mark.django_db +@pytest.mark.job_runtime_vars +def test_jt_provisioning_callback(mocker, survey_spec_factory, job_template_prompts, post, admin_user, host): + job_template = job_template_prompts(True) + job_template.host_config_key = "foo" + job_template.survey_enabled = True + job_template.survey_spec = survey_spec_factory('survey_var') + job_template.save() + + with mocker.patch('awx.main.access.BaseAccess.check_license'): + mock_job = mocker.MagicMock(spec=Job, id=968, extra_vars={"job_launch_var": 3, "survey_var": 4}) + with mocker.patch.object(JobTemplate, 'create_unified_job', return_value=mock_job): + with mocker.patch('awx.api.serializers.JobSerializer.to_representation', return_value={}): + with mocker.patch('awx.api.views.JobTemplateCallback.find_matching_hosts', return_value=[host]): + response = post( + reverse('api:job_template_callback', args=[job_template.pk]), + dict(extra_vars={"job_launch_var": 3, "survey_var": 4}, host_config_key="foo"), + admin_user, expect=201, format='json') + assert JobTemplate.create_unified_job.called + assert JobTemplate.create_unified_job.call_args == ({'extra_vars': {'survey_var': 4, + 'job_launch_var': 3}, + 'launch_type': 'callback', + 'limit': 'single-host'},) + + mock_job.signal_start.assert_called_once() From 2e2d88516e73c0639f6be0e326dfb82944610208 Mon Sep 17 00:00:00 2001 From: Aaron Tan Date: Mon, 20 Mar 2017 12:05:35 -0400 Subject: [PATCH 5/5] Negative functional test added. --- awx/main/models/jobs.py | 2 -- .../functional/api/test_job_runtime_params.py | 27 +++++++++++++++++-- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/awx/main/models/jobs.py b/awx/main/models/jobs.py index fd7718514b..388be47d17 100644 --- a/awx/main/models/jobs.py +++ b/awx/main/models/jobs.py @@ -300,8 +300,6 @@ class JobTemplate(UnifiedJobTemplate, JobOptions, SurveyJobTemplateMixin, Resour Return whether job template can be used to start a new job without requiring any user input. ''' - # It is worthwhile to find out if this function is now only used by - # provisioning callback. variables_needed = False if callback_extra_vars: extra_vars_dict = parse_yaml_or_json(callback_extra_vars) diff --git a/awx/main/tests/functional/api/test_job_runtime_params.py b/awx/main/tests/functional/api/test_job_runtime_params.py index 4412325043..e63a074965 100644 --- a/awx/main/tests/functional/api/test_job_runtime_params.py +++ b/awx/main/tests/functional/api/test_job_runtime_params.py @@ -348,7 +348,7 @@ def test_job_launch_unprompted_vars_with_survey(mocker, survey_spec_factory, job @pytest.mark.django_db @pytest.mark.job_runtime_vars -def test_jt_provisioning_callback(mocker, survey_spec_factory, job_template_prompts, post, admin_user, host): +def test_callback_accept_prompted_extra_var(mocker, survey_spec_factory, job_template_prompts, post, admin_user, host): job_template = job_template_prompts(True) job_template.host_config_key = "foo" job_template.survey_enabled = True @@ -360,7 +360,7 @@ def test_jt_provisioning_callback(mocker, survey_spec_factory, job_template_prom with mocker.patch.object(JobTemplate, 'create_unified_job', return_value=mock_job): with mocker.patch('awx.api.serializers.JobSerializer.to_representation', return_value={}): with mocker.patch('awx.api.views.JobTemplateCallback.find_matching_hosts', return_value=[host]): - response = post( + post( reverse('api:job_template_callback', args=[job_template.pk]), dict(extra_vars={"job_launch_var": 3, "survey_var": 4}, host_config_key="foo"), admin_user, expect=201, format='json') @@ -371,3 +371,26 @@ def test_jt_provisioning_callback(mocker, survey_spec_factory, job_template_prom 'limit': 'single-host'},) mock_job.signal_start.assert_called_once() + + +@pytest.mark.django_db +@pytest.mark.job_runtime_vars +def test_callback_ignore_unprompted_extra_var(mocker, survey_spec_factory, job_template_prompts, post, admin_user, host): + job_template = job_template_prompts(False) + job_template.host_config_key = "foo" + job_template.save() + + with mocker.patch('awx.main.access.BaseAccess.check_license'): + mock_job = mocker.MagicMock(spec=Job, id=968, extra_vars={"job_launch_var": 3, "survey_var": 4}) + with mocker.patch.object(JobTemplate, 'create_unified_job', return_value=mock_job): + with mocker.patch('awx.api.serializers.JobSerializer.to_representation', return_value={}): + with mocker.patch('awx.api.views.JobTemplateCallback.find_matching_hosts', return_value=[host]): + post( + reverse('api:job_template_callback', args=[job_template.pk]), + dict(extra_vars={"job_launch_var": 3, "survey_var": 4}, host_config_key="foo"), + admin_user, expect=201, format='json') + assert JobTemplate.create_unified_job.called + assert JobTemplate.create_unified_job.call_args == ({'launch_type': 'callback', + 'limit': 'single-host'},) + + mock_job.signal_start.assert_called_once()