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

Adds connection to Graph RBAC API #327

Merged
merged 21 commits into from
Aug 23, 2018
Merged

Adds connection to Graph RBAC API #327

merged 21 commits into from
Aug 23, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jul 26, 2018

No description provided.

@ghost ghost requested review from dmccown and jquick July 26, 2018 16:01
Copy link
Contributor

@jquick jquick left a comment

Choose a reason for hiding this comment

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

This looks pretty good. We will want to error if we try to use the MSI connection with Graph or add those connection details.

Ruairi Fennell and others added 10 commits July 30, 2018 11:05
Signed-off-by: Ruairi Fennell <[email protected]>
Signed-off-by: Ruairi Fennell <[email protected]>
Graph client may be used in cases where you need to communicate with
Azure Graph API.

Signed-off-by: David McCown <[email protected]>
Signed-off-by: Ruairi Fennell <[email protected]>
Signed-off-by: Ruairi Fennell <[email protected]>
Signed-off-by: Ruairi Fennell <[email protected]>
@jquick
Copy link
Contributor

jquick commented Aug 20, 2018

We will have to confirm all the upstream inspec artifact builds are ok with 'azure_graph_rbac'. To do this create a inspec branch based off this train branch and then create a ad-hoc inspec build in our pipeline.

@@ -54,14 +56,21 @@ def platform
force_platform!('azure', @platform_details)
end

def graph_client
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be confusing as the main client object for all azure clients is under this. I think it may be clearer to just put some logic into the azure_client method to pick out the graph client. This way we don't end up with inspec.backend.graph_client in inspec, which is a type of azure_client.

Copy link
Author

Choose a reason for hiding this comment

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

I did initially have have this logic within the azure_client method, however @dmccown and I thought this convenience method was clearer. We do indeed then have inspec.backend.graph_client present in Inspec here - is this bad?

Copy link
Contributor

Choose a reason for hiding this comment

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

After talking this through, we'll change azure_client to handle the case that a client passes in ::Azure::GraphRbac::Profiles::Latest::Client. The returned client will be fully hydrated and ready for consumption. So essentially it will fix an existing bug in cases you asked for a Graph client and the returned client didn't work.

Signed-off-by: Ruairi Fennell <[email protected]>
@jquick
Copy link
Contributor

jquick commented Aug 22, 2018

This is looking pretty good but you have some failing tests.

miah
miah previously approved these changes Aug 22, 2018
Copy link
Contributor

@miah miah left a comment

Choose a reason for hiding this comment

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

Thanks @r-fennell

@miah miah dismissed their stale review August 22, 2018 16:52

Failing tests

miah
miah previously requested changes Aug 22, 2018
Copy link
Contributor

@miah miah left a comment

Choose a reason for hiding this comment

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

This looks good @r-fennell, but the tests need to be corrected.

Copy link
Contributor

@dmccown dmccown left a comment

Choose a reason for hiding this comment

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

Just a couple of nitpicks. Nothing I'd block on (assuming require_relative is ok). I had the impression we should just use require over require_relative.

require 'socket'
require 'timeout'
require 'train/transports/helpers/azure/file_credentials'
require_relative 'clients/azure/graph_rbac'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just use require here?

@@ -55,13 +57,27 @@ def platform
end

def azure_client(klass = ::Azure::Resources::Profiles::Latest::Mgmt::Client)
return klass.new(@credentials) unless cache_enabled?(:api_call)
# Return early if we can
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove the comment.

Copy link
Contributor

@jquick jquick left a comment

Choose a reason for hiding this comment

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

Thanks @r-fennell !

Signed-off-by: Ruairi Fennell <[email protected]>
David McCown and others added 3 commits August 23, 2018 09:50
This test could fail in cases where you already have a credentails file
in the default path (~/.azure/credentials).

I am moving the default
behavior to `azure.rb` so `file_credentials` can operate on what you
pass in. This makes an easier contract as the caller must provide a file,
and the test easier since there's no defaulting behavior.

Signed-off-by: David McCown <[email protected]>
Signed-off-by: David McCown <[email protected]>
Signed-off-by: Ruairi Fennell <[email protected]>
@jquick jquick merged commit 701f6da into master Aug 23, 2018
@jquick jquick deleted the Graph-API branch August 23, 2018 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants