Feature/dns skip wait and partial state #1052
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,19 @@ | ||
| #!/usr/bin/env bash | ||
| # This script lints the SDK modules and the internal examples | ||
| # Pre-requisites: golangci-lint (provided by Makefile or system) | ||
| set -eo pipefail | ||
|
|
||
| ROOT_DIR=$(git rev-parse --show-toplevel) | ||
| GOLANG_CI_YAML_PATH="${ROOT_DIR}/golang-ci.yaml" | ||
| GOLANG_CI_ARGS="--allow-parallel-runners --timeout=5m --config=${GOLANG_CI_YAML_PATH}" | ||
|
|
||
| # Use provided golangci-lint binary or fallback to system installation | ||
| GOLANGCI_LINT_BIN="${1:-golangci-lint}" | ||
|
|
||
| if [ ! -x "${GOLANGCI_LINT_BIN}" ] && ! type -p "${GOLANGCI_LINT_BIN}" >/dev/null; then | ||
| echo "golangci-lint not found at ${GOLANGCI_LINT_BIN} and not installed in PATH, unable to proceed." | ||
| exit 1 | ||
| fi | ||
|
|
||
| cd ${ROOT_DIR} | ||
| ${GOLANGCI_LINT_BIN} run ${GOLANG_CI_ARGS} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ package dns | |
| import ( | ||
| "context" | ||
| "fmt" | ||
| "net/http" | ||
| "strings" | ||
|
|
||
| "github.com/hashicorp/terraform-plugin-framework-validators/int64validator" | ||
|
|
@@ -16,6 +17,7 @@ import ( | |
| "github.com/hashicorp/terraform-plugin-framework/schema/validator" | ||
| "github.com/hashicorp/terraform-plugin-framework/types" | ||
| "github.com/hashicorp/terraform-plugin-log/tflog" | ||
| "github.com/stackitcloud/stackit-sdk-go/core/oapierror" | ||
| "github.com/stackitcloud/stackit-sdk-go/services/dns" | ||
| "github.com/stackitcloud/stackit-sdk-go/services/dns/wait" | ||
| "github.com/stackitcloud/terraform-provider-stackit/stackit/internal/conversion" | ||
|
|
@@ -219,15 +221,27 @@ func (r *recordSetResource) Create(ctx context.Context, req resource.CreateReque | |
| } | ||
|
|
||
| // Write id attributes to state before polling via the wait handler - just in case anything goes wrong during the wait handler | ||
| recordSetId := *recordSetResp.Rrset.Id | ||
| model.RecordSetId = types.StringValue(recordSetId) | ||
| model.Id = utils.BuildInternalTerraformId(projectId, zoneId, recordSetId) | ||
|
|
||
| // Set all unknown/null fields to null before saving state | ||
| if err := utils.SetModelFieldsToNull(ctx, &model); err != nil { | ||
|
Member
There was a problem hiding this comment. Sorry, but I just don't get why one would want to have this. What's the point of this?
Contributor
Author
There was a problem hiding this comment. It might be because of weird clients. Currently we only set project_id and zone_id in the state before waiting. This lead to the following error if the waiting is skipped which some clients want: So I have set the id as well in the helper function. I think I observed an error in the past that the client got an error because some field in the state were "unknown". But I can no longer find this error message anymore. So currently I get: Because of it the client wants to destroy the resource: For some reason if you set the fields to null instead of unknown the client accepts it and proceeds correctly. Maybe we need to take a look together into the topic. If you have some better ways to handle this case feel free to suggest :)
Member
There was a problem hiding this comment. Just to make sure I don't mess things up here, what do you mean with
Contributor
Author
There was a problem hiding this comment. crossplane+upjet that then executes terraform cli commands
Member
There was a problem hiding this comment. 
Messages like this are there for a reason by Terraform. You would break this behavior with this change.
Contributor
Author
There was a problem hiding this comment. I suspect that is a problem with complex objects, list of lists and list of complex objects in the utils function SetModelFieldsToNull. Can you provide the input parameters so I can add unit tests for this case to verify if it happens in the implementation or not?
Contributor
Author
There was a problem hiding this comment. or do you imply that it is perfectly fine to have errors? because if we want to use upjet to generate a crossplane provider we cannot accept such error since it simply does not work :D
Member
There was a problem hiding this comment.
Clear no.
Well, I guess it doesn't work because you modified the code of the terraform provider and didn't understand the impacts of your changes. I have to start from scratch here: Unknown values are a core concept of Terraform (see https://developer.hashicorp.com/terraform/plugin/framework/handling-data/terraform-concepts#unknown-values). Unknown values are important for Terraform to apply resources in the correct order, ... But what does this mean for us? After a 
Whenever you get a message like this it's clear that this is a bug in the Terraform provider. And I'm going to lean myself out of the window here and say this doesn't happen for the Let me explain whyWe create the resource on API side and then use the wait handler. After the wait handler we use the Now comes the important part: Here is the section in the Well, and after that the model struct must be persisted in the Terraform state (this doesn't happen automatically): To sum it up, here's what happens in the main branch implementation of this resource:
Now to your changesNow to your changes and why it's not working (without setting all fields to null using your new reflection-powered util func): In your
if !utils.ShouldWait() {
tflog.Info(ctx, "Skipping wait; async mode for Crossplane/Upjet")
return
}The problem is: This doesn't only skip the wait handler (no. 3 above), but also the Again, you just skip this. This is a core part of the resource implementation. You don't call it. That's why Terraform complains about unknown values. Terraform says this is a bug in the provider implementation, and it's correct. But it's sadly not a bug in our implementation on the main branch, but in your implementation. You circumvent this problem by setting all fields of the Terraform resource state model explicitly to null by using your new util func. This circumvents the problem (Terraform doesn't complain anymore about unknown values), but it doesn't really fix the problem (at least not in a clean way). In fact setting all fields of the Terraform resource model struct to null circumvents existing checks of Terraform which we want to take advantage of during our resource implementations (at least for pure Terraform usage, without thinking of crossplane here). [1] Btw, if you forget to set one field of the Terraform resource model struct to a value of null here during the implementation of the Terraform resource you will also get exactly the error |
||
| core.LogAndAddError(ctx, &resp.Diagnostics, "Error creating record set", fmt.Sprintf("Setting model fields to null: %v", err)) | ||
| return | ||
| } | ||
|
|
||
| diags = resp.State.Set(ctx, model) | ||
| resp.Diagnostics.Append(diags...) | ||
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } | ||
|
|
||
| if !utils.ShouldWait() { | ||
| tflog.Info(ctx, "Skipping wait; async mode for Crossplane/Upjet") | ||
| return | ||
| } | ||
|
|
||
| waitResp, err := wait.CreateRecordSetWaitHandler(ctx, r.client, projectId, zoneId, *recordSetResp.Rrset.Id).WaitWithContext(ctx) | ||
| if err != nil { | ||
| core.LogAndAddError(ctx, &resp.Diagnostics, "Error creating record set", fmt.Sprintf("Instance creation waiting: %v", err)) | ||
|
|
@@ -264,8 +278,19 @@ func (r *recordSetResource) Read(ctx context.Context, req resource.ReadRequest, | |
| ctx = tflog.SetField(ctx, "zone_id", zoneId) | ||
| ctx = tflog.SetField(ctx, "record_set_id", recordSetId) | ||
|
|
||
| if recordSetId == "" || zoneId == "" || projectId == "" { | ||
PatrickKoss marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| tflog.Info(ctx, "Record set ID, zone ID, or project ID is empty, removing resource") | ||
| resp.State.RemoveResource(ctx) | ||
| return | ||
| } | ||
|
|
||
| recordSetResp, err := r.client.GetRecordSet(ctx, projectId, zoneId, recordSetId).Execute() | ||
| if err != nil { | ||
| oapiErr, ok := err.(*oapierror.GenericOpenAPIError) //nolint:errorlint //complaining that error.As should be used to catch wrapped errors, but this error should not be wrapped | ||
PatrickKoss marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if ok && (oapiErr.StatusCode == http.StatusNotFound || oapiErr.StatusCode == http.StatusGone) { | ||
| resp.State.RemoveResource(ctx) | ||
| return | ||
| } | ||
| core.LogAndAddError(ctx, &resp.Diagnostics, "Error reading record set", fmt.Sprintf("Calling API: %v", err)) | ||
| return | ||
| } | ||
|
|
@@ -319,6 +344,12 @@ func (r *recordSetResource) Update(ctx context.Context, req resource.UpdateReque | |
| core.LogAndAddError(ctx, &resp.Diagnostics, "Error updating record set", err.Error()) | ||
| return | ||
| } | ||
|
|
||
| if !utils.ShouldWait() { | ||
| tflog.Info(ctx, "Skipping wait; async mode for Crossplane/Upjet") | ||
| return | ||
| } | ||
|
|
||
| waitResp, err := wait.PartialUpdateRecordSetWaitHandler(ctx, r.client, projectId, zoneId, recordSetId).WaitWithContext(ctx) | ||
| if err != nil { | ||
| core.LogAndAddError(ctx, &resp.Diagnostics, "Error updating record set", fmt.Sprintf("Instance update waiting: %v", err)) | ||
|
|
@@ -358,8 +389,22 @@ func (r *recordSetResource) Delete(ctx context.Context, req resource.DeleteReque | |
| // Delete existing record set | ||
| _, err := r.client.DeleteRecordSet(ctx, projectId, zoneId, recordSetId).Execute() | ||
| if err != nil { | ||
| // If resource is already gone (404 or 410), treat as success for idempotency | ||
| oapiErr, ok := err.(*oapierror.GenericOpenAPIError) //nolint:errorlint //complaining that error.As should be used to catch wrapped errors, but this error should not be wrapped | ||
| if ok && | ||
| (oapiErr.StatusCode == http.StatusNotFound || oapiErr.StatusCode == http.StatusGone) { | ||
| tflog.Info(ctx, "Record set already deleted") | ||
| return | ||
| } | ||
| core.LogAndAddError(ctx, &resp.Diagnostics, "Error deleting record set", fmt.Sprintf("Calling API: %v", err)) | ||
| return | ||
| } | ||
|
|
||
| if !utils.ShouldWait() { | ||
| tflog.Info(ctx, "Skipping wait; async mode for Crossplane/Upjet") | ||
| return | ||
| } | ||
|
|
||
| _, err = wait.DeleteRecordSetWaitHandler(ctx, r.client, projectId, zoneId, recordSetId).WaitWithContext(ctx) | ||
| if err != nil { | ||
| core.LogAndAddError(ctx, &resp.Diagnostics, "Error deleting record set", fmt.Sprintf("Instance deletion waiting: %v", err)) | ||
|
|
@@ -380,11 +425,23 @@ func (r *recordSetResource) ImportState(ctx context.Context, req resource.Import | |
| return | ||
| } | ||
|
|
||
| var model Model | ||
| model.ProjectId = types.StringValue(idParts[0]) | ||
| model.ZoneId = types.StringValue(idParts[1]) | ||
| model.RecordSetId = types.StringValue(idParts[2]) | ||
| model.Id = utils.BuildInternalTerraformId(idParts[0], idParts[1], idParts[2]) | ||
|
|
||
| if err := utils.SetModelFieldsToNull(ctx, &model); err != nil { | ||
| core.LogAndAddError(ctx, &resp.Diagnostics, "Error importing zone", fmt.Sprintf("Setting model fields to null: %v", err)) | ||
| return | ||
| } | ||
|
|
||
| diags := resp.State.Set(ctx, model) | ||
| resp.Diagnostics.Append(diags...) | ||
| if diags.HasError() { | ||
| return | ||
| } | ||
|
|
||
| tflog.Info(ctx, "DNS record set state imported") | ||
| } | ||
|
|
||
|
|
||
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.
Running bash scripts blindly from a master branch of another repository is a no-go for me, sorry
Overall, what's the point of this? This whole thing feels wrong to me. For managing development dependencies there are things like dev containers, devenvs, nix flakes, ...
I'm aware we're not providing any of these currently, but this download process inside the Makefile seems pretty hacky to me 😄
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.
yeah that was a bit too ambitious. You know once you copy it from somewhere you always copy it :P
I replaced it with downloading from the releases which should be secure.
It is actually quite typically to download binaries that are needed to interact with the application (like linting, kubectl, kind, helm, etc) via scripts/make. In many stackit projects that is already the case. And there are also many opensource projects that do similar things like:
I guess many ways solve the same problem. Currently my biggest problem is that I cannot lint locally since there are version diffs between my installed golangci lint and the one in the pipeline. Therefore I want to have a make command that runs the same version in the pipeline as in our local env. Some might say that is the shift left approach.