-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
r\iothub
: Support managed service identity
#14354
Conversation
r\iothub
: Support managed identityr\iothub
: Support managed service identity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @myc2h6o
Thanks for this PR.
I've taken a look through and left some comments inline, but if we can fix those up then we can take another look.
Thanks!
var identityIds map[string]*devices.ArmUserIdentity | ||
if len(config.UserAssignedIdentityIds) != 0 { | ||
identityIds = map[string]*devices.ArmUserIdentity{} | ||
for _, id := range config.UserAssignedIdentityIds { | ||
identityIds[id] = &devices.ArmUserIdentity{} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this'll ensure these are always removed:
var identityIds map[string]*devices.ArmUserIdentity | |
if len(config.UserAssignedIdentityIds) != 0 { | |
identityIds = map[string]*devices.ArmUserIdentity{} | |
for _, id := range config.UserAssignedIdentityIds { | |
identityIds[id] = &devices.ArmUserIdentity{} | |
} | |
} | |
identityIds = make(map[string]*devices.ArmUserIdentity, 0) | |
for _, id := range config.UserAssignedIdentityIds { | |
identityIds[id] = &devices.ArmUserIdentity{} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have updated the code, but I didn't fully understand the comment, is it related to the garbage collection mechanism?
And I found that identityIds has to be nil for a SystemAssigned identity. An error will be thrown if an empty map is given. So I've updated the code to first create the ArmUserIdentity
without identityIds
and set it only when provided. Please check if it looks good.
website/docs/r/iothub.html.markdown
Outdated
|
||
* `identity_ids` - (Optional) A list of User Managed Identity ID's which should be assigned to the Iot Hub. | ||
|
||
~> **NOTE:** This is required when `type` is set to `UserAssigned`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also required when it's set to SystemAssigned/UserAssigned:
~> **NOTE:** This is required when `type` is set to `UserAssigned`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
@tombuildsstuff thanks for reviewing the change. I've updated the code, please take another look. |
0811b41
to
5528f59
Compare
5528f59
to
aff5802
Compare
Rebased to latest main to resolve conflict |
Hi @tombuildsstuff I saw you added new identity types in the linked pr, I've updated this change to use the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @myc2h6o - LGTM 💎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for pushing those changes @myc2h6o
This functionality has been released in v2.91.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fix #9139
This is required when Iot Hub needs to talk to other resources on a private v-net. Support for
AuthenticatinType
in routing, file upload, device import/export will be added in following prsTest result: