onelogin / terraform-provider-onelogin

GNU General Public License v3.0
27 stars 19 forks source link

user_mapping 'position' var type #60

Open rjhornsby opened 3 years ago

rjhornsby commented 3 years ago

The resource onelogin_user_mappings requires the argument position with this explanation:

position - (Required) Indicates the ordering of the mapping. When null this will be placed last.

Internally, position is declared as a type schema.TypeInt. I don't know GO well enough yet to know for sure, but it seems like providing null for the value of position is a problem. When writing the resource argument this way position = null, Terraform (0.15.5) reports an error

Error: "position": required field is not set

Setting position to empty string or the string literal null is an error - not an int. The argument is required, so it can't be left out. Setting the position to an arbitrary value greater than the number of roles (ie 100) fails as well:

Error: error: context: [ol http service], error_message: [{"code":422,"message":"Validation Failed","errors":[{"field":"position","message":["Invalid position value: 100. Must be an integer between 1 and 57"]}]}]

dcaponi commented 3 years ago

@rjhornsby our API doesnt handle omitting the field and the workaround has been to explicitly send some number and bring to your attention that a number should be set.

When I use the API in cURL I am able to send a position: 0 and that gets handled as a valid request and sets that user_mapping to the last position (in your case, based on your error, it would set the position to actually being 57, if you set position = 0)

I agree this isn't the greatest experience, and since in GO it always sets 0 for the "zero value" on an int (there's no primitive type null or nil otherwise this and our API would get along nicely) I could make this field optional and always send number or "zero value" (0) back.

Could you do me a solid though before I do that, and try setting the value to 0 and let me know if it actually sets your user_mapping to be at the last position? If that works, this should hopefully be a pretty quick tweak.

rjhornsby commented 3 years ago

Thanks for that tip. position = 0 works properly, and adds things to the end of the list.

One of the additional things required to make this work is to set the ignore_changes for that parameter.

  lifecycle {
    ignore_changes = [position]
  }

Otherwise, the position is read by TF from the API as whatever absolute position it actually is in the list, and Terraform wants to change it to 0.

Can't actually test this atm but I believe it to be correct - the second part to watch out for is if you set two things to be "last", TF will make them compete on each run. That is, in a list of 10 things, mapping "A" is at position 9, because mapping "B" is pos 10. TF will see that "A" isn't last, and move it to the end. When it works "B", it will see that it isn't last, and will move it to the end, effectively moving "A" back to pos 9.

Items added to the list outside of Terraform will also cause Terraform to try to move A and B to the bottom.

This might just be a documentation thing, rather than a technical one. ie "If you set the position to 0 (meaning last)" but simply meant for it to be appended to the list rather than enforce its location to always be last, be sure to add position to ignore_changes"

dcaponi commented 3 years ago

Yeah this one is kinda weird IMO and probably something that needs to be addressed in the API. I went back & forth on the requirement of this field, and figured requiring it would be most prudent and encourage users to explicitly define their user_mapping positions since application of those mappings is order dependent and position is really important.

If you're appending everything, I believe you're right in that TF will make it compete every run, and therefore the order in which you apply your user_mappings is non-deterministic which may be detrimental to your experience.

I'm tempted to document it and say "if you want to set position = 0 on one or more user_mappings, you may end up with non-deterministic results and therefore it is our recommendation that you define the ordering yourself".

I also don't really have a pulse on how TF users like to set up their user_mappings. For some customers, ordering really matters, and maybe TF users are creating a lot of mappings and ordering isn't as critical. How important is the order in which user_mappings get applied?

rjhornsby commented 3 years ago

While I can only speak for us, I don't think the order matters. My goal was basically just to append to the list of mappings because it didn't make a difference where it appeared. I'm not sure we're using the roles and mappings correctly, but our OL mappings I think are almost entirely composed of 1:1 AD group -> OL role. The OL roles are used to control who has access to what OL configured applications.

The AD groups are used further, passed using OIDC scope, to set authorization policies within the app itself.