From b660800c5da63da4d40df5f1758ccaf1947931b4 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Wed, 14 Aug 2019 14:06:01 -0400 Subject: [PATCH 1/3] remove deprecated WFJT node credential field --- awx/api/serializers.py | 75 ++--------------------------------------- awx/main/models/jobs.py | 21 ------------ 2 files changed, 2 insertions(+), 94 deletions(-) diff --git a/awx/api/serializers.py b/awx/api/serializers.py index f4ec276d8c..bc35ed4710 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -1319,12 +1319,6 @@ class ProjectOptionsSerializer(BaseSerializer): return super(ProjectOptionsSerializer, self).validate(attrs) - def to_representation(self, obj): - ret = super(ProjectOptionsSerializer, self).to_representation(obj) - if obj is not None and 'credential' in ret and not obj.credential: - ret['credential'] = None - return ret - class ProjectSerializer(UnifiedJobTemplateSerializer, ProjectOptionsSerializer): @@ -3440,12 +3434,6 @@ class LaunchConfigurationBaseSerializer(BaseSerializer): ret['extra_data'] = obj.display_extra_vars() return ret - def get_summary_fields(self, obj): - summary_fields = super(LaunchConfigurationBaseSerializer, self).get_summary_fields(obj) - # Credential would be an empty dictionary in this case - summary_fields.pop('credential', None) - return summary_fields - def validate(self, attrs): db_extra_data = {} if self.instance: @@ -3527,7 +3515,6 @@ class LaunchConfigurationBaseSerializer(BaseSerializer): class WorkflowJobTemplateNodeSerializer(LaunchConfigurationBaseSerializer): - credential = DeprecatedCredentialField() success_nodes = serializers.PrimaryKeyRelatedField(many=True, read_only=True) failure_nodes = serializers.PrimaryKeyRelatedField(many=True, read_only=True) always_nodes = serializers.PrimaryKeyRelatedField(many=True, read_only=True) @@ -3535,7 +3522,7 @@ class WorkflowJobTemplateNodeSerializer(LaunchConfigurationBaseSerializer): class Meta: model = WorkflowJobTemplateNode - fields = ('*', 'credential', 'workflow_job_template', '-name', '-description', 'id', 'url', 'related', + fields = ('*', 'workflow_job_template', '-name', '-description', 'id', 'url', 'related', 'unified_job_template', 'success_nodes', 'failure_nodes', 'always_nodes',) def get_related(self, obj): @@ -3551,14 +3538,6 @@ class WorkflowJobTemplateNodeSerializer(LaunchConfigurationBaseSerializer): pass return res - def build_field(self, field_name, info, model_class, nested_depth): - # have to special-case the field so that DRF will not automagically make it - # read-only because it's a property on the model. - if field_name == 'credential': - return self.build_standard_field(field_name, - self.credential) - return super(WorkflowJobTemplateNodeSerializer, self).build_field(field_name, info, model_class, nested_depth) - def build_relational_field(self, field_name, relation_info): field_class, field_kwargs = super(WorkflowJobTemplateNodeSerializer, self).build_relational_field(field_name, relation_info) # workflow_job_template is read-only unless creating a new node. @@ -3567,65 +3546,15 @@ class WorkflowJobTemplateNodeSerializer(LaunchConfigurationBaseSerializer): field_kwargs.pop('queryset', None) return field_class, field_kwargs - def validate(self, attrs): - deprecated_fields = {} - if 'credential' in attrs: # TODO: remove when v2 API is deprecated - deprecated_fields['credential'] = attrs.pop('credential') - view = self.context.get('view') - attrs = super(WorkflowJobTemplateNodeSerializer, self).validate(attrs) - ujt_obj = None - if 'unified_job_template' in attrs: - ujt_obj = attrs['unified_job_template'] - elif self.instance: - ujt_obj = self.instance.unified_job_template - if 'credential' in deprecated_fields: # TODO: remove when v2 API is deprecated - cred = deprecated_fields['credential'] - attrs['credential'] = cred - if cred is not None: - if not ujt_obj.ask_credential_on_launch: - raise serializers.ValidationError({"credential": _( - "Related template is not configured to accept credentials on launch.")}) - cred = Credential.objects.get(pk=cred) - view = self.context.get('view', None) - if (not view) or (not view.request) or (view.request.user not in cred.use_role): - raise PermissionDenied() - return attrs - - def create(self, validated_data): # TODO: remove when v2 API is deprecated - deprecated_fields = {} - if 'credential' in validated_data: - deprecated_fields['credential'] = validated_data.pop('credential') - obj = super(WorkflowJobTemplateNodeSerializer, self).create(validated_data) - if 'credential' in deprecated_fields: - if deprecated_fields['credential']: - obj.credentials.add(deprecated_fields['credential']) - return obj - - def update(self, obj, validated_data): # TODO: remove when v2 API is deprecated - deprecated_fields = {} - if 'credential' in validated_data: - deprecated_fields['credential'] = validated_data.pop('credential') - obj = super(WorkflowJobTemplateNodeSerializer, self).update(obj, validated_data) - if 'credential' in deprecated_fields: - existing = obj.credentials.filter(credential_type__kind='ssh') - new_cred = deprecated_fields['credential'] - if new_cred not in existing: - for cred in existing: - obj.credentials.remove(cred) - if new_cred: - obj.credentials.add(new_cred) - return obj - class WorkflowJobNodeSerializer(LaunchConfigurationBaseSerializer): - credential = DeprecatedCredentialField() success_nodes = serializers.PrimaryKeyRelatedField(many=True, read_only=True) failure_nodes = serializers.PrimaryKeyRelatedField(many=True, read_only=True) always_nodes = serializers.PrimaryKeyRelatedField(many=True, read_only=True) class Meta: model = WorkflowJobNode - fields = ('*', 'credential', 'job', 'workflow_job', '-name', '-description', 'id', 'url', 'related', + fields = ('*', 'job', 'workflow_job', '-name', '-description', 'id', 'url', 'related', 'unified_job_template', 'success_nodes', 'failure_nodes', 'always_nodes', 'do_not_run',) diff --git a/awx/main/models/jobs.py b/awx/main/models/jobs.py index ceff1c8cb7..f3fc42bd46 100644 --- a/awx/main/models/jobs.py +++ b/awx/main/models/jobs.py @@ -920,27 +920,6 @@ class LaunchTimeConfigBase(BaseModel): def display_extra_data(self): return self.display_extra_vars() - @property - def _credential(self): - ''' - Only used for workflow nodes to support backward compatibility. - ''' - try: - return [cred for cred in self.credentials.all() if cred.credential_type.kind == 'ssh'][0] - except IndexError: - return None - - @property - def credential(self): - ''' - Returns an integer so it can be used as IntegerField in serializer - ''' - cred = self._credential - if cred is not None: - return cred.pk - else: - return None - class LaunchTimeConfig(LaunchTimeConfigBase): ''' From f230da543792dc0db0bb8f67bad9442062039e69 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Thu, 15 Aug 2019 09:45:05 -0400 Subject: [PATCH 2/3] update tests for credential removal --- .../functional/api/test_workflow_node.py | 75 +++++++++---------- 1 file changed, 35 insertions(+), 40 deletions(-) diff --git a/awx/main/tests/functional/api/test_workflow_node.py b/awx/main/tests/functional/api/test_workflow_node.py index 6de3e5b533..3caddbd286 100644 --- a/awx/main/tests/functional/api/test_workflow_node.py +++ b/awx/main/tests/functional/api/test_workflow_node.py @@ -129,6 +129,12 @@ class TestNodeCredentials: under the "credentials" key - WFJT nodes have a many-to-many relationship corresponding to this, and it must follow rules consistent with other prompts ''' + @pytest.fixture + def job_template_ask(self, job_template): + job_template.ask_credential_on_launch = True + job_template.save() + return job_template + def test_not_allows_non_job_models(self, post, admin_user, workflow_job_template, project, machine_credential): node = WorkflowJobTemplateNode.objects.create( @@ -146,21 +152,6 @@ class TestNodeCredentials: ) assert 'cannot accept credentials on launch' in str(r.data['msg']) - -@pytest.mark.django_db -class TestOldCredentialField: - ''' - The field `credential` on JTs & WFJT nodes is deprecated, but still supported - - TODO: remove tests when JT vault_credential / credential / other stuff - is removed - ''' - @pytest.fixture - def job_template_ask(self, job_template): - job_template.ask_credential_on_launch = True - job_template.save() - return job_template - def test_credential_accepted_create(self, workflow_job_template, post, admin_user, job_template_ask, machine_credential): r = post( @@ -168,16 +159,16 @@ class TestOldCredentialField: 'api:workflow_job_template_workflow_nodes_list', kwargs = {'pk': workflow_job_template.pk} ), - data = {'credential': machine_credential.pk, 'unified_job_template': job_template_ask.pk}, + data = {'unified_job_template': job_template_ask.pk}, user = admin_user, expect = 201 ) - assert r.data['credential'] == machine_credential.pk node = WorkflowJobTemplateNode.objects.get(pk=r.data['id']) + post(url=r.data['related']['credentials'], data={'id': machine_credential.pk}, user=admin_user, expect=204) assert list(node.credentials.all()) == [machine_credential] @pytest.mark.parametrize('role,code', [ - ['use_role', 201], + ['use_role', 204], ['read_role', 403] ]) def test_credential_rbac(self, role, code, workflow_job_template, post, rando, @@ -186,39 +177,41 @@ class TestOldCredentialField: role_obj.members.add(rando) job_template_ask.execute_role.members.add(rando) workflow_job_template.admin_role.members.add(rando) - post( + r = post( reverse( 'api:workflow_job_template_workflow_nodes_list', kwargs = {'pk': workflow_job_template.pk} ), - data = {'credential': machine_credential.pk, 'unified_job_template': job_template_ask.pk}, + data = {'unified_job_template': job_template_ask.pk}, user = rando, - expect = code + expect = 201 ) + creds_url = r.data['related']['credentials'] + post(url=creds_url, data={'id': machine_credential.pk}, user=rando, expect=code) - def test_credential_add_remove(self, node, patch, machine_credential, admin_user): + def test_credential_add_remove(self, node, get, post, machine_credential, admin_user): node.unified_job_template.ask_credential_on_launch = True node.unified_job_template.save() url = node.get_absolute_url() - patch( - url, - data = {'credential': machine_credential.pk}, + r = get(url=url, user=admin_user, expect=200) + post( + url = r.data['related']['credentials'], + data = {'id': machine_credential.pk}, user = admin_user, - expect = 200 + expect = 204 ) node.refresh_from_db() - assert node.credential == machine_credential.pk - patch( - url, - data = {'credential': None}, + post( + url = r.data['related']['credentials'], + data = {'id': machine_credential.pk, 'disassociate': True}, user = admin_user, - expect = 200 + expect = 204 ) node.refresh_from_db() assert list(node.credentials.values_list('pk', flat=True)) == [] - def test_credential_replace(self, node, patch, credentialtype_ssh, admin_user): + def test_credential_replace(self, node, get, post, credentialtype_ssh, admin_user): node.unified_job_template.ask_credential_on_launch = True node.unified_job_template.save() cred1 = Credential.objects.create( @@ -230,12 +223,14 @@ class TestOldCredentialField: name='machine-cred2', inputs={'username': 'test_user', 'password': 'pas4word'}) node.credentials.add(cred1) - assert node.credential == cred1.pk url = node.get_absolute_url() - patch( - url, - data = {'credential': cred2.pk}, - user = admin_user, - expect = 200 - ) - assert node.credential == cred2.pk + r = get(url=url, user=admin_user, expect=200) + creds_url = r.data['related']['credentials'] + # cannot do it this way + r2 = post(url=creds_url, data={'id': cred2.pk}, user=admin_user, expect=400) + assert 'This launch configuration already provides a Machine credential' in r2.data['msg'] + # guess I will remove that existing one + post(url=creds_url, data={'id': cred1.pk, 'disassociate': True}, user=admin_user, expect=204) + # okay, now I will add the new one + post(url=creds_url, data={'id': cred2.pk}, user=admin_user, expect=204) + assert list(node.credentials.values_list('id', flat=True)) == [cred2.pk] From 4e99ad3e27b652c95491a9206fe995959145813e Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Thu, 15 Aug 2019 22:41:06 -0400 Subject: [PATCH 3/3] minor doc update --- docs/workflow.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/workflow.md b/docs/workflow.md index e4becca74f..8043b0f37e 100644 --- a/docs/workflow.md +++ b/docs/workflow.md @@ -149,7 +149,7 @@ The resulting state of the workflow job run above would be 'successful'. Althoug * Verify that a subtree of execution will never start if its root node is successfully canceled. * Verify that cancelling a workflow job that is cancellable will consequently cancel any of its cancellable spawned jobs and thus interrupts the whole workflow execution. * Verify that during a workflow job run, deleting its spawned jobs are prohibited. -* Verify that at the beginning of each spawned job run, its prompted fields will be populated by the wrapping workflow job node with corrected values. For example, `credential` field of workflow job node goes to `credential` field of spawned job. +* Verify that at the beginning of each spawned job run, its prompted fields will be populated by the wrapping workflow job node with corrected values. For example, related `credentials` of workflow job node go to `credentials` of spawned job. * Verify that notification templates can be successfully (dis)associated with a workflow job template. Later when its spawned workflow jobs finish running, verify that the correct type of notifications will be sent according to the job status. * Verify that a workflow job can be successfully relaunched.