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: [sc-110727] troubleshoot: collector/analyzer for wildcard dns #1606

Merged
merged 6 commits into from
Sep 11, 2024

Conversation

nvanthao
Copy link
Member

@nvanthao nvanthao commented Sep 2, 2024

Story details: https://app.shortcut.com/replicated/story/110727

Demo: https://asciinema.org/a/C9TJdETq9Hn21jefJaxSnWyga

Updates in existing DNS collector

  • use dig instead of nslookup
  • test DNS resolution for a non resolvable domain
  • make DNS utility image configurable, default to registry.k8s.io/e2e-test-images/jessie-dnsutils:1.3
  • add nonResolveable config for non-resolvable domain, defaulted to non-existent-domain

Current JSON output that can be used with others analyzer such as JSON analyzer

{
  "kubernetesClusterIP": "10.43.0.1",
  "podResolvConf": "search default.svc.cluster.local svc.cluster.local cluster.local\nnameserver 10.43.0.10\noptions ndots:5\n",
  "query": {
    "kubernetes": {
      "name": "kubernetes",
      "address": "10.43.0.1"
    },
    "nonResolvableDomain": {
      "name": "foo.bar.xyz",
      "address": ""
    }
  },
  "kubeDNSPods": [
    "coredns-77ccd57875-76dt4"
  ],
  "kubeDNSService": "10.43.0.10",
  "kubeDNSEndpoints": "10.42.0.6:53"
}

Sample YAML spec

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
metadata:
  name: support-bundle
spec:
  collectors:
    - clusterResources:
        exclude: true
    - dns:
        image: registry.k8s.io/e2e-test-images/jessie-dnsutils:1.3
        nonResolvable: foo.bar.xyz 
  analyzers:
    - jsonCompare:
        checkName: Check for wildcard DNS config
        fileName: dns/debug.json
        path: "query.nonResolvableDomain.address"
        value: |
          ""
        outcomes:
          - fail:
              when: "false"
              message: Possible wildcard DNS configured. Non-existent domain resolved to {{ .query.nonResolvableDomain.address }}.
          - pass:
              when: "true"
              message: Confirmed that non-existent domain not resolved.

@nvanthao nvanthao added the type::feature New feature or request label Sep 2, 2024
@nvanthao nvanthao requested a review from a team as a code owner September 2, 2024 04:49
pkg/collect/dns.go Show resolved Hide resolved
pkg/collect/dns.go Outdated Show resolved Hide resolved
namespace := "default"
command := []string{"/bin/sh", "-c", `
set -x
command := []string{"/bin/sh", "-c", fmt.Sprintf(`
Copy link
Member

@banjoh banjoh Sep 3, 2024

Choose a reason for hiding this comment

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

Is there a reason why you switched from nslookup to dig? People will need find an image that has dig installed which is an additional hurdle to jump. Images such as busybox and alpine package nslookup. Embedded Cluster uses busybox which, if not overridden, will be present in the airgap bundle

It might be slightly more code, but you can attempt dig and if the binary is not present, try nslookup. The collector will be more robust this way.

Copy link
Member Author

@nvanthao nvanthao Sep 4, 2024

Choose a reason for hiding this comment

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

The nslookup command in busybox is not working correctly

E.g.

k run -it --rm debug --image busybox -- /bin/sh
If you don't see a command prompt, try pressing enter.
/ #
/ # nslookup kubernetes.default
Server:         10.43.0.10
Address:        10.43.0.10:53

** server can't find kubernetes.default: NXDOMAIN

I've changed to dig simply because of the base image has dig installed, we only want to resolve an address, and the dig +short is easier to parse.

Copy link
Member

Choose a reason for hiding this comment

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

Could you please add this as a comment in the code for future context on the technical choice?

Copy link
Member Author

Choose a reason for hiding this comment

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

many thanks @banjoh! I'll update the docs accordingly as well.

@nvanthao nvanthao requested a review from banjoh September 4, 2024 03:02
@nvanthao nvanthao force-pushed the gerard/sc-110727/wildcard-dns branch from edb554c to 36c03da Compare September 9, 2024 00:11
Copy link
Member

@banjoh banjoh left a comment

Choose a reason for hiding this comment

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

LGTM

@nvanthao nvanthao merged commit 7484b10 into main Sep 11, 2024
27 checks passed
@nvanthao nvanthao deleted the gerard/sc-110727/wildcard-dns branch September 11, 2024 04:35
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.

2 participants