feat(iam, secretsmanager): add secretsmanager IAM rolebinding resources#1388
Open
rubenhoenle wants to merge 1 commit intomainfrom
Open
feat(iam, secretsmanager): add secretsmanager IAM rolebinding resources#1388rubenhoenle wants to merge 1 commit intomainfrom
rubenhoenle wants to merge 1 commit intomainfrom
Conversation
rubenhoenle
force-pushed
the
feat/iam-rolebindings
branch
from
95496f9 to
e75b053
Compare
rubenhoenle
commented
Apr 17, 2026
| ctx = tflog.SetField(ctx, "region", region) | ||
| ctx = tflog.SetField(ctx, "resource_id", resourceId) | ||
|
|
||
| // TODO: remove |
Member
Author
There was a problem hiding this comment.
resolve before merge
rubenhoenle
commented
Apr 17, 2026
relates to STACKITTPR-497
rubenhoenle
force-pushed
the
feat/iam-rolebindings
branch
from
e48153e to
5e12fb9
Compare
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Comment on lines
+191
to
+195
| roleBindingResp, err := r.ExecReadRequest(ctx, r.apiClient, region, resourceId, model.Role.ValueString(), model.Subject.ValueString()) | ||
| if err != nil { | ||
| core.LogAndAddError(ctx, &resp.Diagnostics, fmt.Sprintf("Error reading %s %s role binding", r.ApiName, r.ResourceType), fmt.Sprintf("Calling API: %v", err)) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
in #1303 a contributor implemented, that on read a 404 leads to the resources being removed from the state.
I'd add this here. Not every ExecReadRequest will return a 404, but in case it does not it doesn't matter.
| ctx = tflog.SetField(ctx, "resource_id", resourceId) | ||
|
|
||
| err := r.ExecDeleteRequest(ctx, r.apiClient, region, resourceId, model.Role.ValueString(), model.Subject.ValueString()) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
I'd also add an 404 check here and just return early on 404, TF will then remove the resource from state
| return b | ||
| } | ||
|
|
||
| // UpdateStep is the first step in your acceptance test and updates the resources |
Contributor
There was a problem hiding this comment.
looks like copy paste error from CreateStep
Comment on lines
+108
to
+116
| if b.createStep != nil { | ||
| tc.Steps = append(tc.Steps, *b.createStep) | ||
| } | ||
|
|
||
| if b.importStep != nil { | ||
| tc.Steps = append(tc.Steps, *b.importStep) | ||
| } | ||
|
|
||
| if b.updateStep != nil { |
Contributor
There was a problem hiding this comment.
I'd fail on these nil checks instead. The generic resource looks like these steps are required and thus should also be required in the test.
Contributor
There was a problem hiding this comment.
If these test steps are required: we could change the builder API to make it more clear that it's not optional to add these steps.
| if instances[i].Id == nil { | ||
| continue | ||
| } | ||
| if utils.Contains(instancesToDestroy, *instances[i].Id) { |
Member
Author
There was a problem hiding this comment.
code was only moved by me, but i will adjust it
Comment on lines
+8
to
+16
| {{/* Check whether this is a iam role binding resource. The check looks cursed because there's no hasSuffix function available here */}} | ||
| {{- $isRoleBinding := false -}} | ||
|
|
||
| {{- $parts := split .Name "_role_binding" -}} | ||
| {{- $lastItem := "" -}} | ||
|
|
||
| {{- range $parts -}} | ||
| {{- $lastItem = . -}} | ||
| {{- end -}} |
Contributor
There was a problem hiding this comment.
maybe it would be worth it to create an issue at tfplugin-docs to allow adding custom template funcs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
relates to STACKITTPR-497
IAM role binding API
This PR introduces the IAM role binding resources for the secretsmanager API. More services will adopt the role binding API in the future, so I've chosen a pretty generic approach for the implementation which allows us to implement further role binding resources with as little as possible effort.
Note: The IAM role binding API is a standardized API, so all upcoming role binding resources will look exactly the same. That's why using a generic approach is even possible at all 😅
Implementation of new role binding resource
Due to the generic implementation you only have to define some callback functions when implementing another role binding resource. It's pretty straightforward and is only ~40 lines of code.
terraform-provider-stackit/stackit/internal/services/iam/rolebindings/services/secretsmanager/secret_group.go
Lines 13 to 51 in 9df5f98
These two lines below determine the name of your resource, e.g. here it would be
stackit_secretsmanager_instance_role_bindingterraform-provider-stackit/stackit/internal/services/iam/rolebindings/services/secretsmanager/secret_group.go
Lines 15 to 16 in 9df5f98
Acceptance tests for new role binding resource
For the acceptance tests I implemented a builder pattern which allows us to implement the tests for every resource without duplicating all the boilerplate code every time.
This is how to implement an acceptance test for the
stackit_secretsmanager_instance_role_bindingresource:First you define your terraform config like you know it:
terraform-provider-stackit/stackit/internal/services/iam/rolebindings/services/secretsmanager/testdata/instance.tf
Lines 1 to 15 in 9df5f98
Now you use the role binding acc test builder instead of writing all the boilerplate:
terraform-provider-stackit/stackit/internal/services/iam/rolebindings/services/secretsmanager/iam_rolebindings_secretsmanager_acc_test.go
Lines 16 to 45 in 9df5f98
Generation of docs
But not only implementation of new role binding resources is trivial and less effort. You also don't have to provide examples and import statement for new resources on your own. Instead they are generated for you automatically:
terraform-provider-stackit/templates/resources.md.tmpl
Lines 26 to 33 in 9df5f98
terraform-provider-stackit/templates/resources.md.tmpl
Lines 50 to 57 in 9df5f98
Checklist
make fmtexamples/directory)make generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)