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

Don't set OSProfile (Username/password) on images that use a Specialized Parent #308

Merged
merged 10 commits into from
Jul 14, 2023

Conversation

JenGoldstrich
Copy link
Contributor

@JenGoldstrich JenGoldstrich commented Jun 22, 2023

This PR fixes an issue added with plugin version v1.4.3

Description

In that plugin version we added support for creating a specialized ACG image with Packer, however when trying to use that specialized ACG image as a source for another image, we would incorrectly set the username and password, which is already defined in non-generalized VM Images.

Changes

  • Whenever a source SIG image is specialized, we remove the OS Profile field from the VM Deployment Template
  • Added an acceptance test which requires an SSH PEM Private key file, the path of which is set under the environment variable ARM_SSH_PRIVATE_KEY_FILE
  • ARM Acceptance tests no longer skip CLI auth tests, instead they fail the test file if Azure CLI auth is not set up

Closes #307

@JenGoldstrich JenGoldstrich force-pushed the dont_set_osprofile_for_specialized_sourced_builds branch from 4ac92ab to facd28e Compare June 23, 2023 22:05
@JenGoldstrich JenGoldstrich marked this pull request as ready for review June 29, 2023 23:20
@JenGoldstrich JenGoldstrich requested a review from a team as a code owner June 29, 2023 23:20
@JenGoldstrich JenGoldstrich requested a review from nywilken June 29, 2023 23:20
// First a parent Specialized ARM 64 Linux VM to a Shared Image Gallery/Compute Gallery
// Then a second Specialized ARM64 Linux VM that uses the first as its source/parent image
func TestBuilderAcc_SharedImageGallery_ARM64SpecializedLinuxSIG_WithChildImage(t *testing.T) {
t.Parallel()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests can run a LOT faster if we just parallelize them all!

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Left a couple of questions regarding the implementation, but overall this looks good to me, I'll do another pass after you've reviewed my comments and addressed or discarded them

builder/azure/arm/builder_acc_test.go Outdated Show resolved Hide resolved
builder/azure/arm/template_factory.go Show resolved Hide resolved
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

This is looking good. I left a few questions and a few suggestions on naming but otherwise it looks good.

builder/azure/arm/builder.go Show resolved Hide resolved
builder/azure/arm/builder_acc_test.go Outdated Show resolved Hide resolved
builder/azure/common/template/template_builder.go Outdated Show resolved Hide resolved
@@ -517,6 +516,15 @@ func (s *TemplateBuilder) SetSecurityProfile(secureBootEnabled bool, vtpmEnabled
return nil
}

func (s *TemplateBuilder) SetSpecializedVM() error {
resource, err := s.getResourceByType(resourceVirtualMachine)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to validate the the virtualMachine has the specialized property set?

@JenGoldstrich JenGoldstrich force-pushed the dont_set_osprofile_for_specialized_sourced_builds branch from 6b00da6 to af090b4 Compare July 8, 2023 02:10
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Code wise this looks good go me. But I learned that I could only get the acceptance test to pass if I used an RSA key. I then found https://learn.microsoft.com/en-us/troubleshoot/azure/virtual-machines/ed25519-ssh-keys

Maybe that should be part of the description for the env var. Or call rename to ARM_SSH_RSA_KEY_FILE

@JenGoldstrich JenGoldstrich merged commit efcaf7c into main Jul 14, 2023
@JenGoldstrich JenGoldstrich deleted the dont_set_osprofile_for_specialized_sourced_builds branch July 14, 2023 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating an Image from an Azure Compute Gallery specialized image does not work
3 participants