API driven, pluggable, provider creation and validation

[skateman]

The API doesn't provide any kind of credential validation when adding provider, while the UI implements this feature. Therefore, it is possible to add a provider with invalid credentials through the API and we have no endpoint even to check if these credentials are correct.

There's a method included in each provider to create a MiqTask that checks the credentials, however, the input arguments for this task are different for each provider. On the UI side this difference is compensated via 🚒 🍝 🔥 if-else structures testing which provider is currently used.

case ems.to_s
when 'ManageIQ::Providers::Openstack::CloudManager'
  something
when 'ManageIQ::Providers::Amazon::CloudManager'
  something_else
when 'ManageIQ::Providers::Azure::CloudManager'
  something_totally_different
...

This goes totally in the opposite direction to our philosophy of having pluggable providers. Ideally we don't want anything in the UI that sends data to the backend based on the provider's type.

We're working with @Hyperkid123 on the API-driven provider forms and this is kinda blocking us.

ping @martinpovolny @Ladas @agrare

TODO:

• Add a method to aggregate provider create params for each provider (Add a common validate_credentials API and method for getting all provider create params #19120) (@agrare)
• Add params_for_create and verify_credentials for each provider (@agrare)
  • Amazon: Add JSON-Schema create-params and a common Validate Credentials API manageiq-providers-amazon#551, Rename validate_credentials to verify_credentials manageiq-providers-amazon#563
  • Ansible Tower: Add DDF based verify_credentials manageiq-providers-ansible_tower#198 (@Fryguy)
  • Azure: Add DDF based verify_credentials manageiq-providers-azure#357 (@Fryguy)
  • Azure Stack: Add methods for for provider creation pluggable UI manageiq-providers-azure_stack#14 (@agrare)
  • Foreman: Add params_for_create and verify_credentials manageiq-providers-foreman#46 (@agrare)
  • Google: Add DDF based verify_credentials manageiq-providers-google#108 (@Fryguy)
  • Kubernetes: Add params_for_create and verify_credentials manageiq-providers-kubernetes#341 (@agrare)
  • Kubevirt: Add methods for for provider creation pluggable UI manageiq-providers-kubevirt#149 (@agrare)
  • Lenovo: Add params_for_create and verify_credentials manageiq-providers-lenovo#287 (@agrare )
  • Nuage: Add params_for_create and verify_credentials manageiq-providers-nuage#194 (@agrare )
  • OpenShift: Add default_port needed for params_for_create manageiq-providers-openshift#155 (@agrare)
  • OpenStack: Add params_for_create and verify_credentials manageiq-providers-openstack#532 (@agrare)
  • Ovirt: Data-Driven create params and validate_credentials manageiq-providers-ovirt#408, Rename validate_credentials and other fixes manageiq-providers-ovirt#413
  • Redfish: Add params_for_create and verify_credentials manageiq-providers-redfish#99 (@agrare)
  • SCVMM: Add verify_credentials and params_for_create manageiq-providers-scvmm#136 (@agrare)
  • Vmware: Add a common verify_creds and params_for_create manageiq-providers-vmware#457 (@agrare )
• Add an API GET/OPTIONS endpoint to return the params_for_create (@skateman
  Expose DDF schema for a provider when asking using OPTIONS + type manageiq-api#645)
• Add a new verify_credentials_task method to call the common verify_credentials method on the providers (replacing the one which calls .raw_connect) (@agrare Add a verify_credentials_task method #19346)
  • Change .verify_credentials_task to .raw_connect_task so that we can add the new validate_credentials_task before the UI is updated without breaking anything We decided to name the new method verify_credentials_task and deprecate validate_credentials_task, given the methods on the provider are called verify_credentials
• Add an API POST endpoint to call the new verify credentials method and return a task (@skateman) Expose the verify_credentials action for /api/providers manageiq-api#682
• Add a universal method for creating/editing providers from a submitted DDF schema - DDF: Methods for creating/editing EMSes with nested attributes #20157
• Expose an API endpoint for adding/editing providers that understands DDF schemas - DDF: schema-based validation and revised create/update for providers manageiq-api#830
• React component that renders the DDF schema and talks to the API - Replace cloud provider forms with an API-driven DDF approach manageiq-ui-classic#6698
• Update UI code to use the new component
  • Cloud - Replace cloud provider forms with an API-driven DDF approach manageiq-ui-classic#6698
  • Infra - Replace angular-based infra provider forms with the DDF component manageiq-ui-classic#7181
  • Physical Infra - Replace the physical infra provider forms with the DDF-based new one manageiq-ui-classic#7091
  • Containers - Replace old container provider forms with the new DDF version manageiq-ui-classic#7155
  • Networking - Replace the old network provider forms with the DDF-based ProviderForm manageiq-ui-classic#7090
  • Configuration - Replace old angular config provider form with the new ProviderForm manageiq-ui-classic#7047
  • Automation - Replace old angular ansible tower form with a react based DDF manageiq-ui-classic#7176
• Update the provider generator with an example DDF schema - Updated the provider generator's dummy code with an example DDF #20165
• Update our guides
• Verify parity between the old and new screens ensuring that all fields are accounted for, and the user experience is mostly similar (@agrare and @Fryguy)
• Delete the old code (@jrafanie? 😆) Clean up and delete the code related to the old provider forms manageiq-ui-classic#7251

[martinpovolny]

Ping @dmetzger57 , @abellotti , @agrare .

Guys, this is an API feature request that is past what the UI team members so far did on the API. We need someone from the API team to implement this. Or at least provide some instructions on how this is to be done in the API so that it will get merged.

Can we get such help, please?

[dmetzger57]

As this is an API request, I look to @gtanzillo and @abellotti to assess when (I didn't say "if" 😄 ) we can help

[skateman]

I kinda started working on the API support but ran into the obstacles I've described above. I would be able to implement this thing in the API with some copy-paste engineering from the UI, but I don't think anyone would merge such 🍝

My idea was to utilize the AuthenticationMixin.validate_credentials_task method in an /api/providers/validate endpoint that would return us a task ID. We already have a mechanism to poll a task's status via the API, however, the generation of the input argument of the previously mentioned method is a mess...

[agrare]

There's a method included in each provider to create a MiqTask that checks the credentials, however, the input arguments for this task are different for each provider.

Yeah I never liked that this used raw_connect because the parameters are different for every provider. Would rather we design on a common ems.connect() interface that can be used generically.

[Fryguy]

Related to ManageIQ/manageiq-api#585 ?

[Fryguy]

I personally think this is extremely important for a solid API experience and greater pluggability on the UI side.

[Fryguy]

raw_connect was supposed to be the common interface, but then as new providers came on board, the pattern wasn't followed.

[agrare]

👍 Yeah agreed

[agrare]

Oh really? Usually all the raw_ methods are the provider specific version of the common interface
I don't think any of the raw_connect methods have the same parameters but I'll have to check

[Fryguy]

Amazon introduced the "service" name, and at the time, if I recall, we added an options parameter to deal with provider specifics...then it got refactored...and then I have no idea what happened 😆

[agrare]

Haha okay, well here is what we've got to work with:

./manageiq/app/models/manageiq/providers/embedded_ansible/provider.rb:  def self.raw_connect(base_url, username, password, verify_ssl)
./manageiq-providers-amazon/app/models/manageiq/providers/amazon/manager_mixin.rb:    def raw_connect(access_key_id, secret_access_key, service, region,
./manageiq-providers-redfish/app/models/manageiq/providers/redfish/manager_mixin.rb:    def raw_connect(username, password, host, port, protocol)
./manageiq-providers-kubevirt/app/models/manageiq/providers/kubevirt/infra_manager.rb:  def self.raw_connect(opts)
./manageiq-providers-google/app/models/manageiq/providers/google/manager_mixin.rb:    def raw_connect(google_project, google_json_key, options, proxy_uri = nil, validate = false)
./manageiq-providers-ovirt/app/models/manageiq/providers/redhat/infra_manager/api_integration.rb:    def raw_connect(opts = {})
./manageiq-providers-openshift/app/models/manageiq/providers/openshift/container_manager_mixin.rb:    def raw_connect(hostname, port, options)
./manageiq-providers-ansible_tower/app/models/manageiq/providers/ansible_tower/shared/provider.rb:    def raw_connect(base_url, username, password, verify_ssl)
./manageiq-providers-vmware/app/models/manageiq/providers/vmware/manager_auth_mixin.rb:    def raw_connect(server, port, username, password, api_version = '5.5', validate = false)
./manageiq-providers-vmware/app/models/manageiq/providers/vmware/infra_manager/vim_connect_mixin.rb:    def raw_connect(options)
./manageiq-providers-nuage/app/models/manageiq/providers/nuage/manager_mixin.rb:    def raw_connect(username, password, endpoint_opts)
./manageiq-providers-openstack/lib/manageiq/providers/openstack/legacy/openstack_handle/handle.rb:    def self.raw_connect_try_ssl(username, password, address, port, service = "Compute", options = nil,
./manageiq-providers-openstack/lib/manageiq/providers/openstack/legacy/openstack_handle/handle.rb:    def self.raw_connect(username, password, auth_url, service = "Compute", extra_opts = nil)
./manageiq-providers-openstack/app/models/manageiq/providers/openstack/manager_mixin.rb:    def raw_connect(password, params, service = "Compute")
./manageiq-providers-lenovo/app/models/manageiq/providers/lenovo/manager_mixin.rb:    def raw_connect(username, password, host, port, auth_type, verify_ssl, user_agent_label, validate = false, timeout: nil)
./manageiq-providers-kubernetes/app/models/manageiq/providers/kubernetes/virtualization_manager_mixin.rb:    def raw_connect(options)
./manageiq-providers-kubernetes/app/models/manageiq/providers/kubernetes/container_manager.rb:  def self.raw_connect(hostname, port, options)
./manageiq-providers-kubernetes/app/models/manageiq/providers/kubernetes/monitoring_manager_mixin.rb:    def raw_connect(options)
./manageiq-providers-scvmm/app/models/manageiq/providers/microsoft/infra_manager.rb:  def self.raw_connect(connect_params, validate = false)
./manageiq-providers-azure/app/models/manageiq/providers/azure/manager_mixin.rb:    self.class.raw_connect(client_id, client_key, azure_tenant_id, subscription, options[:proxy_uri] || http_proxy_uri, provider_region, default_endpoint)
./manageiq-providers-azure/app/models/manageiq/providers/azure/manager_mixin.rb:    def raw_connect(client_id, client_key, azure_tenant_id, subscription, proxy_uri = nil, provider_region = nil, endpoint = nil)
./manageiq-providers-foreman/app/models/manageiq/providers/foreman/provider.rb:  def self.raw_connect(base_url, username, password, verify_ssl)

So some of them take just an options hash but the vast majority take whatever they need for that provider

[Fryguy]

@agrare Ok, let's design a consistent interface at the model level somehow and then we can expose that via the /api/providers/validate that @skateman is proposing. We can take inspiration on how to design that form the if-else constructs at the UI level (since that's basically everything we'll need to do anyway)

[skateman]

bump

[Fryguy]

@agrare Assigning to you just for tracking...we still need to design this.

[agrare]

@Fryguy and I discussed this and we are thinking something like this:

At the provider class level each provider would define their "parameters_for_create" in a JSON-schema format so taking Amazon as an example:

ManageIQ::Providers::Amazon::CloudManager.params_for_create

Then OPTIONS on /api/providers would return the params for the different provider types.

We would add a (names just an example, I'm terrible at naming) /api/providers#verify_credentials action which would take the filled in parameters and pass them to a new ExtManagementSystem.verify_credentials method common to all providers.

This will return a task which can be tracked for success/failure.

I'm thinking that the format for params_for_create will be either DDF like we have in sources-api for the different source_types (https://github.com/ManageIQ/sources-api/blob/master/db/seeds.rb#L23-L31) or something like an openapi spec.

[skateman]

@agrare this is perfect, cc @Hyperkid123

[Hyperkid123]

I'm thinking that the format for params_for_create will be either DDF like

Since we plan to use data driven forms for the component i would be great to use the DDF format for the authentication partials.

[agrare]

@Hyperkid123 sounds good to me! I was planning on using the parameters from sources-api for amazon as a starting point ref

[agrare]

Updated the description with a list of TODO items to complete this.

[skateman]

I'll start working on the verify_credentials API endpoint.

[agrare]

@skateman we created a method on ExtManagementSystem that you can call from the API (#19346). It will return a task_id which can be given to the UI to wait for it to finish. It will update the task message with the error if it failed validation.

[skateman]

@agrare @Fryguy there's an issue with network manager providers as they're implicitly tied to a parent_manager, so ManageIQ::Providers::Nuage::NetworkManager.supported_for_create? will return false. I know that all the other network managers are tied to a parent manager, but Nuage and also NSX-T aren't.

[chessbyte]

Kudos and big thank you to @skateman @himdel @agrare @martinpovolny @Hyperkid123 @Fryguy and anybody else involved in getting this epic across the finish line!