Re: [PATCH 3/3] Allow private IP address to be assigned to instances created on OCI

[Chandan Babu R <chandanbabu@kernel.org>]

On Fri, May 02, 2025 at 12:31:58 PM -0400, Chuck Lever wrote:
> On 5/2/25 11:30 AM, Chuck Lever wrote:
>> On 5/2/25 2:46 AM, Chandan Babu R wrote:
>>> The OCI tenancy available to kernel developers from Oracle allows only private
>>> IP addresses to be assigned to compute instances. Hence this commit brings
>>> back the ability to work with OCI instances having private IP addresses.
>>>
>>> The variable "public_ip_map" defined in terraform/*/output.tf files has also
>>> been changed to "ansible_ip_map" in order to abstract away the type of the IP
>>> address being used.
>
> Duh. I know I suggested the name, but "ansible_ip_map" might suggest
> that Ansible defines this variable, like all other variables named
> ansible_something.
>
> Other possibilities:
>
>  ips_for_ansible
>  controller_ip_map
>

I think controller_ip_map is a better name. I will use that.

>
>
>>> Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>
>>> ---
>>>
>>> Please note that since I have only an OCI account, I haven't tested
>>> the changes on other cloud providers. However, the changes are limited
>>> to renaming "public_ip_map" to "ansible_ip_map" in terraform files.
>>>
>>>  playbooks/roles/terraform/tasks/main.yml          |  8 ++++----
>>>  playbooks/roles/terraform/templates/ssh_config.j2 |  4 ++--
>>>  scripts/status_terraform.sh                       |  2 +-
>>>  terraform/aws/output.tf                           |  2 +-
>>>  terraform/azure/output.tf                         |  2 +-
>>>  terraform/gce/output.tf                           |  2 +-
>>>  terraform/oci/output.tf                           | 14 +++++++++-----
>>>  terraform/openstack/output.tf                     |  2 +-
>>>  8 files changed, 20 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/playbooks/roles/terraform/tasks/main.yml b/playbooks/roles/terraform/tasks/main.yml
>>> index d04dc997..d1c94264 100644
>>> --- a/playbooks/roles/terraform/tasks/main.yml
>>> +++ b/playbooks/roles/terraform/tasks/main.yml
>>> @@ -7,22 +7,22 @@
>>>    tags:
>>>      - bringup
>>>  
>>> -- name: Retrieve the public_ip_map from terraform
>>> +- name: Retrieve the ansible_ip_map from terraform
>>>    delegate_to: localhost
>>>    run_once: true
>>>    ansible.builtin.command:
>>>      chdir: "{{ topdir_path }}/terraform/{{ kdevops_terraform_provider }}"
>>> -    cmd: "terraform output -json public_ip_map"
>>> +    cmd: "terraform output -json ansible_ip_map"
>>>    register: terraform_output
>>>    changed_when: false
>>>    tags:
>>>      - ssh
>>>  
>>> -- name: Convert the retrieved public_ip_map into a dictionary
>>> +- name: Convert the retrieved ansible_ip_map into a dictionary
>>>    delegate_to: localhost
>>>    run_once: true
>>>    ansible.builtin.set_fact:
>>> -    public_ip_map: "{{ terraform_output.stdout | from_json }}"
>>> +    ansible_ip_map: "{{ terraform_output.stdout | from_json }}"
>>>    tags:
>>>      - ssh
>>>  
>>> diff --git a/playbooks/roles/terraform/templates/ssh_config.j2 b/playbooks/roles/terraform/templates/ssh_config.j2
>>> index d9b057b1..c6e62a61 100644
>>> --- a/playbooks/roles/terraform/templates/ssh_config.j2
>>> +++ b/playbooks/roles/terraform/templates/ssh_config.j2
>>> @@ -1,5 +1,5 @@
>>> -Host {{ inventory_hostname }} {{ public_ip_map[inventory_hostname] }}
>>> -	HostName {{ public_ip_map[inventory_hostname] }}
>>> +Host {{ inventory_hostname }} {{ ansible_ip_map[inventory_hostname] }}
>>> +	HostName {{ ansible_ip_map[inventory_hostname] }}
>>>  	User {{ kdevops_terraform_ssh_config_user }}
>>>  	Port 22
>>>  	IdentityFile {{ kdevops_terraform_ssh_config_privkey_file }}
>>> diff --git a/scripts/status_terraform.sh b/scripts/status_terraform.sh
>>> index dc82d79e..bc4ad309 100755
>>> --- a/scripts/status_terraform.sh
>>> +++ b/scripts/status_terraform.sh
>>> @@ -13,5 +13,5 @@ if [[ ${COUNT} -eq 1 ]]; then
>>>  else
>>>    echo "There are ${COUNT} active terraform resources on '$1'."
>>>  fi
>>> -terraform output public_ip_map
>>> +terraform output ansible_ip_map
>>>  exit 0
>>> diff --git a/terraform/aws/output.tf b/terraform/aws/output.tf
>>> index ab18bc55..b5e59eb1 100644
>>> --- a/terraform/aws/output.tf
>>> +++ b/terraform/aws/output.tf
>>> @@ -3,7 +3,7 @@
>>>  # Each provider's output.tf needs to define a public_ip_map. This
>>>  # map is used to build the Ansible controller's ssh configuration.
>>>  # Each map entry contains the node's hostname and public IP address.
>>> -output "public_ip_map" {
>>> +output "ansible_ip_map" {
>>>    description = "The public IP addresses assigned to each instance"
>>>    value       = zipmap(var.kdevops_nodes[*], aws_eip.kdevops_eip[*].public_ip)
>>>  }
>>> diff --git a/terraform/azure/output.tf b/terraform/azure/output.tf
>>> index 38104c90..deb7d141 100644
>>> --- a/terraform/azure/output.tf
>>> +++ b/terraform/azure/output.tf
>>> @@ -1,7 +1,7 @@
>>>  # Each provider's output.tf needs to define a public_ip_map. This
>>>  # map is used to build the Ansible controller's ssh configuration.
>>>  # Each map entry contains the node's hostname and public IP address.
>>> -output "public_ip_map" {
>>> +output "ansible_ip_map" {
>>>    description = "The public IP addresses assigned to each instance"
>>>    value       = zipmap(var.kdevops_nodes[*], azurerm_public_ip.kdevops_publicip[*].ip_address)
>>>  }
>>> diff --git a/terraform/gce/output.tf b/terraform/gce/output.tf
>>> index 15889cc4..9246216f 100644
>>> --- a/terraform/gce/output.tf
>>> +++ b/terraform/gce/output.tf
>>> @@ -3,7 +3,7 @@
>>>  # Each provider's output.tf needs to define a public_ip_map. This
>>>  # map is used to build the Ansible controller's ssh configuration.
>>>  # Each map entry contains the node's hostname and public IP address.
>>> -output "public_ip_map" {
>>> +output "ansible_ip_map" {
>>>    description = "The public IP addresses assigned to each instance"
>>>    value = zipmap(var.kdevops_nodes[*],
>>>    google_compute_instance.kdevops_instance[*].network_interface[0].access_config[0].nat_ip)
>>> diff --git a/terraform/oci/output.tf b/terraform/oci/output.tf
>>> index 1657b804..6562a1e8 100644
>>> --- a/terraform/oci/output.tf
>>> +++ b/terraform/oci/output.tf
>>> @@ -1,9 +1,13 @@
>>>  # All generic output goes here
>>>  
>>> -# Each provider's output.tf needs to define a public_ip_map. This
>>> +# Each provider's output.tf needs to define a ansible_ip_map. This
>>>  # map is used to build the Ansible controller's ssh configuration.
>>> -# Each map entry contains the node's hostname and public IP address.
>>> -output "public_ip_map" {
>>> -  description = "The public IP addresses assigned to each instance"
>>> -  value       = zipmap(var.kdevops_nodes[*], oci_core_instance.kdevops_instance.*.public_ip)
>>> +# Each map entry contains the node's hostname and public/private IP
>>> +# address.
>> 
>> Can you update the documenting comment in the output.tf file for the
>> other providers as well?
>> 
>>

Ok.

>>> +output "ansible_ip_map" {
>>> +  description = "The IP addresses assigned to each instance"
>> 
>> Also, update the description for the ansible_ip_map output variable in
>> all the output.tf files.
>>

Sure, I will do that.

>> 
>>> +  value       = zipmap(var.kdevops_nodes[*],
>>> +    var.oci_assign_public_ip ?
>>> +    oci_core_instance.kdevops_instance.*.public_ip :
>>> +    oci_core_instance.kdevops_instance.*.private_ip)
>> 
>> Weird indentation. I assume this was the result of "terraform fmt" ?

Sorry, I had used my text editor's default indentation. I will fix it.

>> 
>> 
>>>  }
>>> diff --git a/terraform/openstack/output.tf b/terraform/openstack/output.tf
>>> index d5d3a6b1..3dd139a3 100644
>>> --- a/terraform/openstack/output.tf
>>> +++ b/terraform/openstack/output.tf
>>> @@ -21,7 +21,7 @@ output "kdevops_hosts_and_ipv4" {
>>>  # Each provider's output.tf needs to define a public_ip_map. This
>>>  # map is used to build the Ansible controller's ssh configuration.
>>>  # Each map entry contains the node's hostname and public IP address.
>>> -output "public_ip_map" {
>>> +output "ansible_ip_map" {
>>>    description = "The public IP addresses assigned to each instance"
>>>    value       = zipmap(var.kdevops_nodes[*], openstack_compute_instance_v2.kdevops_instances[*].access_ip_v4)
>>>  }
>> 
>> This patch makes sense to me, just needs a little polish.
>> 

Thanks for reviewing all the patches.

I will make the required modifications and post V2 soon.

-- 
Chandan