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

Standardize config files #1084

Open
cob-web-corner opened this issue Oct 21, 2024 · 15 comments
Open

Standardize config files #1084

cob-web-corner opened this issue Oct 21, 2024 · 15 comments

Comments

@cob-web-corner
Copy link

cob-web-corner commented Oct 21, 2024

Hello,

This is a rather large ask but would make things much easier for parsing, IaC, clustering, dynamic setup, provisioning from config files, Kubernetes config map, docker configuration, etc. Currently the config files located in /etc/dns are of an arbitrary nature. It would be nice to have these in some sort of standardized format that could be statically typed and parsed to classes (i.e. yaml/json). It would additionally significantly reduce your config file writers/readers and map to classes that are already defined.

Some of the apps are already standardized (dnsApp.config for example)

@ShreyasZare
Copy link
Member

Thanks for the post. I have had this thought already but the task will need a lot of changes since entire code base is designed to serialize/de-serialize data in binary formats. From development perspective, the readers/writers are not much of an issue since changes are incremental. The original idea to have binary format was to discourage manual editing of the config so that the HTTP API is preferred instead. That was opinionated design decision so that the DNS server can do proper validation with HTTP API and also apply those changes immediately without need to reload/restart like with usual daemons.

@cob-web-corner
Copy link
Author

I have had this thought already but the task will need a lot of changes since entire code base is designed to serialize/de-serialize data in binary formats

Yeah when I took a quick look through the code on the config read/writer side it definitely seemed like it would be an effort

That was opinionated design decision so that the DNS server can do proper validation with HTTP API and also apply those changes immediately without need to reload/restart like with usual daemons.

In this case those classes that do the validation could be abstracted to also work when loading the config files

Totally understood overall it would be a large effort. My worker around right now is to load a scratch instance, inject everything through the API and then grab the generated config files. It's an init container that runs, a python script runs everything in a yaml file through the API and then the config gets generated that I can copy/backup/use for other purposes

@ShreyasZare
Copy link
Member

In this case those classes that do the validation could be abstracted to also work when loading the config files

With such a design, if there is validation failure then the server fails to start. This is what the current design is trying to prevent by discouraging manual config edits.

My worker around right now is to load a scratch instance, inject everything through the API and then grab the generated config files. It's an init container that runs, a python script runs everything in a yaml file through the API and then the config gets generated that I can copy/backup/use for other purposes

If you can describe your use-case in details then it will help me understand it better so that I can think of some solution to make it easier.

@cob-web-corner
Copy link
Author

cob-web-corner commented Oct 22, 2024

With such a design, if there is validation failure then the server fails to start. This is what the current design is trying to prevent by discouraging manual config edits.

Such is the case with any software really, but you cannot guard rail the stove because someone might burn their hand

If you can describe your use-case in details then it will help me understand it better so that I can think of some solution to make it easier.

In general the use case is deploying a ready to go system with it's configuration, basically IaC with provisioning after. Think dynamic failovers not from backups, deploying the server using pipelines, running it in kubernetes, etc

@ShreyasZare
Copy link
Member

With such a design, if there is validation failure then the server fails to start. This is what the current design is trying to prevent by discouraging manual config edits.

Such is the case with any software really, but you cannot guard rail the stove because someone might burn their hand

That's true. The concern here was due to DNS being critical service, when it fails, it affects the entire system. So, this was to make sure that the DNS server always starts with correct config.

If you can describe your use-case in details then it will help me understand it better so that I can think of some solution to make it easier.

In general the use case is deploying a ready to go system with it's configuration, basically IaC with provisioning after. Think dynamic failovers not from backups, deploying the server using pipelines, running it in kubernetes, etc

Thanks for the description. Will see if something can be done to make it better.

@cob-web-corner
Copy link
Author

With such a design, if there is validation failure then the server fails to start. This is what the current design is trying to prevent by discouraging manual config edits.

Such is the case with any software really, but you cannot guard rail the stove because someone might burn their hand

That's true. The concern here was due to DNS being critical service, when it fails, it affects the entire system. So, this was to make sure that the DNS server always starts with correct config.

I understand the idea

If you can describe your use-case in details then it will help me understand it better so that I can think of some solution to make it easier.

In general the use case is deploying a ready to go system with it's configuration, basically IaC with provisioning after. Think dynamic failovers not from backups, deploying the server using pipelines, running it in kubernetes, etc

Thanks for the description. Will see if something can be done to make it better.

Alright thanks for considering it.

An outside the box idea would be a terraform provider, but that's a whole project/effort on its own. Terraform providers basically wrap existing APIs

@IngmarStein
Copy link
Contributor

I'd like to add one more argument in favor of human-readable config files: many people like to put configuration files under version control (e.g. as an audit trail, for changelogs, or easy rollbacks). This is arguably a lot more useful with human-readable diffs.

@timhae
Copy link

timhae commented Nov 17, 2024

I realize it would probably take quite some time to implement this so I have a suggestion: what do you think of a "sidecar application" that translates a config file into api calls? This would give you the flexibility to fine tune the config format and make changes under the hood that implement the config file one setting at a time. This could also live in this repo and can be written in any language, also c# if you prefer that.

@ShreyasZare
Copy link
Member

I realize it would probably take quite some time to implement this so I have a suggestion: what do you think of a "sidecar application" that translates a config file into api calls? This would give you the flexibility to fine tune the config format and make changes under the hood that implement the config file one setting at a time. This could also live in this repo and can be written in any language, also c# if you prefer that.

Thanks for the suggestion. Such a tool would be redundant. This is since you can just install the DNS server on any laptop, use the GUI to set a desired config and then use the Backup Settings option to export it for use.

@timhae
Copy link

timhae commented Nov 17, 2024

The purpose of that would be to allow you not having to do any configuration through the webui but instead have a potentially scm tracked file as mentioned earlier in this issue and other issues regarding configuration files. If you aren't interested, please let me know then I will just implement that downstream in nixpkgs/nixos

@ShreyasZare
Copy link
Member

The purpose of that would be to allow you not having to do any configuration through the webui but instead have a potentially scm tracked file as mentioned earlier in this issue and other issues regarding configuration files.

If you are looking for some kind of text config to API call then the API can be updated to accept the settings in json format, the same format the get settings api returns. You can then keep the json locally and track it.

If you aren't interested, please let me know then I will just implement that downstream in nixpkgs/nixos

I can update the API as described above when I have some time available. Creating and maintaining a set of separate tools for the conversion will not be feasible for me.

@timhae
Copy link

timhae commented Nov 17, 2024

You mean this endpoint? If it would accept json instead of parameters that would get me 90% there. If you want I can also prepare a PR. Thanks for your time and effort :)

@ShreyasZare
Copy link
Member

You mean this endpoint? If it would accept json instead of parameters that would get me 90% there. If you want I can also prepare a PR. Thanks for your time and effort :)

Yes, the Set DNS Settings can be changed to accept json for POST requests. The json can be exact same format as that in the Get DNS Settings call.

@timhae
Copy link

timhae commented Nov 21, 2024

This is what I have so far:

From a6648c2e2322f7aca1777a62183979091fde4b80 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20H=C3=A4ring?= <tim.haering@gmail.com>
Date: Thu, 21 Nov 2024 21:53:03 +0100
Subject: [PATCH 1/1] feat: accept json body in SetDnsSettings endpoint

---
 DnsServerCore/WebServiceSettingsApi.cs | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/DnsServerCore/WebServiceSettingsApi.cs b/DnsServerCore/WebServiceSettingsApi.cs
index d99d1d75..b9d8646f 100644
--- a/DnsServerCore/WebServiceSettingsApi.cs
+++ b/DnsServerCore/WebServiceSettingsApi.cs
@@ -31,6 +31,7 @@ using System.Net.Mail;
 using System.Net.Sockets;
 using System.Text;
 using System.Text.Json;
+using System.Text.Json.Nodes;
 using System.Threading;
 using System.Threading.Tasks;
 using TechnitiumLibrary;
@@ -570,7 +571,7 @@ namespace DnsServerCore
             WriteDnsSettings(jsonWriter);
         }

-        public void SetDnsSettings(HttpContext context)
+        public async void SetDnsSettings(HttpContext context)
         {
             UserSession session = context.GetCurrentSession();

@@ -587,10 +588,25 @@ namespace DnsServerCore
             bool _webServiceEnablingTls = false;

             HttpRequest request = context.Request;
+            JsonDocument jsonDoc = null;
+            JsonElement jsonElem = default;
+
+            if (request.HasJsonContentType())
+            {
+                using (var reader = new StreamReader(request.Body))
+                {
+                    var body = await reader.ReadToEndAsync();
+                    jsonDoc = JsonDocument.Parse(body);
+                }
+            }

             //general
             if (request.TryGetQueryOrForm("dnsServerDomain", out string dnsServerDomain))
             {
+                if (jsonDoc != null && jsonDoc.RootElement.TryGetProperty("dnsServerDomain", out jsonElem))
+                {
+                    dnsServerDomain = jsonElem.GetString();
+                }
                 dnsServerDomain = dnsServerDomain.TrimEnd('.');

                 if (!_dnsWebService.DnsServer.ServerDomain.Equals(dnsServerDomain, StringComparison.OrdinalIgnoreCase))
@@ -601,6 +617,10 @@ namespace DnsServerCore
             }

             string dnsServerLocalEndPoints = request.QueryOrForm("dnsServerLocalEndPoints");
+            if (jsonDoc != null && jsonDoc.RootElement.TryGetProperty("dnsServerLocalEndPoints", out jsonElem))
+            {
+                dnsServerLocalEndPoints = jsonElem.GetString();
+            }
             if (dnsServerLocalEndPoints is not null)
             {
                 if (dnsServerLocalEndPoints.Length == 0)
@@ -636,6 +656,10 @@ namespace DnsServerCore
             }

             string dnsServerIPv4SourceAddresses = request.QueryOrForm("dnsServerIPv4SourceAddresses");
+            if (jsonDoc != null && jsonDoc.RootElement.TryGetProperty("dnsServerIPv4SourceAddresses", out jsonElem))
+            {
+                dnsServerIPv4SourceAddresses = jsonElem.GetString();
+            }
             if (dnsServerIPv4SourceAddresses is not null)
                 DnsClientConnection.IPv4SourceAddresses = ParseNetworkAddresses(dnsServerIPv4SourceAddresses);

--
2.47.0

A couple of notes:

  • I decided to make the function async, not sure if that is a good idea since now you would basically need a lock to handle concurrent requests
  • I don't parse the body json into a struct since you don't have that struct currently and I think the dynamic access is good enough for this and can be improved further down the road
  • I always give the json settings predecence because that was easier to implement

If you think this is going in the right direction, I will continue for the rest of the many settings and then open a PR. Please let me know what you think.

@ShreyasZare
Copy link
Member

Thanks for the details. I would suggest to define generic sub functions like you see in the Extentions class. These functions would read from the json if its available else they would call the intended extension method. Thus the code that reads the settings would almost remain the same.

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

No branches or pull requests

4 participants