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

azurerm_batch_pool's support user_assigned_identity_id #17104

Merged

Conversation

ayyagari74
Copy link
Contributor

@ayyagari74 ayyagari74 commented Jun 5, 2022

updated resource_file of azurerm_batch_pool (data source and resource) to support managed identity of user.

resource "azurerm_batch_pool" "test" {
   ...
   resource_file {
      file_path          = "python-3.10.4-amd64.exe"
      http_url           = "https://<>.blob.core.windows.net/pythoninstall/python-3.10.4-amd64.exe"
      identity_id = "/subscriptions/<>/resourceGroups/<>/providers/Microsoft.ManagedIdentity/userAssignedIdentities/<>"
    }
   ...
    
    
}

Copy link
Collaborator

@favoretti favoretti left a comment

Choose a reason for hiding this comment

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

Hey, thank you for this PR. A few remarks. Aside of adding it to schema, we need to use and set that value in Create/Update/Read functions and we need to extend acceptance tests so that this functionality will be tested.

@favoretti
Copy link
Collaborator

I started working on that as well, but I'll let you finish your PR if you want. Here's an example of the code I literally had open just now:

+++ b/internal/services/batch/batch_pool.go
@@ -157,6 +157,9 @@ func flattenBatchPoolStartTask(startTask *batch.StartTask) []interface{} {
                        if armResourceFile.FileMode != nil {
                                resourceFile["file_mode"] = *armResourceFile.FileMode
                        }
+                       if armResourceFile.IdentityReference != nil {
+                               resourceFile["identity_reference"] = *armResourceFile.IdentityReference.ResourceID
+                       }
                        resourceFiles = append(resourceFiles, resourceFile)
                }
        }
@@ -496,6 +499,12 @@ func ExpandBatchPoolStartTask(list []interface{}) (*batch.StartTask, error) {
                                resourceFile.FileMode = &fileMode
                        }
                }
+               if v, ok := resourceFileValue["identity_reference"]; ok {
+                       identityReference := batch.ComputeNodeIdentityReference{
+                               ResourceID: utils.String(v.(string)),
+                       }
+                       resourceFile.IdentityReference = &identityReference
+               }
                resourceFiles = append(resourceFiles, resourceFile)
        }

@github-actions github-actions bot added size/M and removed size/XS labels Jun 5, 2022
@ayyagari74
Copy link
Contributor Author

ayyagari74 commented Jun 5, 2022

@favoretti : thank you for the valuable feedback. incorporated the changes.Also tested with following terraform code locally generated azure provider. Request you to take a look.

provider "azurerm" {
  features {}
}

resource "azurerm_resource_group" "test" {
  name     = "testaccbatch"
  location = "eastus2"
}

resource "azurerm_batch_account" "test" {
  name                = "testaccbatch"
  resource_group_name = azurerm_resource_group.test.name
  location            = azurerm_resource_group.test.location
}

resource "azurerm_user_assigned_identity" "test" {
  resource_group_name = azurerm_resource_group.test.name
  location            = azurerm_resource_group.test.location

  name = "useridentity"
}

resource "azurerm_batch_pool" "test" {
  name                = "testaccpool"
  resource_group_name = azurerm_resource_group.test.name
  account_name        = azurerm_batch_account.test.name
  node_agent_sku_id   = "batch.node.ubuntu 18.04"
  vm_size             = "Standard_A1"

  fixed_scale {
    target_dedicated_nodes = 1
  }

  storage_image_reference {
    publisher = "Canonical"
    offer     = "UbuntuServer"
    sku       = "18.04-lts"
    version   = "latest"
  }

  start_task {
    command_line       = "echo 'Hello World from $env'"
    task_retry_maximum = 1
    wait_for_success   = true

    common_environment_properties = {
      env = "TEST"
      bu  = "Research&Dev"
    }

    user_identity {
      user_name = "testUserIndentity"
    }

    resource_file {
      http_url  = "https://raw.githubusercontent.com/hashicorp/terraform-provider-azurerm/main/README.md"
      file_path = "README.md"
      identity_id = azurerm_user_assigned_identity.test.id
    }
  }
}

@ayyagari74 ayyagari74 requested a review from favoretti June 5, 2022 13:05
@@ -268,6 +268,8 @@ A `resource_file` block supports the following:

* `storage_container_url` - (Optional) The URL of the blob container within Azure Blob Storage. This URL must be readable and listable using anonymous access; that is, the Batch service does not present any credentials when downloading the blob. There are two ways to get such a URL for a blob in Azure storage: include a Shared Access Signature (SAS) granting read and list permissions on the blob, or set the ACL for the blob or its container to allow public access.

* `identity_reference` - (Optional) An identity reference from pool's user assigned managed identity list.
Copy link
Collaborator

Choose a reason for hiding this comment

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

what sort of identity is returned? and id? a guid? is it managed? could it be user or msi?

Suggested change
* `identity_reference` - (Optional) An identity reference from pool's user assigned managed identity list.
* `identity_id` - (Optional) An identity reference from pool's user assigned managed identity list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ayyagari74 ayyagari74 requested a review from katbyte July 15, 2022 01:04
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

build failure:

[Step 4/5] in directory: /opt/teamcity-agent/work/5e6516bb4d10eb66/internal/services/batch
[05:32:15]	[Step 4/5] # github.com/hashicorp/terraform-provider-azurerm/internal/services/batch
[05:32:15]	[Step 4/5] ./batch_pool.go:160:23: armResourceFile.IdentityId undefined (type "github.com/Azure/azure-sdk-for-go/services/batch/mgmt/2022-01-01/batch".ResourceFile has no field or method IdentityId)
[05:32:15]	[Step 4/5] ./batch_pool.go:161:52: armResourceFile.IdentityId undefined (type "github.com/Azure/azure-sdk-for-go/services/batch/mgmt/2022-01-01/batch".ResourceFile has no field or method IdentityId)
[05:32:15]	[Step 4/5] ./batch_pool.go:507:25: undefined: batch.ComputeNodeIdentityId
[05:32:15]	[Step 4/5] ./batch_pool.go:510:18: resourceFile.IdentityId undefined (type "github.com/Azure/azure-sdk-for-go/services/batch/mgmt/2022-01-01/batch".ResourceFile has no field or method IdentityId)
[05:32:15]	[Step 4/5] Process exited with code 2

@dkuzmenok
Copy link
Contributor

@katbyte @ayyagari74 Duplicate of #17416 ?

@katbyte
Copy link
Collaborator

katbyte commented Aug 18, 2022

@dkuzmenok - this seems to be adding identity_id to resource_files while #17416 is adding it to container_registeries?

@dkuzmenok
Copy link
Contributor

@katbyte Ah, you are right. Sorry.
Then I would vote for renaming it to user_assigned_identity_id instead of identity_id.

identityReference	
ComputeNodeIdentityReference
The reference to the user assigned identity to use to access Azure Blob Storage specified by storageContainerUrl or httpUrl
The reference to a user assigned identity associated with the Batch pool which a compute node will use.

@Rama-Ayyagari
Copy link

Ok.
Started working on it

@katbyte
Copy link
Collaborator

katbyte commented Aug 18, 2022

agreed @dkuzmenok

@ayyagari74
Copy link
Contributor Author

ayyagari74 commented Sep 9, 2022

we can not rename identity_reference to neither user_assigned_identity_id nor identity_id.Because there is dependency on azure-adk-for-go library, in which the field is called IdentityReference.

// ResourceFile ...
type ResourceFile struct {

	IdentityReference *ComputeNodeIdentityReference `json:"identityReference,omitempty"`
}

so changed back to identity_reference.

Build and unit tests are passed.
request you to take a final look and approve it, as team is waiting for this PR to be merged

@ayyagari74 ayyagari74 requested review from katbyte and removed request for favoretti September 9, 2022 06:52
@katbyte
Copy link
Collaborator

katbyte commented Sep 13, 2022

@ayyagari74 - i'm not sure i follow why you cant change the schema to user_assigned_identity_id? the schema can be named something different then the helpers?

@ayyagari74
Copy link
Contributor Author

ayyagari74 commented Sep 13, 2022

internal/services/batch/batch_pool.go file we are using armResourceFile class. Which is defined in azure-sdk-for-go library.
in that library resource file is defined as below

// ResourceFile ...
type ResourceFile struct {

IdentityReference *ComputeNodeIdentityReference `json:"identityReference,omitempty"`

}

Yes technically i can do that.
But that leads to confusion because we need map the attribute from library (azure-sdk-for-go -> ResourceFile -> IdentityReference : [terraform-provider-azurerm] -> Batch -> ResourceFile -> user_assigned_identity_id).

@ayyagari74
Copy link
Contributor Author

done.
changed identity_reference to user_assigned_identity_id.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

thanks @ayyagari74 ! LGTM 🌩️

@katbyte katbyte changed the title Support for Managed Identities in azurerm_batch_pool's resourceFiles #17067 azurerm_batch_pool's support user_assigned_identity_id Sep 15, 2022
@katbyte katbyte merged commit 98522bd into hashicorp:main Sep 15, 2022
@github-actions github-actions bot added this to the v3.23.0 milestone Sep 15, 2022
katbyte added a commit that referenced this pull request Sep 15, 2022
@github-actions
Copy link

This functionality has been released in v3.23.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@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 Oct 16, 2022
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

5 participants