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

Add getnameinfo as a fall through case for fqdn resolution #1810

Merged
merged 11 commits into from
Oct 4, 2023

Conversation

tpowell-progress
Copy link
Contributor

@tpowell-progress tpowell-progress commented Oct 3, 2023

Description

node['fqdn'] missing the DNS Suffix on windows nodes

If canonname includes multiple parts ('.'), then assume that that is a FQDN and return.

Otherwise, use getnameinfo as a fallback. If that return value is not an IP, return it.

If all else fails, just return what we originally received as an argument.

Related Issue

Internal issue: CHEF-605

Broken in PR #1705

Fixes #1733

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
@tpowell-progress tpowell-progress marked this pull request as ready for review October 3, 2023 16:42
@tpowell-progress tpowell-progress requested review from a team as code owners October 3, 2023 16:42
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Copy link
Collaborator

@jaymzh jaymzh left a comment

Choose a reason for hiding this comment

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

how's this?

lib/ohai/mixin/network_helper.rb Outdated Show resolved Hide resolved
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
lib/ohai/mixin/network_helper.rb Show resolved Hide resolved
lib/ohai/mixin/network_helper.rb Outdated Show resolved Hide resolved
lib/ohai/mixin/network_helper.rb Show resolved Hide resolved
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Copy link
Collaborator

@jaymzh jaymzh left a comment

Choose a reason for hiding this comment

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

one more minor fix

Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
Signed-off-by: Thomas Powell <thomas.powell@progress.com>
@sonarcloud
Copy link

sonarcloud bot commented Oct 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@tpowell-progress tpowell-progress changed the title Change from canonname -> getnameinfo Add getnameinfo as a fall through case for fqdn resolution Oct 4, 2023
@johnmccrae
Copy link
Contributor

johnmccrae commented Oct 4, 2023

On non-domain joined devices, there is no FQDN

Important

By default the computer name of a computer that is not joined to a domain is a host name, and not a fully qualified domain name (FQDN). Topology Builder uses FQDNs, not host names. So, you must configure a DNS suffix on the name of the computer to be deployed as an Edge Server that is not joined to a domain. Use only standard characters (including A-Z, a-z, 0-9, and hyphens) when assigning FQDNs to your servers running Skype for Business Server. Do not use Unicode characters or underscores. Nonstandard characters in an FQDN are often not supported by external DNS and public CAs (that is, when the FQDN must be assigned to the SN in the certificate).

Source - Microsoft

@Stromweld
Copy link

That's M$ narrow minded attempt to say that you have to be joined to an active directory domain and should be using their AD managed DNS. In reality if there is a DNS server defining a domain and a host entry for it, then there is a FQDN for a host It's just being managed by other means.

@jaymzh
Copy link
Collaborator

jaymzh commented Oct 4, 2023

On non-domain joined devices, there is no FQDN

Important

By default the computer name of a computer that is not joined to a domain is a host name, and not a fully qualified domain name (FQDN). Topology Builder uses FQDNs, not host names. So, you must configure a DNS suffix on the name of the computer to be deployed as an Edge Server that is not joined to a domain. Use only standard characters (including A-Z, a-z, 0-9, and hyphens) when assigning FQDNs to your servers running Skype for Business Server. Do not use Unicode characters or underscores. Nonstandard characters in an FQDN are often not supported by external DNS and public CAs (that is, when the FQDN must be assigned to the SN in the certificate).

Source - Microsoft

That's just garbage. That's only true in the "Windows Domain" sense, not in the "real world of networks" sense. If you have a DNS name pointing at you from a domain, you have an FQDN.

@tpowell-progress
Copy link
Contributor Author

I'm going forward with this. The fall through handles the (mostly Windows-specific) use case, restoring the previous functionality there. If all else fails we'll return the original hostname.

@tpowell-progress tpowell-progress merged commit 2974dcb into main Oct 4, 2023
28 of 29 checks passed
@tpowell-progress tpowell-progress deleted the tp/chef-605-fqdn-fix branch October 4, 2023 23:11
@tpowell-progress tpowell-progress restored the tp/chef-605-fqdn-fix branch October 5, 2023 18:58
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.

node['fqdn'] missing the DNS Suffix on windows nodes
4 participants