API Authentication create #14217

jntullo · 2017-03-07T16:29:17Z

cc: @h-kataria
@miq-bot add_label api, wip, enhancement
@miq-bot assign @abellotti

abellotti · 2017-03-09T14:40:34Z

app/controllers/api/subcollections/authentications.rb

@@ -4,6 +4,15 @@ module Authentications
      def authentications_query_resource(object)
        object.respond_to?(:authentications) ? object.authentications : []
      end
+
+      def authentications_create_resource(parent, _type, _id, data)
+        raise 'must supply a type' unless data['type']


why is type needed when creating as a subcollection ?

or is it the manager reference that's no longer needed.

@abellotti it's the manager reference that's no longer needed

abellotti · 2017-03-09T14:41:25Z

app/controllers/api/authentications_controller.rb

@@ -1,4 +1,20 @@
 module Api
  class AuthenticationsController < BaseController
+    def create_resource(_type, _id, data)
+      validate_auth_attrs(data)
+      klass = data['type'].constantize


we shouldn't be constantizing based on user specified data. Let's have the model lookup data['type'] for us and return the appropriate klass.

@abellotti I thought we decided the other day we would take in the type?

abellotti · 2017-03-09T14:46:50Z

app/controllers/api/authentications_controller.rb

+      klass = data['type'].constantize
+      # TODO: Temporary validation - remove
+      raise 'type not currently supported' unless klass.respond_to?(:create_in_provider_queue)
+      klass.create_in_provider_queue(parse_id(data['manager'], :providers), data.except('type', 'manager'))


is authentications always targeted to manager type CI's or could they belong to other types ? if so, I wonder if we should have a more generic reference (maybe "resource" : { "href" : ... })

abellotti · 2017-03-09T14:48:52Z

spec/requests/api/authentications_spec.rb

+    it 'requires that the type support create_in_provider_queue' do
+      api_basic_authorize collection_action_identifier(:authentications, :create, :post)
+
+      run_post(authentications_url, :action => 'create', :type => 'Authentication', :manager => 'blah')


I see, type is 'Authentication', should we default to this type if not specified ? (it is /api/authentications after all)

jntullo · 2017-03-13T16:07:51Z

app/controllers/api/authentications_controller.rb

+      raise 'type not currently supported' unless klass.respond_to?(:create_in_provider_queue)
+      manager_resource_id = parse_href(data['manager_resource']['href']).last
+      task_id = klass.create_in_provider_queue(manager_resource_id, data.except('type', 'manager_resource'))
+      MiqTask.find(task_id)


@Fryguy @jameswnl assuming we should be returning the actual task from the ID that is returned?

jntullo · 2017-03-13T16:20:06Z

@miq-bot remove_label wip

abellotti · 2017-03-13T17:36:16Z

app/controllers/api/authentications_controller.rb

+      # TODO: Temporary validation - remove
+      raise 'type not currently supported' unless klass.respond_to?(:create_in_provider_queue)
+      task_id = klass.create_in_provider_queue(attrs['manager_resource'], attrs.except('type', 'manager_resource'))
+      MiqTask.find(task_id)


This and the equivalent return as subcollection create should return the action_result signature which supports tasks, i.e action_result(true, desc, :task_id => task_id). create return signatures is either object created or action result, Thanks.

abellotti · 2017-03-15T21:11:42Z

app/controllers/api/authentications_controller.rb

+                                    parse_href(data['manager_resource']['href']).last
+                                  elsif data['manager_resource'].key?('id')
+                                    data['manager_resource']['id']
+                                  end


couple of questions here.

what is the expected value of manager_resource for create_in_provider_queue ? Is it an id ? if so an id to which class ?

parse_href returns collection, id. If user sends in an href to a different resource, say /api/users/10, that means we're sending in a bogus 10. In which case could create_in_provider_queue supports an object for the first parameter ?

what is the else case, user specified "manager_resource" : 10 ?

@abellotti good question.

It's an ID, and for what we're starting with here, it is an ExtManagementSystem.

I originally limited it to providers.

I believe @jameswnl mentioned that technically, a manager can be a different type of resource. Is that correct? And @jameswnl - do you think we can update create_in_provider_queue to accept an object rather than the ID?

A credential is an Authentication and is related to a manager through the polymorphic resource_type/resource_id. So in some other user cases, an Authentication is related to other things, e.g. a Vm.

In our use case here in create_in_provider[_queue], it has to be a manager aka ExtManagementSystem.

Yes, we can have create_in_provider[_queue] to take in a manager. But I don't know if there's any other use case prefer the id. @Fryguy or @bdunne ?

@abellotti how do you think we handle this? Should we limit it to certain collections?

I am thinking perhaps we store a constant on the model that stores the manager type? And can parse it based off of that.

ping @abellotti

miq-bot · 2017-03-16T14:56:43Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2017-03-16T15:51:47Z

This pull request is not mergeable. Please rebase and repush.

abellotti

some organization changes.

@Fryguy may have other recommendations.

Thanks.

abellotti · 2017-03-22T14:04:46Z

app/controllers/api/authentications_controller.rb

+      raise 'must supply a manager resource' unless data['manager_resource']
+      attrs = data.dup
+      collection, id = parse_href(data['manager_resource']['href'])
+      raise "#{collection} is not a valid manager resource" unless ::Authentication::MANAGER_TYPES.include?(collection)


collection is API centric, model shouldn't need to know about this. If anything we could have the model check the equivalent class, i.e. check against collection_class(collection). Maybe model should provide a method to check the class for us (dealing with descendants and such).

As per @Fryguy the class validation logic is probably better in create_in_provider_queue logic.

right, I'll have a PR for that.

abellotti · 2017-03-22T14:06:20Z

app/controllers/api/subcollections/authentications.rb

+        action_result(true, 'Creating Authentication', :task_id => task_id)
+      rescue => err
+        action_result(false, err.to_s)
+      end


duplicate code with primary collection authentication create code. Maybe add authentications service common methods in the api in lib/services/api/

add class method on authentication to return the type

use action_result remove constantize use action_result

abellotti

Looking better, thanks @jntullo a couple of minor changes.

abellotti · 2017-03-22T20:52:09Z

app/controllers/api/authentications_controller.rb

@@ -9,6 +9,14 @@ def edit_resource(type, id, data)
      action_result(false, err.to_s)
    end

+    def create_resource(_type, _id, data)
+      manager_resource, attrs = validate_auth_attrs(data)
+      task_id = AuthenticationService.create_authentication(manager_resource, attrs)


would still prefer if we send in the object instead of the id, so this would work with future CI's.

abellotti · 2017-03-22T20:58:29Z

app/controllers/api/authentications_controller.rb

+                         elsif data['manager_resource'].key?('id')
+                           data['manager_resource']['id']
+                         end
+      [manager_resource, attrs]


lines 48-52 could be replaced with:

manager_collection, manager_id = parse_href(data['manager_resource']['href'] raise 'invalid manger_resource href specified' unless manager_collection && manager_id manager_resource = resource_search(manager_id, manager_collection, collection_class(manager_collection)

Not sure we want to support manager_resource id references since it would be vague for handling multiple types.

abellotti · 2017-03-23T01:35:30Z

Thanks @jntullo for making all those updates.

@Fryguy any else that needs changing ?

jntullo · 2017-03-23T15:21:01Z

Another concern just came up. @mzazrivec saw the issue:

AnsibleTowerClient::ClientError: {"detail":["Missing 'user', 'team', or 'organization'."]}

Should it be the API's responsibility of adding in the user if not specified, based off of the request's user?

cc: @jameswnl

jameswnl · 2017-03-23T15:34:25Z

@mzazrivec we want to always have default organization. @Fryguy input?

mzazrivec · 2017-03-23T15:36:04Z

@jameswnl right, but we don't really want to work with this Ansible organization info on ManageIQ API level, do we?

Fryguy · 2017-03-23T15:55:24Z

@jameswnl right, but we don't really want to work with this Ansible organization info on ManageIQ API level, do we?

No, the API caller should not care about that. We should default the organization in the call on the backend for the embedded Provider.

mzazrivec · 2017-03-23T19:26:48Z

Another problem I noticed with current master:

$ curl -X POST -d '{"type":"ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential", "name":"scm-credential-04", "manager_resource":{"href":"http://localhost:3000/api/providers/10000000000043"}, "organization":1}'  http://admin:smartvm@localhost:3000/api/authentications|json_reformat
{
    "results": [
        {
            "success": true,
            "message": "Creating Authentication",
            "task_id": 10000000660499,
            "task_href": "http://localhost:3000/api/tasks/10000000660499"
        }
    ]
}

$ curl http://admin:smartvm@localhost:3000/api/tasks/10000000660499|json_reformat
{
    "href": "http://localhost:3000/api/tasks/10000000660499",
    "id": 10000000660499,
    "name": "Creating ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential with name=",
    "state": "Finished",
    "status": "Error",
    "message": "Couldn't find ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential",
    "userid": "system",
    "created_on": "2017-03-23T19:09:29Z",
    "updated_on": "2017-03-23T19:14:19Z"
}

jntullo · 2017-03-23T19:30:09Z

@mzazrivec I believe that's an issue with the create_in_provider_queue, @jameswnl ?

jntullo · 2017-03-23T19:32:52Z

@mzazrivec actually I thought of something, can you try it now?

jameswnl · 2017-03-23T19:33:13Z

@mzazrivec can you locate the logs in evm.log where this task is being executed?

mzazrivec · 2017-03-23T19:51:16Z

The problem above can also be reproduced from rails console:


```irb(main):030:0> ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential.create_in_provider(ManageIQ::Providers::EmbeddedAnsible::AutomationManager.first.id, {:name => 'scm-credential-02', :organization => 1})
ActiveRecord::RecordNotFound: Couldn't find ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ScmCredential
        from /home/milan/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/activerecord-5.0.2/lib/active_record/core.rb:212:in `find_by!'
        from /home/milan/src/manage-iq/manageiq/app/models/manageiq/providers/ansible_tower/shared/automation_manager/credential.rb:12:in `create_in_provider'
        from (irb):30
        from /home/milan/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/railties-5.0.2/lib/rails/commands/console.rb:65:in `start'
        from /home/milan/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/railties-5.0.2/lib/rails/commands/console_helper.rb:9:in `start'
        from /home/milan/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/railties-5.0.2/lib/rails/commands/commands_tasks.rb:78:in `console'
        from /home/milan/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/railties-5.0.2/lib/rails/commands/commands_tasks.rb:49:in `run_command!'
        from /home/milan/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/railties-5.0.2/lib/rails/commands.rb:18:in `<top (required)>'
        from bin/rails:4:in `require'
        from bin/rails:4:in `<main>'

mzazrivec · 2017-03-23T19:54:10Z

The part from evm.log:

[----] E, [2017-03-23T20:48:27.294089 #23305:2af3078bb10c] ERROR -- : MIQ(MiqQueue#deliver) Message id: [10000023218120], Error: [Couldn't find ManageIQ::Providers::Embedded
Ansible::AutomationManager::ScmCredential]
[----] E, [2017-03-23T20:48:27.294255 #23305:2af3078bb10c] ERROR -- : [ActiveRecord::RecordNotFound]: Couldn't find ManageIQ::Providers::EmbeddedAnsible::AutomationManager::
ScmCredential  Method:[rescue in deliver]
[----] E, [2017-03-23T20:48:27.294365 #23305:2af3078bb10c] ERROR -- : /home/milan/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/activerecord-5.0.2/lib/active_record/core.rb
:212:in `find_by!'
/home/milan/src/manage-iq/manageiq/app/models/manageiq/providers/ansible_tower/shared/automation_manager/credential.rb:12:in `create_in_provider'
/home/milan/src/manage-iq/manageiq/app/models/miq_queue.rb:347:in `block in deliver'
/home/milan/.rbenv/versions/2.3.1/lib/ruby/2.3.0/timeout.rb:91:in `block in timeout'
/home/milan/.rbenv/versions/2.3.1/lib/ruby/2.3.0/timeout.rb:33:in `block in catch'
/home/milan/.rbenv/versions/2.3.1/lib/ruby/2.3.0/timeout.rb:33:in `catch'
/home/milan/.rbenv/versions/2.3.1/lib/ruby/2.3.0/timeout.rb:33:in `catch'
/home/milan/.rbenv/versions/2.3.1/lib/ruby/2.3.0/timeout.rb:106:in `timeout'
/home/milan/src/manage-iq/manageiq/app/models/miq_queue.rb:343:in `deliver'
/home/milan/src/manage-iq/manageiq/app/models/miq_queue_worker_base/runner.rb:106:in `deliver_queue_message'
/home/milan/src/manage-iq/manageiq/app/models/miq_queue_worker_base/runner.rb:134:in `deliver_message'
/home/milan/src/manage-iq/manageiq/app/models/miq_queue_worker_base/runner.rb:152:in `block in do_work'
/home/milan/src/manage-iq/manageiq/app/models/miq_queue_worker_base/runner.rb:146:in `loop'
/home/milan/src/manage-iq/manageiq/app/models/miq_queue_worker_base/runner.rb:146:in `do_work'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker/runner.rb:334:in `block in do_work_loop'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker/runner.rb:331:in `loop'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker/runner.rb:331:in `do_work_loop'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker/runner.rb:153:in `run'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker/runner.rb:128:in `start'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker/runner.rb:21:in `start_worker'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker.rb:339:in `block in start_runner'
/home/milan/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/nakayoshi_fork-0.0.3/lib/nakayoshi_fork.rb:24:in `fork'
/home/milan/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/nakayoshi_fork-0.0.3/lib/nakayoshi_fork.rb:24:in `fork'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker.rb:337:in `start_runner'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker.rb:348:in `start'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker.rb:266:in `start_worker'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker.rb:150:in `block in sync_workers'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker.rb:150:in `times'
/home/milan/src/manage-iq/manageiq/app/models/miq_worker.rb:150:in `sync_workers'
/home/milan/src/manage-iq/manageiq/app/models/miq_server/worker_management/monitor.rb:53:in `block in sync_workers'
/home/milan/src/manage-iq/manageiq/app/models/miq_server/worker_management/monitor.rb:50:in `each'
/home/milan/src/manage-iq/manageiq/app/models/miq_server/worker_management/monitor.rb:50:in `sync_workers'
/home/milan/src/manage-iq/manageiq/app/models/miq_server.rb:160:in `start'
/home/milan/src/manage-iq/manageiq/app/models/miq_server.rb:251:in `start'
/home/milan/src/manage-iq/manageiq/lib/workers/evm_server.rb:65:in `start'
/home/milan/src/manage-iq/manageiq/lib/workers/evm_server.rb:91:in `start'
/home/milan/src/manage-iq/manageiq/lib/workers/bin/evm_server.rb:4:in `<main>'

mzazrivec · 2017-03-23T22:06:01Z

The above error will go away when "kind":"scm" is used in the creation request, so it's not an issue for this PR.

gtanzillo

👍 LGTM, @Fryguy are you good with merging this?

Fryguy · 2017-03-24T16:13:38Z

lib/services/api/authentication_service.rb

@@ -0,0 +1,10 @@
+module Api
+  class AuthenticationService
+    def self.create_authentication(manager_resource, attrs)


Minor, but I feel like this should be named create_authenticaton_queue or create_authenticaton_task or something. Then the caller knows they are getting back a task, and it's clear that it's not a synchronous call.

Fryguy · 2017-03-24T16:14:39Z

app/models/authentication.rb

+    type = descendants.find { |klass| klass.name == data['type'] }
+    raise _('Must be an Authentication type') unless type
+    type
+  end


I don't like this on the model here. It feels like it's a generic thing that's being put into a single model

This feels like a duplication of a lot of the work that the NewWithTypeStiMixin does. That is, finding the right subclass to instantiate via type. Given that, I feel like the "type" logic belongs to create_in_provider_queue itself, somehow at the base class, in a similar manner to how NewWithTypeStiMixin works.

create_authentication to create_authentication_task

miq-bot · 2017-03-24T16:42:52Z

Checked commits jntullo/manageiq@d1115a0~...eb89cb8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 0 offenses detected
Everything looks good. 🏆

Fryguy · 2017-03-24T17:30:51Z

Merging on 🔴 anyway since these failures are a test failure with master and are unrelated to the code in question.

miq-bot added api enhancement wip labels Mar 7, 2017

miq-bot assigned abellotti Mar 7, 2017

abellotti reviewed Mar 9, 2017

View reviewed changes

mzazrivec mentioned this pull request Mar 9, 2017

CRUD for embedded Ansible credentials ManageIQ/manageiq-ui-classic#641

Merged

jntullo commented Mar 13, 2017

View reviewed changes

jntullo changed the title ~~[WIP] API Authentication create~~ =API Authentication create Mar 13, 2017

jntullo changed the title ~~=API Authentication create~~ API Authentication create Mar 13, 2017

miq-bot removed the wip label Mar 13, 2017

abellotti reviewed Mar 13, 2017

View reviewed changes

abellotti reviewed Mar 15, 2017

View reviewed changes

miq-bot added unmergeable and removed unmergeable labels Mar 16, 2017

Fryguy self-requested a review March 21, 2017 14:21

abellotti reviewed Mar 22, 2017

View reviewed changes

Jillian Tullo added 5 commits March 22, 2017 11:48

authentication create

d1115a0

create in provider queue

ca7a524

add class method on authentication to return the type

require resource_manager

9abeafe

use action_result remove constantize use action_result

require href and validate the collection passed to manager_resource

24084ba

create authentications service

e2e1811

abellotti reviewed Mar 22, 2017

View reviewed changes

abellotti approved these changes Mar 23, 2017

View reviewed changes

gtanzillo approved these changes Mar 24, 2017

View reviewed changes

blomquisg mentioned this pull request Mar 24, 2017

flag required attributes for Ansible credentials #14474

Merged

Fryguy reviewed Mar 24, 2017

View reviewed changes

remove support for ID

eb89cb8

create_authentication to create_authentication_task

Fryguy merged commit ee08f3c into ManageIQ:master Mar 24, 2017

Fryguy added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 24, 2017

jntullo mentioned this pull request Mar 27, 2017

Update API to use descendant_get to return valid types #14521

Merged

jntullo deleted the enhancement/credential_create branch November 28, 2017 19:41

API Authentication create #14217

API Authentication create #14217

Conversation

jntullo commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jntullo commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jntullo Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Mar 16, 2017

miq-bot commented Mar 16, 2017

abellotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Mar 23, 2017

jntullo commented Mar 23, 2017

jameswnl commented Mar 23, 2017

mzazrivec commented Mar 23, 2017

Fryguy commented Mar 23, 2017

mzazrivec commented Mar 23, 2017

jntullo commented Mar 23, 2017

jntullo commented Mar 23, 2017

jameswnl commented Mar 23, 2017

mzazrivec commented Mar 23, 2017

mzazrivec commented Mar 23, 2017

mzazrivec commented Mar 23, 2017

gtanzillo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Mar 24, 2017

Fryguy commented Mar 24, 2017

jntullo Mar 15, 2017 •

edited

Loading