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

Add Application, ServicePrincipal, Role, Group, RoleAssignment support #297

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vaspahomov
Copy link

@vaspahomov vaspahomov commented Sep 29, 2021

Description of your changes

Add support of:

  • Application
  • ServicePrincipal
  • RoleAssignment

This patch allow:

  • Create Applications
  • Create ServicePrincipal
  • Assign roles on ServicePrincipal
  • Assign roles on AD groups

I have:

  • Read and followed Crossplane's [contribution process].
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

  • Unit tests
  • Manually tested

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Hi @vaspahomov,
Thank you for the PR. I have left some comments. Could you please also mention in the PR description how you tested these new MRs?

apis/rbac/v1alpha1/doc.go Outdated Show resolved Hide resolved
apis/rbac/v1alpha1/referencers.go Outdated Show resolved Hide resolved
apis/rbac/v1alpha1/referencers.go Outdated Show resolved Hide resolved
apis/rbac/v1alpha1/referencers.go Outdated Show resolved Hide resolved
apis/rbac/v1alpha1/referencers.go Outdated Show resolved Hide resolved
pkg/controller/rbac/application/managed.go Show resolved Hide resolved
}

const (
appCredsValidYears = 5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make this part of the spec (i.e., make it a parameter)?

Copy link
Author

Choose a reason for hiding this comment

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

I think better to implement creds autoupdates, I added todo note about this

pkg/controller/rbac/roleassignment/managed.go Outdated Show resolved Hide resolved
pkg/controller/rbac/serviceprincipal/managed.go Outdated Show resolved Hide resolved
apis/rbac/v1alpha1/types.go Outdated Show resolved Hide resolved
apis/rbac/v1alpha1/types.go Show resolved Hide resolved
apis/rbac/v1alpha1/types.go Outdated Show resolved Hide resolved
apis/rbac/v1alpha1/types.go Outdated Show resolved Hide resolved
apis/rbac/v1alpha1/types.go Outdated Show resolved Hide resolved
Signed-off-by: vaspahomov <vas2142553@gmail.com>
@vaspahomov
Copy link
Author

Hi! @ulucinar
I've commit fixes

@lovelinuxalot
Copy link

Any updates when this PR is getting merged?

@ulucinar
Copy link
Collaborator

Hi @vaspahomov,
Could you please check the failing tests?

@vaspahomov
Copy link
Author

Hi @ulucinar,
Removed unused code in tests

@ulucinar
Copy link
Collaborator

Hi @ulucinar, Removed unused code in tests

Thank you very much @vaspahomov for bearing with us on this PR. Together with @sergenyalcin, we took another pass over the PR. Among the resolved issues, I did not fully understand the logic here:
https://github.com/crossplane/provider-azure/blob/6c8e8ee885d8d1867992df56902aa1aa49f862c1/pkg/controller/rbac/application/managed.go#L105
or,
https://github.com/crossplane/provider-azure/blob/6c8e8ee885d8d1867992df56902aa1aa49f862c1/pkg/controller/rbac/serviceprincipal/managed.go#L84

Could you please shed some light on these probably with comments?

@goober
Copy link

goober commented Jan 26, 2022

Do I understand correctly that this change uses the Active Directory Graph API for communication with Azure?

If that is the case it should be noted that those API:s are deprecated and support will be removed on June 30th, 2022.
https://techcommunity.microsoft.com/t5/azure-active-directory-identity/update-your-applications-to-use-microsoft-authentication-library/ba-p/1257363

@mikebollandajw
Copy link

we are in need of this

@mikebollandajw mikebollandajw mentioned this pull request Feb 23, 2022
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.

None yet

5 participants