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.

Sign up for GitHub

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

Jump to bottom

Fix vm retirement initiated on global region #20244

Merged
merged 2 commits into from Jun 4, 2020
Merged

Fix vm retirement initiated on global region #20244

merged 2 commits into from Jun 4, 2020

Conversation

yrudman
Copy link
Contributor

@yrudman yrudman commented Jun 3, 2020 •
edited

ISSUE:
When InterRegionApiMethodRelay.api_relay_class_method invoked over queue User.current_userid is not defined, so nil passed as user in

ManageIQ::API::Client.new(
      :url      => url,
      :miqtoken => region.api_system_auth_token(user),
      :ssl      => {:verify => false}
    )

It cause

 ERROR -- : MIQ(Api::ApiController.rescue in authenticate_with_system_token) Authentication Failed with System Token
X-MIQ-Token: v2:{URXb7g5cpwP5fzGAY8U1rRozucGWZ2vpIMfk1dj5jx3rch749Kttmlnq2uRmY/Xs0TKhXvUQvX8lIKKecNdHGsEillUBb4Ky8XYFPJhO07IaRQSMdC1Jpqk2BAn7LQwkgp1RGupxtBWD8W0WvX90+g==}
Error: undefined method `downcase' for nil:NilClass

in Authentication.authenticate_with_system_token on remote region

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1839770

@miq-bot add-label bug, ivanchuk/yes, jansa/yes

@miq-bot miq-bot added the wip label Jun 3, 2020
@yrudman yrudman force-pushed the fix-retirement-from-global branch from 0ad35b1 to 0c58d19 Compare June 3, 2020 22:17
@yrudman yrudman force-pushed the fix-retirement-from-global branch from 0c58d19 to a4d3daf Compare June 4, 2020 01:35
@yrudman yrudman requested a review from gtanzillo as a code owner June 4, 2020 12:24
carbonin
carbonin reviewed Jun 4, 2020
View reviewed changes
Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

Can you describe what the actual problem was and why this fixes it? Based on the diff it looks like there was some problem with the user id, but I'm not exactly sure. It would be good to have a real description of the issue in case we need to look back on this.

app/models/mixins/inter_region_api_method_relay.rb
api_args ||= {}
collection = api_client_connection_for_region(region).public_send(collection_name)
user_name = api_args[:requester]&.fetch("user_name")
user_name = User.current_user&.userid if user_name.blank?
Copy link
Member

Choose a reason for hiding this comment

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

Why the &. here?

Copy link
Contributor Author

@yrudman yrudman Jun 4, 2020 •
edited

Choose a reason for hiding this comment

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

Retirement request now going throw queue, so, User.current_user (or User.curent_userid) is nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for retirement request we need &

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this fix does does make sense right now only for retirement request, but going forward all operations toward remote region over queue can use it

@yrudman yrudman changed the title [WIP] Fix vm retirement initiated on global region Fix vm retirement initiated on global region Jun 4, 2020
@miq-bot miq-bot removed the wip label Jun 4, 2020
carbonin
carbonin reviewed Jun 4, 2020
View reviewed changes
app/models/mixins/inter_region_api_method_relay.rb Outdated
@@ -70,8 +70,11 @@ def self.api_client_connection_for_region(region_number, user = User.current_use

def self.exec_api_call(region, collection_name, action, api_args = nil, id = nil)
require 'manageiq-api-client'
_log.info("Executing InterRegion api call for region #{region} with arguments: #{api_args}")
Copy link
Member

Choose a reason for hiding this comment

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

Could these args have something sensitive in them that we maybe don't want to log? I think the api logs have a special parser that sanitizes things which we wouldn't have here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, ok. I wanted to provide more info for future troubleshooting.
Will remove logging

@yrudman yrudman force-pushed the fix-retirement-from-global branch from bea62a6 to 9cde7d7 Compare June 4, 2020 15:12
@miq-bot
Copy link
Member

miq-bot commented Jun 4, 2020

Checked commits yrudman/manageiq@a4d3daf~...9cde7d7 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@carbonin carbonin merged commit 97c2e5b into ManageIQ:master Jun 4, 2020
@carbonin carbonin self-assigned this Jun 4, 2020
@yrudman yrudman deleted the fix-retirement-from-global branch June 4, 2020 15:39
@yrudman
Copy link
Contributor Author

yrudman commented Jun 4, 2020

@miq-bot add-label ivanchuk/yes, jansa/yes

simaishi pushed a commit that referenced this pull request Jun 5, 2020
@carbonin @simaishi
Merge pull request #20244 from yrudman/fix-retirement-from-global
3e237e9 
Fix vm retirement initiated on global region

(cherry picked from commit 97c2e5b)
@simaishi
Copy link
Contributor

simaishi commented Jun 5, 2020

Jansa backport details:

$ git log -1
commit 3e237e9cbad280c699ba744c6834666d38d042f4
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Thu Jun 4 11:35:27 2020 -0400

    Merge pull request #20244 from yrudman/fix-retirement-from-global

    Fix vm retirement initiated on global region

    (cherry picked from commit 97c2e5b9d7992d84d67ed65cd4c6cad07dfdabdb)

abellotti added a commit to abellotti/manageiq-api that referenced this pull request Jun 8, 2020
@abellotti
Fixed specs so we don't fail with the changes that came in
033b4d3 
with ManageIQ/manageiq#20244
for global region initiated requests.
@abellotti abellotti mentioned this pull request Jun 8, 2020
Merged
simaishi pushed a commit that referenced this pull request Jun 18, 2020
@carbonin @simaishi
Merge pull request #20244 from yrudman/fix-retirement-from-global
5fa5813 
Fix vm retirement initiated on global region

(cherry picked from commit 97c2e5b)
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit 5fa5813476646ab3ad99f5f2e50b4d48e44fb99e
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Thu Jun 4 11:35:27 2020 -0400

    Merge pull request #20244 from yrudman/fix-retirement-from-global

    Fix vm retirement initiated on global region

    (cherry picked from commit 97c2e5b9d7992d84d67ed65cd4c6cad07dfdabdb)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbonin carbonin carbonin approved these changes

@agrare agrare Awaiting requested review from agrare agrare is a code owner

@Fryguy Fryguy Awaiting requested review from Fryguy Fryguy is a code owner

@jrafanie jrafanie Awaiting requested review from jrafanie

@kbrock kbrock Awaiting requested review from kbrock kbrock is a code owner

@gtanzillo gtanzillo Awaiting requested review from gtanzillo

Assignees

@carbonin carbonin

Labels
bug core ivanchuk/backported jansa/backported lifecycle/retirement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yrudman @miq-bot @simaishi @carbonin @gtanzillo