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

value on service_principal_password is deprecated but docs still list it as required. #479

Closed
robin-wayve opened this issue Jul 8, 2021 · 7 comments · Fixed by #493
Closed

Comments

@robin-wayve
Copy link

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritise this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritise the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform (and AzureAD Provider) Version

Terraform v0.13.4
+ provider registry.terraform.io/hashicorp/azuread v1.5.1

Affected Resource(s)

  • azuread_service_principal_password

Terraform Configuration Files

resource "azuread_service_principal_password" "example" {
  service_principal_id = azuread_service_principal.example.id
  value                = random_password.password.result
  end_date_relative    = "48h"
}

Debug Output

- Attribute is deprecated

Expected Behavior

The documentation should list value as deprecated and optional.

Actual Behavior

The documentation lists value as required while terraform plan informs me the attribute is deprecated.
In a test, I was also able to create a service_principal_password without specifying the value.
Removing the value also does not modify existing resources.

Steps to Reproduce

  1. terraform apply

Important Factoids

References

value - (Required) The Password for this Service Principal.

The value field will become read-only as Azure Active Directory no longer accepts user-supplied password values. Passwords will instead be auto-generated by Azure and will be exported as attributes by the resource.

@manicminer
Copy link
Contributor

manicminer commented Jul 8, 2021

Hi @robin-wayve , thanks for reporting this docs discrepency! We've already merged some docs changes for the upcoming v2.0 which should correctly reflect that the value property is now a read-only attribute (see microsoft-graph.md and service_principal_password.md). Unfortunately this was missed in the v1.5/v1.6 releases but it will be corrected shortly when v2.0 is released. As we don't publish documentation for older versions, and the 2.0 docs are now committed ready for release, I've added this issue to the 2.0.0 milestone and it will be closed at release time, but if you notice any other discrepancies where corrections have not been merged, we would very much welcome further issue reports. Thanks again!

@robin-wayve
Copy link
Author

@manicminer can I confirm which version changed the behaviour? I actually saw the message about this being deprecated even on 1.4, so was wondering when it really became optional / read-only.

@manicminer
Copy link
Contributor

manicminer commented Jul 8, 2021

@robin-wayve We issued a deprecation notice in v1.5.0 for the value properties of azuread_service_principal_password and azuread_application_password. This release also introduced MS Graph support, and when a v1.x provider is configured to use MS Graph, you cannot set the value property (but it is returned by the API on creation). In order to achieve compatibility for users switching between APIs, we also added a password generator (using sethvargo/go-password) so that value can be omitted by users even when using the legacy API. In v2.0 value will become Computed (not settable by users). When upgrading to v1.5 or later, you should be able to remove the value property from your configuration and the existing value will remain in your state (and in AAD).

For the display_name / description and start_date / end_date properties, in v1.5.0 there's a bug that prevents these being set for azuread_application_password, however in v1.5.1 and later this is fixed and you can set these. There's an API issue with azuread_service_principal_password that means you can no longer set a display name or dates when using MS Graph (this looks likely to persist in v2.0).

We do try and keep the changelog updated with this sort of info, whilst it might not give the full context it's the best place to look. Hope this helps clarify the changes :)

@robin-wayve
Copy link
Author

When upgrading to v1.5 or later, you should be able to remove the value property from your configuration and the existing value will remain in your state (and in AAD).

@manicminer Thanks, this is what I was hoping was the case. 👍🏻

I did wind up reading the changelog but there aren't any references to azuread_service_principal_password between 1.5.1 (the bugfix, unrelated to value) and 0.10.0 -- was something missed there too?

@manicminer
Copy link
Contributor

manicminer commented Jul 8, 2021

In 1.5.0 there were so many deprecations, many with additional subtlety (like this one), so the changelog pointed to the Upgrade Guide. In retrospect this could have been done better, especially as the upgrade guide has since received some updates in light of the aforementioned bug in 1.5.0

I'm sorry that it wasn't made clearer in the docs that passwords would be preserved on upgrade, I will make sure to add this.

@robin-wayve
Copy link
Author

It's all good, these changes are a nice improvement, just wanted to make sure I was upgrading safely.

@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants