[Feature] Terrafron Provider for Open Cloud Datacenter API#205
[Feature] Terrafron Provider for Open Cloud Datacenter API#205sameera-87 wants to merge 1 commit into
Conversation
|
|
📝 WalkthroughWalkthroughThis PR introduces a complete Terraform provider ( DC-API Terraform Provider
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
docs/dc-api-reference.md-27-27 (1)
27-27: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the broken
PrivateDnsZonetable-of-contents link.Line 27 points to
#resource-privatednszoned, so Markdown renderers will not jump to thePrivateDnsZonesection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/dc-api-reference.md` at line 27, The table-of-contents entry for PrivateDnsZone uses a broken anchor, so update the markdown link target to match the actual section heading slug generated for PrivateDnsZone. Fix the TOC entry itself rather than the section, and verify the anchor resolves correctly in the docs renderer.Source: Linters/SAST tools
examples/vm/main.tf-82-85 (1)
82-85: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse
subnet_uuidhere as well.This output is documented as the subnet UUID, but it currently returns
dcapi_subnet.app.id. The same file already usesdcapi_subnet.app.subnet_uuidon Line 60 for the actual API identifier, so this output will otherwise expose the wrong value to downstream consumers.Suggested fix
output "subnet_id" { - value = dcapi_subnet.app.id + value = dcapi_subnet.app.subnet_uuid description = "UUID of the created Subnet." }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/vm/main.tf` around lines 82 - 85, The subnet output is returning the Terraform resource ID instead of the API UUID, so update the output block named subnet_id in main.tf to use dcapi_subnet.app.subnet_uuid just like the existing subnet_uuid reference in the same configuration. Keep the description aligned with the value being exported so downstream consumers receive the actual subnet UUID rather than the provider-generated id.examples/subnet/main.tf-49-52 (1)
49-52: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winOutput the subnet UUID instead of Terraform's state ID.
This block says it returns the created subnet UUID, but
dcapi_subnet.app.idis the Terraform resource ID. The provider already exposes the API UUID separately (subnet_uuid), and the VM example consumes that field for downstream wiring. Returning.idhere will hand callers the wrong identifier shape.Suggested fix
output "subnet_id" { - value = dcapi_subnet.app.id + value = dcapi_subnet.app.subnet_uuid description = "UUID of the created Subnet." }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/subnet/main.tf` around lines 49 - 52, The subnet output is currently returning Terraform’s resource ID instead of the API UUID. Update the `output "subnet_id"` block in the subnet example to use the `dcapi_subnet.app.subnet_uuid` attribute, matching the provider’s UUID field and the way the VM example wires subnet values downstream. Keep the output name and description as-is, but ensure the value references the UUID-exposing symbol on `dcapi_subnet.app`.internal/resources/vnet.go-253-260 (1)
253-260: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe 409 guidance points users at the wrong attribute.
dcapi_subnet.vnet_idexpects the bare UUID, but this message tells users to referencedcapi_vnet.example.id, which is the compositetenant/project/uuidstate ID. Following that advice will build an invalid subnet path; the example should usedcapi_vnet.example.vnet_uuid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/vnet.go` around lines 253 - 260, The 409 delete guidance in the VNet error path is pointing users to the wrong reference attribute. Update the message in the VNet deletion logic (the strings.Contains(err.Error(), "HTTP 409") branch) so the example Terraform reference uses dcapi_vnet.example.vnet_uuid instead of dcapi_vnet.example.id, since dcapi_subnet.vnet_id requires the bare UUID and not the composite state ID.internal/resources/tenant.go-14-19 (1)
14-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
Importerwiring for these resources.schema.Resourcedoesn’t setImporterhere, soterraform importwon’t work fordcapi_tenantor the other new resources ininternal/resources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/tenant.go` around lines 14 - 19, The tenant resource is missing import support, so terraform import cannot work for this and the other new resources. Update ResourceTenant and the corresponding Resource constructors in internal/resources to wire schema.Resource.Importer, using the appropriate StateContext importer helper already used by other resources. Make sure the import setup is added alongside CreateContext, ReadContext, UpdateContext, and DeleteContext so dcapi_tenant and related resources can be imported consistently.internal/resources/node_pool.go-276-281 (1)
276-281: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat HTTP 404 as a successful delete.
internal/client/node_pool.go:103-111bubbles any DELETE error, and Lines 276-281 return immediately on that error before the poller runs. If the pool was already removed out of band, destroy fails instead of converging on the desired absent state. NormalizeHTTP 404to success in the delete path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/node_pool.go` around lines 276 - 281, Treat HTTP 404 as a successful delete in the node pool destroy flow. Update the DeleteNodePool path used by node pool deletion so a not-found response is normalized to nil instead of bubbling an error, and make the delete handler in internal/client/node_pool.go return success when the pool is already gone. Keep the existing waitForNodePoolDeleted poller behavior in internal/resources/node_pool.go so the resource still converges on absent state when DeleteNodePool is called.
🧹 Nitpick comments (4)
docs/dc-api-reference.md (1)
419-429: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMark the VM and Cluster networking selectors as user inputs, not computed fields.
network_name,vnet_id, andsubnet_idappear in the create request bodies and must be set by configuration for the chosen networking mode. Labeling themCOMPUTED_OPTIONALflips that contract and makes these tables inconsistent with the examples above.Also applies to: 661-663
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/dc-api-reference.md` around lines 419 - 429, The field classification table is labeling VM and Cluster networking selectors as computed, but `network_name`, `vnet_id`, and `subnet_id` are user-provided inputs in the create request bodies. Update the Field Classification entries in the relevant documentation sections so these selectors are marked as user inputs for their respective networking modes, keeping the wording consistent with the request examples and the related Cluster table.Architecture.md (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate this architecture doc to match the resource set shipped in
0.1.0.These sections still describe only the tenant/project/vnet/subnet/vm slice, but this PR also ships Bastion, Cluster, NodePool, RouteTable, RouteTableAssociation, NetworkSecurityGroup, NSGAttachment, VNetPeering, and ServiceAccount. The resource hierarchy is also incomplete for VPC-backed workloads. That makes the architecture doc stale on merge day.
Also applies to: 11-28, 30-46, 370-399
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Architecture.md` at line 3, Update the architecture documentation to reflect the full 0.1.0 resource set instead of only the tenant/project/vnet/subnet/vm slice. In the Architecture.md sections currently describing the provider scope and resource hierarchy, add Bastion, Cluster, NodePool, RouteTable, RouteTableAssociation, NetworkSecurityGroup, NSGAttachment, VNetPeering, and ServiceAccount, and expand the VPC-backed workload hierarchy accordingly. Use the existing architecture headings and resource overview areas in Architecture.md so the doc stays aligned with the shipped provider surface.internal/client/project.go (1)
87-92: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftStop classifying 404s by parsing error strings.
doRequestonly returns formattederrors today, so the read paths are forced to inspect message text. Add a typed/status-bearing error and branch on that instead ofstrings.Contains(err.Error(), "HTTP 404"); otherwise a wording change or unrelated wrapped error can misclassify drift. The same pattern exists ininternal/client/vnet.go,internal/client/subnet.go,internal/client/service_account.go,internal/client/vm.go,internal/client/nsg.go,internal/client/route_table.go,internal/client/node_pool.go,internal/client/cluster.go, andinternal/client/bastion.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/project.go` around lines 87 - 92, The 404 handling in GetProjectByID is still parsing err.Error() text, which should be replaced with a typed/status-bearing error from doRequest and checked directly in the read path. Update doRequest to return an error that exposes the HTTP status, then change GetProjectByID to branch on that status instead of strings.Contains(err.Error(), "HTTP 404"), preserving the nil,nil behavior for not-found. Apply the same pattern to the other client read methods that currently inspect error strings, including the ones in vnet, subnet, service_account, vm, nsg, route_table, node_pool, cluster, and bastion.internal/client/vm.go (1)
91-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a status-based helper for 404 handling.
GetVMand the other getters ininternal/clientall depend ondoRequestemitting"HTTP 404"in its error text. A small shared helper ininternal/client/client.gowould make the 404→nilcontract explicit and avoid breaking drift detection if the formatter changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/vm.go` around lines 91 - 96, The 404 handling in GetVM currently depends on matching the text from doRequest, which is brittle across getters in internal/client. Add a shared status-based helper in internal/client/client.go that detects a 404 from the request error or status and returns the nil/not-found result explicitly, then update GetVM and the other getter methods to use that helper instead of strings.Contains on err.Error(). Keep the existing nil-on-404 contract and preserve the existing wrapping for non-404 errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dc-api-reference.md`:
- Line 66: The auth example in docs/dc-api-reference.md still shows unsupported
provider arguments (`tenant_id` and `project_id`) even though the provider
examples only take `endpoint` and `token`; update the example to use the same
supported auth fields as the rest of the docs. Also tighten the credential
guidance in the Terraform section by removing the “member” minimum for tenant
operations and reflecting that tenant actions are admin-only, using the relevant
provider example blocks and tenant auth text as the source of truth.
In `@GNUmakefile`:
- Around line 4-12: The install target currently copies the provider as
terraform-provider-dcapi, but Terraform discovers plugins by the versioned
binary name; update the GNUmakefile install flow so the copy step in install
uses the Terraform versioned filename for the built artifact, while keeping
BINARY and INSTALL_DIR aligned with the provider version. Use the existing build
and install targets to locate the change and ensure the installed filename
includes v0.1.0.
In `@go.mod`:
- Around line 70-78: The dependency graph still contains vulnerable pinned
versions in go.mod, so refresh the affected modules before release. Update the
transitive dependencies pulled in through terraform-plugin-sdk/v2 that bring in
golang.org/x/crypto and google.golang.org/grpc, then run go mod tidy to
regenerate the module graph and lock in the upgraded versions.
In `@internal/client/client.go`:
- Around line 6-14: The shared HTTP client used by doRequest currently relies on
the default client with no timeout, which can leave plan/apply blocked on hung
connections. Update the client initialization in client.go to use a bounded
timeout, or accept a timeout from provider configuration and thread it into the
client used by doRequest. Keep the change centered around the shared HTTP client
path and the request flow that all resource calls use.
In `@internal/client/node_pool.go`:
- Around line 28-35: The NodePoolUpdateRequest shape in node_pool.go cannot use
nil slices/maps to mean “omit” because encoding/json will emit null for Taints
and Labels, which can unintentionally clear server state on count-only updates.
Update the request model and its PATCH serialization path so omitted fields are
truly excluded from the JSON payload, or add custom marshaling/pointer fields in
NodePoolUpdateRequest to preserve the intended omit-vs-clear behavior for Taints
and Labels.
- Around line 72-73: The node-pool request path is interpolating poolName
directly in DCAPIClient.GetNodePool, which can break GET/PATCH/DELETE when the
name contains reserved characters. Update the node-pool URL construction to
escape the poolName segment before formatting the path, and apply the same fix
anywhere the node-pool path is built so the full name remains intact even
without a ValidateFunc on name.
In `@internal/client/subnet.go`:
- Around line 51-56: CreateSubnet currently returns wrapper.Resource without
verifying it was actually present, which can let a missing 202 response body
slip through as (nil, nil). Update CreateSubnet in internal/client/subnet.go to
validate the unmarshaled SubnetCreateResponse has a non-nil Resource before
returning, and if it is missing, return an explicit error from the CreateSubnet
path instead of nil; keep the existing JSON parsing flow but add the check right
after json.Unmarshal and before returning wrapper.Resource.
In `@internal/client/vm.go`:
- Around line 77-82: The CreateVM response handling currently returns a parsed
VMCreateResponse even when the accepted payload is structurally incomplete.
After json.Unmarshal in CreateVM, validate that the returned resp includes a
non-empty resource.id before returning it, and if it is missing, fail fast with
a clear error instead of returning a nil/empty state that will break later
polling.
In `@internal/client/vnet.go`:
- Around line 49-54: The CreateVNet response handling currently returns
wrapper.Resource unchecked, which can turn an empty 202 response into (nil,
nil). Update the CreateVNet logic in vnet.go to validate that the unmarshaled
VNetCreateResponse contains a non-nil Resource before returning; if it is
missing, return a descriptive error from this path instead of a nil result so
callers get a clear diagnostic.
In `@internal/provider/provider.go`:
- Around line 45-59: The provider registration in ResourcesMap is missing the
dcapi_vnet_peering resource, so add the corresponding entry alongside the other
dcapi_* resources in provider initialization. Use the existing resource factory
pattern from symbols like ResourceVNet, ResourceCluster, and
ResourceRouteTableAssociation, and ensure the new map key exactly matches
dcapi_vnet_peering so the resource can be declared and imported.
- Around line 31-38: The provider token resolution currently only uses the
provider block and DCAPI_TOKEN env fallback, but the advertised dcctl credential
fallback is missing. Update the token handling in provider initialization and
related validation paths in the provider schema/config code so that when token
is empty it also attempts to load existing dcctl credentials before failing. Use
the existing token schema field and the provider setup/validation logic
referenced by the token attribute to wire this fallback in cleanly.
In `@internal/resources/cluster.go`:
- Around line 84-89: The system_pool.disk_gb schema in the cluster resource is
causing perpetual ForceNew diffs when omitted from config because Read writes
the API value into state without a matching config value. Update the disk_gb
field definition in the cluster schema to either mark it as Computed or set an
explicit default that matches the API behavior, and make sure the resource’s
Read/update flow stays consistent with that choice so omitted config does not
trigger replacement.
In `@internal/resources/node_pool.go`:
- Around line 74-79: The `disk_gb` handling in `node_pool.go` is silently
treating invalid values like 0 as “unset,” which causes the API default to be
applied instead of failing. Update the `disk_gb` schema on the node pool
resource to validate a minimum of 40, and adjust the create/update path in
`nodePool` so `disk_gb` is only omitted when the attribute is truly not set
rather than when it is non-positive. Make sure the logic around the current
`disk_gb` handling preserves explicit user input and does not coerce invalid
values into defaults.
In `@internal/resources/route_table.go`:
- Around line 79-92: The route table schema allows an invalid
`virtual_appliance` route to proceed without `next_hop_ip`, and
`routeTableResource` only copies `next_hop_ip` opportunistically during apply.
Add validation in the `routeTableResource`/schema path so `next_hop_ip` is
required when `next_hop_type` is `virtual_appliance`, and reject it for the
non-`virtual_appliance` hop types as well. Keep the check close to the existing
`next_hop_type` and `next_hop_ip` fields so the bad combination is caught before
apply.
In `@internal/resources/service_account.go`:
- Around line 19-25: Add import support to the Resource schemas by setting an
Importer on ResourceServiceAccount and the other affected resource constructors
(node_pool, nsg, nsg_attachment, route_table, route_table_association). Also
update each Read handler, especially resourceServiceAccountRead, to parse the
composite import ID and repopulate every ForceNew parent field from it (for
example both tenant_id and project_id) so imported state is complete.
In `@internal/resources/tenant.go`:
- Around line 229-233: The tenant delete path currently reports success while
only removing the resource from Terraform state, which leaves the remote tenant
orphaned. Update resourceTenantDelete to stop pretending deletion succeeded:
instead of clearing the ID and returning nil, return a diagnostic indicating
tenant deletion is unsupported until the DC-API provides a real delete endpoint.
Keep the change localized to resourceTenantDelete so terraform destroy surfaces
a failure rather than silently succeeding.
- Around line 51-67: The quota cap schema fields in tenant resource are causing
perpetual diffs because the Terraform defaults of 0 conflict with the
API-resolved values written back into state. Update the resource schema entries
for cpu_cores_cap, memory_gb_cap, and storage_gb_cap in the tenant resource to
use Optional plus Computed (or add equivalent diff suppression/normalization) so
omitted or zero-valued configs do not constantly plan changes against the
resolved 80/256/2000 values.
In `@test/main.tf`:
- Around line 12-16: The dcapi_tenant resource example is using the wrong
attribute name, which causes the config to fail validation. Update the tenant
resource block in test/main.tf to use tenant_id instead of id, matching the
tenant examples and docs used elsewhere in this PR. Keep the rest of the
resource fields unchanged.
---
Minor comments:
In `@docs/dc-api-reference.md`:
- Line 27: The table-of-contents entry for PrivateDnsZone uses a broken anchor,
so update the markdown link target to match the actual section heading slug
generated for PrivateDnsZone. Fix the TOC entry itself rather than the section,
and verify the anchor resolves correctly in the docs renderer.
In `@examples/subnet/main.tf`:
- Around line 49-52: The subnet output is currently returning Terraform’s
resource ID instead of the API UUID. Update the `output "subnet_id"` block in
the subnet example to use the `dcapi_subnet.app.subnet_uuid` attribute, matching
the provider’s UUID field and the way the VM example wires subnet values
downstream. Keep the output name and description as-is, but ensure the value
references the UUID-exposing symbol on `dcapi_subnet.app`.
In `@examples/vm/main.tf`:
- Around line 82-85: The subnet output is returning the Terraform resource ID
instead of the API UUID, so update the output block named subnet_id in main.tf
to use dcapi_subnet.app.subnet_uuid just like the existing subnet_uuid reference
in the same configuration. Keep the description aligned with the value being
exported so downstream consumers receive the actual subnet UUID rather than the
provider-generated id.
In `@internal/resources/node_pool.go`:
- Around line 276-281: Treat HTTP 404 as a successful delete in the node pool
destroy flow. Update the DeleteNodePool path used by node pool deletion so a
not-found response is normalized to nil instead of bubbling an error, and make
the delete handler in internal/client/node_pool.go return success when the pool
is already gone. Keep the existing waitForNodePoolDeleted poller behavior in
internal/resources/node_pool.go so the resource still converges on absent state
when DeleteNodePool is called.
In `@internal/resources/tenant.go`:
- Around line 14-19: The tenant resource is missing import support, so terraform
import cannot work for this and the other new resources. Update ResourceTenant
and the corresponding Resource constructors in internal/resources to wire
schema.Resource.Importer, using the appropriate StateContext importer helper
already used by other resources. Make sure the import setup is added alongside
CreateContext, ReadContext, UpdateContext, and DeleteContext so dcapi_tenant and
related resources can be imported consistently.
In `@internal/resources/vnet.go`:
- Around line 253-260: The 409 delete guidance in the VNet error path is
pointing users to the wrong reference attribute. Update the message in the VNet
deletion logic (the strings.Contains(err.Error(), "HTTP 409") branch) so the
example Terraform reference uses dcapi_vnet.example.vnet_uuid instead of
dcapi_vnet.example.id, since dcapi_subnet.vnet_id requires the bare UUID and not
the composite state ID.
---
Nitpick comments:
In `@Architecture.md`:
- Line 3: Update the architecture documentation to reflect the full 0.1.0
resource set instead of only the tenant/project/vnet/subnet/vm slice. In the
Architecture.md sections currently describing the provider scope and resource
hierarchy, add Bastion, Cluster, NodePool, RouteTable, RouteTableAssociation,
NetworkSecurityGroup, NSGAttachment, VNetPeering, and ServiceAccount, and expand
the VPC-backed workload hierarchy accordingly. Use the existing architecture
headings and resource overview areas in Architecture.md so the doc stays aligned
with the shipped provider surface.
In `@docs/dc-api-reference.md`:
- Around line 419-429: The field classification table is labeling VM and Cluster
networking selectors as computed, but `network_name`, `vnet_id`, and `subnet_id`
are user-provided inputs in the create request bodies. Update the Field
Classification entries in the relevant documentation sections so these selectors
are marked as user inputs for their respective networking modes, keeping the
wording consistent with the request examples and the related Cluster table.
In `@internal/client/project.go`:
- Around line 87-92: The 404 handling in GetProjectByID is still parsing
err.Error() text, which should be replaced with a typed/status-bearing error
from doRequest and checked directly in the read path. Update doRequest to return
an error that exposes the HTTP status, then change GetProjectByID to branch on
that status instead of strings.Contains(err.Error(), "HTTP 404"), preserving the
nil,nil behavior for not-found. Apply the same pattern to the other client read
methods that currently inspect error strings, including the ones in vnet,
subnet, service_account, vm, nsg, route_table, node_pool, cluster, and bastion.
In `@internal/client/vm.go`:
- Around line 91-96: The 404 handling in GetVM currently depends on matching the
text from doRequest, which is brittle across getters in internal/client. Add a
shared status-based helper in internal/client/client.go that detects a 404 from
the request error or status and returns the nil/not-found result explicitly,
then update GetVM and the other getter methods to use that helper instead of
strings.Contains on err.Error(). Keep the existing nil-on-404 contract and
preserve the existing wrapping for non-404 errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ead4a6d-bb46-4aef-b4da-9824a528de03
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
Architecture.mdGNUmakefiledocs/dc-api-reference.mdexamples/bastion/main.tfexamples/cluster/main.tfexamples/node_pool/main.tfexamples/project/main.tfexamples/provider/main.tfexamples/service_account/main.tfexamples/subnet/main.tfexamples/tenant/main.tfexamples/vm/main.tfexamples/vnet/main.tfgo.modinternal/client/bastion.gointernal/client/client.gointernal/client/cluster.gointernal/client/node_pool.gointernal/client/nsg.gointernal/client/project.gointernal/client/route_table.gointernal/client/service_account.gointernal/client/subnet.gointernal/client/tenant.gointernal/client/vm.gointernal/client/vnet.gointernal/provider/provider.gointernal/resources/bastion.gointernal/resources/cluster.gointernal/resources/node_pool.gointernal/resources/nsg.gointernal/resources/nsg_attachment.gointernal/resources/project.gointernal/resources/route_table.gointernal/resources/route_table_association.gointernal/resources/service_account.gointernal/resources/subnet.gointernal/resources/tenant.gointernal/resources/vm.gointernal/resources/vnet.gomain.gotest/main.tf
| Login endpoint: GET /v1/auth/login (browser/BFF flow only — not suitable for provider use) | ||
| ``` | ||
|
|
||
| **For Terraform**: use a service account token. It is long-lived, never expires unless the SA is deleted, and requires no OIDC flow. Create a dedicated SA with at minimum `member` role; use `owner` role only if the provider needs to manage members or service accounts. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove unsupported provider arguments from the auth example and clarify the required credential scope.
Lines 2185-2186 document tenant_id and project_id as provider arguments, but every other provider example in this PR only accepts endpoint/token; users copying this block will hit unsupported-argument errors. This section also overstates the “member service account” guidance even though Line 151 documents tenant operations as admin-only.
Proposed fix
provider "dcapi" {
endpoint = "https://dcapi.example.com"
token = var.dcapi_sa_token # dcapi_sa_<lookup_id>_<secret>
- tenant_id = "my-org"
- project_id = "my-project"
}Also applies to: 151-151, 2177-2187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dc-api-reference.md` at line 66, The auth example in
docs/dc-api-reference.md still shows unsupported provider arguments (`tenant_id`
and `project_id`) even though the provider examples only take `endpoint` and
`token`; update the example to use the same supported auth fields as the rest of
the docs. Also tighten the credential guidance in the Terraform section by
removing the “member” minimum for tenant operations and reflecting that tenant
actions are admin-only, using the relevant provider example blocks and tenant
auth text as the source of truth.
| BINARY = terraform-provider-dcapi | ||
| INSTALL_DIR = $(HOME)/.terraform.d/plugins/registry.terraform.io/wso2/dcapi/0.1.0/$(OS)_$(ARCH) | ||
|
|
||
| build: | ||
| go build -o $(BINARY) . | ||
|
|
||
| install: build | ||
| mkdir -p $(INSTALL_DIR) | ||
| cp $(BINARY) $(INSTALL_DIR)/$(BINARY) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo files of interest:\n'
git ls-files | rg '(^|/)(GNUmakefile|README|go\.mod|main\.go|provider|terraform)' || true
printf '\nGNUmakefile:\n'
cat -n GNUmakefile
printf '\nSearch for provider name/version references:\n'
rg -n "terraform-provider-dcapi|registry\.terraform\.io|filesystem mirror|mirror|terraform init|v0\.1\.0|versioned provider" .Repository: wso2/open-cloud-datacenter
Length of output: 4695
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'README install-related excerpt:\n'
sed -n '35,130p' README.md | cat -n
printf '\nArchitecture install-related excerpt:\n'
sed -n '100,130p' Architecture.md | cat -n
printf '\nProvider package metadata:\n'
sed -n '1,220p' main.go | cat -nRepository: wso2/open-cloud-datacenter
Length of output: 6055
🌐 Web query:
Terraform filesystem mirror provider binary filename versioned executable local install "terraform-provider-" "_v" registry.terraform.io filesystem_mirror
💡 Result:
To install a Terraform provider via a local filesystem mirror, you must follow a specific directory structure and binary naming convention that Terraform CLI expects [1][2][3]. Directory Structure The filesystem mirror must adhere to a rigid path layout that mirrors the registry address [2][3]. The required structure is: /////_/ Example: /opt/terraform/providers/registry.terraform.io/hashicorp/aws/5.31.0/linux_amd64/ Binary Naming Convention Inside the version/platform directory, the provider executable must follow this naming pattern [3][4]: terraform-provider-_v For example, a provider for the "aws" service at version "5.31.0" would be named: terraform-provider-aws_v5.31.0 If the provider is for Windows, the filename must also include the appropriate extension (e.g., terraform-provider-aws_v5.31.0.exe) [3]. Important Implementation Notes: 1. File Packaging: Terraform typically expects providers in a filesystem mirror to be provided as.zip archives [1][2]. However, Terraform CLI can also discover and use executable binaries placed directly in the correct directory if they follow the required naming convention [3][4]. 2. Configuration: You must configure the provider_installation block in your CLI configuration file (~/.terraformrc or terraform.rc) to point to the root [5][6]: provider_installation { filesystem_mirror { path = "/opt/terraform/providers" include = ["registry.terraform.io//"] } } 3. Automation: The most reliable way to populate this directory is using the terraform providers mirror command, which automatically downloads the necessary providers and creates the correct directory structure and naming, including the required.zip archives [1][7][2]. Manual placement is generally reserved for custom or in-house providers [3][8].
Citations:
- 1: https://developer.hashicorp.com/terraform/cli/commands/providers/mirror
- 2: https://www.mintlify.com/hashicorp/terraform/cli/providers-mirror
- 3: https://repost.aws/articles/ARV5Ey5FhbTIq2F487X3EoyQ/testing-terraform-providers-written-in-go
- 4: https://github.com/hashicorp/terraform/blob/master/internal/providercache/cached_provider.go
- 5: https://developer.hashicorp.com/terraform/cli/config/config-file
- 6: https://support.hashicorp.com/hc/en-us/articles/23562100651923-How-to-use-Terraform-CLI-with-Local-Mirror-for-Provider-Plugins-for-system-without-internet-access
- 7: https://developer.hashicorp.com/terraform/internals/provider-network-mirror-protocol
- 8: https://developer.hashicorp.com/terraform/cli/commands/providers/lock
Rename the installed provider binary to Terraform’s versioned name.
install copies terraform-provider-dcapi into the mirror path, but Terraform expects terraform-provider-dcapi_v0.1.0 there, so terraform init won’t discover the plugin. Adjust the copied filename to include the version.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@GNUmakefile` around lines 4 - 12, The install target currently copies the
provider as terraform-provider-dcapi, but Terraform discovers plugins by the
versioned binary name; update the GNUmakefile install flow so the copy step in
install uses the Terraform versioned filename for the built artifact, while
keeping BINARY and INSTALL_DIR aligned with the provider version. Use the
existing build and install targets to locate the change and ensure the installed
filename includes v0.1.0.
| golang.org/x/crypto v0.23.0 // indirect | ||
| golang.org/x/mod v0.16.0 // indirect | ||
| golang.org/x/net v0.23.0 // indirect | ||
| golang.org/x/sys v0.20.0 // indirect | ||
| golang.org/x/text v0.15.0 // indirect | ||
| golang.org/x/tools v0.13.0 // indirect | ||
| google.golang.org/appengine v1.6.8 // indirect | ||
| google.golang.org/genproto/googleapis/rpc v0.0.0-20240227224415-6ceb2ff114de // indirect | ||
| google.golang.org/grpc v1.63.2 // indirect |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod excerpt =="
nl -ba go.mod | sed -n '60,90p'
echo
echo "== dependency references =="
rg -n 'golang.org/x/crypto|google.golang.org/grpc|terraform-plugin-sdk/v2' go.mod go.sum
echo
echo "== module graph (if available) =="
if command -v go >/dev/null 2>&1; then
go list -m all 2>/dev/null | rg 'golang.org/x/crypto|google.golang.org/grpc|terraform-plugin-sdk/v2' || true
else
echo "go tool not available"
fiRepository: wso2/open-cloud-datacenter
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod excerpt =="
sed -n '60,90p' go.mod
echo
echo "== dependency references in go.mod/go.sum =="
grep -nE 'golang.org/x/crypto|google.golang.org/grpc|terraform-plugin-sdk/v2' go.mod go.sum || true
echo
echo "== available go toolchain check =="
command -v go >/dev/null 2>&1 && go version || echo "go tool not available"Repository: wso2/open-cloud-datacenter
Length of output: 2404
🌐 Web query:
golang.org/x/crypto v0.23.0 advisory OSV vulnerability google.golang.org/grpc v1.63.2 advisory OSV vulnerability
💡 Result:
Regarding the specified versions of the Go modules, both are associated with historical security advisories tracked in the OSV database. golang.org/x/crypto v0.23.0 This version is listed on pkg.go.dev as having numerous security advisories [1]. While v0.23.0 is a specific point-in-time release, it is affected by several identified vulnerabilities documented in the OSV database, such as GO-2024-3321 (related to CVE-2024-45337, an authorization bypass in SSH server configurations) [2][1] and GO-2026-5023 (an issue involving permissions skip in VerifiedPublicKeyCallback) [3]. Because security patches are ongoing, users are encouraged to consult the official OSV reports linked on the package's pkg.go.dev page to determine if their specific usage is impacted and to update to the latest available version [1]. google.golang.org/grpc v1.63.2 This version is associated with security vulnerabilities tracked in the OSV database, including GHSA-p77j-4mvh-x3m3 (also identified as CVE-2026-33186) [4][5][6]. This specific vulnerability is an authorization bypass resulting from improper input validation of the HTTP/2:path pseudo-header [7][8]. The issue affects gRPC-Go servers that use path-based authorization interceptors and have security policies with specific "deny" rules [7][6]. The vulnerability was addressed in version 1.79.3, which ensures requests missing a leading slash in the:path are rejected [7][6][8]. Users of v1.63.2 should upgrade to 1.79.3 or later to mitigate this risk [7][8].
Citations:
- 1: https://pkg.go.dev/golang.org/x/crypto@v0.23.0
- 2: https://test.osv.dev/vulnerability/CVE-2024-45337
- 3: https://osv.dev/vulnerability/GO-2026-5023
- 4: https://pkg.go.dev/google.golang.org/grpc@v1.63.2
- 5: https://go.googlesource.com/vulndb/+/747f75d97d85e97cbc1658321c4df59d452c020a/data/reports/GO-2026-4762.yaml
- 6: https://nvd.nist.gov/vuln/detail/CVE-2026-33186
- 7: https://osv.dev/vulnerability/GHSA-p77j-4mvh-x3m3
- 8: https://deps.dev/advisory/osv/GHSA-p77j-4mvh-x3m3
Refresh the dependency graph before release.
golang.org/x/crypto v0.23.0 and google.golang.org/grpc v1.63.2 are still pinned in go.mod and are associated with OSV advisories. Upgrade the affected modules (likely via terraform-plugin-sdk/v2) and run go mod tidy before shipping.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Misuse of connection.serverAuthenticate may cause authorization bypass in golang.org/x/crypto
(GO-2024-3321)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Potential denial of service in golang.org/x/crypto
(GO-2025-3487)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Potential denial of service in golang.org/x/crypto/ssh/agent
(GO-2025-4116)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Unbounded memory consumption in golang.org/x/crypto/ssh
(GO-2025-4134)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Malformed constraint may cause denial of service in golang.org/x/crypto/ssh/agent
(GO-2025-4135)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking key constraints not enforced in golang.org/x/crypto/ssh/agent
(GO-2026-5005)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking agent constraints dropped when forwarding keys in golang.org/x/crypto/ssh/agent
(GO-2026-5006)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking byte arithmetic causes underflow and panic in golang.org/x/crypto/ssh
(GO-2026-5013)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking bypass of certificate restrictions in golang.org/x/crypto/ssh
(GO-2026-5014)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking server panic during CheckHostKey/Authenticate in golang.org/x/crypto/ssh
(GO-2026-5015)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking memory leak when rejecting channels can lead to DoS in golang.org/x/crypto/ssh
(GO-2026-5016)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking client can cause server deadlock on unexpected responses in golang.org/x/crypto/ssh
(GO-2026-5017)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking pathological RSA/DSA parameters may cause DoS in golang.org/x/crypto/ssh
(GO-2026-5018)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking bypass of FIDO/U2F security keys physical interaction in golang.org/x/crypto/ssh
(GO-2026-5019)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking infinite loop on large channel writes in golang.org/x/crypto/ssh
(GO-2026-5020)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking auth bypass via unenforced @revoked status in golang.org/x/crypto/ssh/knownhosts
(GO-2026-5021)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking VerifiedPublicKeyCallback permissions skip enforcement in golang.org/x/crypto/ssh
(GO-2026-5023)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Invoking pathological inputs can lead to client panic in golang.org/x/crypto/ssh/agent
(GO-2026-5033)
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: golang.org/x/crypto/ssh/agent vulnerable to panic if message is malformed due to out of bounds read
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: golang.org/x/crypto Vulnerable to Denial of Service (DoS) via Slow or Incomplete Key Exchange
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: golang.org/x/crypto/ssh allows an attacker to cause unbounded memory consumption
[CRITICAL] 70-70: golang.org/x/crypto 0.23.0: Misuse of ServerConfig.PublicKeyCallback may cause authorization bypass in golang.org/x/crypto
[CRITICAL] 78-78: google.golang.org/grpc 1.63.2: Authorization bypass in gRPC-Go via missing leading slash in :path in google.golang.org/grpc
(GO-2026-4762)
[CRITICAL] 78-78: google.golang.org/grpc 1.63.2: gRPC-Go has an authorization bypass via missing leading slash in :path
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@go.mod` around lines 70 - 78, The dependency graph still contains vulnerable
pinned versions in go.mod, so refresh the affected modules before release.
Update the transitive dependencies pulled in through terraform-plugin-sdk/v2
that bring in golang.org/x/crypto and google.golang.org/grpc, then run go mod
tidy to regenerate the module graph and lock in the upgraded versions.
Source: Linters/SAST tools
| import ( | ||
| "bytes" | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "strings" | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Set a non-zero timeout on the shared HTTP client.
Every resource call funnels through doRequest, and this provider polls long-running operations. With http.DefaultClient, a hung TCP connection can block a single request indefinitely and stall plan/apply; give the client a bounded timeout or inject one from provider configuration.
Suggested fix
import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
"net/http"
"strings"
+ "time"
)
@@
func NewClient(baseURL, token string) (*DCAPIClient, error) {
return &DCAPIClient{
baseURL: strings.TrimRight(baseURL, "/"),
token: token,
- httpClient: http.DefaultClient,
+ httpClient: &http.Client{Timeout: 30 * time.Second},
}, nil
}Also applies to: 29-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/client/client.go` around lines 6 - 14, The shared HTTP client used
by doRequest currently relies on the default client with no timeout, which can
leave plan/apply blocked on hung connections. Update the client initialization
in client.go to use a bounded timeout, or accept a timeout from provider
configuration and thread it into the client used by doRequest. Keep the change
centered around the shared HTTP client path and the request flow that all
resource calls use.
| // NodePoolUpdateRequest maps to PATCH .../clusters/{clusterID}/node-pools/{poolName}. | ||
| // Taints and Labels use full-replace semantics — send the complete desired state. | ||
| // An empty slice/map clears the existing values; omitting (nil) leaves them unchanged. | ||
| type NodePoolUpdateRequest struct { | ||
| Count int `json:"count"` | ||
| Taints []NodePoolTaint `json:"taints"` | ||
| Labels map[string]string `json:"labels"` | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
nil cannot mean “omit” with this request shape.
Lines 29-30 say nil taints/labels should leave server state unchanged, but encoding/json will serialize these fields as null, not omit them. A count-only update can therefore clear or invalidate taints/labels instead of leaving them untouched.
Suggested fix
type NodePoolUpdateRequest struct {
- Count int `json:"count"`
- Taints []NodePoolTaint `json:"taints"`
- Labels map[string]string `json:"labels"`
+ Count int `json:"count"`
+ Taints *[]NodePoolTaint `json:"taints,omitempty"`
+ Labels *map[string]string `json:"labels,omitempty"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // NodePoolUpdateRequest maps to PATCH .../clusters/{clusterID}/node-pools/{poolName}. | |
| // Taints and Labels use full-replace semantics — send the complete desired state. | |
| // An empty slice/map clears the existing values; omitting (nil) leaves them unchanged. | |
| type NodePoolUpdateRequest struct { | |
| Count int `json:"count"` | |
| Taints []NodePoolTaint `json:"taints"` | |
| Labels map[string]string `json:"labels"` | |
| } | |
| // NodePoolUpdateRequest maps to PATCH .../clusters/{clusterID}/node-pools/{poolName}. | |
| // Taints and Labels use full-replace semantics — send the complete desired state. | |
| // An empty slice/map clears the existing values; omitting (nil) leaves them unchanged. | |
| type NodePoolUpdateRequest struct { | |
| Count int `json:"count"` | |
| Taints *[]NodePoolTaint `json:"taints,omitempty"` | |
| Labels *map[string]string `json:"labels,omitempty"` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/client/node_pool.go` around lines 28 - 35, The NodePoolUpdateRequest
shape in node_pool.go cannot use nil slices/maps to mean “omit” because
encoding/json will emit null for Taints and Labels, which can unintentionally
clear server state on count-only updates. Update the request model and its PATCH
serialization path so omitted fields are truly excluded from the JSON payload,
or add custom marshaling/pointer fields in NodePoolUpdateRequest to preserve the
intended omit-vs-clear behavior for Taints and Labels.
| "next_hop_type": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| Description: "\"vnet_local\"|\"internet\"|\"virtual_appliance\"|\"none\".", | ||
| ValidateFunc: validation.StringInSlice([]string{ | ||
| "vnet_local", "internet", "virtual_appliance", "none", | ||
| }, false), | ||
| }, | ||
| "next_hop_ip": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Description: "Next-hop IP. Required when next_hop_type is \"virtual_appliance\".", | ||
| ValidateFunc: validation.IsIPAddress, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce the virtual_appliance/next_hop_ip contract before apply.
The schema says next_hop_ip is required for virtual_appliance, but nothing validates that combination and Lines 245-247 only copy the field when it happens to be present. That lets a known-invalid route survive planning and fail later at apply time. Please reject missing next_hop_ip for virtual_appliance routes, and ideally reject it for the other hop types too.
Also applies to: 232-247
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/resources/route_table.go` around lines 79 - 92, The route table
schema allows an invalid `virtual_appliance` route to proceed without
`next_hop_ip`, and `routeTableResource` only copies `next_hop_ip`
opportunistically during apply. Add validation in the
`routeTableResource`/schema path so `next_hop_ip` is required when
`next_hop_type` is `virtual_appliance`, and reject it for the
non-`virtual_appliance` hop types as well. Keep the check close to the existing
`next_hop_type` and `next_hop_ip` fields so the bad combination is caught before
apply.
| func ResourceServiceAccount() *schema.Resource { | ||
| return &schema.Resource{ | ||
| // No UpdateContext — every field is ForceNew; changes require destroy + recreate. | ||
| CreateContext: resourceServiceAccountCreate, | ||
| ReadContext: resourceServiceAccountRead, | ||
| DeleteContext: resourceServiceAccountDelete, | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== service_account ==\n'
ast-grep outline internal/resources/service_account.go --view expanded || true
printf '\n== node_pool ==\n'
ast-grep outline internal/resources/node_pool.go --view expanded || true
printf '\n== nsg ==\n'
ast-grep outline internal/resources/nsg.go --view expanded || true
printf '\n== nsg_attachment ==\n'
ast-grep outline internal/resources/nsg_attachment.go --view expanded || true
printf '\n== route_table ==\n'
ast-grep outline internal/resources/route_table.go --view expanded || true
printf '\n== route_table_association ==\n'
ast-grep outline internal/resources/route_table_association.go --view expanded || true
printf '\n== importer search ==\n'
rg -n "Importer|ImportStatePassthroughContext|ImportState" internal/resources/{service_account.go,node_pool.go,nsg.go,nsg_attachment.go,route_table.go,route_table_association.go} || true
printf '\n== force-new fields and read IDs ==\n'
rg -n "ForceNew: true|Set\\(|Parse|Split|tenant_id|project_id|cluster_id|nsg_id|route_table_id|association_id" internal/resources/{service_account.go,node_pool.go,nsg.go,nsg_attachment.go,route_table.go,route_table_association.go} || trueRepository: wso2/open-cloud-datacenter
Length of output: 17095
Import support is missing for these resources
internal/resources/service_account.go,node_pool.go,nsg.go,nsg_attachment.go,route_table.go, androute_table_association.godo not set anImporter, soterraform importwill fail.- Their
Readhandlers also do not restore every ForceNew parent field from the composite ID; for example,service_accountrepopulatestenant_idbut notproject_id, so imported state would still be incomplete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/resources/service_account.go` around lines 19 - 25, Add import
support to the Resource schemas by setting an Importer on ResourceServiceAccount
and the other affected resource constructors (node_pool, nsg, nsg_attachment,
route_table, route_table_association). Also update each Read handler, especially
resourceServiceAccountRead, to parse the composite import ID and repopulate
every ForceNew parent field from it (for example both tenant_id and project_id)
so imported state is complete.
| "cpu_cores_cap": { | ||
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| Default: 0, | ||
| Description: "CPU core quota ceiling. 0 = platform default (80). Updatable.", | ||
| }, | ||
| "memory_gb_cap": { | ||
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| Default: 0, | ||
| Description: "Memory quota ceiling in GB. 0 = platform default (256). Updatable.", | ||
| }, | ||
| "storage_gb_cap": { | ||
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| Default: 0, | ||
| Description: "Storage quota ceiling in GB. 0 = platform default (2000). Updatable.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby logic.
git ls-files internal/resources/tenant.go
wc -l internal/resources/tenant.go
cat -n internal/resources/tenant.go | sed -n '1,280p'
# Find related schema/state handling for these fields elsewhere.
rg -n '"cpu_cores_cap"|"memory_gb_cap"|"storage_gb_cap"|cpu_cores_cap|memory_gb_cap|storage_gb_cap' internal -S
# Inspect any helpers that might normalize defaults or suppress diffs.
rg -n 'Default: *0|StateFunc|DiffSuppress|CustomizeDiff|SetNew|ForceNew|Computed|Optional: *true' internal -SRepository: wso2/open-cloud-datacenter
Length of output: 26981
Avoid perpetual diffs on defaulted quota caps. Default: 0 conflicts with writing the API-resolved 80/256/2000 values back into state, so omitted/zeroed configs will keep planning an update. Use Optional: true + Computed: true (or normalize/suppress the diff) for these fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/resources/tenant.go` around lines 51 - 67, The quota cap schema
fields in tenant resource are causing perpetual diffs because the Terraform
defaults of 0 conflict with the API-resolved values written back into state.
Update the resource schema entries for cpu_cores_cap, memory_gb_cap, and
storage_gb_cap in the tenant resource to use Optional plus Computed (or add
equivalent diff suppression/normalization) so omitted or zero-valued configs do
not constantly plan changes against the resolved 80/256/2000 values.
| // resourceTenantDelete removes the resource from Terraform state only. | ||
| // The DC-API has no delete endpoint for tenants — the real tenant continues to exist. | ||
| func resourceTenantDelete(_ context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics { | ||
| d.SetId("") | ||
| return nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Delete should not succeed without deleting the remote tenant.
This makes terraform destroy drop the tenant from state while the real tenant keeps existing. That leaves an orphaned remote object, and a later apply of the same tenant_id is likely to fail on create. Until the API supports deletion, returning a diagnostic is safer than reporting success.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/resources/tenant.go` around lines 229 - 233, The tenant delete path
currently reports success while only removing the resource from Terraform state,
which leaves the remote tenant orphaned. Update resourceTenantDelete to stop
pretending deletion succeeded: instead of clearing the ID and returning nil,
return a diagnostic indicating tenant deletion is unsupported until the DC-API
provides a real delete endpoint. Keep the change localized to
resourceTenantDelete so terraform destroy surfaces a failure rather than
silently succeeding.
| resource "dcapi_tenant" "test" { | ||
| id = "example-tenant" | ||
| name = "Example Tenant" | ||
| description = "Testing the Terraform provider" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use tenant_id, not id, in the tenant resource.
Line 13 uses an argument name that does not match the rest of this PR's tenant examples/docs. As written, this config will fail with an unsupported argument instead of creating the tenant.
Proposed fix
resource "dcapi_tenant" "test" {
- id = "example-tenant"
+ tenant_id = "example-tenant"
name = "Example Tenant"
description = "Testing the Terraform provider"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resource "dcapi_tenant" "test" { | |
| id = "example-tenant" | |
| name = "Example Tenant" | |
| description = "Testing the Terraform provider" | |
| } | |
| resource "dcapi_tenant" "test" { | |
| tenant_id = "example-tenant" | |
| name = "Example Tenant" | |
| description = "Testing the Terraform provider" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/main.tf` around lines 12 - 16, The dcapi_tenant resource example is
using the wrong attribute name, which causes the config to fail validation.
Update the tenant resource block in test/main.tf to use tenant_id instead of id,
matching the tenant examples and docs used elsewhere in this PR. Keep the rest
of the resource fields unchanged.
2 participants
What this changes
There was no Terraform provider for DC-API — provisioning was imperative, via
dcctland direct REST calls, with no plan step, no dependency ordering, and no drift detection. This adds a provider (registry.terraform.io/wso2/dcapi) that manages DC-API resources declaratively through Terraform's plan/apply lifecycle. Design and scope were agreed in #200; this is the first contribution, shipping the core resource set as0.1.0.Behaviour
d.SetId()is set only after create is confirmed, so a failed poll produces a clean failure rather than a tainted resource in state.404, treating404as success (already gone). A409on VNet delete while child subnets still exist is surfaced as an actionable error rather than a silent retry.DCAPI_TOKEN→dcctlcredentials file; endpoint fromDCAPI_ENDPOINT. The provider never becomes a second store of long-lived secrets.tenant_id,project_id) are persisted into state so attribute references resolve and Terraform Core can infer create/destroy ordering. Sensitive outputs (private_key,console_password) are markedsensitive.dcctl.Surface
registry.terraform.io/wso2/dcapi; resources nameddcapi_<resource>(e.g.dcapi_vnet,dcapi_virtual_machine).required_providersblock, provider configuration with optional explicitendpoint/token(both default to env /dcctl).Out of scope
The five deferred resources — PrivateEndpoint, KeyVault, PrivateDnsZone, DnsRecord, TenantMember — deliberately aren't implemented here; they follow in subsequent PRs once the core hierarchy is merged. The SDKv2 → Plugin Framework migration (planned via
terraform-plugin-mux) is also a separate, later change.Verification
Race-enabled unit tests using
httptestto fake the DC-API server (DTO decode, CRUD handler logic, state surfacing). Acceptance tests covering create/read/update/delete/import were run against a local DC-API server (make run). The full acceptance suite against a shared/staging DC-API instance wasn't run here.Summary by CodeRabbit
New Features
Bug Fixes