Support geo-replication for azurerm_container_registry resource

[jcorioland]

This PR improve the azurerm_container_registry resource provider to support ACR geo-replication.

• possible to enable geo-replication in multiple Azure locations
• possible to add/remove locations on update
• acceptance tests cover enabling geo-replication in multiple locations, updating locations, updating Sku from Premium to Basic (that does not supports geo-replication)

Fixes #2047

[jsturtevant]

Looks like a good start. There are a few areas that could be consolidated (georeplication_enabled and georeplication_locations) that would remove edge cases as we discussed.

[jcorioland]

@tombuildsstuff I think this PR is ready for being reviewed. Thanks!

[tombuildsstuff]

hey @jcorioland

Thanks for this PR :)

I've taken a look through and left a couple of comments inline - but I'm wondering if this'd be better as a separate resource (with a lock on the container registry name to ensure multiple changes aren't made in parallel) - rather than being bundled up within the main azurerm_container_registry resource. This'd allow each replication location to be specified independently/using count rather than in-line - what do you think?

Thanks!

[jcorioland]

@tombuildsstuff thanks for the review ! :) I will do the suggested modifications. I also thought about externalizing the geo-replication into its own resource. The thing that makes me choose to bundle it with the container registry resource is that it is really dependent on the Sku.

For example, if you create an ACR in Basic mode, and then want to add a geo replication, you need to upgrade to Premium, so you need to 1. Update the ACR resource, and 2. add the geo-replication. Same thing if you want to downgrade the Sku, you need to remove the geo-replication first or it will fail. This is why I chose to bundle everything in the container registry resource. Happy to discuss this point with you if you think it does not make sense at the end.

[jcorioland]

@tombuildsstuff please let me know if you think there are some remaining blockers to merge this PR. Thanks!

[katbyte]

Hi @jcorioland,

I've taken a deeper look through the resource and left a number of comments inline. Outside of some nil checks the most critical are:

• I don't think the diff suppress and state func you have on georeplication_locations will work
• Curious as to why the name property is not exposed and we just default it to the location?
• in the acceptance tests checking the state not directly querying the API
• the documentation needs to be updated

[katbyte]

I am not seeing this used anywhere else, so could we pull it out?

[jcorioland]

I am not sure about what should be pulled out?

[katbyte]

This function isn't used anywhere else so i think its code should be put here.

Also, i think this could all be vastly simplified:

createLocations := make(map[string]bool)
	for _, nl := range newLocations {
		createLocations[nl] = false
	}
	for _, ol := range oldLocations {
		if _, ok := createLocations[ol]; ok {
			createLocations[ol] = true
		}
	}
	
	for _, locationToCreate := range createLocations {
		if ! createLocations[locationToCreate]  {
			continue
		}
		//create location
	}

	// delete not needed geo-replication locations
	for _, locationToDelete := range replicationLocationsToDelete {
		if _, ok := createLocations[locationToDelete]; !ok  {
			continue
		}
		// delete locatio
	}

what do you think?

[jcorioland]

Hey @katbyte - I've been able to refactor the update as you've suggested. But I've got a difficulty with using a TypeSet for location, so I rollback to TypeList.

When using a Set, and updating the location from "eastus","westus" to "eastus","westeurope", the eastus region is not considered as a new value (which make sense in a way) but this behavior does not allow to do a diff between old and new locations:

When using a list, I don't have these issue and I am able to diff:

I am relatively new to Terraform, so maybe there is another option I can explore?

[katbyte]

@jcorioland,

Could you write a test exhibiting this behaviour so I can see it myself and try and fix it? If that is the case, I would suggest using d.HasChange, and then if that is true get the entire new set with d.Get instead of just the change.

[jcorioland]

@katbyte yes will do that by the end of week. Thanks

[jcorioland]

Hi @katbyte - I've just created a new branch, with a fake resource and a fake unit test to illustrate the problem I have replacing List by a Set for the locations: jcorioland@46f2a46

The azurerm_issue_use_set resource has a simple schema with two properties:

• locations_list, a list of locations
• locations_set, a set of locations

The unit test is creating the fake resource with this config:

resource "azurerm_issue_use_set" "test" {
  locations_list      = ["eastus", "westus"]
  locations_set		  = ["eastus", "westus"]
}

The create opration only loops on the set/list values and display the content:

########## CREATE LIST ##########
Location (list): eastus
Location (list): westus
########## CREATE SET ##########
Location (set): westus
Location (set): eastus

Then, the test updates the config:

resource "azurerm_issue_use_set" "test" {
  locations_list      = ["eastus", "westeurope"]
  locations_set		  = ["eastus", "westeurope"]
}

The update function also just display the content of the set and the list, using both d.Get and d.GetChange:

########## UPDATE LIST ##########
locations_list variable has change
Location (list): eastus
Location (list): westeurope
Old location (list): eastus
Old location (list): westus
New location (list): eastus
New location (list): westeurope
########## UPDATE SET ##########
locations_set variable has change
Location (set): westeurope
Old location (set): westus
Old location (set): eastus
New location (set): westeurope

For the List variable, the d.Get returns eastus and westeurope
For the Set variable, the d.Get returns only westeurope
For the List variable, the d.GetChange returns old = eastus,westus - new = eastus, westeurope
For the Set variable, the d.GetChange returns old = eastus,westus - new = westeurope
For Both List & Set, the d.HasChange returns true.

With the List type, I am able to diff the old and new location and know that I need to keep eastus but need to delete westus.
With the Set type, I am not able to do it. The only information I have is that I need to add westeurope, but I am not able to determine what I need to do with the two old locations (or I am really missing something ^^)

Hope this unit tests will help to understand the issue I am facing :)

Thanks!
Julien

[katbyte]

Thank you for the very explicit unit test @jcorioland, much appreciated as it made tracking this down pretty quick.

It seems that the issue is not with d.* calls, but the schema diff/state functions. By changing it to this:

			"locations_set": {
				Type:     schema.TypeSet,
				MinItems: 1,
				Optional: true,
				Elem: &schema.Schema{
					Type: schema.TypeString,
					ValidateFunc: validation.NoZeroValues, //todo at some point we should write a location validation function..
				},
				Set: func(v interface{}) int {
					return hashcode.String(azureRMNormalizeLocation(v.(string)))
				},
			},

The tests now passes. I guess those functions were doing something weird to the state. The set function is just the HashString with a call to normalize location. I think that should work as intended now. I would recommend pulling that out into a HashAzureLocation() function.

[jcorioland]

Hi @katbyte - Thank you very much for your support! I was able to move back on Set for the geo-replication locations. Everything is working fine now.
I have also added sample code for container-registry and updated the docs.

LMK if I need to update anything else in order to get this PR merge.

Thanks a lot!

[katbyte]

Glad to hear its working now @jcorioland,

Its almost there, i've left some minor comments inline with the only real issues being:

• not checking the number of replication locations in state
• using westeurope as a replication location, we use that as the primary test location for our CI tests so it'll be a failure
• there are some new failing tests:

------- Stdout: -------
=== RUN   TestAccAzureRMContainerRegistry_basicClassic
=== PAUSE TestAccAzureRMContainerRegistry_basicClassic
=== CONT  TestAccAzureRMContainerRegistry_basicClassic
--- FAIL: TestAccAzureRMContainerRegistry_basicClassic (2.85s)
	testing.go:538: Step 0 error: Error planning: 1 error(s) occurred:
		
		* azurerm_container_registry.test: 1 error(s) occurred:
		
		* azurerm_container_registry.test: `storage_account_id` must be specified for a Classic (unmanaged) Sku.
FAIL```

Also adding the examples is greatly appreciated 

[jcorioland]

@katbyte thanks for the review. I've done the updates and validated that all unit tests pass for ContainerRegistry.

[katbyte]

Thanks for the updates @jcorioland! tests look good now:

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!