sap-linuxlab / community.sap_infrastructure Goto Github PK

Related to #1 and PR #4 - replace usage of Ansible Task block using environment: to set credentials in each Infrastructure Platform's code.

The Environment Var approach used on Ansible Task Block level to avoid repetition of credentials in each Ansible Task calling the respective Ansible Module, has been shown to leak credentials in -vvv debug mode.

This requires change and re-test across all Infrastructure Platforms, as this was used as a common approach. There may be some hidden regression impacts, so a re-test of every Infrastructure Platform after the re-code is necesary.

Currently, this role requires particular group names in the inventory. This needs to be more customized for generic use.

At least the group should be defined as a variable and can default to the now hardcoded names

Ideally, I would prefer a generic role to set a temp IP to "inventory hostname" or similar to get a more generic approach. Just pass current IP or Interface and virtual IP as parameters.

@sean-freeman sap_vm_provision role is currently directly linked with AP4S and it cannot be executed separately.

common/set_ansible_vars.yml requires all AP4S variables to not fail set_facts, but they are not documented anywhere as requirement.

Proposal:

1. Uncouple both repositories by giving optional variable that would allow to use this role separately.
2. Update Readme to say it cannot run without AP4S

   community.sap_infrastructure/roles/sap_vm_provision/README.md

   Line 7 in a67f6b8

   Primarily, this Ansible Role was designed to be executed end-to-end (i.e. Provision host/s, configure OS for SAP Software, install SAP Software, instantiate the SAP System); such as the [Ansible Playbooks for SAP](https://github.com/sap-linuxlab/ansible.playbooks_for_sap).

sap_vm_provision: avoid health check port on re-run

logic would be:

• already exists
• with health check port starting 6xxxx
• therefore assume re-run and do not perform changes

When using ansible-terraform the terraform command called by community.sap_infrastructure.sap_vm_provision reported this:

...
Warning: Argument is deprecated
  
      with module.run_account_init_module.azurerm_subnet.vnet_subnet,
      on .terraform/modules/run_account_init_module/msazure_vm/account_init/network_vnet_new.tf line 18, in resource "azurerm_subnet" "vnet_subnet":
      18:   private_endpoint_network_policies_enabled     = true
  
    `private_endpoint_network_policies_enabled` will be removed in favour of the
    property `private_endpoint_network_policies` in version 4.0 of the AzureRM
    Provider
...

Looks like technical debt during split of code into a separate Ansible Role, test whether the following can be removed:

https://github.com/sap-linuxlab/community.sap_infrastructure/blob/main/roles/sap_vm_provision/tasks/common/set_ansible_vars.yml#L16

sap_vm_provision: Add overwrite: true to Ansible AWS Route53 DNS Records for hosts

@sean-freeman Current implementation works great idempotently but gets stopped on route53 step because it will not overwrite by default.

TASK [community.sap_infrastructure.sap_vm_provision : Ansible AWS Route53 DNS Records for hosts] *********************************************
fatal: [nw750abaphdb -> localhost]: FAILED! => {"changed": false, "msg": "Record already exists with different value. Set 'overwrite' to replace it"}

Allow Private DNS on MS Azure to use Auto Registration, append new variable and logic sap_vm_provision_msazure_private_dns_auto_register that would skip the DNS Record entries created for the VM (but still append the DNS Record entries for HA Virtual IP).

NOTE:

At this time, the code will not be altered to allow no Private DNS to be specified. it remains best practice to:

• use the DNS Zone record (s4hana.myorg.cloud) for the inter-Cloud communications
• use the Corporate DNS Server’s Subzone record (s4hana.myorg.corp) for the internal traffic
• use the Corporate DNS Server’s Subdomain Delegation record (s4hana-webdisp.myorg.com) for the public internet traffic

The sap_vm_provision attempts to keep as much homogeneity as possible for Infrastructure Platforms, and each Cloud Service Provider has a Private DNS in their designs for SAP Landscapes.

Issues identified when testing AWS HA:

1. execute_setup_ha: IAM Role overwrites previous roles, meaning you cannot host multiple clusters in same account because new execution will replace IAM policies, invalidating Fencing rules for existing cluster.

• Add system specific input or override variable like we added for Loadbalancers to create new IAM role HA-Role-Pacemaker-<SID>
• Split existing HA-Role-Pacemaker into shared role for DataProvider with resources * as it is now

2. Change IAM policy implementation from invalid resource: instance/IP to valid instance/instance_id

Hardcoded values for:

• Load Balancer names (MS AZ, GCP, IBM Cloud)

Default values for:

• Virtual IPs

Replacing multiple occurrence of openshift-cnv with a constant. Consider adding vars.yml
In case we need to make changes we just have one place to change.

example:

  tasks:
    - name: Ensure the OperatorGroup is present
      kubernetes.core.k8s:
        state: present
        definition:
          apiVersion: operators.coreos.com/v1
          kind: OperatorGroup
          metadata:
            name: kubevirt-hyperconverged-group
            namespace: "{{ openshift_namespace }}"
          spec:
            targetNamespaces:
              - "{{ openshift_namespace }}"

Would like maintainers to think about this proposal so that i can proceed.

append placement strategy for all other Cloud vendors

Enhance sap_vm_provision role Red Hat OpenShift Virtualization flavor to use instance types.
Amount of cores should be specified when creating the instance type template by the admin.

This applies to MS Azure and IBM Cloud which use Resource Groups (and Tags).

There is an undeclared assumption that the same Resource Group is used throughout the provisioning, however it is more common that a Private DNS may be assigned to a separate Resource Group - particularly for hub/spoke design.

Need to append following code logic change to allow the following optional variables to be set:

• sap_vm_provision_msazure_private_dns_resource_group_name
• sap_vm_provision_ibmcloud_private_dns_resource_group_name

Must document the following assumption:

• Virtual Machine and associated resources (Disks, Network Interfaces, Load Balancer if HA) will be provisioned to the same Resource Group. This must be the same Resource Group as the target VNet and VNet Subnet.
• Private DNS may exist in a separate Resource Group from the host/s resources, and may be defined by a variable (e.g. sap_vm_provision_msazure_private_dns_resource_group_name, as you suggested)

@sean-freeman There are multiple task files that are consuming AWS Credentials as input variables/environment, which are visible when running -vvv debug. Please add no_log:true to all of them.

https://github.com/sap-linuxlab/community.sap_infrastructure/blob/main/roles/sap_vm_provision/tasks/platform_ansible/aws_ec2_vs/execute_main.yml

https://github.com/sap-linuxlab/community.sap_infrastructure/blob/main/roles/sap_vm_provision/tasks/platform_ansible_to_terraform/aws_ec2_vs/execute_main.yml

Address msazure_vm/execute_provision.yml#L45 which reads the SSH Key directly in the provisioning of Azure VMs.

Instead, replace with azure_rm_ssh_public_key Ansible Module to import Public SSH Key into Azure platform now that azure_rm_ssh_public_key: missing functionality #1170 is resolved.

amazon.aws.ec2_instance module does not support creation of Spot instances

https://docs.ansible.com/ansible/latest/collections/amazon/aws/ec2_instance_module.html
This module does not support creating EC2 Spot instances.

It would be good to add support for Spot instances by using module amazon.aws.ec2_spot_instance based on user specified variable with extra inputs for Spot specific inputs like Launch Group. https://docs.ansible.com/ansible/latest/collections/amazon/aws/ec2_spot_instance_module.html