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

Fix creating Kubernetes or OSE with credentials.auth_key #13317

Merged
merged 1 commit into from
Feb 16, 2017

Conversation

cben
Copy link
Contributor

@cben cben commented Dec 26, 2016

Passing credentials containing auth_key was failing with:

Could not create the new provider - Unsupported credential attributes auth_key specified

Worked with Openshift.

(The recommended way to create Openshift with hawkular metrics is pass connection_configurations, which works.
But passing hostname/ip, port and credentials should work when you don't need a hawkular endpoint (and currently will still attempt metrics if there is hawkular on same hostname with default port 443).)

@miq-bot add-label bug, providers/containers, api

Tests (this part already got merged in #13369)

Extended api/providers_spec.rb to run the container tests for all 3 concrete container manager classes, instead of just Openshift::ContainerManager.
I also tried using shared_example instead of loops, found the results less satisfactory.

This file now takes ~17s instead of ~15s. IMO, despite exercising mostly the same mixing code it's well worth it:

  • In addition to the bug I'm fixing now, this testing uncovered bug Can't create Kubernetes provider with port other than 6443 #13306!
    Marked that test pending, will address later.
  • Beside 2 Manager Mixins included in 3 concrete manager classes, there are complex interactions with Authentication, Endpoint, AuthenticationMixin, UI + API, single-endpoint interfaces like credentials vs connection_configurations, and fallbacks from hawkular endpoint to default endpoint...
    I find it very hard to reason about, and to even start simplifying it requires test coverage.

P.S. The "schedules a new credentials check if endpoint change" test is known to be a lie, should fail (#13167). Will address in later PR.

@yaacov @dkorn Please review.

@yaacov
Copy link
Member

yaacov commented Dec 26, 2016

LGTM 👍
[ except that the pending thing is not nice ...
can you fix the port issue and remove the pending ? ]

@cben cben force-pushed the container_supported_auth_attributes branch from 6e2416e to 73d4490 Compare December 26, 2016 12:59
@cben
Copy link
Contributor Author

cben commented Jan 5, 2017

I'm working on several PRs that will all depend on / benefit from the extended tests here.
(To Yaacov's point, one of these PRs will fix the pending #13306.
UPDATE: #13369 fixed this pending test, #13306 still open as root cause affects other places.)
@abellotti Can you take a look?

@simon3z
Copy link
Contributor

simon3z commented Jan 10, 2017

(The recommended way to create Openshift with hawkular metrics is pass connection_configurations, which works.
But passing hostname/ip, port and credentials should work when you don't need a hawkular endpoint (and currently will still attempt metrics if there is hawkular on same hostname with default port 443).)

@cben can you provide an example of the request that works and the one that is not working but should work?

@cben
Copy link
Contributor Author

cben commented Jan 10, 2017

Didn't work (Unsupported credential attributes auth_key specified), fixed here:

    {
      "type"              : "ManageIQ::Providers::OpenshiftEnterprise::ContainerManager",
      "name"              : "sample containers",
      "port"              : 18443,
      "hostname"          : "sample_containers.provider.com",
      "ipaddress"         : "100.200.300.3",
      "credentials"       : {
        "auth_type" : "bearer",
        "auth_key"  : "good token"
      }
    }

Worked:

    {
      "type"                      : "ManageIQ::Providers::OpenshiftEnterprise::ContainerManager",
      "name"                      : "sample containers provider with multiple endpoints",
      "connection_configurations" : [
        {
          "endpoint"      : {
            "role"     : "default",
            "hostname" : "sample_containers_multi_end_point.provider.com",
            "port"     : 18443
          },
          "authentication": {
            "role"     : "bearer",
            "auth_key" : "good token"
          }
        }
      ]
    }

CORRECTION: Openshift already worked, changed examples to OpenshiftEnterprise.
P.S. @yaacov asked why both hostname & ipaddress. Yes, redundant.

@miq-bot
Copy link
Member

miq-bot commented Jan 17, 2017

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

@cben cben force-pushed the container_supported_auth_attributes branch from 243dfae to a7a74b1 Compare January 22, 2017 12:22
@cben cben force-pushed the container_supported_auth_attributes branch from a7a74b1 to fb74f8a Compare January 22, 2017 12:52
@yaacov
Copy link
Member

yaacov commented Jan 22, 2017

LGTM 👍

@miq-bot
Copy link
Member

miq-bot commented Jan 30, 2017

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

…_key`

Passing `credentials` containing `auth_key` was failing with:

    Could not create the new provider -
    Unsupported credential attributes auth_key specified

Worked with Openshift.

The recommended way to create Openshift with hawkular metrics is pass
`connection_configurations`, which works.
But passing hostname/ip, port and `credentials` should work when you
don't need a hawkular endpoint (and currently will still attempt metrics
if there is hawkular on same hostname with default port 443).
@cben cben force-pushed the container_supported_auth_attributes branch from fb74f8a to 76548ed Compare January 31, 2017 21:54
@miq-bot
Copy link
Member

miq-bot commented Jan 31, 2017

Checked commit cben@76548ed with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🏆

@cben
Copy link
Contributor Author

cben commented Feb 1, 2017

Rebased. @blomquisg @abellotti, this is a tiny fix.

@simon3z
Copy link
Contributor

simon3z commented Feb 9, 2017

LGTM 👍
ping @blomquisg @abellotti

@miq-bot assign blomquisg

@miq-bot miq-bot assigned blomquisg and unassigned simon3z Feb 9, 2017
@abellotti
Copy link
Member

I'm good with API changes. Thanks.

@blomquisg blomquisg merged commit fad636d into ManageIQ:master Feb 16, 2017
@blomquisg blomquisg added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 16, 2017
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