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

sap_swpm: manage sap_swpm_db_schema_abap_password in hdbuserstore #802

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

remrozk
Copy link

@remrozk remrozk commented Jul 11, 2024

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
Copy link
Member

@rob0d @sean-freeman @remrozk After merging #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
Copy link
Member

@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
Copy link
Contributor

rob0d commented Jul 17, 2024

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
Copy link
Author

remrozk commented Jul 22, 2024

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
Copy link
Member

@rob0d @remrozk

  1. See sap_swpm: hdbuserstore default entry changed incorrectly in the post installation steps #747 (comment) , repeating here for ease "Original code is from PR sap_swpm: fix post-execution update of the hdbuserstore with correct host name #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
Copy link
Member

sean-freeman commented Jul 22, 2024

@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
Copy link
Author

remrozk commented Jul 23, 2024

@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
Copy link
Member

@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 #748 . Which is why there is a Merge Conflict in this PR

@rob0d
Copy link
Contributor

rob0d commented Jul 24, 2024

Hi @sean-freeman.

  1. It's difficult to prove it now and I don't mean any disrespect to the person who raised sap_swpm: fix post-execution update of the hdbuserstore with correct host name #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 sap_swpm: hdbuserstore default connection should use sap_swpm_db_schema_abap_password #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
Copy link
Author

remrozk commented Jul 25, 2024

@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
Copy link
Author

remrozk commented Jul 25, 2024

@sean-freeman I made additional tests.

  1. The when condition should be:
when: loop_var is undefined or loop_var == ""

Otherwise, it never matches.

  1. Unfortunately, the when condition loop_var == "" cannot work 😕. 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

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.

4 participants