dell / dell-terraform-providers

Terraform providers will provide easy and seamless integration with Dell Infrastructure.
Mozilla Public License 2.0
3 stars 1 forks source link

[BUG] powerscale provider v1.2.0 resource Filesystem fails to create when UID/GID is not resolvable on cluster #16

Closed petew-nfx closed 4 months ago

petew-nfx commented 7 months ago

We have workflows where filesystem access is resolved on the client, not the cluster. This means the cluster cannot resolve UID/GIDs, but still needs to be able to create directories with UID/GID alone.

Attempting to create a directory with numeric UID/GID as owner/group fails due to an attempt to resolve in the zone.

resource "powerscale_filesystem" "ifs_zone1_uid_test" {
    access_control = "0770"
    directory_path = "ifs/zone1"
    full_path      = "/ifs/zone1/uid_test"
    group          = {
        id = "GID:99"
    }
    id             = "ifs/zone1/uid_test"
    name           = "uid_test"
    overwrite      = false
    owner          = {
        id = "UID:99"
    }
    recursive      = true
}

Running terraform apply it throws the error:

Could not create file systems unable to validate user information with error: Failed to find user for 'UID:99': No such user  404 Not Found

Most likely from the here: https://github.com/dell/terraform-provider-powerscale/blob/f288f4b809b1d52f60bfdbcf28873d154a8e21e2/powerscale/provider/file_system_resource.go#L257

forrestxia commented 7 months ago

@taohe1012 can you please help check on this issue? thanks!

taohe1012 commented 7 months ago

@taohe1012 can you please help check on this issue? thanks!

yes, will check this issue

taohe1012 commented 7 months ago

hi @petew-nfx , bugfix for this issue: if the owner/group is invalid, the filesystem will be created, but we will throw Error to report the owner/group invalid and set acl failed.

petew-nfx commented 7 months ago

Hi @taohe1012 , I think errors are inappropriate here. When setting owner/group with only numeric UID/GID it should be understood that the intention is not to resolve them. Reporting errors under these circumstances would be confusing.

taohe1012 commented 7 months ago

thanks @petew-nfx , will do some adjustment.

taohe1012 commented 7 months ago

hi @petew-nfx , will change owner and group from required to optional, the user can choose to input owner/group or not. And if the user choose to input values and owner/group are invalid, we will still throw errors.

petew-nfx commented 7 months ago

When using the REST API when owner/group are passed with id (numeric) no attempt is made to resolve this, by design. If resolution is required the name is populated (and id is not given) which causes resolution to occur and failure to resolve is reported. The provider should reflect how the API works, which honors the administrators intentions.

taohe1012 commented 7 months ago

@petew-nfx ,however, this has some conflict with terraform. If you input invalid user: UID: 100000000(not exist), Name: UserA, but the PowerScale will auto-populate UID: 0, Name: root as its acl owner. Then the terraform will received the acl user is UID: 0, Name: root, which is conflict with UID: 100000000, Name: UserA in tf config, and terraform will report state and plan value mismatch Error. We need to skip this conflict, and need to report error if the user is invalid.

petew-nfx commented 7 months ago

The scenario you describe does not seem to be the case when using the REST API.

When ACLs are updated via REST using both id and name for group and/or user, the id is used without validation and the name ignored. To be clear: the cluster does not try to verify the UID/GID exists in the zone and the name is ignored.

Example: Using the following body to update ACLs.

{
  "acl": [
  ],
  "authoritative": "acl",
  "action": "replace",
  "group": {
    "id": "GID:12345",
    "name": "FAKE_GROUP",
    "type": "group"
  },
  "mode": "0770",
  "owner": {
    "id": "UID:67890",
    "name": "FAKE_USER",
    "type": "user"
  }
}

results in a successful call with the the directory with the following owner/group:

d--------- +   2 67890  12345  0 Apr 23 20:39 rest_made_directory
 OWNER: user:67890
 GROUP: group:12345

However, if the id fields are removed the cluster attempts to resolve the name and the call fails.

{
  "acl": [
  ],
  "authoritative": "acl",
  "action": "replace",
  "group": {
    "name": "FAKE_GROUP",
    "type": "group"
  },
  "mode": "0770",
  "owner": {
    "name": "FAKE_USER",
    "type": "user"
  }
}

failure:

 {
  "errors": [
    {
      "code": "AEC_BAD_REQUEST",
      "message": "invalid owner or group"
    }
  ]
}

This shows when given an id the REST API will ignore the name and use it without validation in the zone. However, when only name is given zone validation will occur.

The terraform provider should act in a manner consistent with the API to allow setting of owner/group by id and not report spurious errors.

taohe1012 commented 7 months ago

@petew-nfx thanks for this case, but from GUI, when first creation, you can input invalid owner or group; after creation, its owner default to be root, and group default to group. image image

and I still find one mismatched case by API---> you can try uid:100000 or uid:1000000, below are my get user result, set acl result and get acl results--> the owner is is not expected image image image

taohe1012 commented 7 months ago

@petew-nfx Due to various circumstances, so choose to adopt that solution after consideration. If we still decide to follow behaviors of the API, we can also do it. Thanks.

petew-nfx commented 7 months ago

I appreciate you taking the time to consider the problem from an operational perspective.

In regards to the user problem you were seeing: It looks like the UID is in the cluster range for generated UIDs for user mapping which defaults to 1000000-2000000. If your cluster has auto generated those UIDs, that would explain why you get a SID.

taohe1012 commented 7 months ago

@petew-nfx thanks for the knowledge, and still last one more issue: in your example, you input

"owner": {
    "id": "UID:67890",
    "name": "FAKE_USER",
    "type": "user"
  }

but when getting acl, the API returns:

  "owner": {
    "id": "UID:6789"
  }

this is also one mismatched case in terraform, you will see image

And we may have one solution to skip above issue: we can saved the value to terraform state to skip that issue:

"owner": {
    "id": "UID:67890",
    "name": "FAKE_USER",
    "type": "user"
  }

but this is also will be one confusion point to customer, and the customer won't know this is valid or invalid uid directly, and it's not consistent with api return result.

petew-nfx commented 7 months ago

My example input was used to test how the REST API handled various combinations of id and name. In my experience id and name would not be supplied together. id is used when there is no name to resolve to, and name is used when resolution is expected.

Providing the id for a resolvable name allows the name to change, but keep the id. Whereas providing name allows the id to change for the given name.

taohe1012 commented 7 months ago

@petew-nfx as we sometimes encounter users with the same name on different providers, such as we can have one admin in Sysem:local and one admin user in System:file provider, and we must distinguish them through uid '. Adding both the name and ID should be a rigorous usage approach for some customer.

petew-nfx commented 7 months ago

I agree that if name and id are given a vigorous check should be undertaken, however that is not how the API works today.

As I mentioned, the cases I am concerned about are those in which only id or name are given. I would expect the provider to align with the REST API.

taohe1012 commented 7 months ago

@petew-nfx sure, the cases with only id or name should align with the REST API, and also we still need consider both name and id. I will do the fix to align with the REST API when only name or id firstly, and then will try to be compliance both name and id. Thanks.

petew-nfx commented 7 months ago

Thanks for digging into this and all the nuances around it. I appreciate it.

taohe1012 commented 4 months ago

Close this since PR 155 with fix has been merged and released in v1.4.0.