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

AWS STS Assume role feature support #5621

Conversation

AlexanderZagaynov
Copy link

@AlexanderZagaynov AlexanderZagaynov commented May 23, 2019

This is support PR for AWS Security Token Service's "assume role" feature.

Basically, it's just a new field on form, and related validation and saving code changes.

Here is the primary PR with technical info: ManageIQ/manageiq-providers-amazon#538
Related core repo PR: ManageIQ/manageiq#18810

Pictures:

  • "New" form with account without proper permissions.
    Screenshot from 2019-05-24 16-55-31
  • "New" form with same account validated successfully with Assume role ARN.
    Screenshot from 2019-05-24 16-58-14
  • "Edit" form with successful validation (I've changed my old record with privileged credentials to those from above).
    Screenshot from 2019-05-24 17-01-21
  • "Edit" form with incorrectly formatted ARN (those error descriptions come from AWS side).
    Screenshot from 2019-05-24 17-03-33
  • "Edit" form with wrong ARN.
    Screenshot from 2019-05-24 17-03-56

@miq-bot miq-bot added the wip label May 23, 2019
@bronaghs
Copy link

Can you replace the default description with a descriptive one that follows the guidelines.

@AlexanderZagaynov
Copy link
Author

Sure will do, once PR will not be WIP :)

@AlexanderZagaynov AlexanderZagaynov force-pushed the features/aws_sts_assume_role branch from 4a38fab to a56baaa Compare May 24, 2019 15:22
@AlexanderZagaynov AlexanderZagaynov changed the title [WIP] AWS STS Assume role feature support AWS STS Assume role feature support May 24, 2019
@miq-bot miq-bot removed the wip label May 24, 2019
@AlexanderZagaynov
Copy link
Author

AlexanderZagaynov commented May 24, 2019

@agrare Adam, that's the second (UI) part of "assume role" story, I'm not sure who to ask to review it, can you please help?

@agrare
Copy link
Member

agrare commented May 24, 2019

@martinpovolny @h-kataria can you review?

app/controllers/mixins/ems_common/angular.rb Outdated Show resolved Hide resolved
app/controllers/mixins/ems_common_angular.rb Outdated Show resolved Hide resolved
app/controllers/mixins/ems_common_angular.rb Outdated Show resolved Hide resolved
@AlexanderZagaynov AlexanderZagaynov force-pushed the features/aws_sts_assume_role branch from 5705643 to 17cb0e7 Compare May 28, 2019 13:36
@martinpovolny
Copy link
Member

@agrare, @AlexanderZagaynov : The UI team is working on redoing the forms in the "spare" time.

To get you a better insight into what is happening, here's a current list of API issues that need resolving before we can redo the forms on the client and get rid of this ugly code:

ManageIQ/manageiq-api#579
ManageIQ/manageiq-api#581
ManageIQ/manageiq-api#585
ManageIQ/manageiq-api#584

@martinpovolny
Copy link
Member

@AlexanderZagaynov : Any updates? Do you need some help? Shall this be marked as "WIP"?

@AlexanderZagaynov AlexanderZagaynov force-pushed the features/aws_sts_assume_role branch 2 times, most recently from be649fa to cfeda86 Compare August 1, 2019 15:02
@AlexanderZagaynov
Copy link
Author

@martinpovolny @mzazrivec I've updated this PR, however please do not merge till I'll prepare PRs for core and Amazon repos.

@AlexanderZagaynov
Copy link
Author

Those PRs should be merged first, and tests should be re-run after that:

@AlexanderZagaynov
Copy link
Author

tests rerun

@AlexanderZagaynov
Copy link
Author

@martinpovolny @mzazrivec both related PRs were merged, only this one left

@martinpovolny
Copy link
Member

Travis failure seems relevant.

@AparnaKarve
Copy link
Contributor

@AlexanderZagaynov To add that additional ARN field, we probably don't need changes in the credentials hamls at all.
Will pull down your branch shortly, and then suggest you additional changes.

@AlexanderZagaynov
Copy link
Author

AlexanderZagaynov commented Sep 6, 2019

@AparnaKarve
Copy link
Contributor

The patch that I sent you earlier has been attached below. It needs to be applied on top of this PR. Hopefully that helps.

Not going to comment inline in your code because it is unnecessarily complicating things, when it should not.

arn_in_new_haml.txt

@himdel
Copy link
Contributor

himdel commented Sep 6, 2019

$scope.angularForm.$valid = updatedValue;
$scope.angularForm.$invalid = !updatedValue;

Ugh, sorry @AlexanderZagaynov but this is really not the right fix.

If you need to set $valid or $invalid manually, it probably means you're doing something outside the angular change detection cycle, otherwise, the form validation should kick in and set the valid state of the form.

You need to look into why angularForm is not valid, there should be an $errors field which will tell you which validation actually failed.

@AlexanderZagaynov AlexanderZagaynov force-pushed the features/aws_sts_assume_role branch from ca0d960 to a292b63 Compare September 6, 2019 12:13
@AlexanderZagaynov
Copy link
Author

AlexanderZagaynov commented Sep 6, 2019

@AparnaKarve @himdel done

@AlexanderZagaynov AlexanderZagaynov force-pushed the features/aws_sts_assume_role branch from a292b63 to fc9d55a Compare September 6, 2019 12:43
@miq-bot
Copy link
Member

miq-bot commented Sep 6, 2019

Checked commits AlexanderZagaynov/manageiq-ui-classic@1cf4616~...fc9d55a with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🍪

@@ -357,6 +366,10 @@ def ems_form_fields
host_default_vnc_port_end = @ems.host_default_vnc_port_end.to_s
end

if @ems.supports?(:assume_role)
service_account = @ems.authentication_service_account
Copy link
Contributor

@himdel himdel Sep 9, 2019

Choose a reason for hiding this comment

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

this will break if ManageIQ::Providers::Google::CloudManager ever decides to supports :assume_role

Because GCE is already using service_account to mean something else (see line 374)

If these 2 are the same concept, shouldn't they be treated the same? (and use layouts/angular/auth_service_account_angular)

But it seems more likely that those 2 are not related at all, so I think you shouldn't be using the service_account variable to mean 2 different things, maybe prefix this one with amazon_ or default_ or something like that?

Copy link
Author

Choose a reason for hiding this comment

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

Those are different things, we discussed with @agrare and agreed to avoid new db field. GCE is not broken, because I didn't touch anything related to it. You can see it's lines above mine I've added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing to do with DB fields.

You have a local variable called service_account in this ems_form_fields function.
That gets sent as data.service_account, but depending on the provider, the value comes from @ems.authentication_service_acount or from @ems.authentication_token.

This local variable ends up as data.service_account in the angular JS controller, where it gets used 2 different ways, in 2 different unrelated fields.

@@ -158,6 +159,7 @@ ManageIQ.angular.app.controller('emsCommonFormController', ['$http', '$scope', '
$scope.emsCommonModel.ssh_keypair_userid = data.ssh_keypair_userid;

$scope.emsCommonModel.service_account = data.service_account;
$scope.emsCommonModel.default_service_account = data.service_account;
Copy link
Contributor

Choose a reason for hiding this comment

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

and here (https://github.com/ManageIQ/manageiq-ui-classic/pull/5621/files#r322236441)

data.service_account should not mean 2 different things

Copy link
Author

Choose a reason for hiding this comment

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

See the line above - it's for GCE. I was trying to stay with it, but you asked me to use default_ prefix - here you are.

Copy link
Contributor

Choose a reason for hiding this comment

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

That prefix is needed for any value in the Default authentication tab. It has nothing to do with how the value is named in the database.

@AlexanderZagaynov
Copy link
Author

AlexanderZagaynov commented Sep 9, 2019

  1. My task originally was to add this feature - and it present already in the core and aws repos. Technically it works, user can add such credentials via rails console.
  2. I'm not from UI team, and I'm tired for everyday's nit-picking.
    Every time it is something new, every time it is somebody new.

My task is done, my work is over, this PR was only a goodwill gesture for UI team.
If UI team don't want it - I'm ok with this. Feel free to make it on your own.

@martinpovolny
Copy link
Member

martinpovolny commented Sep 9, 2019

@himdel is taking over.

Can we get the RFE BZ, please?

@Loicavenel
Copy link

@martinpovolny https://bugzilla.redhat.com/show_bug.cgi?id=1631845 .. I have to re-open it

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.