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_general_preconfigure: Don't ignore DNS A/PTR record inconsistencies, this should fail the check is negative #765

Merged
merged 7 commits into from
Jun 26, 2024

Conversation

rob0d
Copy link
Contributor

@rob0d rob0d commented Jun 20, 2024

Also included a minor task description change to make it clearer what has failed.

@sean-freeman sean-freeman changed the title Don't ignore DNS A/PTR record inconsistencies, this should fail the check is negative sap_general_preconfigure: Don't ignore DNS A/PTR record inconsistencies, this should fail the check is negative Jun 20, 2024
@marcelmamula marcelmamula requested a review from berndfinger June 21, 2024 09:03
collection: merge dev to main for release 1.4.1
@sean-freeman
Copy link
Member

Would suggest the Ansible Tasks are updated as suggested in #743 to correctly indicate to the end-user, that dig runs from the host to check the DNS Resolver contains the A/PTR records.

  • Check DNS Resolver in resolv.conf contains A Record for host
  • Check DNS Resolver in resolv.conf contains reverse PTR Record for host

Otherwise I'd say this PR is an easy one to merge 👍

@berndfinger berndfinger self-assigned this Jun 26, 2024
@berndfinger
Copy link
Member

Would suggest the Ansible Tasks are updated as suggested in #743 to correctly indicate to the end-user, that dig runs from the host to check the DNS Resolver contains the A/PTR records.

  • Check DNS Resolver in resolv.conf contains A Record for host
  • Check DNS Resolver in resolv.conf contains reverse PTR Record for host

We should not include one specific DNS name resolution method because there are other methods which do not rely on an entry in /etc/resolv.conf. But I agree that the task names can be more detailed, as in:

  • Verify the correct DNS hostname to IP address resolution (A record) using the FQDN
  • Verify the correct DNS hostname to IP address resolution (A record) using the hostname and the search list
  • Verify the correct DNS IP address to FQDN resolution (PTR record)

Just for the records: This check can be skipped by using the tag sap_general_preconfigure_dns_name_resolution in --skip-tags, so there should be no need to add another role parameter for this purpose.

ignore_errors: true
#ignore_errors: true
register: __sap_general_preconfigure_register_dns_test_a
failed_when: __sap_general_preconfigure_register_dns_test_a.rc != 0

### BUG: dig does not use search path in resolv.con on PPCle
Copy link
Member

Choose a reason for hiding this comment

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

typo: resolv.con -> resolv.conf

@rob0d
Copy link
Contributor Author

rob0d commented Jun 26, 2024

@berndfinger @sean-freeman I've updated it again to improve the text based on the latest recommendation from both of you. Is this acceptable now?

Copy link
Member

@sean-freeman sean-freeman left a comment

Choose a reason for hiding this comment

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

lgtm 👍 Would prefer mention of DNS Resolver is what is being verified, but I'm not going to hold up a simple fix for such acute purposes

Copy link
Member

@berndfinger berndfinger left a comment

Choose a reason for hiding this comment

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

LGTM

@berndfinger berndfinger merged commit 8c760ad into sap-linuxlab:dev Jun 26, 2024
3 of 4 checks passed
@rob0d rob0d deleted the dns_check branch June 29, 2024 10:28
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.

3 participants