[HPR-1606] Add support specifying disk encryption set keys across replicated regions #371

nywilken · 2024-02-05T13:47:13Z

Add support for target_region block in SharedImageGalleryDestination

As an alternative to specifying replication_regions for a Shared Image
Gallery Destination, users can not use one or more target_region block
attributes to define the target regions for a shared image version. The
target_region block attribute supports the setting of individual disk
encryption key id for multi-region replication of encrypted
disks using CMKs.

The target_region attribute is meant to replace the now deprecated
replication_regions attribute. Internally, any values set for
replication_regions will be transferred into a set of target_region
blocks to provide the same level of functionality. The one difference being that
Packer will no longer add a default entry for the SIG build location
when using target_region blocks.

Closes: #264

Example Config

variable "resource_group" {
  type    = string
  default = "${env("ARM_RESOURCE_GROUP)}"
}

source "azure-arm" "autogenerated_1" {
  use_azure_cli_auth                = true
  image_offer                       = "UbuntuServer"
  image_publisher                   = "Canonical"
  image_sku                         = "16.04-LTS"
  location                          = "East US"
  os_type                           = "Linux"
  vm_size         = "Standard_DS2_v2"
  shared_image_gallery_destination {
    gallery_name        = "PackerSigGallery"
    image_name          = "PackerSigImageDefinition"
    image_version       = "1.0.1"
    resource_group      = var.resource_group
    target_region {
        name = "West US 2"
        disk_encryption_set_id = "KEY"
    }
    target_region {
        name = "East US"
        disk_encryption_set_id = "KEY"
    }
  }
}

build {
  sources = ["source.azure-arm.autogenerated_1"]

  provisioner "shell" {
    execute_command = "chmod +x {{ .Path }}; {{ .Vars }} sudo -E sh '{{ .Path }}'"
    inline          = ["apt-get update", "apt-get upgrade -y", "/usr/sbin/waagent -force -deprovision+user && export HISTSIZE=0 && sync"]
    inline_shebang  = "/bin/sh -x"
  }

}

builder/azure/arm/builder.go

JenGoldstrich · 2024-02-08T20:23:28Z

builder/azure/arm/builder.go

 		}
+
 		// TODO It would be better if validation could be handled in a central location


This code looks good but I wonder if we should add this validation to its own function or its own step, that way we could more easily unit test this logic in isolation, feel free to take that feedback or leave it, but I think it'd be good to add some sort of unit test coverage to ensure that we don't break the backwards compatibility you've established here in future updates to this code

I moved this into config.go and added a few tests for it. Let me know what you think.

JenGoldstrich · 2024-02-08T20:25:37Z

builder/azure/arm/builder.go

-		if foundMandatoryReplicationRegion == false {
-			b.config.SharedGalleryDestination.SigDestinationReplicationRegions = append(normalizedReplicationRegions, buildLocation)
+		// Validate target region settings; it can be the deprecated replicated_regsions attribute or multiple target_region blocks
+		if (len(b.config.SharedGalleryDestination.SigDestinationReplicationRegions) > 0) && (len(b.config.SharedGalleryDestination.SigDestinationTargetRegions) > 0) {


Could we check for this before we compute the regions? Like in the config validation we could just check if target_regions and replicated_regions are set together, that way we fail a bit earlier when the config is invalid, what do you think?

Yeah we can do that. This checks seems like it is bit late!

JenGoldstrich

This largely LGTM, nice work on this issue Wilken, I left a thought about a block in builder.go I think we could break off into a function and unit test in isolation, but I'm happy to do that refactoring in a follow up PR if you'd prefer, lemme know what you think!

JenGoldstrich · 2024-02-09T21:20:47Z

builder/azure/arm/config_test.go

@@ -1362,6 +1362,116 @@ func TestConfigShouldAcceptShallowReplicationWithWithUnsetReplicaCount(t *testin
 		t.Fatalf("expected config to accept shallow replication with unset replica count build: %v", err)
 	}
 }
+
+func TestConfigValidateShallowReplicationRegion(t *testing.T) {


Awesome! thanks for adding these

JenGoldstrich · 2024-02-09T21:24:18Z

builder/azure/arm/builder.go

-				return nil, fmt.Errorf("when `use_shallow_replication` is enabled the value of `replicated_regions` must match the build region specified by `location` or match the region of `build_resource_group_name`.")
+
+		// Convert deprecated replication_regions to []TargetRegion
+		if len(b.config.SharedGalleryDestination.SigDestinationReplicationRegions) > 0 {


Not blocking and I think this could be done in a follow up PR but I think we should move this block as well, into a new function called ConvertReplicatedRegions in can still exist in the builder but that way we can unit test it in isolation so that we don't break backwards compatibility in the future with this unintentionally.

FWIW I started down this path, as I agree this could help with testing but there is a bunch of nuance logic happening there that I wouldn't want to hide in a function. Breaking out the for loop is straight forward but we would run it after normalization. There are a few tests already that if given replication_regions asserts that the the correct number of target_regions are crated. If that helps.

That makes sense, thanks for clarifying!

I thought about how to move this stuff into a step_* but 😵 - maybe after a day something comes up. Thanks for this suggestion.

As an alternative to specifying replication_regions for a Shared Image Gallery Destination, users can not use one or more `target_region` block attributes to define the target regions for a shared image version. The target_region block attribute supports the setting of individual disk encryption key id for multi-region replication of encrypted disks using CMKs. The `target_region` attribute is meant to replace the now deprecated `replication_regions` attribute. Internally, any values set for replication_regions will be transferred into a set of target_region blocks to provide the same level of functionality. The one difference being that Packer will no longer add a default entry for the SIG build location when using target_region blocks.

…licated regions (hashicorp#371) * Add support for target_region block in SharedImageGalleryDestination As an alternative to specifying replication_regions for a Shared Image Gallery Destination, users can not use one or more `target_region` block attributes to define the target regions for a shared image version. The target_region block supports attributes for setting individual disk encryption key id to support multi-region replication of encrypted disks using CMKs. The `target_region` block attribute is meant to replace the now deprecated `replication_regions` attributed. Internally, any values set for replication_regions will be transferred into a set of target_regions to provide the same level of functionality. The one difference being that Packer will no longer add a default entry for the SIG build location when using target_region blocks.

nywilken added the enhancement label Feb 5, 2024

nywilken changed the title ~~nywilken/target regions for sig~~ [HPR-1606] Add support specifying DES across replicated regions Feb 5, 2024

nywilken force-pushed the nywilken/target-regions-for-sig branch 3 times, most recently from 1e4d28b to 89dcd8b Compare February 5, 2024 20:28

nywilken changed the title ~~[HPR-1606] Add support specifying DES across replicated regions~~ [HPR-1606] Add support specifying disk encryption set keys across replicated regions Feb 8, 2024

nywilken commented Feb 8, 2024

View reviewed changes

builder/azure/arm/builder.go Outdated Show resolved Hide resolved

nywilken commented Feb 8, 2024

View reviewed changes

builder/azure/arm/builder.go Outdated Show resolved Hide resolved

nywilken marked this pull request as ready for review February 8, 2024 18:09

nywilken requested a review from a team as a code owner February 8, 2024 18:09

JenGoldstrich reviewed Feb 8, 2024

View reviewed changes

JenGoldstrich approved these changes Feb 9, 2024

View reviewed changes

nywilken force-pushed the nywilken/target-regions-for-sig branch from 78a0b7f to 1a207d9 Compare February 13, 2024 16:32

nywilken mentioned this pull request Feb 13, 2024

Add Security Type Option for Confidential and Trusted Launch VMs #373

Merged

nywilken force-pushed the nywilken/target-regions-for-sig branch from 1a207d9 to baf7527 Compare February 16, 2024 17:15

nywilken added 4 commits February 16, 2024 12:20

Update HCL2 examples to use target_region block

3f98601

Move replication regions validation check to config

a71e87a

Move shallow replication validation to config.go

e7dc560

nywilken force-pushed the nywilken/target-regions-for-sig branch from baf7527 to e7dc560 Compare February 16, 2024 17:22

nywilken merged commit 66da753 into main Feb 16, 2024
12 checks passed

nywilken deleted the nywilken/target-regions-for-sig branch February 16, 2024 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HPR-1606] Add support specifying disk encryption set keys across replicated regions #371

[HPR-1606] Add support specifying disk encryption set keys across replicated regions #371

nywilken commented Feb 5, 2024 •

edited

Loading

JenGoldstrich Feb 8, 2024

nywilken Feb 9, 2024

JenGoldstrich Feb 8, 2024

nywilken Feb 9, 2024

nywilken Feb 9, 2024

JenGoldstrich left a comment

JenGoldstrich Feb 9, 2024

JenGoldstrich Feb 9, 2024

nywilken Feb 13, 2024 •

edited

Loading

JenGoldstrich Feb 13, 2024

nywilken Feb 13, 2024

		}

		// TODO It would be better if validation could be handled in a central location

[HPR-1606] Add support specifying disk encryption set keys across replicated regions #371

[HPR-1606] Add support specifying disk encryption set keys across replicated regions #371

Conversation

nywilken commented Feb 5, 2024 • edited Loading

Example Config

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JenGoldstrich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken commented Feb 5, 2024 •

edited

Loading

nywilken Feb 13, 2024 •

edited

Loading