crossplane-contrib / provider-jet-azure

Apache License 2.0
17 stars 20 forks source link

Cannot expose terraform attribute if it has the same name as a spec argument #176

Closed ytsarev closed 2 years ago

ytsarev commented 2 years ago

What happened?

postgresql_server has both identity argument https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/postgresql_server#identity which goes to the spec with value type: SystemAssigned and an identity attribute which should be accessible after resource creation

An identity block exports the following:

[principal_id](https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/postgresql_server#principal_id) - The Principal ID associated with this Managed Service Identity.

[tenant_id](https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/postgresql_server#tenant_id) - The Tenant ID associated with this Managed Service Identity.

I've made an attempt to propagate identity to status following https://github.com/crossplane/terrajet/blob/main/docs/configuring-a-resource.md#overriding-terraform-resource-schema but it appears to be mutually exclusive with the exposing identity in spec, currently, it is one or another

The ability to expose identity in status is crucial for creating Data Encryption enabled PostgreSQL Server utilizing the recently added https://github.com/crossplane-contrib/provider-jet-azure/pull/174

Without the exposure, it is not possible to create associated AccessPolicy in an automated way as it requires Managed Identity object id in its spec.

This issue most probably can be fixed in core Terrajet project, but reporting it here as it is more contextual and closer to the actual technical blocker.

How can we reproduce it?

Try to configure the resource postgresql_server resource with

+               if s, ok := r.TerraformResource.Schema["identity"]; ok {
+                       s.Optional = false
+                       s.Computed = true
+               }

Observe that identity part of the spec disappears

What environment did it happen in?

Crossplane version: universal-crossplane-1.6.3-up.1

muvaf commented 2 years ago

@ytsarev Do you think this is happening because of https://github.com/crossplane/terrajet/issues/27 ? cc @sergenyalcin who has a PR to fix it https://github.com/crossplane/terrajet/pull/230

ytsarev commented 2 years ago

thanks @muvaf , it looks very close. What is special in this issue is that identity spec(tf parameters) and observation(tf attributes) have different fields in the relevant blocks, not sure if this is important from the fix standpoint. Will be happy to test it when it's ready

jbw976 commented 2 years ago

@sergenyalcin also assigning you to this issue from @muvaf comment in https://github.com/crossplane-contrib/provider-jet-azure/issues/176#issuecomment-1102783634

ytsarev commented 2 years ago

@sergenyalcin I am in the middle of testing provider-jet-azure with the terrajet changes from https://github.com/crossplane/terrajet/pull/230

I did

# in terrajet dir
gh pr checkout 230
# in provider-jet-azure dir
go mod edit -replace github.com/crossplane/terrajet=/Users/xnull/upstream/terrajet
make generate

It looks seriously promising, the git diff on CRD under interest contains exactly the fields I need!

diff --git a/package/crds/dbforpostgresql.azure.jet.crossplane.io_servers.yaml b/package/crds/dbforpostgresql.azure.jet.crossplane.io_servers.yaml
index 351b39ea..739815ad 100644
--- a/package/crds/dbforpostgresql.azure.jet.crossplane.io_servers.yaml
+++ b/package/crds/dbforpostgresql.azure.jet.crossplane.io_servers.yaml
@@ -313,6 +313,13 @@ spec:
                     type: string
                   id:
                     type: string
+                  identity:
+                    properties:
+                      principalId:
+                        type: string
+                      tenantId:
+                        type: string
+                    type: object
                 type: object
               conditions:
                 description: Conditions of the resource.

I've executed make run to test the provider. It failed with

 make run
From https://github.com/hashicorp/terraform-provider-azurerm
 * tag                   v2.78.0    -> FETCH_HEAD
HEAD is now at d841e6917 v2.78.0
17:20:15 [ .. ] go build darwin_arm64
go build: -i flag is deprecated
github.com/crossplane-contrib/provider-jet-azure/cmd/provider
# github.com/crossplane-contrib/provider-jet-azure/cmd/provider
cmd/provider/main.go:98:90: cannot use "registry.terraform.io/" + *providerSource (type string) as type "github.com/crossplane/terrajet/pkg/terraform".SharedGRPCRunnerOption in argument to "github.com/crossplane/terrajet/pkg/terraform".NewSharedProvider
17:20:16 [FAIL]
make: *** [go.build] Error 1

I've temporarily modified the .NewSharedProvider invocation to unblock myself

diff --git a/cmd/provider/main.go b/cmd/provider/main.go
index 673b8758..3ac46ef9 100644
--- a/cmd/provider/main.go
+++ b/cmd/provider/main.go
@@ -95,7 +95,8 @@ func main() {
        // This removes some complexity for setting up development environments.
        var runner terraform.ProviderRunner = terraform.NewNoOpProviderRunner()
        if len(*nativeProviderPath) != 0 {
-               runner = terraform.NewSharedProvider(log, *nativeProviderPath, "registry.terraform.io/"+*providerSource)
+               //      runner = terraform.NewSharedProvider(log, *nativeProviderPath, "registry.terraform.io/"+*providerSource)
+               runner = terraform.NewSharedProvider(log, *nativeProviderPath)
        }

        o := tjcontroller.Options{

It worked and the make run became operational.

The provider under test properly reconciles MRs except for the resources affected by the change.

  Warning  CannotObserveExternalResource    63s (x11 over 8m27s)   managed/sql.azure.jet.crossplane.io/v1alpha2, kind=mssqlserver  cannot run refresh: refresh failed: missing expected [: : File name: main.tf.json

I've reproduced the failure for mssqlserver.sql.azure.jet.crossplane.io and server.dbforpostgresql.azure.jet.crossplane.io which are part of the investigation in this specific issue.

I've also retrieved the terraform workspace of the failing azurerm_postgresql_server and reproduced it locally:

terraform apply
╷
│ Error: missing expected [
│
│   with azurerm_postgresql_server.postgresql-server-data-encryption-example,
│   on main.tf.json line 1, in resource.azurerm_postgresql_server.postgresql-server-data-encryption-example:
│    1: {"provider":{"azurerm":{"client_id":"omitted","client_secret":"omitted","features":{},"skip_provider_registration":true,"subscription_id":"omitted","tenant_id":"omitted"}},"resource":{"azurerm_postgresql_server":{"postgresql-server-data-encryption-example":{"administrator_login":"mssqladminun","administrator_login_password":"omitted","identity":[{"type":"SystemAssigned"}],"lifecycle":{"prevent_destroy":true},"location":"East US","name":"postgresql-server-data-encryption-example","resource_group_name":"postgresql-server-data-encryption-example","sku_name":"GP_Gen5_2","ssl_enforcement_enabled":true,"tags":{"provisioner":"crossplane"},"version":"11"}}},"terraform":{"required_providers":{"azurerm":{"source":"hashicorp/azurerm","version":"2.78.0"}}}}
│
╵
ulucinar commented 2 years ago

Hi @ytsarev, @sergenyalcin, Regarding the make run error, I did not check but most probably we need to rebase Sergen's terrajet branch to get rid of it. @sergenyalcin, is it possible to rebase your branch?

sergenyalcin commented 2 years ago

Hi @ulucinar, @ytsarev I rebased my branch.

sergenyalcin commented 2 years ago

Hİ @ytsarev I investigated the problem that you observed. Thank you for this observation because it is an important finding for this PR.

We are generating the Schema and Resource (terraform sdk schema types) as slices in root Parameter/Observation structs. As an example, assume that we have an Example resource. This has ExampleObservation field (this is root type I mean) and also we have a nested type in this root type ConfigObservation. We are putting the ConfigObservation to the root type as following:

ExampleObservation {
    Config []ConfigObservation `json:"config,omitempty" tf:"config,omitempty"`
}

In my branch, I missed this point and did not generate these fields as slices. Thanks to these findings you observed during the test you did, I fixed it. I tested last state of my PR for postgresqlserver resource by using the example manifest in repo: https://github.com/crossplane-contrib/provider-jet-azure/blob/main/examples/postgresql/postgresqlserver.yaml This test was successfully completed. Could you please test it your scenario again? Thank you @ytsarev

ytsarev commented 2 years ago

@sergenyalcin that's amazing, thanks a lot for the fix! I will give it a shot soon!

ytsarev commented 2 years ago

@sergenyalcin it works as expected, thanks a ton!

k get server.dbforpostgresql.azure.jet.crossplane.io/postgresql-server-data-encryption-example -o yaml
...
status:
  atProvider:
    fqdn: postgresql-server-data-encryption-example.postgres.database.azure.com
    id: /subscriptions/<redadcted>/resourceGroups/postgresql-server-data-encryption-example/providers/Microsoft.DBforPostgreSQL/servers/postgresql-server-data-encryption-example
    identity:
    - principalId: c5e9f5c4-3d56-4ac3-a4c5-f0fafcf1da6e
      tenantId: <redacted>
...
ytsarev commented 2 years ago

@sergenyalcin @ulucinar @turkenh guys, to get this one fixed I would need to incorporate https://github.com/crossplane/terrajet/pull/230 terrajet fix.

Should I wait for release ( requested https://github.com/crossplane/terrajet/pull/230#issuecomment-1144637433 ) or should I just bump go.mod pointing to the terrajet commit incorporating the fix? What would be a maintainers' recommendation here?

turkenh commented 2 years ago

@ytsarev you can just bump go.mod. It is not rare that we use untagged versions of library projects in Crossplane codebase, e.g. see tools and runtime dependencies here: https://github.com/crossplane-contrib/provider-aws/blob/master/go.mod#L28-L29

ytsarev commented 2 years ago

@turkenh cool, i will just bump then 👍 thank you!