onelogin / terraform-provider-onelogin

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

App Rule Action not selecting from list #70

Closed maxrabin closed 2 years ago

maxrabin commented 2 years ago

When I use the Onelogin Go SDK to create an App Rule, eg:

var actions [1]apprules.AppRuleActions
action := "set_groups"
action_values := []string{"01pxezwc29uu74j"}

actions[0] = apprules.AppRuleActions{
    Action: &action,
    Value: action_values,
}
....
newAppRule := apprules.AppRule{
  ...
  Actions: actions[:],
}
err1 := sdkClient.Services.AppRulesV2.Create(&newAppRule)

it works nicely, I can see that the group was selected from the list of available groups. I also see when running GET on /app/:app_id/rules that it looks like this:

    "actions": [
      {
        "action": "set_groups",
        "value": [
          "01pxezwc29uu74j"
        ]
      }
    ]

But when using this terraform inside of onelogin_app_rules

 actions {
   action = "set_groups"
   value = ["01pxezwc29uu74j"]
 }

then in the UI it has switched to a "map from onelogin" instead of choosing from list. Also when running GET on /app/:app_id/rules that it looks like this:

    "actions": [
      {
        "action": "set_groups",
        "value": [
          "01pxezwc29uu74j"
        ],
        "expression": ""
      }
    ]

I see that "expression" has been added as an empty string. I think that's the culprit. But I don't know why it is added.

dcaponi commented 2 years ago

It might be a while before I can do anything since I'm not actively maintaining this (I'm no longer with OneLogin officially) so I'm leaving these notes here for the next person who might be able to pick up the mantle.

If making the request through the Go SDK works for you, I bet you can reproduce this error via the Go SDK by adding an empty string expression field in your SDK. If that reproduces the error then something in the Terraform provider must be mis-handling the zero value when constructing the request via Terraform.

For clarity, all Terraform does is translate this HCL to a domain object (the AppRule struct) as defined in the Go SDK and then that payload is converted to JSON for an HTTP POST call under the hood via the Go SDK. We confirmed that the HTTP requests are valid (it works via the Go SDK) which is why I suspect the problem might be in translating the HCL to a domain object properly or, that domain object has the expression field not defined as a pointer to a string (i.e. nullable).

maxrabin commented 2 years ago

Thanks @dcaponi for the response. I suspect the error is in ol_schema/rules/actions/actions.go where I suspect it is getting to the line

out.Expression = oltypes.String(exp)

even though I didn't add a field "expression" in the HCL.

dcaponi commented 2 years ago

@maxrabin in the interest of reproducing this, could you tell me the type of app you're using please. Ideally if I can get the part of your terraform where you create the app and rule that'd be great!

maxrabin commented 2 years ago

This happens to me both in type "G Suite" and "Amazon Web Services (AWS) Multi Account" I created the apps in the website, not in Terraform.

dcaponi commented 2 years ago

Thanks - I was able to repro the problem, but this is one of those that is really tricky to fix. What's happening is, Terraform is passing me what it says is an empty value to the provider. When I get it, I have no way of knowing if you set the value to "" (which is valid) or if you meant null (i.e. don't set this value to anything, not even ""). I think there was a similar issue where we had this hacky workaround (if you see a few lines above the one you point out) in ol_schema/rules/actions/actions.go

I havent had a chance to get to fixing this in a cleaner way before I left OneLogin. If this is a critical fix you need now, I can have a PR out for you. Otherwise I'd leave it to the new maintainers.

dcaponi commented 2 years ago

I also attempted to send a request straight to the API via postman with the following bodies


{
    "name": "asdf",
    "match": "all",
    "enabled": true,
    "conditions": [],
    "actions": [
        {
            "action": "set_groups",
            "value": ["member_of"],
            "expression": null
        }
    ]
}

{
    "name": "asdf",
    "match": "all",
    "enabled": true,
    "conditions": [],
    "actions": [
        {
            "action": "set_groups",
            "value": ["member_of"]
        }
    ]
}

and both times I was greeted with the following error


    "code": 422,
    "message": "Validation Failed",
    "errors": [
        {
            "field": "actions",
            "message": [
                "Must pass expression for value"
            ]
        }
    ]
}

so I'm not sure how your first attempt was able to go through. I think this requires some investigation into the API behavior as well ☹️

maxrabin commented 2 years ago

This is how I made the request with CURL (I already included the example in GO) which succeeded (got a 200 response) and created the App Rule the way I expected, without "expression".

curl 'https://api.eu.onelogin.com/api/2/apps/XXXXXX/rules' \
-X POST \
-H "Authorization: bearer $ONELOGIN_BEARER" \
-H "Content-Type: application/json" \
-d '{
    "name": "XXXXXXX",
    "match": "all",
    "enabled": true,
    "position": null,
    "conditions": [
        {
            "source": "has_role",
            "operator": "ri",
            "value": "XXXXX"
        }
    ],
    "actions": [
        {
            "action": "set_groups",
            "value": ["XXXXXXXXXXX"]
        }
    ]
}'
mishi13 commented 2 years ago

Same issue here, with the cURL call everything worked as expected. Using Terraform completely screwed the configuration. The examples below are showing the results created by the API call and by Terraform. Also, on the next plan after the apply, Terraform was trying to change the faulty resource. Running another apply does not fix the issue.

Rule created with the cURL:

created_by_api

Same rule created with Terraform:

created_by_terraform

Plan the changes with Terraform:

pre_apply

Running plan after the apply:

post_apply

JSON payload that was used with cURL:

json_payload

maxrabin commented 2 years ago

@dcaponi do you have an ETA for this?

bzvestey commented 2 years ago

@maxrabin for an update on this, I am currently working on a fix. we had a similar issue with set_role and so I am extending the work around to also work with set_groups. Once this fix is out you will use set_groups_from_exisiting to signify the special path, like:


resource "onelogin_app_rules" "terraform_app_rule" {
  ...
  actions {
    action = "set_groups_from_existing"
    value = ["group1", "group2"]
  }
}

A proper fix would require moving over to the newer Terraform Plugin Framework, and would add some additional requirements onto this.

bzvestey commented 2 years ago

@maxrabin This has been released in ver v0.2.0