Skip to content
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

[ISSUE] User name skipped when importing a user #471

Closed
with-joy opened this issue Jan 27, 2021 · 5 comments · Fixed by #472
Closed

[ISSUE] User name skipped when importing a user #471

with-joy opened this issue Jan 27, 2021 · 5 comments · Fixed by #472
Assignees

Comments

@with-joy
Copy link

Seems like while importing users, it skips over fields like user name. I think it's a similar issue mentioned in #401

Kind of important since the user name is skipped, terraform tries to recreate the resource even after importing.

Terraform Version

Terraform v0.13.6

Affected Resource(s)

  • databricks_user

Terraform Configuration Files

resource "databricks_user" "xxx_yyyy" {
  user_name = "xxx.yyyy@ddd.com"
  active    = true
}

Debug Output

2021-01-27T06:53:57.333+0100 [INFO]  plugin.terraform-provider-databricks_v0.2.9: [INFO] Using DEFAULT profile from ~/.databrickscfg
2021-01-27T06:53:57.333+0100 [INFO]  plugin.terraform-provider-databricks_v0.2.9: [INFO] Using Bearer authentication from ~/.databrickscfg
2021-01-27T06:53:57.333+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9: [DEBUG] GET /preview/scim/v2/Users/2222222 
2021-01-27T06:53:58.057+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9: [DEBUG] 200 OK {
2021-01-27T06:53:58.057+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:   "active": true,
2021-01-27T06:53:58.057+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:   "emails": [
2021-01-27T06:53:58.057+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:     {
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:       "primary": true,
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:       "type": "work",
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:       "value": "xxx.yyyy@ddd.com"
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:     }
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:   ],
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:   "groups": [
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:     {
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:       "$ref": "Groups/11111",
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:       "display": "AAA BBBB",
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:       "type": "direct",
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:       "value": "1111111"
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:     }
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:   ],
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:   "id": "2222222",
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:   "roles": [
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:     {
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:       "value": "arn:aws:iam::23213213213:instance-profile/databricks"
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:     }
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:   ],
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:   "schemas": [
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:     "urn:ietf:params:scim:schemas:core:2.0:User",
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:     "urn:ietf:params:scim:schemas:extension:workspace:2.0:User"
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:   ],
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9:   "userName": "xxx.yyyy@ddd.com"
2021-01-27T06:53:58.058+0100 [DEBUG] plugin.terraform-provider-databricks_v0.2.9: } <- GET /preview/scim/v2/Users/100043
2021-01-27T06:53:58.058+0100 [TRACE] plugin.terraform-provider-databricks_v0.2.9: [TRACE] Removing default fields sent back by server: user_name - "xxx.yyyy@ddd.com"
2021-01-27T06:53:58.058+0100 [TRACE] plugin.terraform-provider-databricks_v0.2.9: [TRACE] skipping empty display_name &reflect.Value{typ:(*reflect.rtype)(0x1bce440), ptr:(unsafe.Pointer)(0xc00036af40), flag:0x98}
2021-01-27T06:53:58.058+0100 [TRACE] plugin.terraform-provider-databricks_v0.2.9: [TRACE] Removing default fields sent back by server: active - true
2021-01-27T06:53:58.058+0100 [TRACE] plugin.terraform-provider-databricks_v0.2.9: [TRACE] Removing default fields sent back by server: allow_cluster_create - false
2021-01-27T06:53:58.058+0100 [TRACE] plugin.terraform-provider-databricks_v0.2.9: [TRACE] Removing default fields sent back by server: allow_instance_pool_create - false

Expected Behavior

Should import user fields

Actual Behavior

Seems like its skipping fields like user_name. as seen in the logs above. Eg. state file:

{
  "version": 4,
  "terraform_version": "0.13.6",
  "serial": 1,
  "lineage": "910af8bf-48f0-968a-f438-edf7635d651f",
  "outputs": {},
  "resources": [
    {
      "mode": "managed",
      "type": "databricks_user",
      "name": "xxx_yyy",
      "provider": "provider[\"registry.terraform.io/databrickslabs/databricks\"]",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            "active": null,
            "allow_cluster_create": null,
            "allow_instance_pool_create": null,
            "display_name": null,
            "id": "2222222",
            "user_name": null
          },
          "private": "eyJzY2aaaaaaaVyc2lvbiI6IjAifQ=="
        }
      ]
    }
  ]
}

Steps to Reproduce

Please list the steps required to reproduce the issue, for example:

  1. terraform import databricks_user.xxx_yyy "2222222"
@alexott
Copy link
Contributor

alexott commented Jan 27, 2021

I was able to reproduce this issue, trying to find the reason for it...

@stikkireddy
Copy link
Contributor

For anyone looking into this @alexott the reason is in StructToData method in internal/reflect_resource.go

https://github.com/databrickslabs/terraform-provider-databricks/blob/ac1d3d132bbf58259514b70932f9616b5506c7ff/internal/reflect_resource.go#L321-L325

The lines there cause the user_name during import to be disgarded. Quick way to reproduce via building binary is to add this log statement or switch logging to print out the traces:

log.Printf("Removing value when setting: Field Path: %s \t - %s", fieldPath, fieldValue)

Unfortunately i dont know a quick fix for this yet.

@stikkireddy
Copy link
Contributor

hmm one way we can try to approach this is to change common_resource.go rather than touching changing logic in reflect_resource.

we can change:

		Importer: &schema.ResourceImporter{
		        StateContext: schema.ImportStatePassthroughContext,
		},

to

		Importer: &schema.ResourceImporter{
			StateContext: func(ctx context.Context, d *schema.ResourceData, m interface{}) (data []*schema.ResourceData, e error) {
				d.Id()
				newCtx := context.WithValue(ctx, "importing", true)
				err := r.Read(newCtx, d, m.(*common.DatabricksClient))
				return []*schema.ResourceData{d}, err
			},
		},

This will allow the ctx.WithValue("importing") to yield a true. This can be used in the logic statement that during an import do not reject server value.

The con of this is that we would need to touch reflect_resource (allow for ctx to pass through), common_resource(new importing) and this also causes two get requests to be called for every import where as importpassthrough does not.

Alternatively the least amount of code change option is to make username in UserEntity to be computed and not required. This does not coinside with how the api is build but it should work. (I have done very limited testing)

UserName                string `json:"user_name,omitempty" tf:"computed"`

@stikkireddy
Copy link
Contributor

This issue is not just for user_name but for a lot of the fields that are not computed are imported. We should add these to the acceptance test cases to the resources that support imports:

                        {
				ResourceName:      "<resource_type>.<resource_id>",
				ImportState:       true,
				ImportStateVerify: true,
			}

Resolving the issue should pass the import test cases.
https://www.terraform.io/docs/extend/resources/import.html#resource-acceptance-testing-implementation

Examples:
https://github.com/hashicorp/terraform-provider-google/blob/master/google/resource_compute_attached_disk_test.go

This only helps identify issues with resources but in terms of a wholistic fix for all resources any ideas @nfx.

alexott added a commit that referenced this issue Jan 27, 2021
alexott added a commit that referenced this issue Jan 27, 2021
@alexott alexott self-assigned this Jan 27, 2021
@nfx
Copy link
Contributor

nfx commented Jan 28, 2021

@stikkireddy the approach here is the correct one, but we just need to verify it working.

@nfx nfx closed this as completed in #472 Jan 29, 2021
nfx pushed a commit that referenced this issue Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants