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

Fix for endpoint_params #202 #231

Closed
wants to merge 1 commit into from
Closed

Conversation

harshavmb
Copy link

@harshavmb harshavmb commented Aug 12, 2023

As of now endpoint_params part of oauth_client_credentials block isn't functional. Below issues were/are flagged::

This PR is a proposal to get it working with the limitations in mind. I disabled the map validation for complex values like this one, there is no support & it's discussed here. I changed the var type of endpoint_params from map of a list of strings to map of strings as the go oauth2 SDK expects in the below format::

EndpointParams: url.Values {
    "audience": {"audience1"}
 },

To me this is a weird format it's neither list nor string but interface{}. Anyways, with this format being expected, list isn't accepted at all. Hence, I chose the json. It's partly inspired from azurerm-provider.

An example configuration would look like below::

provider "restapi" {
  uri                  = "https://example.com/"
  debug                = true
  write_returns_object = true

  oauth_client_credentials {
    oauth_client_id      = "xxxx-yyyy-zzzz"
    oauth_client_secret  = "ssshsecret"
    oauth_token_endpoint = "https://login.example.com/abcde/oauth2/token"
    endpoint_params = <<ENDPOINT_PARAMS
    {
      "resource": "https://example.com/"
    }
    ENDPOINT_PARAMS
  }
}

@mhriemers
Copy link

Any update on when this will be merged?

@emmanuelgautier
Copy link

This PR is definitely helpful. Some authorization server require to pass audience for example.

Is there anything we can do to help you merge this PR?

@DRuggeri
Copy link
Member

DRuggeri commented Aug 5, 2024

Apologies for the VERY long radio silence on this one - it somehow slipped through my reviews and it wasn't until the recent comments did it pop up on my radar!

I like the idea of ultimately leveling this down from a map of string lists to just a map of strings, but I'm not sure I'm sold on the idea of encoding it in JSON. I haven't played in this area of the TF SDK for a while, but is that done in the PR, @harshavmb, because the terraform schema validation doesn't let us declare an arbitrary map with unknown keys and string values?

@DRuggeri
Copy link
Member

DRuggeri commented Aug 5, 2024

I have put together a patch based on this one that will use a more "native" terraform definition format. Does anyone on the thread happen to have a way to test it? I'm embarrassed that this bug crept in so long ago, but it's due to the complexities of creating an oauth stub in the test cases that this usecase can integrate with.

I have posted a release candidate here, if so: https://github.com/Mastercard/terraform-provider-restapi/releases/tag/v1.20.0.RC1

@harshavmb
Copy link
Author

Hi @DRuggeri ,

Thanks for your reply.

Unfortunately, the release tag doesn't contain the changes I wrote. I just compared with the last released version, no changes..

Regarding json encoding, I am also not sure why I chose that as it was almost an year ago :). Having said that I am sure it worked back then & even when I tested with my changes, it still works.

image

@DRuggeri
Copy link
Member

DRuggeri commented Aug 6, 2024

My apologies for that. I don't know how the tag got messed up in the release, but I straightened it out manually. It includes some other bumps and changes, so if you'd like to review just the commit itself, have a look at cae0619.

The linked release page includes a binary if that'd be easier to test with rather than a local build.

@DRuggeri
Copy link
Member

Following up - anyone able to test the release candidate here?

@harshavmb
Copy link
Author

harshavmb commented Aug 20, 2024

Hi @DRuggeri ,

I did build & test locally (unfortunately, not with the linux_amd64 binary as I have Mac M1). It worked as expected. Below is my configuration::

provider "restapi" {
  uri = "https://northeurope.management.azure.com"

  oauth_client_credentials {
    oauth_client_id      = "<<client-id>>"
    oauth_client_secret  = "<<client-secret>>"
    oauth_token_endpoint = "https://login.microsoftonline.com/<<tenant-id>>/oauth2/token"
    endpoint_params      = <<ENDPOINT_PARAMS
    {
      "resource": "https://management.core.windows.net/"
    }
    ENDPOINT_PARAMS
  }
}

data "restapi_object" "vms" {
  path         = "<<azure-resource-id>>"
  search_key   = "name"
  search_value = "<<azure-resource-name>>"
  id_attribute = "name"
  results_key  = "value"
  debug        = true
  query_string = "api-version=2023-03-01"
}

Of course, it doesn't support all forms of oauth like password based etc., etc., but it's not in the scope of this PR anyways.

@DRuggeri
Copy link
Member

Thanks so much for verifying! I have just pushed the proposed RC as the next version. Builds are running now and should be available shortly!

@DRuggeri DRuggeri closed this Aug 26, 2024
@harshavmb
Copy link
Author

harshavmb commented Aug 26, 2024

Thanks @DRuggeri, I did check with v1.20.0, all looking good.

Below was my configuration without ENDPOINT_PARAMS

provider "restapi" {
  uri = "https://northeurope.management.azure.com"

  oauth_client_credentials {
    oauth_client_id      = "<<client-id>>"
    oauth_client_secret  = "<<client-secret>>"
    oauth_token_endpoint = "https://login.microsoftonline.com/<<tenant-id>>/oauth2/token"
    endpoint_params      = {
      resource = "https://management.core.windows.net/"
    }
  }
}

data "restapi_object" "vms" {
  path         = "<<azure-resource-id>>"
  search_key   = "name"
  search_value = "<<azure-resource-name>>"
  id_attribute = "name"
  results_key  = "value"
  debug        = true
  query_string = "api-version=2023-03-01"
}

BTW, another PR I'll open soon for restapi provider to support username/password credentials type for password grant type. As of now, the workaround is to use body with alias provider but it is indeed oauth type.

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 this pull request may close these issues.

4 participants