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

[deployments-k8s#2412] Fix DNS #1054

Conversation

Bolodya1997
Copy link

@Bolodya1997 Bolodya1997 commented Aug 5, 2021

Description

Fixes DNS logic:

Old logic:

resolv.conf
---
nameserver a.a.a.a -> nameserver <default nameserver>
...
Corefile
---
. {
    forward . a.a.a.a
    log
    reload
}
...

This is incorrect, because:

  1. User cannot both use and DNS configs passed from Endpoints, because there is no 127.0.0.1 nameserver entry in resolv.conf in such case.
  2. In cmd-nsc-init + cmd-nsc case we have a.a.a.a == 127.0.0.1 at the cmd-nsc startup, so we have such files:
resolv.conf
---
nameserver 127.0.0.1
...
Corefile
---
. {
    forward . 127.0.0.1
    log
    reload
}
...

In such case we have no access to any OS default NS servers (we even cannot ping google.com) and it leads to loop DNS queries.

New logic:

resolv.conf
---
nameserver 127.0.0.1
nameserver a.a.a.a
...
Corefile
---
. {
    forward . <default nameserver> # <-- or just without this line if it is not set
    log
    reload
}
...

Issue link

Closes networkservicemesh/deployments-k8s#2412.

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

Types of changes

  • Bug fix
  • New functionallity
  • Documentation
  • Refactoring
  • CI

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Bolodya1997 Bolodya1997 force-pushed the deployments-k8s#2412/resolv-conf branch from 9919744 to 871b196 Compare August 6, 2021 03:23
@@ -73,7 +73,7 @@ func Test_DNSUsecase(t *testing.T) {
err = ioutil.WriteFile(resolveConfigPath, []byte("nameserver 8.8.4.4\nsearch example.com\n"), os.ModePerm)
require.NoError(t, err)

const expectedCorefile = ". {\n\tforward . 8.8.4.4\n\tlog\n\treload\n}\nmy.domain1 {\n\tfanout . 8.8.4.4 8.8.8.8\n\tlog\n}"
const expectedCorefile = ". {\n\tlog\n\treload\n}\nmy.domain1 {\n\tfanout . 8.8.4.4 8.8.8.8\n\tlog\n}\n"
Copy link
Member

@denis-tingaikin denis-tingaikin Aug 8, 2021

Choose a reason for hiding this comment

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

This looks wrong.

As I can see you're ignoring queries for any domains and it is totally incorrect.

Copy link
Author

Choose a reason for hiding this comment

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

Here are such cases:

  • Do we have some nameserver entry in resolv.conf before running NSM client?
  • Do we have some default name server set in NSM client options?
  1. No (nameserver), no (default name server) - such container initially has no name server, we are adding name servers only for NSM resources, other resources like google.com are not accessible by domain name:
resolv.conf
---
nameserver 127.0.0.1
...
Corefile
---
. {
    log
    reload # to reload coredns on Corefile changes
}
my-coredns-service . {
    ...
}
...
  1. No (nameserver), yes (default name server) - such container initially has no name server, we are adding name servers for NSM resources + default name server for other resources like google.com.
resolv.conf
---
nameserver 127.0.0.1
...
Corefile
---
. {
    forward 1.1.1.1
    log
    reload
}
my-coredns-service . {
    ...
}
...
  1. Yes (nameserver), no (default name server) - such container initially has a name server, so it should still have access to google.com after all NSM configurations made.
resolv.conf
---
127.0.0.1  # <-- first tries this
10.96.0.10 # <-- after tries this
...
Corefile
---
. {
    log
    reload
}
my-coredns-service . {
    ...
}
...
  1. Yes (nameserver), yes (default name server) - such container initially has a name server, but we want to additionally configure it with NSM for using another name server as default.
resolv.conf
---
127.0.0.1
10.96.0.10
...
Corefile
---
. {
    forward 1.1.1.1
    log
    reload
}
my-coredns-service . {
    ...
}
...

@denis-tingaikin denis-tingaikin merged commit 31cb2ac into networkservicemesh:main Aug 9, 2021
nsmbot pushed a commit to networkservicemesh/cmd-nsc-init that referenced this pull request Aug 9, 2021
…k@main

PR link: networkservicemesh/sdk#1054

Commit: 31cb2ac
Author: Vladimir Popov
Date: 2021-08-09 15:02:54 +0700
Message:
  - Fix DNS (#1054)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-kernel that referenced this pull request Aug 9, 2021
…k@main

PR link: networkservicemesh/sdk#1054

Commit: 31cb2ac
Author: Vladimir Popov
Date: 2021-08-09 15:02:54 +0700
Message:
  - Fix DNS (#1054)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr that referenced this pull request Aug 9, 2021
…k@main

PR link: networkservicemesh/sdk#1054

Commit: 31cb2ac
Author: Vladimir Popov
Date: 2021-08-09 15:02:54 +0700
Message:
  - Fix DNS (#1054)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-proxy-dns that referenced this pull request Aug 9, 2021
…k@main

PR link: networkservicemesh/sdk#1054

Commit: 31cb2ac
Author: Vladimir Popov
Date: 2021-08-09 15:02:54 +0700
Message:
  - Fix DNS (#1054)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-vfio that referenced this pull request Aug 9, 2021
…k@main

PR link: networkservicemesh/sdk#1054

Commit: 31cb2ac
Author: Vladimir Popov
Date: 2021-08-09 15:02:54 +0700
Message:
  - Fix DNS (#1054)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-icmp-responder that referenced this pull request Aug 9, 2021
…k@main

PR link: networkservicemesh/sdk#1054

Commit: 31cb2ac
Author: Vladimir Popov
Date: 2021-08-09 15:02:54 +0700
Message:
  - Fix DNS (#1054)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-k8s that referenced this pull request Aug 9, 2021
…k@main

PR link: networkservicemesh/sdk#1054

Commit: 31cb2ac
Author: Vladimir Popov
Date: 2021-08-09 15:02:54 +0700
Message:
  - Fix DNS (#1054)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr-proxy that referenced this pull request Aug 9, 2021
…k@main

PR link: networkservicemesh/sdk#1054

Commit: 31cb2ac
Author: Vladimir Popov
Date: 2021-08-09 15:02:54 +0700
Message:
  - Fix DNS (#1054)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-memory that referenced this pull request Aug 9, 2021
…k@main

PR link: networkservicemesh/sdk#1054

Commit: 31cb2ac
Author: Vladimir Popov
Date: 2021-08-09 15:02:54 +0700
Message:
  - Fix DNS (#1054)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
denis-tingaikin added a commit to denis-tingaikin/sdk that referenced this pull request May 12, 2022
denis-tingaikin added a commit to denis-tingaikin/sdk that referenced this pull request May 12, 2022
Signed-off-by: Denis Tingaikin <denis.tingajkin@xored.com>
edwarnicke pushed a commit that referenced this pull request May 13, 2022
* Revert "Fix DNS (#1054)"

Signed-off-by: Denis Tingaikin <denis.tingajkin@xored.com>

* fix ci fail

Signed-off-by: Denis Tingaikin <denis.tingajkin@xored.com>

* rework #1070

Signed-off-by: Denis Tingaikin <denis.tingajkin@xored.com>
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.

TestRunFeatureSuite/TestDns test is unstable
3 participants