feat(preflights): add host dns wildcard check #1210

nvanthao · 2024-09-20T16:10:56Z

What this PR does / why we need it:

Add Host DNS Wildcard preflight check

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

Which issue(s) this PR fixes:

Does this PR require a test?

NONE

Does this PR require a release note?

NONE

Does this PR require documentation?

NONE

github-actions · 2024-09-20T17:13:09Z

This PR has been released (on staging) and is available for download with a embedded-cluster-smoke-test-staging-app license ID.

Online Installer:

curl "https://staging.replicated.app/embedded/embedded-cluster-smoke-test-staging-app/ci/appver-dev-2669a7a" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Airgap Installer (may take a few minutes before the airgap bundle is built):

curl "https://staging.replicated.app/embedded/embedded-cluster-smoke-test-staging-app/ci-airgap/appver-dev-2669a7a?airgap=true" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Happy debugging!

chris-sanders · 2024-09-23T19:05:32Z

@nvanthao please do not run linting on a code base in the some change as your actual change. This PR has several changes from " to ' which are not related to this in any way. It's also undesirable to have every person's personal inter changing things back and forth on every PR causing unnecessary noise in the change history.

If you can undo your linting and just submit the changed portions.

ajp-io · 2024-09-23T20:44:09Z

pkg/preflights/host-preflight.yaml

+        outcomes:
+          - fail:
+              when: 'false'
+              message: 'Possible wildcard DNS entry detected at: {{ "{{" }}  .resolvedFromSearch {{ "}}" }} . Remove the search domain OR remove the wildcard DNS entry.'


I'm not going to write a code suggestion because I can't test it easily, but here is my desired output (open to suggestions too):

Wildcard DNS entry *.foo.testcluster.net detected. Remove the wildcard DNS entry or the foo.testcluster.net search domain from resolv.conf.

ajp-io · 2024-09-23T20:50:10Z

pkg/preflights/host-preflight.yaml

@@ -367,17 +371,30 @@ spec:
        regex: 'nameserver\s*(localhost|127\.0\.0\.1)'
        outcomes:
          - fail:
-              when: "true"
+              when: 'true'
              message: "Neither 'nameserver localhost' nor 'nameserver 127.0.0.1' can be present in resolv.conf. Remove them to continue."


Makes me wonder if we should reword this one a bit to match more closely. If this wording looks right.

Suggested change

message: "Neither 'nameserver localhost' nor 'nameserver 127.0.0.1' can be present in resolv.conf. Remove them to continue."

message: "Local DNS resolver detected. Remove the localhost and/or 127.0.0.1 nameserver entries from resolv.conf."

nvanthao · 2024-09-25T03:17:10Z

@nvanthao please do not run linting on a code base in the some change as your actual change. This PR has several changes from " to ' which are not related to this in any way. It's also undesirable to have every person's personal inter changing things back and forth on every PR causing unnecessary noise in the change history.

If you can undo your linting and just submit the changed portions.

Hi @chris-sanders, I've made the request changes. Please let me know how it goes.

chris-sanders

That addresses my comments I'm good now.

ajp-io · 2024-09-25T16:20:42Z

I just ran this, and I think there are a few issues with the templating. It looks like this:

Wildcard DNS entry .foo.testcluster.net. detected. Remove the wildcard DNS entry or the .foo.testcluster.net. search domain from resolv.conf.

The {{ "{{" }} .resolvedFromSearch {{ "}}" }} templating results in a period at the end, which seems incorrect. You end up with a period in the middle of the sentence too ("entry .foo.testcluster.net. detected").
That templating also puts a period at the beginning, which is fine for the wildcard DNS entry but is wrong for the search domain (because the search domain is foo.testcluster.net, not .foo.testcluster.net).

The quickest fix might be to remove the period from the end of the templating (that just seems wrong to me), and then remove the templating from the second sentence. It would be nice if we could be specific about which search domain to remove, but I don't want to say it with a period at the beginning if that's not what will be in resolv.conf.

nvanthao · 2024-09-26T05:17:50Z

The .resolvedFromSearch field is coming from here https://github.com/replicatedhq/troubleshoot/blob/e97b9613a55a727a99ac40cbc0b041794ccb13b4/pkg/collect/host_dns.go#L178-L180

I will create a seperate PR in troubleshoot repo to trim the prefix and suffix .

That being said, please let me know if there's any changes required in this PR.

nvanthao · 2024-09-30T01:22:31Z

PR created and merged in troubleshoot repo replicatedhq/troubleshoot#1629

nvanthao · 2024-10-04T00:33:00Z

Hi @ajp-io, could you help me with the review?

ajp-io · 2024-10-04T18:21:25Z

Can you show me a screenshot of what a failure looks like?

nvanthao · 2024-10-07T01:46:17Z

Failure screenshot

ajp-io · 2024-10-07T12:53:27Z

pkg/preflights/host-preflight.yaml

+        outcomes:
+          - fail:
+              when: 'false'
+              message: 'Wildcard DNS entry {{ "{{" }}  .resolvedFromSearch {{ "}}" }} detected. Remove the wildcard DNS entry or the {{ "{{" }}  .resolvedFromSearch {{ "}}" }} search domain from resolv.conf.'


The wildcard DNS entry would look like this, right?

Suggested change

message: 'Wildcard DNS entry {{ "{{" }} .resolvedFromSearch {{ "}}" }} detected. Remove the wildcard DNS entry or the {{ "{{" }} .resolvedFromSearch {{ "}}" }} search domain from resolv.conf.'

message: 'Wildcard DNS entry *.{{ "{{" }} .resolvedFromSearch {{ "}}" }} detected. Remove the wildcard DNS entry or the {{ "{{" }} .resolvedFromSearch {{ "}}" }} search domain from resolv.conf.'

Gotcha, I've updated the text accordingly.

Looks like you did it backwards. The asterisk would be before the period. *.example.com

Apologies, it's updated now.

nvanthao added the type::feature label Sep 20, 2024

chris-sanders requested review from laverya and emosbaugh September 23, 2024 19:02

ajp-io reviewed Sep 23, 2024

View reviewed changes

nvanthao force-pushed the gerard/host-preflight-dns branch from 78bb11f to ebfe34c Compare September 25, 2024 03:00

nvanthao requested a review from ajp-io September 25, 2024 03:16

chris-sanders previously approved these changes Sep 25, 2024

View reviewed changes

nvanthao dismissed chris-sanders’s stale review via a2917ab September 27, 2024 06:45

nvanthao force-pushed the gerard/host-preflight-dns branch from 6ab57c9 to a2917ab Compare September 27, 2024 06:45

nvanthao mentioned this pull request Sep 30, 2024

change resolvedFromSearch content replicatedhq/troubleshoot#1629

Merged

6 tasks

nvanthao closed this in replicatedhq/troubleshoot#1629 Sep 30, 2024

nvanthao reopened this Sep 30, 2024

nvanthao force-pushed the gerard/host-preflight-dns branch 2 times, most recently from 01d6acb to b216bd7 Compare October 4, 2024 00:30

nvanthao force-pushed the gerard/host-preflight-dns branch from b216bd7 to 2901c1c Compare October 7, 2024 01:35

ajp-io reviewed Oct 7, 2024

View reviewed changes

nvanthao force-pushed the gerard/host-preflight-dns branch from 2901c1c to 80de68a Compare October 8, 2024 03:36

nvanthao requested a review from ajp-io October 8, 2024 03:55

nvanthao force-pushed the gerard/host-preflight-dns branch from 111f790 to d44025e Compare October 9, 2024 03:07

ajp-io previously approved these changes Oct 9, 2024

View reviewed changes

add host dns wildcard check

46dca80

nvanthao added 4 commits October 14, 2024 08:37

update from code review

18e613a

rebase

666f751

update message

b5d5405

update message

2669a7a

nvanthao dismissed ajp-io’s stale review via 2669a7a October 13, 2024 21:38

nvanthao force-pushed the gerard/host-preflight-dns branch from 5bc4e75 to 2669a7a Compare October 13, 2024 21:38

laverya approved these changes Oct 14, 2024

View reviewed changes

laverya merged commit d39db4a into main Oct 14, 2024
57 checks passed

laverya deleted the gerard/host-preflight-dns branch October 14, 2024 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(preflights): add host dns wildcard check #1210

feat(preflights): add host dns wildcard check #1210

nvanthao commented Sep 20, 2024

github-actions bot commented Sep 20, 2024 •

edited

Loading

chris-sanders commented Sep 23, 2024

ajp-io Sep 23, 2024

ajp-io Sep 23, 2024

nvanthao commented Sep 25, 2024

chris-sanders left a comment

ajp-io commented Sep 25, 2024

nvanthao commented Sep 26, 2024

nvanthao commented Sep 30, 2024

nvanthao commented Oct 4, 2024

ajp-io commented Oct 4, 2024

nvanthao commented Oct 7, 2024

ajp-io Oct 7, 2024

nvanthao Oct 8, 2024

ajp-io Oct 8, 2024

nvanthao Oct 9, 2024

	message: "Neither 'nameserver localhost' nor 'nameserver 127.0.0.1' can be present in resolv.conf. Remove them to continue."
	message: "Local DNS resolver detected. Remove the localhost and/or 127.0.0.1 nameserver entries from resolv.conf."

	message: 'Wildcard DNS entry {{ "{{" }} .resolvedFromSearch {{ "}}" }} detected. Remove the wildcard DNS entry or the {{ "{{" }} .resolvedFromSearch {{ "}}" }} search domain from resolv.conf.'
	message: 'Wildcard DNS entry *.{{ "{{" }} .resolvedFromSearch {{ "}}" }} detected. Remove the wildcard DNS entry or the {{ "{{" }} .resolvedFromSearch {{ "}}" }} search domain from resolv.conf.'

feat(preflights): add host dns wildcard check #1210

feat(preflights): add host dns wildcard check #1210

Conversation

nvanthao commented Sep 20, 2024

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR require a test?

Does this PR require a release note?

Does this PR require documentation?

github-actions bot commented Sep 20, 2024 • edited Loading

chris-sanders commented Sep 23, 2024

ajp-io Sep 23, 2024

Choose a reason for hiding this comment

ajp-io Sep 23, 2024

Choose a reason for hiding this comment

nvanthao commented Sep 25, 2024

chris-sanders left a comment

Choose a reason for hiding this comment

ajp-io commented Sep 25, 2024

nvanthao commented Sep 26, 2024

nvanthao commented Sep 30, 2024

nvanthao commented Oct 4, 2024

ajp-io commented Oct 4, 2024

nvanthao commented Oct 7, 2024

ajp-io Oct 7, 2024

Choose a reason for hiding this comment

nvanthao Oct 8, 2024

Choose a reason for hiding this comment

ajp-io Oct 8, 2024

Choose a reason for hiding this comment

nvanthao Oct 9, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 20, 2024 •

edited

Loading