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

feat: host collector for DNS #1617

Merged
merged 14 commits into from
Sep 19, 2024
Merged

feat: host collector for DNS #1617

merged 14 commits into from
Sep 19, 2024

Conversation

nvanthao
Copy link
Member

@nvanthao nvanthao commented Sep 18, 2024

Description, Motivation and Context

New host collector DNS that will run DNS query to various domains. Will help with DNS issue such as detect wildcard DNS used.

Fixes: https://app.shortcut.com/replicated/story/112277/analyse-search-domains-with-wildcard-dns-records-on-hosts
Demo: https://asciinema.org/a/Ma1i4T0xp50ODNswrfeHTrOTo

Sample spec

kind: SupportBundle
metadata:
  name: sample
spec:
  hostCollectors:
    - dns:
        collectorName: "wildcard-check"
        hostnames:
          - "*"
  analyzers:
    - jsonCompare:
        checkName: Detect wildcard DNS
        fileName: host-collectors/dns/wildcard-check/result.json
        path: "resolvedFromSearch"
        value: |
          ""
        outcomes:
          - fail:
              when: "false"
              message: "Possible wildcard DNS detected at: {{ .resolvedFromSearch }}. Please remove the search domain OR remove the wildcard DNS entry."
          - pass:
              when: "true"
              message: No wildcard DNS detected.

Sample output when there's wildcard DNS configured

{
  "query": {
    "*": [
      {
        "server": "127.0.0.53",
        "search": ".foo.testcluster.net.",
        "name": "*.foo.testcluster.net.",
        "answer": "*.foo.testcluster.net.\t60\tIN\tA\t192.1.2.3",
        "record": "192.1.2.3"
      },
      {
        "server": "127.0.0.53",
        "search": ".c.replicated-qa.internal.",
        "name": "*.c.replicated-qa.internal.",
        "answer": "",
        "record": ""
      },
      {
        "server": "127.0.0.53",
        "search": ".google.internal.",
        "name": "*.google.internal.",
        "answer": "",
        "record": ""
      },
      {
        "server": "127.0.0.53",
        "search": ".",
        "name": "*.",
        "answer": "",
        "record": ""
      }
    ]
  },
  "resolvedFromSearch": ".foo.testcluster.net."
}

Checklist

  • New and existing tests pass locally with introduced changes.
  • Tests for the changes have been added (for bug fixes / features)
  • The commit message(s) are informative and highlight any breaking changes
  • Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here

Does this PR introduce a breaking change?

  • Yes
  • No

@nvanthao nvanthao added the type::feature New feature or request label Sep 18, 2024
@nvanthao nvanthao requested a review from a team as a code owner September 18, 2024 13:10
}
defer file.Close()

config, err := dns.ClientConfigFromFile(file.Name())
Copy link
Member

Choose a reason for hiding this comment

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

Should we have an option on the collector to specify ndots, and then if its set update the DNS config here to set that value?

https://github.com/miekg/dns/blob/master/clientconfig.go#L12-L19

Copy link
Member

Choose a reason for hiding this comment

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

ndots is a good example, but anywhere someone might want to override the config from the collector could be useful to expose.

Copy link
Member

Choose a reason for hiding this comment

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

Looking further, since you're iterating and appending every search domain found on the host to see if one of them resolves something we suspect it shouldn't, then the ndots stuff probably isn't needed? So whether you decide to add that or not completely up to you at this stage.

pkg/apis/troubleshoot/v1beta2/hostcollector_shared.go Outdated Show resolved Hide resolved
pkg/collect/host_dns.go Outdated Show resolved Hide resolved
@nvanthao nvanthao merged commit 8823f7d into main Sep 19, 2024
27 checks passed
@nvanthao nvanthao deleted the gerard/f-host-dns branch September 19, 2024 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type::feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants