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

apimanagement upgrade from 2019-12-01 to 2020-12-01 #11146

Merged
merged 2 commits into from
Apr 29, 2021

Conversation

MattiasAng
Copy link
Contributor

Requires go sdk v52.6.0 like #11108

Dependency to be able to implement #10138 and probably a couple of more issues.
Need some help with the tests since it is incredibly slow and my MSDN subscription is running out of money. Ran tests for those resources where changes were required.

@manicminer
Copy link
Member

@MattiasAng Thanks for this PR! Do you need to upgrade v52.5.0 to v52.6.0? SDK version upgrades usually affect a lot of services and we will normally do this in a standalone PR.

@MattiasAng
Copy link
Contributor Author

MattiasAng commented Mar 31, 2021

@manicminer SDK update was merged yesterday, see #11108 🙂

@manicminer
Copy link
Member

@MattiasAng Sorry about that, misread the diff there! I'm running through the acctests for this and will hopefully be able to review it tomorrow (API management takes awhile).

@katbyte katbyte added this to the v2.56.0 milestone Apr 7, 2021
@manicminer
Copy link
Member

Hi @MattiasAng, sorry for the delay on reviewing this, could you resolve the conflicts and I'll get to it asap. Thanks!

@manicminer manicminer self-requested a review April 13, 2021 22:00
@MattiasAng
Copy link
Contributor Author

@manicminer Should be fixed now

@tombuildsstuff tombuildsstuff modified the milestones: v2.56.0, v2.57.0 Apr 14, 2021
@manicminer
Copy link
Member

manicminer commented Apr 14, 2021

@MattiasAng Thanks, now awaiting acceptance test results

@manicminer
Copy link
Member

@MattiasAng I've run these a few times to average out the failures. I'm getting a consistent failure for TestAccApiManagementAuthorizationServer_complete with a persistent diff:

          # azurerm_api_management_authorization_server.test will be updated in-place
          ~ resource "azurerm_api_management_authorization_server" "test" {
                api_management_name          = "acctestAM-210419125530427842"
                authorization_endpoint       = "https://azacctest.hashicorptest.com/client/authorize"
                authorization_methods        = [
                    "GET",
                    "POST",
                ]
                bearer_token_sending_methods = [
                    "authorizationHeader",
                ]
                client_id                    = "42424242-4242-4242-4242-424242424242"
                client_registration_endpoint = "https://azacctest.hashicorptest.com/client/register"
                client_secret                = (sensitive value)
                default_scope                = "read write"
                display_name                 = "Test Group"
                grant_types                  = [
                    "authorizationCode",
                ]
                id                           = "/subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/acctestRG-210419125530427842/providers/Microsoft.ApiManagement/service/acctestAM-210419125530427842/authorizationServers/acctestauthsrv-210419125530427842"
                name                         = "acctestauthsrv-210419125530427842"
                resource_group_name          = "acctestRG-210419125530427842"
              + resource_owner_password      = (sensitive value)
              + resource_owner_username      = "rick"
                support_state                = true
                token_endpoint               = "https://azacctest.hashicorptest.com/client/token"
            }

Could you look into that?

@MattiasAng
Copy link
Contributor Author

Thanks for doing the integration tests.

Did a quick test and it seems like it is not being set at all so need to do some API testing to see if there are issues there first.
Will try to get to it next week.

@MattiasAng
Copy link
Contributor Author

MattiasAng commented Apr 26, 2021

Unfortunately this seems to be an API bug. Rest specifications looks correct.

GET /subscriptions/redacted/resourceGroups/acctestRG-redacted/providers/Microsoft.ApiManagement/service/acctestAM-redacted/authorizationServers/acctestauthsrv-redacted?api-version=2020-12-01

{
    "id": "/subscriptions/redacted/resourceGroups/acctestRG-redacted/providers/Microsoft.ApiManagement/service/acctestAM-redacted/authorizationServers/acctestauthsrv-redacted",
    "type": "Microsoft.ApiManagement/service/authorizationServers",
    "name": "acctestauthsrv-redacted",
    "properties": {
        "displayName": "Test Group",
        "description": "",
        "clientRegistrationEndpoint": "https://azacctest.hashicorptest.com/client/register",
        "authorizationEndpoint": "https://azacctest.hashicorptest.com/client/authorize",
        "authorizationMethods": [
            "GET",
            "POST"
        ],
        "clientAuthenticationMethod": [],
        "tokenBodyParameters": [],
        "tokenEndpoint": "https://azacctest.hashicorptest.com/client/token",
        "supportState": true,
        "defaultScope": "read write",
        "grantTypes": [
            "authorizationCode"
        ],
        "bearerTokenSendingMethods": [
            "authorizationHeader"
        ],
        "clientId": "42424242-4242-4242-4242-424242424242"
    }
}

vs

GET /subscriptions/redacted/resourceGroups/acctestRG-redacted/providers/Microsoft.ApiManagement/service/acctestAM-redacted/authorizationServers/acctestauthsrv-redacted?api-version=2019-12-01

{
    "id": "/subscriptions/redacted/resourceGroups/acctestRG-redacted/providers/Microsoft.ApiManagement/service/acctestAM-redacted/authorizationServers/acctestauthsrv-redacted",
    "type": "Microsoft.ApiManagement/service/authorizationServers",
    "name": "acctestauthsrv-redacted",
    "properties": {
        "displayName": "Test Group",
        "description": "",
        "clientRegistrationEndpoint": "https://azacctest.hashicorptest.com/client/register",
        "authorizationEndpoint": "https://azacctest.hashicorptest.com/client/authorize",
        "authorizationMethods": [
            "GET",
            "POST"
        ],
        "clientAuthenticationMethod": [],
        "tokenBodyParameters": [],
        "tokenEndpoint": "https://azacctest.hashicorptest.com/client/token",
        "supportState": true,
        "defaultScope": "read write",
        "grantTypes": [
            "authorizationCode"
        ],
        "bearerTokenSendingMethods": [
            "authorizationHeader"
        ],
        "clientId": "42424242-4242-4242-4242-424242424242",
        "resourceOwnerUsername": "rick",
        "resourceOwnerPassword": "C-193P"
    }
}

What is the best way forward to report these kind of bugs?

@manicminer
Copy link
Member

@MattiasAng I've opened an issue at Azure/azure-rest-api-specs#14128 referencing the example responses you posted.

We can potentially work around this by examining the values in configuration using d.Get() in the Read function, and re-setting that same value in state. It's not ideal since we can't detect service-side drift, but we can accept this for the time being. If you can get that working, just add a TODO comment linking to the upstream issue. Thanks!

@MattiasAng
Copy link
Contributor Author

@manicminer Should be good to go now.

@ghost ghost removed the waiting-response label Apr 28, 2021
@manicminer
Copy link
Member

@MattiasAng Great, thanks for circling back. Running the TestAccApiManagementAuthorizationServer_complete test now.

Copy link
Member

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Test results (failures are present on master):

Screenshot 2021-04-29 at 13 38 39
Screenshot 2021-04-29 at 13 36 24

@manicminer manicminer merged commit a3316b1 into hashicorp:master Apr 29, 2021
manicminer added a commit that referenced this pull request Apr 29, 2021
@ghost
Copy link

ghost commented Apr 30, 2021

This has been released in version 2.57.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.57.0"
}
# ... other configuration ...

katbyte pushed a commit that referenced this pull request May 1, 2021
…11175)

Dependency on #11146

Fixes #10138

Creating this pull request even though dependency is not merged yet since I want feedback on if implementation is OK. Unfortunately Microsoft is not so consistent with which ID of User Assigned Managed Identity they want on resources.
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2021
@MattiasAng MattiasAng deleted the api-mgmt-sdk branch March 23, 2022 14:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants