Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip hostname validation for monitoring manager #220

Merged
merged 1 commit into from
Jan 21, 2018

Conversation

zeari
Copy link

@zeari zeari commented Jan 21, 2018

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1535941

The problem is that the monitoring manager looks for a default endpoint which it does not have.
The parent provider has that endpoint and validates it anyway so no need for the monitoring manager to validate it.

Another solution could be to delegate the default endpoint from the monitoring_manager to the parent provider or just regard the alert endpoint as the default endpoint for monitoring.
Monitoring manager has some prep work for the latter: https://github.com/manageiq/manageiq-providers-kubernetes/blob/master/app/models/manageiq/providers/kubernetes/monitoring_manager_mixin.rb#L59

but ext_management_system doesnt take it into account:
https://github.com/manageiq/manageiq/blob/master/app/models/ext_management_system.rb#L295

@cben @moolitayer Please review

@miq-bot add_label bug, gapridashvili/yes

@zeari zeari force-pushed the monitoring_hostname_invalid branch from c9fb7b1 to d0c4d4e Compare January 21, 2018 10:50
@miq-bot
Copy link
Member

miq-bot commented Jan 21, 2018

@zeari Cannot apply the following label because they are not recognized: gapridashvili/yes

@miq-bot miq-bot added the bug label Jan 21, 2018
@zeari
Copy link
Author

zeari commented Jan 21, 2018

@miq-bot add_label gaprindashvili/yes

@miq-bot
Copy link
Member

miq-bot commented Jan 21, 2018

Checked commit zeari@d0c4d4e with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Contributor

@cben cben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

I'm still on the lookout for a cleaner interface to both UI and API then endpoints-with-side-effects. And I still hate the child delegates :endpoints, :to => :parent_manager lie.
But for now, this is great 👍

@cben
Copy link
Contributor

cben commented Jan 21, 2018

cc @masayag you might want similar fix for kubevirt child manager.

@cben cben requested a review from moolitayer January 21, 2018 11:19
@zeari
Copy link
Author

zeari commented Jan 21, 2018

Is it worth to add tests for this to manageiq-api?
I feel we should but this bug wouldnt have been prevented by a test. It happend due to a change in the main repo: ManageIQ/manageiq#16714

@moolitayer
Copy link

@zeari could we have caught the breakage in the main repo by having a model spec doing
ExtManagemenySystem.create!(#endpoint in a certain order)

If so, do you think it is worth doing?

@moolitayer
Copy link

Verified locally:

Without PR

cat <<EOF | curl -X POST -H "Content-Type: application/json" -u "admin:smartvm" --data @- "localhost:3000/api/providers"
{
"name": "container",
"zone_id": 1,
  "type": "ManageIQ::Providers::Openshift::ContainerManager",
"connection_configurations": [
     {
       "endpoint": {
         "role": "prometheus_alerts",
         "hostname": "alerts.example.com",
         "port": 443,
         "security_protocol": "ssl-without-validation"
       },
       "authentication": {
         "role": "prometheus_alerts",
         "auth_key": "token"
       }
     },
     {
       "endpoint": {
         "role": "default",
         "hostname": "master.example.com",
         "port": 8443,
         "security_protocol": "ssl-without-validation"
       },
       "authentication": {
         "role": "bearer",
         "auth_key": "token"
       }
     },
     {
       "endpoint": {
         "role": "prometheus",
         "hostname": "prometheus.example.com",
         "port": 443,
         "security_protocol": "ssl-without-validation"
       },
       "authentication": {
         "role": "prometheus",
         "auth_key": "token"
       }
     }

   ]
}
EOF
{"error":{"kind":"bad_request","message":"Could not create the new provider - undefined method `ipaddress?' for nil:NilClass","klass":"Api::BadRequestError"}}

With PR:

$ cat <<EOF | curl -X POST -H "Content-Type: application/json" -u "admin:smartvm" --data @- "localhost:3000/api/providers"
{
"name": "container",
"zone_id": 1,
  "type": "ManageIQ::Providers::Openshift::ContainerManager",
"connection_configurations": [
     {
       "endpoint": {
         "role": "prometheus_alerts",
         "hostname": "alerts.example.com",
         "port": 443,
         "security_protocol": "ssl-without-validation"
       },
       "authentication": {
         "role": "prometheus_alerts",
         "auth_key": "token"
       }
     },
     {
       "endpoint": {
         "role": "default",
         "hostname": "master.example.com",
         "port": 8443,
         "security_protocol": "ssl-without-validation"
       },
       "authentication": {
         "role": "bearer",
         "auth_key": "token"
       }
     },
     {
       "endpoint": {
         "role": "prometheus",
         "hostname": "prometheus.example.com",
         "port": 443,
         "security_protocol": "ssl-without-validation"
       },
       "authentication": {
         "role": "prometheus",
         "auth_key": "token"
       }
     }

   ]
}
EOF


{"results":[{"href":"http://localhost:3000/api/providers/33","id":"33","name":"container","created_on":"2018-01-21T17:12:08Z","updated_on":"2018-01-21T17:12:08Z","guid":"dc26f7d0-ee13-43ee-8177-67e8b69de432","zone_id":"1","type":"ManageIQ::Providers::Openshift::ContainerManager","api_version":"v1","tenant_id":"1","enabled":true}]}

PS: (had to rebase the PR or I would get "message":"Could not create the new provider - uninitialized constant ManageIQ::Providers::Kubernetes::VirtualizationManagerMixin","klass":"Api::BadRequestError"`)

Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@moolitayer moolitayer merged commit d64480e into ManageIQ:master Jan 21, 2018
@moolitayer moolitayer added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 21, 2018
@moolitayer moolitayer self-assigned this Jan 21, 2018
simaishi pushed a commit that referenced this pull request Jan 22, 2018
Skip hostname validation for monitoring manager
(cherry picked from commit d64480e)

https://bugzilla.redhat.com/show_bug.cgi?id=1537137
@simaishi
Copy link

Gaprindashvili backport details:

$ git log -1
commit 2fd717fe62f4319b5cdcd486ec3a42f2d98f9943
Author: Mooli Tayer <mtayer@redhat.com>
Date:   Sun Jan 21 19:18:24 2018 +0200

    Merge pull request #220 from zeari/monitoring_hostname_invalid
    
    Skip hostname validation for monitoring manager
    (cherry picked from commit d64480ed1bb566fe4307131846a08772991054f4)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1537137

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants