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 azuread_directory_role_eligibility_schedule_request resource #974

Merged
merged 6 commits into from
Sep 22, 2023

Conversation

JonasBak
Copy link
Contributor

@JonasBak JonasBak commented Jan 12, 2023

This PR adds a azuread_directory_role_eligibility_schedule_request resource. We would like to be able to manage eligible roles in PIM with terraform. This PR lets us do it like this:

resource "azuread_directory_role_eligibility_schedule_request" "example" {
  role_definition_id = "00000000-0000-0000-0000-000000000000"
  principal_id       = "00000000-0000-0000-0000-000000000000"
  directory_scope_id = "/"
  justification      = "abc"
}

This is a pretty bare-bones version of the resource, but it gives us what we need for now. I have also opened a PR in https://github.com/manicminer/hamilton here: manicminer/hamilton#204.

Related to #68.

As I wrote in the other PR, I havn't written any tests, as I wanted to get some input first. The user/service principal requires a role and a permission to be able to use the API, should I set these roles and permissions up as part of the tests?

@manicminer manicminer modified the milestones: v2.33.0, v2.34.0 Jan 12, 2023
@hashicorp hashicorp deleted a comment from github-actions bot Jan 19, 2023
@manicminer manicminer modified the milestones: v2.34.0, v2.35.0 Feb 16, 2023
@manicminer manicminer modified the milestones: v2.35.0, v2.36.0 Feb 23, 2023
@manicminer manicminer modified the milestones: v2.36.0, v2.37.0 Mar 2, 2023
@manicminer manicminer removed this from the v2.37.0 milestone Apr 13, 2023
@joraff
Copy link

joraff commented Apr 26, 2023

Anything I can do to help keep this moving? Will the work in hamilton need to be completed first?

@JonasBak JonasBak force-pushed the main branch 2 times, most recently from a48155a to 48f7188 Compare May 3, 2023 11:31
@JonasBak
Copy link
Contributor Author

JonasBak commented May 3, 2023

I rebased and added a basic test, I did the same for the PR in hamilton.

Copy link
Member

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

Hi @JonasBak, thanks for working on this!

This is off to a great start. I've made a suggestion inline below. It also looks like we're missing documentation for the new resource, if you can look at these then I'll be happy to take another look and we can see about merging this. Thanks!

@StickNitro
Copy link

Any update on this PR, I see the upstream changes are available with v0.62.0, this would be extremely beneficial to a piece of work I need to do

@github-actions github-actions bot added size/XL and removed size/L labels Jul 31, 2023
@JonasBak
Copy link
Contributor Author

I've rebased with main, added some docs, and added helpers.WaitForUpdate() as suggested

@JonasBak
Copy link
Contributor Author

@manicminer Could you take a look at this again?

Copy link
Member

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

@JonasBak Thanks for the reminder and apologies for the delay in re-reviewing. I have only one minor suggestion regarding the import validation (see below) which I'll update. Other than that, this looks great and the test passes!

I had a go at adding support for appScopeId too, but I struggled to work out what to specify for this field as the API seems to silently ignore it instead of erroring (I tried an app ID, object ID of an application, object ID for a service principal). We can look to add this later.

@manicminer
Copy link
Member

Test results

Screenshot 2023-09-22 at 01 49 27

@manicminer manicminer merged commit 9c66730 into hashicorp:main Sep 22, 2023
17 checks passed
@github-actions github-actions bot added this to the v2.43.0 milestone Sep 22, 2023
@joraff
Copy link

joraff commented Sep 22, 2023

@manicminer Thank you for approving this! We've been wanting to use it for a while.

FWIW - I don't think appScopeId is fully implemented; possibly reserved for future use? The default value of / is all that works. directoryScopeId is what can scope to an admin unit.

manicminer added a commit that referenced this pull request Sep 22, 2023
@manicminer
Copy link
Member

No problem, sorry again for the delay. Thanks for the feedback; I was able to scope a request to an application using the Portal, but alas the portal uses the PIM data plane API - so perhaps the MS Graph interface is incomplete as you say.

@joraff
Copy link

joraff commented Sep 22, 2023

Oh! Perhaps it is used now. It's been a while since I've done an assignment via the portal. Thanks to this, it might be a bit longer ;)

dduportal pushed a commit to jenkins-infra/azure that referenced this pull request Sep 23, 2023
<Actions>
<action
id="c2aadc6326b4b0bc58df11ee286b0f67ccdb5888bd77f391e6473570113337ec">
        <h3>Bump Terraform `azuread` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/azuread&#34; updated from &#34;2.42.0&#34; to
&#34;2.43.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>2.43.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azuread/releases/tag/v2.43.0&#xA;FEATURES:&#xA;&#xA;*
**New Resource:** `azuread_directory_role_eligibility_schedule_request`
([#974](https://github.com/hashicorp/terraform-provider-azuread/issues/974))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>
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.

None yet

4 participants