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

dependencies: update go-azure-sdk to v0.20250131.1134653 #28674

Merged
merged 13 commits into from
Feb 6, 2025

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Feb 4, 2025

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Update the go-azure-sdk to v0.20250131.1134653, with a couple of fixes below:

  • Provider config: The GtihubOIDCRequest(URL|Token) fields in the sdk has been renamed to removing the Github prefix. Tested below.
  • The API connection resource model has a type change on its ParameterValues field, causing a compile error. Tested below.

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
    For example: “resource_name_here - description of change e.g. adding property new_property_name_here

Changes to existing Resource / Data Source

  • I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
  • I have written new tests for my resource or datasource changes & updated any relevent documentation.
  • I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
  • (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Provider Github OIDC Test

name: terraform-azurerm-provider OIDC test
on: [workflow_dispatch]

permissions:
  id-token: write
  contents: read
jobs:
  build-and-deploy:
    runs-on: ubuntu-latest
    steps:
      - name: 'Checkout terraform-provider-azurerm repo'
        uses: actions/checkout@v4
        with:
          repository: 'magodo/terraform-provider-azurerm'
          ref: 'sdk_v0.20250131.1134653'
      - name: 'Setup Go'
        uses: actions/setup-go@v5
        with:
          go-version: '1.23'
      - name: 'Go Test'
        run: |
          export TF_ACC=1
          export ARM_SUBSCRIPTION_ID=${{ secrets.AZURE_SUBSCRIPTION_ID }}
          export ARM_TENANT_ID=${{ secrets.AZURE_TENANT_ID }}
          export ARM_CLIENT_ID=${{ secrets.AZURE_CLIENT_ID }}
          go test -v -run="TestAccProvider_githubOidcAuth" ./internal/provider

{4087E7DA-A98F-42DE-8F48-238000A39F8D}

API Connection

terraform-provider-azurerm on  main via 🐹 v1.23.3
💤  TF_ACC=1 go test -v -timeout=20h -parallel=20 -run=TestAccApiConnection_complete ./internal/services/connections
=== RUN   TestAccApiConnection_complete
=== PAUSE TestAccApiConnection_complete
=== CONT  TestAccApiConnection_complete                                                                                                                                                                           --- PASS: TestAccApiConnection_complete (250.39s)                                                                                                                                                                 PASS                                                                                                                                                                                                              ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/connections   250.418s      

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

  • dependencies: update go-azure-sdk to v0.20250131.1134653 [GH-00000]

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes #17143

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

jackofallops
jackofallops previously approved these changes Feb 4, 2025
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Thanks @magodo - As discussed offline, I've pushed some changes to expose the new property for users. Can you check it's working as expected with your ADO setup and post any results you are able to before we merge?

Thanks

internal/provider/framework/provider.go Outdated Show resolved Hide resolved
internal/provider/framework/config.go Outdated Show resolved Hide resolved
internal/provider/framework/config.go Outdated Show resolved Hide resolved
internal/provider/framework/config.go Outdated Show resolved Hide resolved
@magodo
Copy link
Collaborator Author

magodo commented Feb 5, 2025

Hi @jackofallops, I've made some more commits to fix a couple of missing parts. Now it behaves correctly, see the tests below.

Note that all the tests below uses the tool terraform-client-import, which configures the provider, imports and reads a RG. The env vars are all passed through to the provider. This is easier to test the provider behavior than constructing a .tf file and run terraform (with setting up to use a local provider).

Github Action

name: terraform-azurerm-provider OIDC test
on: [workflow_dispatch]

permissions:
  id-token: write
  contents: read
jobs:
  build-and-deploy:
    runs-on: ubuntu-latest
    steps:
      - name: 'Checkout terraform-provider-azurerm repo'
        uses: actions/checkout@v4
        with:
          repository: 'magodo/terraform-provider-azurerm'
          ref: 'sdk_v0.20250131.1134653'
      - name: 'Setup Go'
        uses: actions/setup-go@v5
        with:
          go-version: '1.23'

      - name: 'Unit Test'
        run: |
          export TF_ACC=1
          export ARM_SUBSCRIPTION_ID=${{ secrets.AZURE_SUBSCRIPTION_ID }}
          export ARM_TENANT_ID=${{ secrets.AZURE_TENANT_ID }}
          export ARM_CLIENT_ID=${{ secrets.AZURE_CLIENT_ID }}
          go test -v -run="TestAccProvider_githubOidcAuth" ./internal/provider

      - name: 'E2E Test'
        run: |
          # Install provider
          go install 
          # Install terraform-client-import
          go install github.com/magodo/terraform-client-go/cmd/terraform-client-import@main

          # Import a RG
          export ARM_SUBSCRIPTION_ID=${{ secrets.AZURE_SUBSCRIPTION_ID }}
          export ARM_TENANT_ID=${{ secrets.AZURE_TENANT_ID }}
          export ARM_CLIENT_ID=${{ secrets.AZURE_CLIENT_ID }}
          export ARM_PROVIDER_ENHANCED_VALIDATION=1
          export ARM_RESOURCE_PROVIDER_REGISTRATIONS=none
          export ARM_USE_OIDC=true
          ~/go/bin/terraform-client-import -type azurerm_resource_group -id /subscriptions/${ARM_SUBSCRIPTION_ID}/resourceGroups/zhwen-domain -path ~/go/bin/terraform-provider-azurerm

{32CD06CE-536F-436D-86EF-A8935EBDC88A}

Azure Pipeline

trigger: 
 - none

pool:
   vmImage: 'ubuntu-latest'

resources:
  repositories:
    - repository: terraform-provider-azurerm
      type: github
      endpoint: magodo-pat-read-public-repo
      name: magodo/terraform-provider-azurerm
      ref: sdk_v0.20250131.1134653

steps: 
- task: GoTool@0
  inputs:
    version: '1.23.3'

- checkout: terraform-provider-azurerm

- task: AzureCLI@2
  inputs:
    azureSubscription: $(CONNECTION_ID)
    scriptType: bash
    scriptLocation: "inlineScript"
    inlineScript: |
      set -e

      # Unit Test
      go test -v -run="TestAccProvider_adoOidcAuth" ./internal/provider

      # E2E Test
      go install 
      go install github.com/magodo/terraform-client-go/cmd/terraform-client-import@main
      ~/go/bin/terraform-client-import -type azurerm_resource_group -id /subscriptions/${ARM_SUBSCRIPTION_ID}/resourceGroups/zhwen-domain -path ~/go/bin/terraform-provider-azurerm

  env:
    TF_ACC: 1
    ARM_SUBSCRIPTION_ID: $(AZURE_SUBSCRIPTION_ID)
    ARM_TENANT_ID: $(AZURE_TENANT_ID)
    ARM_CLIENT_ID: $(AZURE_CLIENT_ID) 
    ARM_ADO_PIPELINE_SERVICE_CONNECTION_ID: $(CONNECTION_ID)
    SYSTEM_ACCESSTOKEN: $(System.AccessToken)
    SYSTEM_OIDCREQUESTURI: $(System.OidcRequestUri)
    ARM_PROVIDER_ENHANCED_VALIDATION: 1
    ARM_RESOURCE_PROVIDER_REGISTRATIONS: none
    ARM_USE_OIDC: true

image


The remaining question is that are we fine with the current solution, especially for the auth method selection, based on whether the ADO service connection is specified or not, to choose between Github auth and ADO auth. This won't work if we need to support a 3rd platform with OIDC.

One possible solution is to convert the toggles of use_oidc and use_aks_workload_identity to be a single toggle use_oidc, plus an enum like property oidc_method, with enum values: OIDCAssertionToken, GithubAction, ADOPipeline, etc.

The root issue here is that the interface of sdk/auth is defined as an array of toggles, whilst in the implementation, only one will be picked up, which relies on the order and the existance of other config values. Do you think we shall consider changing the interface to be an enum in the sdk, then the provider do the evaluation about which enum value to use based on the user's config, which can hopefully keep the current behavior/schema unchanged.

Anyway, we can plan to do any of the above in another PR if needed. Whilst, I think we still need to update the provider document in this PR?

@jackofallops
Copy link
Member

Thanks for the change @magodo - As discussed offline, I agree this PR should also update the service_principal_oidc.html.markdown documentation to cover the ADO properties, ideally with an example config. Once that's in, I'm happy to approve and merge this.

Thanks again!

@sreallymatt
Copy link
Collaborator

Fixes #17143

@magodo
Copy link
Collaborator Author

magodo commented Feb 6, 2025

@jackofallops Thanks! I've updated both the service_principal_oidc.html.markdown and the index.html.markdown. Please take another look!

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Thanks @magodo - I've made some wording change suggestions below if you can take a look? As soon as they're addressed I'll get this approved and merged.

Thanks!

website/docs/guides/service_principal_oidc.html.markdown Outdated Show resolved Hide resolved
website/docs/index.html.markdown Outdated Show resolved Hide resolved
website/docs/index.html.markdown Outdated Show resolved Hide resolved
website/docs/index.html.markdown Outdated Show resolved Hide resolved
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Thanks @magodo - this LGTM now 👍

@jackofallops jackofallops merged commit b8cd14f into hashicorp:main Feb 6, 2025
33 checks passed
@github-actions github-actions bot added this to the v4.18.0 milestone Feb 6, 2025
jackofallops added a commit that referenced this pull request Feb 6, 2025
jackofallops added a commit that referenced this pull request Feb 7, 2025
* CHANGELOG.md for v4.18.0

* Update CHANGELOG.md for #28308

* Update for #28447

* Update for #28532

* Update for #28537

* Update CHANGELOG.md for #28674

* Update for #28363

* Update for #28536

* Update for #28416

* Update CHANGELOG.md #28700

* Update for #28673

* Update for #28308

Co-authored-by: Wodans Son <[email protected]>

* Update for #27533

* prep for release

---------

Co-authored-by: stephybun <[email protected]>
Co-authored-by: sreallymatt <[email protected]>
Co-authored-by: Wodans Son <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment