Skip to content

Remove module.ssh-key. Moving tls_private_key inside the module to root directory, output tls keys.#189

Merged
lonegunmanb merged 4 commits intoAzure:masterfrom
lonegunmanb:e-184
Jun 30, 2022
Merged

Remove module.ssh-key. Moving tls_private_key inside the module to root directory, output tls keys.#189
lonegunmanb merged 4 commits intoAzure:masterfrom
lonegunmanb:e-184

Conversation

@lonegunmanb
Copy link
Copy Markdown
Member

@lonegunmanb lonegunmanb commented Jun 20, 2022

This patch removes module.ssh-key and moves tls_private_key inside the module to root directory, then outputs tls keys to fix #184 and #191 issues. Since the moved block was introduced since Terraform v1.1, this patch bumped the version to v1.1 too.

@lonegunmanb lonegunmanb changed the title Rename module.ssh-key to module.ssh_key to meet the community naming convention. [WIP] Rename module.ssh-key to module.ssh_key to meet the community naming convention. Jun 20, 2022
@lonegunmanb lonegunmanb changed the title [WIP] Rename module.ssh-key to module.ssh_key to meet the community naming convention. Remove module.ssh-key. Moving tls_private_key inside the module to root directory, output tls keys. Jun 20, 2022
@zioproto
Copy link
Copy Markdown
Contributor

I understand the goal of this PR is to be automation friendly and avoid a ssh-key saved as a local file.

Here my considerations:

  • The ssh key is completely optional when creating an AKS cluster.
  • The AKS user should never need to ssh into the single AKS nodes.
  • To get a shell on the kubernetes node you can run a privileged container and escape to the host namespace: example is https://github.com/alexei-led/nsenter

This block of code with the linux profile is optional:
https://github.com/Azure/terraform-azurerm-aks/blob/4.16.0/main.tf#L20-L27

I suggest, to be automation friendly, to provide an option to disable the generation of the ssh key and put the linux_profile into a dynamic block.

Note that when creating a AKS cluster using the Azure Portal there is no way to specify the Linux profile and provide a key.

@lonegunmanb what do you think about adding to this PR the ability to disable the ssh-key completely ?

@lonegunmanb
Copy link
Copy Markdown
Member Author

lonegunmanb commented Jun 23, 2022

Hi @zioproto, thanks for your review.

My thought is we just leave the tls_private_key resource untouched and make linux_profile a dynamic block if possible because a private key resource without usage is harmless and it can make things easier.

Now the linux_profile won't be generated by default because I think it can reduce the attack surface but since I'm not an Aks expert, can you confirm that it's better to not generate linux_profile by default?

As all arguments in linux_profile is ForceNew, when the user has existing Aks cluster and want to upgrade module's version, it's extremely important for them to assign proper value for var.admin_username to avoid replacement of the running cluster.

Copy link
Copy Markdown
Contributor

@zioproto zioproto left a comment

Choose a reason for hiding this comment

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

LGTM. You could add a count to the tls_private_key resource for completeness

Comment thread main.tf
to = tls_private_key.ssh
}

resource "tls_private_key" "ssh" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not adding here this ?

count = var.admin_username == null ? 0 : 1

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah that works, but it will also require using index when we reference the resource, and I think an unused private key resource won't do harm, so I choose a shortcut.

…. All existing cluster need to assign `admin_username`(which default value is `azureuser`) explicitly OR THE CLUSTER WILL BE REPLACED!
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.

Refactor module.ssh-key to module.ssh_key

2 participants