sap-linuxlab / community.sap_install

Automation for SAP - Collection of Ansible Roles for various SAP software installation
Apache License 2.0
53 stars 57 forks source link

sap_swpm: manage sap_swpm_db_schema_abap_password in hdbuserstore #802

Open remrozk opened 4 months ago

remrozk commented 4 months ago

In the case where the sap_swpm_db_schema_abap_password variable is set, the DEFAULT key is set with the wrong password sap_swpm_db_system_password.

With the modification, the code handles both cases correctly

berndfinger commented 4 months ago

@rob0d @sean-freeman @remrozk After merging https://github.com/sap-linuxlab/community.sap_install/pull/748, what are your thoughts about the alternative approach suggested here (defaulting to sap_swpm_db_system_password if sap_swpm_db_schema_abap_password is undefined?

sean-freeman commented 4 months ago

@berndfinger I am not in favour of creating defaulting "symlink" (kinda) approach between passwords in the long-term; I'd rather have a new Ansible Task fail: for any password that is not set?

We already have technical debt in this regard dating from the init commit: https://github.com/sap-linuxlab/community.sap_install/blob/main/roles/sap_swpm/tasks/pre_install/password_facts.yml#L17-L21

rob0d commented 4 months ago

Hi guys,

Two points/comments: 1) What is the purpose of this code? It is setting DEFAULT connection in hdbuserstore when saphostagent was installed by SWPM during system installation. However, I am not sure if it's still actually required? SWPM creates and sets this correctly during the installation in first place. Is this to get around a particular bug/feature that may or may not exist anymore?

2) I believe this change has a potential to break the installed system if sap_swpm_db_schema_abap_password is not set. The behaviour of SWPM and this role generally is to use the master password when no specific password is defined. So if sap_swpm_db_schema_abap_password is not defined the master password would have been used when the tenant was created. If this code is still required as per 1), I would suggest that we default to master password if sap_swpm_db_schema_abap_password isn't defined. This will be more transparent and non-breaking.

remrozk commented 4 months ago

Hi guys,

If there is no known reason for this code block, then yes, removing it is the best solution. SAP NW 7.5 and SAP S/4HANA create the required key to enable the connection between the AS with the DB during the SWPM process.

Are you okay with pushing a new commit to remove this block code and run some non-regression tests?

sean-freeman commented 4 months ago

@rob0d @remrozk

  1. See https://github.com/sap-linuxlab/community.sap_install/issues/747#issuecomment-2238752327 , repeating here for ease "Original code is from PR #446 describing hdbuserstore incorrectly populated by SWPM using the IP Address instead of Hostname"

  2. Correct, as highlighted ^^^ when any password is not present - Master Password is attempted/assumed; however sap_swpm_db_system_password is not in the list of defaulting passwords. I've already stated above that I believe this to be technical debt that should be revised > community.sap_install/roles/sap_swpm/tasks/pre_install/password_facts.yml#L17-L21

sean-freeman commented 4 months ago

@remrozk Can you please test whether the following added to community.sap_install/roles/sap_swpm/tasks/pre_install/password_facts.yml provides the same behaviour you are looking for?

- name: SAP SWPM Pre Install - Set fact for any blank passwords to use master password
  ansible.builtin.set_fact:
    "{{ empty_password_var }}": "{{ sap_swpm_master_password }}"
  loop:
    - sap_swpm_db_schema_abap_password
    - sap_swpm_db_schema_java_password
    - sap_swpm_db_schema_password
    - sap_swpm_db_sidadm_password
    - sap_swpm_db_system_password
    - sap_swpm_db_systemdb_password
    - sap_swpm_ddic_000_password
    - sap_swpm_diagnostics_agent_password
    - sap_swpm_master_password
    - sap_swpm_sap_sidadm_password
    - sap_swpm_sapadm_password
    - sap_swpm_tmsadm_password
    - sap_swpm_ume_j2ee_admin_password
    - sap_swpm_ume_j2ee_guest_password
    - sap_swpm_ume_sapjsf_password
    - sap_swpm_wd_backend_rfc_user_password
  loop_control:
    loop_var: empty_password_var
  when: empty_password_var is undefined or empty_password_var == ""
remrozk commented 3 months ago

@sean-freeman In addition to this task, should I remove the task SAP SWPM Post Install - Enforce Connection Info in SAP HANA Client hdbuserstore in community.sap_install/roles/sap_swpm/tasks/post_install.yml?

Because, if I don't remove the task SAP SWPM Post Install - Enforce Connection Info in SAP HANA Client hdbuserstore, the value will be overwritten, even with the new task in the community.sap_install/roles/sap_swpm/tasks/pre_install/password_facts.yml.

sean-freeman commented 3 months ago

@remrozk You do not want to remove that Ansible Task, for reason given above (search page for Original code keywords). The command in question was updated last week in PR https://github.com/sap-linuxlab/community.sap_install/pull/748 . Which is why there is a Merge Conflict in this PR

rob0d commented 3 months ago

Hi @sean-freeman.

1) It's difficult to prove it now and I don't mean any disrespect to the person who raised #446, but based on the description I don't believe their environment was configured correctly and in-line with SAP Note 21151 (and the document attached to that note). There used to be bugs in SWPM which sometimes meant it picked the wrong interface, but I've not seen it on HANA based systems for a very long time (if ever). The SAP (V)LAN should be set as primary interface and should be associated with the server hostname and if the hosts/DNS resolution is consistent, SWPM will pick the correct interface. Environments with more complex needs can use something like dbserver-backlan for the dbserver secondary inteface to force the DB traffic down that interface. Again if dbserver-backlan is provided to SWPM as DBHOST, it will use it as long as hosts/DNS are correct. If think that the code is solving an environmental config issue on customer side, rather than issue with Ansible code/SWPM and most likely isn't needed. Unfortunately currently I don't have a dual homed system where I test this.

2) I understand now what you meant by technical debt. PR #748 inadvertently exposed that as I haven't realised people may not set all the password variables. The code you pasted seems like a good compromise to set all empty variables to the master password. I am testing it now as well. I've noticed that there are more sap_swpm*password variables, but only some are listed. Is that on purpose? You don't want to set all of them?

remrozk commented 3 months ago

@sean-freeman Sorry for the misunderstanding. My reflections were based on the main branch, not the dev branch. My apologies.

As requested, I tested the community.sap_install/roles/sap_swpm/tasks/pre_install/password_facts.yml task. We observe the expected behaviour. The sap_swpm_db_schema_abap_password var is used to enforce the password in the hdbusertstore key, if set. I need to test the case when it is not set.

I had to fix a regression in the community.sap_install/roles/sap_install_media_detect/tasks/prepare/move_files_to_main_directory.yml (#773).

The backslash (\) was excessive. In my case, the sap_hana directory was no longer detected during the installation of the first application (CI), after the DB_instance installation on the same host. SAP HANA files were no longer moved to the __sap_install_media_detect_software_main_director and the SAP Install Media Detect - Prepare - Assert that saphana is present task failed.

Below, the task fixed:

- name: SAP Install Media Detect - Prepare - Move files to parent for known subdirs - Find the relevant non-extract subdirectories # noqa risky-shell-pipe
  ansible.builtin.shell:
    cmd: >
      ls -d
      sap_hana sap_swpm sap_swpm_download_basket
      sapase sapmaxdb oracledb ibmdb2 sap_export_nwas_java sap_export_ecc sap_export_nwas_abap sap_export_solman_java sap_export_ecc_ides
      $({{ __sap_install_media_detect_sapfile_path }} -s) 2>/dev/null |
      awk '{print ("'{{ __sap_install_media_detect_software_main_directory }}'/"$0"/")}'
    chdir: "{{ __sap_install_media_detect_software_main_directory }}"
  register: __sap_install_media_detect_register_subdirectories_phase_1b
  changed_when: false
  failed_when: false
remrozk commented 3 months ago

@sean-freeman I made additional tests.

  1. The when condition should be:

    when: loop_var is undefined or loop_var == ""

    Otherwise, it never matches.

  2. Unfortunately, the when condition loop_var == "" cannot work :confused:. Host facts have a low precedence than extra-vars, if we set the sap_swpm_db_schema_abap_password: "", even if the fact will be set, the sap_swpm_db_schema_abap_password will stay empty.

Below a run example with the sap_swpm_db_schema_abap_password defined, but empty:

root@7f4f7026b9c8:/ansible# ansible-playbook -v playbooks/test.yml -e sap_swpm_db_schema_abap_password="" -e sap_swpm_master_password="master_password"

PLAY [Playbook - SAP installation] ********************************************************************************************************************

TASK [Debug] ******************************************************************************************************************************************
ok: [serv] => {
    "msg": "sap_swpm_db_schema_abap_password: "
}

TASK [SAP SWPM Pre Install - Set fact for any blank passwords to use master password] *****************************************************************
ok: [server1] => (item=sap_swpm_db_schema_abap_password) => {"ansible_facts": {"sap_swpm_db_schema_abap_password": "master_password"}, 
"ansible_loop_var": "empty_password_var", "changed": false, "empty_password_var": "sap_swpm_db_schema_abap_password"}

TASK [Debug] ******************************************************************************************************************************************
ok: [server1] => {
    "msg": "sap_swpm_db_schema_abap_password: "
}

PLAY RECAP ********************************************************************************************************************************************
server1                : ok=3    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

Below a run example with the sap_swpm_db_schema_abap_password not defined:

root@7f4f7026b9c8:/ansible# ansible-playbook -v playbooks/test.yml -e sap_swpm_master_password="master_password"

PLAY [Playbook - SAP installation] *****************************************************************************

TASK [SAP SWPM Pre Install - Set fact for any blank passwords to use master password] **************************
ok: [server1] => (item=sap_swpm_db_schema_abap_password) => {"ansible_facts": {"sap_swpm_db_schema_abap_password": "master_password"}, 
"ansible_loop_var": "empty_password_var", "changed": false, "empty_password_var": "sap_swpm_db_schema_abap_password"}

TASK [Debug] ***************************************************************************************************
ok: [server1] => {
    "msg": "sap_swpm_db_schema_abap_password: master_password"
}

PLAY RECAP *****************************************************************************************************
server1                : ok=2    changed=0    unreachable=0    failed=0    skipped=1    rescued=0    ignored=0

The content of playbooks/test.yml

- name: Playbook - SAP installation
  hosts: all
  become: true
  gather_facts: false
  any_errors_fatal: false
  tasks:
    - name: Debug
      ansible.builtin.debug:
        msg: "sap_swpm_db_schema_abap_password: {{ sap_swpm_db_schema_abap_password }}"
      when: sap_swpm_db_schema_abap_password is defined

    - name: SAP SWPM Pre Install - Set fact for any blank passwords to use master password
      ansible.builtin.set_fact:
        "{{ empty_password_var }}": "{{ sap_swpm_master_password }}"
      loop:
        - sap_swpm_db_schema_abap_password
      loop_control:
        loop_var: empty_password_var
      when: loop_var is undefined or loop_var == ""

    - name: Debug
      ansible.builtin.debug:
        msg: "sap_swpm_db_schema_abap_password: {{ sap_swpm_db_schema_abap_password }}"
      when: sap_swpm_db_schema_abap_password is defined