From 374bb2714a87848eb9346b0bd32e91f65ac00c30 Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Mon, 27 Apr 2026 15:03:45 +0200 Subject: [PATCH 01/10] USHIFT-6902: Add dns.configFile config field and validation Add a new ConfigFile field to the DNS config struct that allows users to specify a custom CoreDNS Corefile path. Validation ensures mutual exclusivity with dns.hosts, and checks that the file is absolute, exists, is readable, non-empty, and within the 1MiB ConfigMap limit. --- .../config/config-openapi-spec.json | 5 ++ docs/user/howto_config.md | 2 + packaging/microshift/config.yaml | 8 +++ pkg/config/config.go | 3 ++ pkg/config/dns.go | 54 ++++++++++++++++++- 5 files changed, 71 insertions(+), 1 deletion(-) diff --git a/cmd/generate-config/config/config-openapi-spec.json b/cmd/generate-config/config/config-openapi-spec.json index 901548610d..4d5d03a2db 100755 --- a/cmd/generate-config/config/config-openapi-spec.json +++ b/cmd/generate-config/config/config-openapi-spec.json @@ -245,6 +245,11 @@ "default": "example.com", "example": "microshift.example.com" }, + "configFile": { + "description": "configFile is the path to a custom CoreDNS Corefile on the host filesystem.\nWhen set, MicroShift uses this file as the Corefile in the dns-default ConfigMap,\nfully replacing the default template-rendered configuration.\nChanges to this file are detected at runtime and applied without restarting MicroShift.\nMutually exclusive with dns.hosts: setting both causes a startup error.", + "type": "string", + "example": "/etc/microshift/dns/Corefile" + }, "hosts": { "description": "Hosts contains configuration for the hosts file.", "type": "object", diff --git a/docs/user/howto_config.md b/docs/user/howto_config.md index cbed9baa28..72cd84df6e 100644 --- a/docs/user/howto_config.md +++ b/docs/user/howto_config.md @@ -40,6 +40,7 @@ debugging: logLevel: "" dns: baseDomain: "" + configFile: "" hosts: file: "" status: "" @@ -198,6 +199,7 @@ debugging: logLevel: Normal dns: baseDomain: example.com + configFile: "" hosts: file: /etc/hosts status: Disabled diff --git a/packaging/microshift/config.yaml b/packaging/microshift/config.yaml index fd45f1a37a..7d54331024 100644 --- a/packaging/microshift/config.yaml +++ b/packaging/microshift/config.yaml @@ -72,6 +72,14 @@ dns: # example: # microshift.example.com baseDomain: example.com + # configFile is the path to a custom CoreDNS Corefile on the host filesystem. + # When set, MicroShift uses this file as the Corefile in the dns-default ConfigMap, + # fully replacing the default template-rendered configuration. + # Changes to this file are detected at runtime and applied without restarting MicroShift. + # Mutually exclusive with dns.hosts: setting both causes a startup error. + # example: + # /etc/microshift/dns/Corefile + configFile: "" # Hosts contains configuration for the hosts file. hosts: # File is the path to the hosts file to monitor. diff --git a/pkg/config/config.go b/pkg/config/config.go index cffb723f2c..192ee26d73 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -212,6 +212,9 @@ func (c *Config) incorporateUserSettings(u *Config) { if u.DNS.BaseDomain != "" { c.DNS.BaseDomain = u.DNS.BaseDomain } + if u.DNS.ConfigFile != "" { + c.DNS.ConfigFile = u.DNS.ConfigFile + } if u.Network.CNIPlugin != "" { c.Network.CNIPlugin = u.Network.CNIPlugin diff --git a/pkg/config/dns.go b/pkg/config/dns.go index d8948449c9..3c42a10aba 100644 --- a/pkg/config/dns.go +++ b/pkg/config/dns.go @@ -27,6 +27,15 @@ type DNS struct { // +kubebuilder:example=microshift.example.com BaseDomain string `json:"baseDomain"` + // configFile is the path to a custom CoreDNS Corefile on the host filesystem. + // When set, MicroShift uses this file as the Corefile in the dns-default ConfigMap, + // fully replacing the default template-rendered configuration. + // Changes to this file are detected at runtime and applied without restarting MicroShift. + // Mutually exclusive with dns.hosts: setting both causes a startup error. + // +optional + // +kubebuilder:example="/etc/microshift/dns/Corefile" + ConfigFile string `json:"configFile,omitempty"` + // Hosts contains configuration for the hosts file. Hosts HostsConfig `json:"hosts,omitempty"` } @@ -59,6 +68,50 @@ func dnsDefaults() DNS { } func (t *DNS) validate() error { + if t.ConfigFile != "" && t.Hosts.Status == HostsStatusEnabled { + return fmt.Errorf("dns.configFile and dns.hosts are mutually exclusive") + } + + if err := t.validateConfigFile(); err != nil { + return err + } + + return t.validateHosts() +} + +func (t *DNS) validateConfigFile() error { + if t.ConfigFile == "" { + return nil + } + + cleanPath := filepath.Clean(t.ConfigFile) + if !filepath.IsAbs(cleanPath) { + return fmt.Errorf("dns config file path must be absolute: got %s", t.ConfigFile) + } + + fi, err := os.Stat(cleanPath) + if os.IsNotExist(err) { + return fmt.Errorf("dns config file %s does not exist", t.ConfigFile) + } else if err != nil { + return fmt.Errorf("error checking dns config file %s: %v", t.ConfigFile, err) + } + + if fi.Size() == 0 { + return fmt.Errorf("dns config file %s is empty", t.ConfigFile) + } + + if fi.Size() > 1048576 { + return fmt.Errorf("dns config file %s exceeds 1MiB ConfigMap size limit (got %d bytes)", t.ConfigFile, fi.Size()) + } + + file, err := os.Open(cleanPath) + if err != nil { + return fmt.Errorf("dns config file %s is not readable: %v", t.ConfigFile, err) + } + return file.Close() +} + +func (t *DNS) validateHosts() error { switch t.Hosts.Status { case HostsStatusEnabled: if t.Hosts.File == "" { @@ -68,7 +121,6 @@ func (t *DNS) validate() error { cleanPath := filepath.Clean(t.Hosts.File) fi, err := os.Stat(cleanPath) - // Enforce ConfigMap requirement: the file must not exceed 1MiB, as it will be mounted into a ConfigMap. if err == nil && fi.Size() > 1048576 { return fmt.Errorf("hosts file %s exceeds 1MiB ConfigMap (and internal buffer) size limit (got %d bytes)", t.Hosts.File, fi.Size()) } From 77239f6fa82a3564dabad23793a9f713801bd48e Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Mon, 27 Apr 2026 15:05:30 +0200 Subject: [PATCH 02/10] USHIFT-6902: Use custom Corefile in DNS controller rendering When dns.configFile is set, read the file and apply its contents directly as the Corefile key in the dns-default ConfigMap, bypassing the default template rendering. When unset, existing behavior is preserved. --- pkg/components/controllers.go | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/pkg/components/controllers.go b/pkg/components/controllers.go index e9bdf7af50..ded367dfca 100644 --- a/pkg/components/controllers.go +++ b/pkg/components/controllers.go @@ -291,10 +291,9 @@ func startDNSController(ctx context.Context, cfg *config.Config, kubeconfigPath "components/openshift-dns/dns/service-account.yaml", "components/openshift-dns/node-resolver/service-account.yaml", } - cm = []string{ - "components/openshift-dns/dns/configmap.yaml", - } - svc = []string{ + cm = "components/openshift-dns/dns/configmap.yaml" + cmList = []string{cm} + svc = []string{ "components/openshift-dns/dns/service.yaml", } ) @@ -303,9 +302,10 @@ func startDNSController(ctx context.Context, cfg *config.Config, kubeconfigPath return err } + hostsEnabled := cfg.DNS.Hosts.Status == config.HostsStatusEnabled extraParams := assets.RenderParams{ "ClusterIP": cfg.Network.DNS, - "HostsEnabled": cfg.DNS.Hosts.Status == config.HostsStatusEnabled, + "HostsEnabled": hostsEnabled, } if err := assets.ApplyServices(ctx, svc, renderTemplate, renderParamsFromConfig(cfg, extraParams), kubeconfigPath); err != nil { @@ -333,10 +333,24 @@ func startDNSController(ctx context.Context, cfg *config.Config, kubeconfigPath klog.Warningf("Failed to apply serviceAccount %v %v", sa, err) return err } - if err := assets.ApplyConfigMaps(ctx, cm, renderTemplate, renderParamsFromConfig(cfg, extraParams), kubeconfigPath); err != nil { - klog.Warningf("Failed to apply configMap %v %v", cm, err) - return err + + if cfg.DNS.ConfigFile != "" { + corefileContent, err := os.ReadFile(cfg.DNS.ConfigFile) + if err != nil { + klog.Warningf("Failed to read custom DNS config file %s: %v", cfg.DNS.ConfigFile, err) + return err + } + if err := assets.ApplyConfigMapWithData(ctx, cm, map[string]string{"Corefile": string(corefileContent)}, kubeconfigPath); err != nil { + klog.Warningf("Failed to apply custom DNS configMap: %v", err) + return err + } + } else { + if err := assets.ApplyConfigMaps(ctx, cmList, renderTemplate, renderParamsFromConfig(cfg, extraParams), kubeconfigPath); err != nil { + klog.Warningf("Failed to apply configMap %v %v", cmList, err) + return err + } } + if err := assets.ApplyDaemonSets(ctx, apps, renderTemplate, renderParamsFromConfig(cfg, extraParams), kubeconfigPath); err != nil { klog.Warningf("Failed to apply apps %v %v", apps, err) return err From 83575142a217ce2cea866d6c23368716c46325a3 Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Mon, 27 Apr 2026 15:11:17 +0200 Subject: [PATCH 03/10] USHIFT-6902: Add DNS configuration watcher for runtime reload Add DNSConfigurationWatcherManager that watches the custom Corefile specified by dns.configFile for changes at runtime, updating the dns-default ConfigMap when the file is modified. Follows the same fsnotify + SHA256 hashing pattern as the existing hosts watcher. --- pkg/cmd/run.go | 1 + pkg/controllers/dnsconfigurationwatcher.go | 231 +++++++++++++++++++++ 2 files changed, 232 insertions(+) create mode 100644 pkg/controllers/dnsconfigurationwatcher.go diff --git a/pkg/cmd/run.go b/pkg/cmd/run.go index bd32bb8782..d8dff2a4f4 100644 --- a/pkg/cmd/run.go +++ b/pkg/cmd/run.go @@ -236,6 +236,7 @@ func RunMicroshift(cfg *config.Config) error { util.Must(m.AddService(controllers.NewClusterID(cfg))) util.Must(m.AddService(controllers.NewTelemetryManager(cfg))) util.Must(m.AddService(controllers.NewHostsWatcherManager(cfg))) + util.Must(m.AddService(controllers.NewDNSConfigurationWatcherManager(cfg))) util.Must(m.AddService(gdp.NewGenericDevicePlugin(cfg))) // Storing and clearing the env, so other components don't send the READY=1 until MicroShift is fully ready diff --git a/pkg/controllers/dnsconfigurationwatcher.go b/pkg/controllers/dnsconfigurationwatcher.go new file mode 100644 index 0000000000..85523174a1 --- /dev/null +++ b/pkg/controllers/dnsconfigurationwatcher.go @@ -0,0 +1,231 @@ +package controllers + +import ( + "context" + "crypto/sha256" + "fmt" + "os" + "path/filepath" + "time" + + "github.com/fsnotify/fsnotify" + "github.com/openshift/microshift/pkg/config" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/clientcmd" + "k8s.io/klog/v2" +) + +const ( + dnsConfigMapNamespace = "openshift-dns" + dnsConfigMapName = "dns-default" + dnsConfigMapKey = "Corefile" +) + +type DNSConfigurationWatcherManager struct { + file string + kubeconfig string +} + +func NewDNSConfigurationWatcherManager(cfg *config.Config) *DNSConfigurationWatcherManager { + return &DNSConfigurationWatcherManager{ + file: cfg.DNS.ConfigFile, + kubeconfig: cfg.KubeConfigPath(config.KubeAdmin), + } +} + +func (s *DNSConfigurationWatcherManager) Name() string { return "dns-configuration-watcher-manager" } +func (s *DNSConfigurationWatcherManager) Dependencies() []string { return []string{"kube-apiserver"} } + +func (s *DNSConfigurationWatcherManager) Run(ctx context.Context, ready chan<- struct{}, stopped chan<- struct{}) error { + defer close(stopped) + + if s.file == "" { + klog.Infof("%s is disabled (not configured)", s.Name()) + close(ready) + return ctx.Err() + } + + kubeClient, err := s.createKubeClient() + if err != nil { + klog.Errorf("%s failed to create Kubernetes client: %v", s.Name(), err) + return err + } + + if err := s.updateConfigMap(ctx, kubeClient); err != nil { + klog.Errorf("%s failed to create initial ConfigMap: %v", s.Name(), err) + return err + } + + watcher, err := fsnotify.NewWatcher() + if err != nil { + klog.Errorf("%s failed to create file watcher: %v", s.Name(), err) + return err + } + defer func() { + if cerr := watcher.Close(); cerr != nil { + klog.Errorf("%s failed to close file watcher: %v", s.Name(), cerr) + } + }() + + if err := s.setupWatches(watcher); err != nil { + return err + } + close(ready) + + lastHash, err := s.getFileHash() + if err != nil { + klog.Warningf("%s failed to get initial file hash: %v", s.Name(), err) + } + + klog.Infof("%s is ready and watching %s", s.Name(), s.file) + + return s.eventLoop(ctx, watcher, kubeClient, lastHash) +} + +func (s *DNSConfigurationWatcherManager) setupWatches(watcher *fsnotify.Watcher) error { + filesToWatch := []string{ + s.file, + filepath.Dir(s.file), + } + for i, file := range filesToWatch { + if err := watcher.Add(file); err != nil { + if i == 0 { + klog.Errorf("%s failed to watch DNS config file %s: %v", s.Name(), s.file, err) + return err + } + klog.Warningf("%s failed to watch DNS config directory %s: %v", s.Name(), file, err) + } + } + return nil +} + +func (s *DNSConfigurationWatcherManager) eventLoop(ctx context.Context, watcher *fsnotify.Watcher, kubeClient kubernetes.Interface, initHash string) error { + lastHash := initHash + + for { + select { + case <-ctx.Done(): + klog.Infof("%s stopping", s.Name()) + return watcher.Close() + + case event, ok := <-watcher.Events: + if !ok { + return fmt.Errorf("%s watcher channel closed", s.Name()) + } + if s.isRelevantEvent(event) { + updated, newHash, updateErr := s.handleChange(ctx, kubeClient, lastHash) + if updateErr != nil { + klog.Errorf("%s failed to process DNS config file change: %v", s.Name(), updateErr) + continue + } + if updated { + lastHash = newHash + } + } + + case err, ok := <-watcher.Errors: + if !ok { + return fmt.Errorf("%s watcher error channel closed", s.Name()) + } + klog.Errorf("%s watcher error: %v", s.Name(), err) + } + } +} + +func (s *DNSConfigurationWatcherManager) isRelevantEvent(event fsnotify.Event) bool { + if event.Name != s.file { + return false + } + return event.Op&fsnotify.Write == fsnotify.Write || event.Op&fsnotify.Create == fsnotify.Create +} + +func (s *DNSConfigurationWatcherManager) handleChange(ctx context.Context, kubeClient kubernetes.Interface, lastHash string) (bool, string, error) { + klog.Infof("%s detected change in DNS config file: %s", s.Name(), s.file) + currentHash, err := s.getFileHash() + if err != nil { + klog.Warningf("%s failed to get file hash after change: %v", s.Name(), err) + return false, lastHash, err + } + if currentHash == lastHash { + klog.V(2).Infof("%s file hash unchanged, skipping update", s.Name()) + return false, lastHash, nil + } + if err := s.updateConfigMap(ctx, kubeClient); err != nil { + klog.Errorf("%s failed to update ConfigMap: %v", s.Name(), err) + return false, currentHash, err + } + klog.Infof("%s successfully updated ConfigMap %s/%s", s.Name(), dnsConfigMapNamespace, dnsConfigMapName) + return true, currentHash, nil +} + +func (s *DNSConfigurationWatcherManager) createKubeClient() (*kubernetes.Clientset, error) { + cfg, err := clientcmd.BuildConfigFromFlags("", s.kubeconfig) + if err != nil { + return nil, fmt.Errorf("failed to build kubeconfig: %w", err) + } + client, err := kubernetes.NewForConfig(cfg) + if err != nil { + return nil, fmt.Errorf("failed to create Kubernetes client: %w", err) + } + return client, nil +} + +func (s *DNSConfigurationWatcherManager) updateConfigMap(ctx context.Context, client kubernetes.Interface) error { + content, err := os.ReadFile(s.file) + if err != nil { + return fmt.Errorf("failed to read DNS config file %s: %w", s.file, err) + } + + return s.createOrUpdateConfigMap(ctx, client, string(content)) +} + +func (s *DNSConfigurationWatcherManager) createOrUpdateConfigMap(ctx context.Context, client kubernetes.Interface, corefileContent string) error { + configMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: dnsConfigMapName, + Namespace: dnsConfigMapNamespace, + Labels: map[string]string{ + "dns.operator.openshift.io/owning-dns": "default", + }, + Annotations: map[string]string{ + "microshift.io/dns-config-file": s.file, + "microshift.io/last-updated": time.Now().Format(time.RFC3339), + }, + }, + Data: map[string]string{ + dnsConfigMapKey: corefileContent, + }, + } + + configMapsClient := client.CoreV1().ConfigMaps(dnsConfigMapNamespace) + + existing, err := configMapsClient.Get(ctx, dnsConfigMapName, metav1.GetOptions{}) + if err != nil { + _, err = configMapsClient.Create(ctx, configMap, metav1.CreateOptions{}) + if err != nil { + return fmt.Errorf("failed to create ConfigMap: %w", err) + } + klog.Infof("%s created ConfigMap %s/%s", s.Name(), dnsConfigMapNamespace, dnsConfigMapName) + } else { + existing.Data = configMap.Data + existing.Annotations = configMap.Annotations + _, err = configMapsClient.Update(ctx, existing, metav1.UpdateOptions{}) + if err != nil { + return fmt.Errorf("failed to update ConfigMap: %w", err) + } + klog.V(2).Infof("%s updated ConfigMap %s/%s", s.Name(), dnsConfigMapNamespace, dnsConfigMapName) + } + + return nil +} + +func (s *DNSConfigurationWatcherManager) getFileHash() (string, error) { + content, err := os.ReadFile(s.file) + if err != nil { + return "", err + } + hash := sha256.Sum256(content) + return fmt.Sprintf("%x", hash), nil +} From 9e0a8d3e258d9288d4285a7c09441230aed0523e Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Mon, 27 Apr 2026 17:40:59 +0200 Subject: [PATCH 04/10] USHIFT-6902: Add unit tests for dns.configFile validation Cover mutual exclusivity with dns.hosts, non-absolute path, missing file, empty file, file exceeding 1MiB limit, valid config, and default behavior when configFile is unset. --- pkg/config/dns_test.go | 128 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 pkg/config/dns_test.go diff --git a/pkg/config/dns_test.go b/pkg/config/dns_test.go new file mode 100644 index 0000000000..ad1766a878 --- /dev/null +++ b/pkg/config/dns_test.go @@ -0,0 +1,128 @@ +package config + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDNS_ValidateConfigFile_MutualExclusivity(t *testing.T) { + tmpFile := createTempCorefile(t, ".:5353 { whoami }") + + dns := DNS{ + ConfigFile: tmpFile, + Hosts: HostsConfig{ + Status: HostsStatusEnabled, + File: "/etc/hosts", + }, + } + err := dns.validate() + assert.ErrorContains(t, err, "dns.configFile and dns.hosts are mutually exclusive") +} + +func TestDNS_ValidateConfigFile_WithHostsDisabled(t *testing.T) { + tmpFile := createTempCorefile(t, ".:5353 { whoami }") + + dns := DNS{ + ConfigFile: tmpFile, + Hosts: HostsConfig{ + Status: HostsStatusDisabled, + }, + } + assert.NoError(t, dns.validate()) +} + +func TestDNS_ValidateConfigFile_EmptyConfigFilePreservesDefault(t *testing.T) { + dns := DNS{ + ConfigFile: "", + Hosts: HostsConfig{ + Status: HostsStatusDisabled, + }, + } + assert.NoError(t, dns.validate()) +} + +func TestDNS_ValidateConfigFile_NonAbsolutePath(t *testing.T) { + dns := DNS{ + ConfigFile: "relative/path/Corefile", + Hosts: HostsConfig{ + Status: HostsStatusDisabled, + }, + } + err := dns.validate() + assert.ErrorContains(t, err, "dns config file path must be absolute") +} + +func TestDNS_ValidateConfigFile_NonExistentFile(t *testing.T) { + dns := DNS{ + ConfigFile: "/tmp/nonexistent-corefile-test-12345", + Hosts: HostsConfig{ + Status: HostsStatusDisabled, + }, + } + err := dns.validate() + assert.ErrorContains(t, err, "does not exist") +} + +func TestDNS_ValidateConfigFile_EmptyFile(t *testing.T) { + tmpFile := createTempCorefile(t, "") + + dns := DNS{ + ConfigFile: tmpFile, + Hosts: HostsConfig{ + Status: HostsStatusDisabled, + }, + } + err := dns.validate() + assert.ErrorContains(t, err, "is empty") +} + +func TestDNS_ValidateConfigFile_ExceedsSizeLimit(t *testing.T) { + tmpDir := t.TempDir() + tmpFile := filepath.Join(tmpDir, "Corefile") + // 1 MiB + 1 byte + data := make([]byte, 1048576+1) + require.NoError(t, os.WriteFile(tmpFile, data, 0644)) + + dns := DNS{ + ConfigFile: tmpFile, + Hosts: HostsConfig{ + Status: HostsStatusDisabled, + }, + } + err := dns.validate() + assert.ErrorContains(t, err, "exceeds 1MiB ConfigMap size limit") +} + +func TestDNS_ValidateConfigFile_ValidFile(t *testing.T) { + tmpFile := createTempCorefile(t, ".:5353 {\n whoami\n reload\n}\n") + + dns := DNS{ + ConfigFile: tmpFile, + Hosts: HostsConfig{ + Status: HostsStatusDisabled, + }, + } + assert.NoError(t, dns.validate()) +} + +func TestDNS_ConfigFile_IncorporatedFromDropIn(t *testing.T) { + tmpFile := createTempCorefile(t, ".:5353 {\n whoami\n reload\n}\n") + + yamlConfig := fmt.Sprintf("dns:\n configFile: %s\n", tmpFile) + config, err := getActiveConfigFromYAMLDropins([][]byte{[]byte(yamlConfig)}) + require.NoError(t, err) + assert.Equal(t, tmpFile, config.DNS.ConfigFile) +} + +func createTempCorefile(t *testing.T, content string) string { + t.Helper() + tmpDir := t.TempDir() + tmpFile := filepath.Join(tmpDir, "Corefile") + require.NoError(t, os.WriteFile(tmpFile, []byte(content), 0644)) + return tmpFile +} From 390a845e84366af236a5ed744b3a624244f39e34 Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Mon, 27 Apr 2026 17:42:07 +0200 Subject: [PATCH 05/10] USHIFT-6903: Add e2e tests for custom DNS configuration RobotFramework tests covering: - Custom Corefile is used by CoreDNS instead of the default - Runtime reload propagates changes without MicroShift restart - Cluster-local resolution works with custom Corefile --- test/assets/dns/Corefile.template | 24 +++ test/suites/standard1/dns-custom-config.robot | 137 ++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 test/assets/dns/Corefile.template create mode 100644 test/suites/standard1/dns-custom-config.robot diff --git a/test/assets/dns/Corefile.template b/test/assets/dns/Corefile.template new file mode 100644 index 0000000000..4039e7cc8b --- /dev/null +++ b/test/assets/dns/Corefile.template @@ -0,0 +1,24 @@ +.:5353 { + bufsize 1232 + errors + health { + lameduck 20s + } + ready + kubernetes cluster.local in-addr.arpa ip6.arpa { + pods insecure + fallthrough in-addr.arpa ip6.arpa + } + prometheus 127.0.0.1:9153 + forward . /etc/resolv.conf { + policy sequential + } + cache 900 { + denial 9984 30 + } + hosts { + ${COREFILE_HOST_IP} ${COREFILE_HOSTNAME} + fallthrough + } + reload +} diff --git a/test/suites/standard1/dns-custom-config.robot b/test/suites/standard1/dns-custom-config.robot new file mode 100644 index 0000000000..79d671cba7 --- /dev/null +++ b/test/suites/standard1/dns-custom-config.robot @@ -0,0 +1,137 @@ +*** Settings *** +Documentation Tests for custom DNS configuration (dns.configFile) + +Resource ../../resources/common.resource +Resource ../../resources/oc.resource +Resource ../../resources/microshift-config.resource +Resource ../../resources/microshift-process.resource +Resource ../../resources/microshift-network.resource +Resource ../../resources/hosts.resource + +Suite Setup Setup Suite With Namespace +Suite Teardown Teardown Suite With Namespace + +Test Tags slow + + +*** Variables *** +${FAKE_LISTEN_IP} 99.99.99.98 +${CUSTOM_COREFILE_PATH} /etc/microshift/dns/Corefile +${COREFILE_TEMPLATE} ./assets/dns/Corefile.template +${HOSTNAME} ${EMPTY} + +${CUSTOM_DNS_CONFIG} SEPARATOR=\n +... --- +... dns: +... \ \ configFile: /etc/microshift/dns/Corefile + + +*** Test Cases *** +Custom DNS Config File Is Used By CoreDNS + [Documentation] Provide a valid custom Corefile and verify CoreDNS uses it + ... instead of the default template-rendered configuration. + [Setup] Setup Custom Corefile With Hosts Entry + Verify ConfigMap Uses Custom Corefile + Resolve Host From Pod ${HOSTNAME} + [Teardown] Teardown Custom Corefile + +Runtime Reload Without MicroShift Restart + [Documentation] Modify the custom Corefile while MicroShift is running + ... and verify CoreDNS picks up the change without restart. + [Setup] Setup Custom Corefile With Hosts Entry + Resolve Host From Pod ${HOSTNAME} + ${updated_hostname}= Generate Random HostName + Update Corefile With New Host ${updated_hostname} + Resolve Host From Pod ${updated_hostname} + [Teardown] Teardown Custom Corefile + +Cluster Local Resolution With Custom Corefile + [Documentation] Verify that a custom Corefile with the kubernetes plugin + ... still resolves cluster-local services correctly. + [Setup] Setup Custom Corefile With Hosts Entry + Resolve Host From Pod kubernetes.default.svc.cluster.local + [Teardown] Teardown Custom Corefile + + +*** Keywords *** +Build Custom Corefile + [Documentation] Build a Corefile from the template file, substituting + ... the hostname and IP placeholders. + [Arguments] ${hostname} ${ip} + ${template}= OperatingSystem.Get File ${COREFILE_TEMPLATE} + VAR ${COREFILE_HOSTNAME}= ${hostname} + VAR ${COREFILE_HOST_IP}= ${ip} + ${corefile}= Replace Variables ${template} + RETURN ${corefile} + +Setup Custom Corefile With Hosts Entry + [Documentation] Create a custom Corefile with a fake hostname entry, + ... upload it, configure MicroShift to use it, and restart. + ${HOSTNAME}= Generate Random HostName + VAR ${HOSTNAME}= ${HOSTNAME} scope=SUITE + Add Fake IP To NIC ${FAKE_LISTEN_IP} + ${corefile}= Build Custom Corefile ${HOSTNAME} ${FAKE_LISTEN_IP} + Create Remote Dir For Path ${CUSTOM_COREFILE_PATH} + Upload String To File ${corefile} ${CUSTOM_COREFILE_PATH} + Drop In MicroShift Config ${CUSTOM_DNS_CONFIG} 20-dns-custom + Restart MicroShift + +Update Corefile With New Host + [Documentation] Add a new hosts entry to the custom Corefile without + ... restarting MicroShift. The file watcher and CoreDNS reload plugin + ... should pick up the change. + [Arguments] ${hostname} + ${corefile}= Build Custom Corefile ${hostname} ${FAKE_LISTEN_IP} + Upload String To File ${corefile} ${CUSTOM_COREFILE_PATH} + +Verify ConfigMap Uses Custom Corefile + [Documentation] Verify the dns-default ConfigMap contains custom content + ... rather than the default template-rendered Corefile. + ${corefile_data}= Oc Get JsonPath + ... configmap openshift-dns dns-default .data.Corefile + Should Contain ${corefile_data} ${HOSTNAME} + +Resolve Host From Pod + [Documentation] Verify DNS resolution from a pod using nslookup + [Arguments] ${hostname} + Wait Until Keyword Succeeds 40x 5s + ... Router Should Resolve Hostname ${hostname} + +Router Should Resolve Hostname + [Documentation] Check if the router pod resolves the given hostname + [Arguments] ${hostname} + ${output}= Oc Exec router-default nslookup ${hostname} openshift-ingress deployment + Should Contain ${output} Name: ${hostname} + +Teardown Custom Corefile + [Documentation] Remove custom Corefile, drop-in config, fake IP, and restart + Run Keywords + ... Remove Drop In MicroShift Config 20-dns-custom + ... AND Remove Custom Corefile + ... AND Remove Fake IP From NIC ${FAKE_LISTEN_IP} + ... AND Restart MicroShift + +Remove Custom Corefile + [Documentation] Remove the custom Corefile from the host + ${stdout} ${stderr} ${rc}= Execute Command + ... rm -f ${CUSTOM_COREFILE_PATH} + ... sudo=True return_rc=True return_stdout=True return_stderr=True + Should Be Equal As Integers 0 ${rc} + +Add Fake IP To NIC + [Documentation] Add the given IP to br-ex temporarily. + [Arguments] ${ip_address}=${FAKE_LISTEN_IP} ${nic_name}=br-ex + ${stdout} ${stderr} ${rc}= SSHLibrary.Execute Command + ... ip address add ${ip_address}/32 dev ${nic_name} + ... sudo=True return_rc=True return_stderr=True return_stdout=True + Log Many ${stdout} ${stderr} + Should Be Equal As Integers 0 ${rc} + +Remove Fake IP From NIC + [Documentation] Remove the given IP from br-ex. + [Arguments] ${ip_address}=${FAKE_LISTEN_IP} ${nic_name}=br-ex + ${stdout} ${stderr} ${rc}= SSHLibrary.Execute Command + ... ip address delete ${ip_address}/32 dev ${nic_name} + ... sudo=True return_rc=True return_stderr=True return_stdout=True + Log Many ${stdout} ${stderr} + Should Be Equal As Integers 0 ${rc} From 1bb1468936d794e0ba8eac1c8a0cb848c43712df Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Tue, 28 Apr 2026 14:32:06 +0200 Subject: [PATCH 06/10] USHIFT-6902: Address review feedback for custom DNS configuration - Use apierrors.IsNotFound for ConfigMap Get errors instead of treating any error as not-found - Merge annotations into existing ConfigMap instead of replacing them to avoid clobbering annotations set by the asset apply - Handle atomic-rename file updates (Rename/Remove events) and re-add the file watch on Create so the watcher survives vim/sed -i edits - Reject non-regular files (directories, FIFOs) in validateConfigFile - Fix Should Contain assertion in robot test that treated the hostname as a failure message instead of a second assertion - Add retry wrapper to ConfigMap verification after restart --- pkg/config/dns.go | 3 +++ pkg/config/dns_test.go | 13 +++++++++ pkg/controllers/dnsconfigurationwatcher.go | 27 +++++++++++++------ test/suites/standard1/dns-custom-config.robot | 11 ++++++-- 4 files changed, 44 insertions(+), 10 deletions(-) diff --git a/pkg/config/dns.go b/pkg/config/dns.go index 3c42a10aba..101866a05d 100644 --- a/pkg/config/dns.go +++ b/pkg/config/dns.go @@ -95,6 +95,9 @@ func (t *DNS) validateConfigFile() error { } else if err != nil { return fmt.Errorf("error checking dns config file %s: %v", t.ConfigFile, err) } + if !fi.Mode().IsRegular() { + return fmt.Errorf("dns config file %s must be a regular file", t.ConfigFile) + } if fi.Size() == 0 { return fmt.Errorf("dns config file %s is empty", t.ConfigFile) diff --git a/pkg/config/dns_test.go b/pkg/config/dns_test.go index ad1766a878..b04b1bc715 100644 --- a/pkg/config/dns_test.go +++ b/pkg/config/dns_test.go @@ -68,6 +68,19 @@ func TestDNS_ValidateConfigFile_NonExistentFile(t *testing.T) { assert.ErrorContains(t, err, "does not exist") } +func TestDNS_ValidateConfigFile_Directory(t *testing.T) { + tmpDir := t.TempDir() + + dns := DNS{ + ConfigFile: tmpDir, + Hosts: HostsConfig{ + Status: HostsStatusDisabled, + }, + } + err := dns.validate() + assert.ErrorContains(t, err, "must be a regular file") +} + func TestDNS_ValidateConfigFile_EmptyFile(t *testing.T) { tmpFile := createTempCorefile(t, "") diff --git a/pkg/controllers/dnsconfigurationwatcher.go b/pkg/controllers/dnsconfigurationwatcher.go index 85523174a1..7a8e663e55 100644 --- a/pkg/controllers/dnsconfigurationwatcher.go +++ b/pkg/controllers/dnsconfigurationwatcher.go @@ -11,6 +11,7 @@ import ( "github.com/fsnotify/fsnotify" "github.com/openshift/microshift/pkg/config" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/clientcmd" @@ -115,6 +116,9 @@ func (s *DNSConfigurationWatcherManager) eventLoop(ctx context.Context, watcher return fmt.Errorf("%s watcher channel closed", s.Name()) } if s.isRelevantEvent(event) { + if event.Op&fsnotify.Create == fsnotify.Create { + _ = watcher.Add(s.file) + } updated, newHash, updateErr := s.handleChange(ctx, kubeClient, lastHash) if updateErr != nil { klog.Errorf("%s failed to process DNS config file change: %v", s.Name(), updateErr) @@ -138,7 +142,8 @@ func (s *DNSConfigurationWatcherManager) isRelevantEvent(event fsnotify.Event) b if event.Name != s.file { return false } - return event.Op&fsnotify.Write == fsnotify.Write || event.Op&fsnotify.Create == fsnotify.Create + const mask = fsnotify.Write | fsnotify.Create | fsnotify.Rename | fsnotify.Remove + return event.Op&mask != 0 } func (s *DNSConfigurationWatcherManager) handleChange(ctx context.Context, kubeClient kubernetes.Interface, lastHash string) (bool, string, error) { @@ -202,17 +207,23 @@ func (s *DNSConfigurationWatcherManager) createOrUpdateConfigMap(ctx context.Con configMapsClient := client.CoreV1().ConfigMaps(dnsConfigMapNamespace) existing, err := configMapsClient.Get(ctx, dnsConfigMapName, metav1.GetOptions{}) - if err != nil { - _, err = configMapsClient.Create(ctx, configMap, metav1.CreateOptions{}) - if err != nil { + switch { + case apierrors.IsNotFound(err): + if _, err = configMapsClient.Create(ctx, configMap, metav1.CreateOptions{}); err != nil { return fmt.Errorf("failed to create ConfigMap: %w", err) } klog.Infof("%s created ConfigMap %s/%s", s.Name(), dnsConfigMapNamespace, dnsConfigMapName) - } else { + case err != nil: + return fmt.Errorf("failed to get ConfigMap: %w", err) + default: existing.Data = configMap.Data - existing.Annotations = configMap.Annotations - _, err = configMapsClient.Update(ctx, existing, metav1.UpdateOptions{}) - if err != nil { + if existing.Annotations == nil { + existing.Annotations = map[string]string{} + } + for k, v := range configMap.Annotations { + existing.Annotations[k] = v + } + if _, err = configMapsClient.Update(ctx, existing, metav1.UpdateOptions{}); err != nil { return fmt.Errorf("failed to update ConfigMap: %w", err) } klog.V(2).Infof("%s updated ConfigMap %s/%s", s.Name(), dnsConfigMapNamespace, dnsConfigMapName) diff --git a/test/suites/standard1/dns-custom-config.robot b/test/suites/standard1/dns-custom-config.robot index 79d671cba7..b04d38de2d 100644 --- a/test/suites/standard1/dns-custom-config.robot +++ b/test/suites/standard1/dns-custom-config.robot @@ -87,9 +87,15 @@ Update Corefile With New Host Verify ConfigMap Uses Custom Corefile [Documentation] Verify the dns-default ConfigMap contains custom content ... rather than the default template-rendered Corefile. + Wait Until Keyword Succeeds 20x 5s + ... ConfigMap Should Contain Hostname ${HOSTNAME} + +ConfigMap Should Contain Hostname + [Documentation] Check the dns-default ConfigMap Corefile data for the hostname + [Arguments] ${hostname} ${corefile_data}= Oc Get JsonPath ... configmap openshift-dns dns-default .data.Corefile - Should Contain ${corefile_data} ${HOSTNAME} + Should Contain ${corefile_data} ${hostname} Resolve Host From Pod [Documentation] Verify DNS resolution from a pod using nslookup @@ -101,7 +107,8 @@ Router Should Resolve Hostname [Documentation] Check if the router pod resolves the given hostname [Arguments] ${hostname} ${output}= Oc Exec router-default nslookup ${hostname} openshift-ingress deployment - Should Contain ${output} Name: ${hostname} + Should Contain ${output} Name: + Should Contain ${output} ${hostname} Teardown Custom Corefile [Documentation] Remove custom Corefile, drop-in config, fake IP, and restart From 457562733cba54598910d2d5a190e0ae0f1e45fa Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Wed, 6 May 2026 12:38:35 +0200 Subject: [PATCH 07/10] USHIFT-6902: Extract shared DNS test keywords into dns.resource Deduplicate keywords shared between dns.robot and dns-custom-config.robot by moving them into a common resource file. Also fixes a bug in dns.robot where Router Should Resolve Hostname passed the hostname as a failure message to Should Contain instead of asserting it separately. --- test/resources/dns.resource | 38 +++++++++++++++++++ test/suites/standard1/dns-custom-config.robot | 32 +--------------- test/suites/standard1/dns.robot | 31 +-------------- 3 files changed, 40 insertions(+), 61 deletions(-) create mode 100644 test/resources/dns.resource diff --git a/test/resources/dns.resource b/test/resources/dns.resource new file mode 100644 index 0000000000..46a494408b --- /dev/null +++ b/test/resources/dns.resource @@ -0,0 +1,38 @@ +*** Settings *** +Documentation Shared keywords for DNS test suites + +Library SSHLibrary +Resource oc.resource + + +*** Keywords *** +Resolve Host From Pod + [Documentation] Verify DNS resolution from a pod using nslookup + [Arguments] ${hostname} + Wait Until Keyword Succeeds 40x 5s + ... Router Should Resolve Hostname ${hostname} + +Router Should Resolve Hostname + [Documentation] Check if the router pod resolves the given hostname + [Arguments] ${hostname} + ${output}= Oc Exec router-default nslookup ${hostname} openshift-ingress deployment + Should Contain ${output} Name: + Should Contain ${output} ${hostname} + +Add Fake IP To NIC + [Documentation] Add the given IP to br-ex temporarily. + [Arguments] ${ip_address} ${nic_name}=br-ex + ${stdout} ${stderr} ${rc}= SSHLibrary.Execute Command + ... ip address add ${ip_address}/32 dev ${nic_name} + ... sudo=True return_rc=True return_stderr=True return_stdout=True + Log Many ${stdout} ${stderr} + Should Be Equal As Integers 0 ${rc} + +Remove Fake IP From NIC + [Documentation] Remove the given IP from br-ex. + [Arguments] ${ip_address} ${nic_name}=br-ex + ${stdout} ${stderr} ${rc}= SSHLibrary.Execute Command + ... ip address delete ${ip_address}/32 dev ${nic_name} + ... sudo=True return_rc=True return_stderr=True return_stdout=True + Log Many ${stdout} ${stderr} + Should Be Equal As Integers 0 ${rc} diff --git a/test/suites/standard1/dns-custom-config.robot b/test/suites/standard1/dns-custom-config.robot index b04d38de2d..3d42cd0ee5 100644 --- a/test/suites/standard1/dns-custom-config.robot +++ b/test/suites/standard1/dns-custom-config.robot @@ -6,6 +6,7 @@ Resource ../../resources/oc.resource Resource ../../resources/microshift-config.resource Resource ../../resources/microshift-process.resource Resource ../../resources/microshift-network.resource +Resource ../../resources/dns.resource Resource ../../resources/hosts.resource Suite Setup Setup Suite With Namespace @@ -97,19 +98,6 @@ ConfigMap Should Contain Hostname ... configmap openshift-dns dns-default .data.Corefile Should Contain ${corefile_data} ${hostname} -Resolve Host From Pod - [Documentation] Verify DNS resolution from a pod using nslookup - [Arguments] ${hostname} - Wait Until Keyword Succeeds 40x 5s - ... Router Should Resolve Hostname ${hostname} - -Router Should Resolve Hostname - [Documentation] Check if the router pod resolves the given hostname - [Arguments] ${hostname} - ${output}= Oc Exec router-default nslookup ${hostname} openshift-ingress deployment - Should Contain ${output} Name: - Should Contain ${output} ${hostname} - Teardown Custom Corefile [Documentation] Remove custom Corefile, drop-in config, fake IP, and restart Run Keywords @@ -124,21 +112,3 @@ Remove Custom Corefile ... rm -f ${CUSTOM_COREFILE_PATH} ... sudo=True return_rc=True return_stdout=True return_stderr=True Should Be Equal As Integers 0 ${rc} - -Add Fake IP To NIC - [Documentation] Add the given IP to br-ex temporarily. - [Arguments] ${ip_address}=${FAKE_LISTEN_IP} ${nic_name}=br-ex - ${stdout} ${stderr} ${rc}= SSHLibrary.Execute Command - ... ip address add ${ip_address}/32 dev ${nic_name} - ... sudo=True return_rc=True return_stderr=True return_stdout=True - Log Many ${stdout} ${stderr} - Should Be Equal As Integers 0 ${rc} - -Remove Fake IP From NIC - [Documentation] Remove the given IP from br-ex. - [Arguments] ${ip_address}=${FAKE_LISTEN_IP} ${nic_name}=br-ex - ${stdout} ${stderr} ${rc}= SSHLibrary.Execute Command - ... ip address delete ${ip_address}/32 dev ${nic_name} - ... sudo=True return_rc=True return_stderr=True return_stdout=True - Log Many ${stdout} ${stderr} - Should Be Equal As Integers 0 ${rc} diff --git a/test/suites/standard1/dns.robot b/test/suites/standard1/dns.robot index 84a690ccac..5d22c4d5cb 100644 --- a/test/suites/standard1/dns.robot +++ b/test/suites/standard1/dns.robot @@ -4,6 +4,7 @@ Documentation Core DNS smoke tests Resource ../../resources/common.resource Resource ../../resources/oc.resource Resource ../../resources/microshift-network.resource +Resource ../../resources/dns.resource Resource ../../resources/microshift-config.resource Resource ../../resources/microshift-process.resource Resource ../../resources/hosts.resource @@ -91,18 +92,6 @@ Get Hosts Config Custom END RETURN ${config} -Resolve Host From Pod - [Documentation] Resolve host from pod - [Arguments] ${hostname} - Wait Until Keyword Succeeds 40x 5s - ... Router Should Resolve Hostname ${hostname} - -Router Should Resolve Hostname - [Documentation] Check if the router pod resolves the given hostname - [Arguments] ${hostname} - ${fuse_device}= Oc Exec router-default nslookup ${hostname} openshift-ingress deployment - Should Contain ${fuse_device} Name: ${hostname} - Setup With Custom Config [Documentation] Install a custom config and restart MicroShift [Arguments] ${config_content} ${hostsFile} @@ -128,24 +117,6 @@ Teardown Hosts File ... AND ... Remove Drop In MicroShift Config 20-dns -Add Fake IP To NIC - [Documentation] Add the given IP to the given NIC temporarily. - [Arguments] ${ip_address}=${FAKE_LISTEN_IP} ${nic_name}=br-ex - ${stdout} ${stderr} ${rc}= SSHLibrary.Execute Command - ... ip address add ${ip_address}/32 dev ${nic_name} - ... sudo=True return_rc=True return_stderr=True return_stdout=True - Log Many ${stdout} ${stderr} - Should Be Equal As Integers 0 ${rc} - -Remove Fake IP From NIC - [Documentation] Remove the given IP from the given NIC. - [Arguments] ${ip_address}=${FAKE_LISTEN_IP} ${nic_name}=br-ex - ${stdout} ${stderr} ${rc}= SSHLibrary.Execute Command - ... ip address delete ${ip_address}/32 dev ${nic_name} - ... sudo=True return_rc=True return_stderr=True return_stdout=True - Log Many ${stdout} ${stderr} - Should Be Equal As Integers 0 ${rc} - Check CoreDNS Hosts Feature [Documentation] Skip suite if CoreDNS hosts feature is not available ${config}= Show Config default From b23bf72c37b28f4ec26fa07c8fe7d2d750b81b3a Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Wed, 6 May 2026 12:54:13 +0200 Subject: [PATCH 08/10] USHIFT-6902: Verify ConfigMap update before DNS resolution in reload test Add a ConfigMap content check after updating the Corefile at runtime, before attempting DNS resolution. This confirms the file watcher propagated the change and provides a faster, clearer failure signal. --- test/suites/standard1/dns-custom-config.robot | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/suites/standard1/dns-custom-config.robot b/test/suites/standard1/dns-custom-config.robot index 3d42cd0ee5..1ba3f7db36 100644 --- a/test/suites/standard1/dns-custom-config.robot +++ b/test/suites/standard1/dns-custom-config.robot @@ -43,6 +43,8 @@ Runtime Reload Without MicroShift Restart Resolve Host From Pod ${HOSTNAME} ${updated_hostname}= Generate Random HostName Update Corefile With New Host ${updated_hostname} + Wait Until Keyword Succeeds 20x 5s + ... ConfigMap Should Contain Hostname ${updated_hostname} Resolve Host From Pod ${updated_hostname} [Teardown] Teardown Custom Corefile From 0157a1cd0843d9f8a4c7a2934ae8f708a5a6f6af Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Wed, 6 May 2026 14:31:46 +0200 Subject: [PATCH 09/10] USHIFT-6902: Fix nits from review feedback - Consolidate duplicate file validation logic in dns.go into a shared validateFilePath helper used by both validateConfigFile and validateHosts. Also fixes validateHosts calling os.Stat twice and missing regular-file and empty-file checks. - Depend on infrastructure-services-manager instead of kube-apiserver directly, ensuring the initial DNS ConfigMap exists before the watcher starts. - Avoid double-closing the fsnotify watcher by returning ctx.Err() on context cancellation and letting the defer handle Close(). - Return lastHash instead of currentHash on updateConfigMap failure so the next identical change is not skipped. --- pkg/config/dns.go | 72 ++++++++-------------- pkg/config/dns_test.go | 2 +- pkg/controllers/dnsconfigurationwatcher.go | 8 ++- 3 files changed, 32 insertions(+), 50 deletions(-) diff --git a/pkg/config/dns.go b/pkg/config/dns.go index 101866a05d..b567cbbc08 100644 --- a/pkg/config/dns.go +++ b/pkg/config/dns.go @@ -83,71 +83,51 @@ func (t *DNS) validateConfigFile() error { if t.ConfigFile == "" { return nil } + return validateFilePath(t.ConfigFile, "dns config file") +} - cleanPath := filepath.Clean(t.ConfigFile) +func (t *DNS) validateHosts() error { + switch t.Hosts.Status { + case HostsStatusEnabled: + if t.Hosts.File == "" { + break + } + return validateFilePath(t.Hosts.File, "hosts file") + case HostsStatusDisabled: + return nil + default: + return fmt.Errorf("invalid hosts status: %s", t.Hosts.Status) + } + return nil +} + +func validateFilePath(path, label string) error { + cleanPath := filepath.Clean(path) if !filepath.IsAbs(cleanPath) { - return fmt.Errorf("dns config file path must be absolute: got %s", t.ConfigFile) + return fmt.Errorf("%s path must be absolute: got %s", label, path) } fi, err := os.Stat(cleanPath) if os.IsNotExist(err) { - return fmt.Errorf("dns config file %s does not exist", t.ConfigFile) + return fmt.Errorf("%s %s does not exist", label, path) } else if err != nil { - return fmt.Errorf("error checking dns config file %s: %v", t.ConfigFile, err) + return fmt.Errorf("error checking %s %s: %v", label, path, err) } if !fi.Mode().IsRegular() { - return fmt.Errorf("dns config file %s must be a regular file", t.ConfigFile) + return fmt.Errorf("%s %s must be a regular file", label, path) } if fi.Size() == 0 { - return fmt.Errorf("dns config file %s is empty", t.ConfigFile) + return fmt.Errorf("%s %s is empty", label, path) } if fi.Size() > 1048576 { - return fmt.Errorf("dns config file %s exceeds 1MiB ConfigMap size limit (got %d bytes)", t.ConfigFile, fi.Size()) + return fmt.Errorf("%s %s exceeds 1MiB size limit (got %d bytes)", label, path, fi.Size()) } file, err := os.Open(cleanPath) if err != nil { - return fmt.Errorf("dns config file %s is not readable: %v", t.ConfigFile, err) + return fmt.Errorf("%s %s is not readable: %v", label, path, err) } return file.Close() } - -func (t *DNS) validateHosts() error { - switch t.Hosts.Status { - case HostsStatusEnabled: - if t.Hosts.File == "" { - break - } - - cleanPath := filepath.Clean(t.Hosts.File) - - fi, err := os.Stat(cleanPath) - if err == nil && fi.Size() > 1048576 { - return fmt.Errorf("hosts file %s exceeds 1MiB ConfigMap (and internal buffer) size limit (got %d bytes)", t.Hosts.File, fi.Size()) - } - if !filepath.IsAbs(cleanPath) { - return fmt.Errorf("hosts file path must be absolute: got %s", t.Hosts.File) - } - - _, err = os.Stat(cleanPath) - if os.IsNotExist(err) { - return fmt.Errorf("hosts file %s does not exist", t.Hosts.File) - } else if err != nil { - return fmt.Errorf("error checking hosts file %s: %v", t.Hosts.File, err) - } - - file, err := os.Open(t.Hosts.File) - if err != nil { - return fmt.Errorf("hosts file %s is not readable: %v", t.Hosts.File, err) - } - return file.Close() - - case HostsStatusDisabled: - return nil - default: - return fmt.Errorf("invalid hosts status: %s", t.Hosts.Status) - } - return nil -} diff --git a/pkg/config/dns_test.go b/pkg/config/dns_test.go index b04b1bc715..b3c21e63e1 100644 --- a/pkg/config/dns_test.go +++ b/pkg/config/dns_test.go @@ -108,7 +108,7 @@ func TestDNS_ValidateConfigFile_ExceedsSizeLimit(t *testing.T) { }, } err := dns.validate() - assert.ErrorContains(t, err, "exceeds 1MiB ConfigMap size limit") + assert.ErrorContains(t, err, "exceeds 1MiB size limit") } func TestDNS_ValidateConfigFile_ValidFile(t *testing.T) { diff --git a/pkg/controllers/dnsconfigurationwatcher.go b/pkg/controllers/dnsconfigurationwatcher.go index 7a8e663e55..ebdc92971c 100644 --- a/pkg/controllers/dnsconfigurationwatcher.go +++ b/pkg/controllers/dnsconfigurationwatcher.go @@ -37,7 +37,9 @@ func NewDNSConfigurationWatcherManager(cfg *config.Config) *DNSConfigurationWatc } func (s *DNSConfigurationWatcherManager) Name() string { return "dns-configuration-watcher-manager" } -func (s *DNSConfigurationWatcherManager) Dependencies() []string { return []string{"kube-apiserver"} } +func (s *DNSConfigurationWatcherManager) Dependencies() []string { + return []string{"infrastructure-services-manager"} +} func (s *DNSConfigurationWatcherManager) Run(ctx context.Context, ready chan<- struct{}, stopped chan<- struct{}) error { defer close(stopped) @@ -109,7 +111,7 @@ func (s *DNSConfigurationWatcherManager) eventLoop(ctx context.Context, watcher select { case <-ctx.Done(): klog.Infof("%s stopping", s.Name()) - return watcher.Close() + return ctx.Err() case event, ok := <-watcher.Events: if !ok { @@ -159,7 +161,7 @@ func (s *DNSConfigurationWatcherManager) handleChange(ctx context.Context, kubeC } if err := s.updateConfigMap(ctx, kubeClient); err != nil { klog.Errorf("%s failed to update ConfigMap: %v", s.Name(), err) - return false, currentHash, err + return false, lastHash, err } klog.Infof("%s successfully updated ConfigMap %s/%s", s.Name(), dnsConfigMapNamespace, dnsConfigMapName) return true, currentHash, nil From 48e380be35cea7ed676bc28d31ad7ac14cc16e39 Mon Sep 17 00:00:00 2001 From: Pablo Acevedo Montserrat Date: Wed, 6 May 2026 15:32:16 +0200 Subject: [PATCH 10/10] USHIFT-6902: Consolidate DNS and hosts file watchers into shared abstraction Extract shared file-watching logic from dnsconfigurationwatcher.go and hostswatcher.go into a common fileWatcher struct in filewatcher.go. Both concrete watchers become thin wrappers that embed fileWatcher and only provide constructors with their specific configuration. Also fixes bugs in the hosts watcher: double-closing the fsnotify watcher, treating any Get error as not-found, and returning currentHash on update failure. --- pkg/controllers/dnsconfigurationwatcher.go | 249 ++---------------- pkg/controllers/filewatcher.go | 280 ++++++++++++++++++++ pkg/controllers/hostswatcher.go | 288 ++------------------- 3 files changed, 321 insertions(+), 496 deletions(-) create mode 100644 pkg/controllers/filewatcher.go diff --git a/pkg/controllers/dnsconfigurationwatcher.go b/pkg/controllers/dnsconfigurationwatcher.go index ebdc92971c..66908fd588 100644 --- a/pkg/controllers/dnsconfigurationwatcher.go +++ b/pkg/controllers/dnsconfigurationwatcher.go @@ -1,244 +1,33 @@ package controllers import ( - "context" - "crypto/sha256" - "fmt" - "os" - "path/filepath" - "time" - "github.com/fsnotify/fsnotify" "github.com/openshift/microshift/pkg/config" - corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes" - "k8s.io/client-go/tools/clientcmd" - "k8s.io/klog/v2" -) - -const ( - dnsConfigMapNamespace = "openshift-dns" - dnsConfigMapName = "dns-default" - dnsConfigMapKey = "Corefile" ) type DNSConfigurationWatcherManager struct { - file string - kubeconfig string + fileWatcher } func NewDNSConfigurationWatcherManager(cfg *config.Config) *DNSConfigurationWatcherManager { - return &DNSConfigurationWatcherManager{ - file: cfg.DNS.ConfigFile, - kubeconfig: cfg.KubeConfigPath(config.KubeAdmin), - } -} - -func (s *DNSConfigurationWatcherManager) Name() string { return "dns-configuration-watcher-manager" } -func (s *DNSConfigurationWatcherManager) Dependencies() []string { - return []string{"infrastructure-services-manager"} -} - -func (s *DNSConfigurationWatcherManager) Run(ctx context.Context, ready chan<- struct{}, stopped chan<- struct{}) error { - defer close(stopped) - - if s.file == "" { - klog.Infof("%s is disabled (not configured)", s.Name()) - close(ready) - return ctx.Err() - } - - kubeClient, err := s.createKubeClient() - if err != nil { - klog.Errorf("%s failed to create Kubernetes client: %v", s.Name(), err) - return err - } - - if err := s.updateConfigMap(ctx, kubeClient); err != nil { - klog.Errorf("%s failed to create initial ConfigMap: %v", s.Name(), err) - return err - } - - watcher, err := fsnotify.NewWatcher() - if err != nil { - klog.Errorf("%s failed to create file watcher: %v", s.Name(), err) - return err - } - defer func() { - if cerr := watcher.Close(); cerr != nil { - klog.Errorf("%s failed to close file watcher: %v", s.Name(), cerr) - } - }() - - if err := s.setupWatches(watcher); err != nil { - return err - } - close(ready) - - lastHash, err := s.getFileHash() - if err != nil { - klog.Warningf("%s failed to get initial file hash: %v", s.Name(), err) - } - - klog.Infof("%s is ready and watching %s", s.Name(), s.file) - - return s.eventLoop(ctx, watcher, kubeClient, lastHash) -} - -func (s *DNSConfigurationWatcherManager) setupWatches(watcher *fsnotify.Watcher) error { - filesToWatch := []string{ - s.file, - filepath.Dir(s.file), - } - for i, file := range filesToWatch { - if err := watcher.Add(file); err != nil { - if i == 0 { - klog.Errorf("%s failed to watch DNS config file %s: %v", s.Name(), s.file, err) - return err - } - klog.Warningf("%s failed to watch DNS config directory %s: %v", s.Name(), file, err) - } - } - return nil -} - -func (s *DNSConfigurationWatcherManager) eventLoop(ctx context.Context, watcher *fsnotify.Watcher, kubeClient kubernetes.Interface, initHash string) error { - lastHash := initHash - - for { - select { - case <-ctx.Done(): - klog.Infof("%s stopping", s.Name()) - return ctx.Err() - - case event, ok := <-watcher.Events: - if !ok { - return fmt.Errorf("%s watcher channel closed", s.Name()) - } - if s.isRelevantEvent(event) { - if event.Op&fsnotify.Create == fsnotify.Create { - _ = watcher.Add(s.file) - } - updated, newHash, updateErr := s.handleChange(ctx, kubeClient, lastHash) - if updateErr != nil { - klog.Errorf("%s failed to process DNS config file change: %v", s.Name(), updateErr) - continue - } - if updated { - lastHash = newHash - } - } - - case err, ok := <-watcher.Errors: - if !ok { - return fmt.Errorf("%s watcher error channel closed", s.Name()) - } - klog.Errorf("%s watcher error: %v", s.Name(), err) - } - } -} - -func (s *DNSConfigurationWatcherManager) isRelevantEvent(event fsnotify.Event) bool { - if event.Name != s.file { - return false - } - const mask = fsnotify.Write | fsnotify.Create | fsnotify.Rename | fsnotify.Remove - return event.Op&mask != 0 -} - -func (s *DNSConfigurationWatcherManager) handleChange(ctx context.Context, kubeClient kubernetes.Interface, lastHash string) (bool, string, error) { - klog.Infof("%s detected change in DNS config file: %s", s.Name(), s.file) - currentHash, err := s.getFileHash() - if err != nil { - klog.Warningf("%s failed to get file hash after change: %v", s.Name(), err) - return false, lastHash, err - } - if currentHash == lastHash { - klog.V(2).Infof("%s file hash unchanged, skipping update", s.Name()) - return false, lastHash, nil - } - if err := s.updateConfigMap(ctx, kubeClient); err != nil { - klog.Errorf("%s failed to update ConfigMap: %v", s.Name(), err) - return false, lastHash, err - } - klog.Infof("%s successfully updated ConfigMap %s/%s", s.Name(), dnsConfigMapNamespace, dnsConfigMapName) - return true, currentHash, nil -} - -func (s *DNSConfigurationWatcherManager) createKubeClient() (*kubernetes.Clientset, error) { - cfg, err := clientcmd.BuildConfigFromFlags("", s.kubeconfig) - if err != nil { - return nil, fmt.Errorf("failed to build kubeconfig: %w", err) - } - client, err := kubernetes.NewForConfig(cfg) - if err != nil { - return nil, fmt.Errorf("failed to create Kubernetes client: %w", err) - } - return client, nil -} - -func (s *DNSConfigurationWatcherManager) updateConfigMap(ctx context.Context, client kubernetes.Interface) error { - content, err := os.ReadFile(s.file) - if err != nil { - return fmt.Errorf("failed to read DNS config file %s: %w", s.file, err) - } - - return s.createOrUpdateConfigMap(ctx, client, string(content)) -} - -func (s *DNSConfigurationWatcherManager) createOrUpdateConfigMap(ctx context.Context, client kubernetes.Interface, corefileContent string) error { - configMap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: dnsConfigMapName, - Namespace: dnsConfigMapNamespace, - Labels: map[string]string{ - "dns.operator.openshift.io/owning-dns": "default", - }, - Annotations: map[string]string{ - "microshift.io/dns-config-file": s.file, - "microshift.io/last-updated": time.Now().Format(time.RFC3339), - }, + return &DNSConfigurationWatcherManager{fileWatcher{cfg: fileWatcherConfig{ + serviceName: "dns-configuration-watcher-manager", + dependencies: []string{"infrastructure-services-manager"}, + file: cfg.DNS.ConfigFile, + kubeconfig: cfg.KubeConfigPath(config.KubeAdmin), + enabled: cfg.DNS.ConfigFile != "", + configMapNamespace: "openshift-dns", + configMapName: "dns-default", + configMapDataKey: "Corefile", + labels: map[string]string{ + "dns.operator.openshift.io/owning-dns": "default", }, - Data: map[string]string{ - dnsConfigMapKey: corefileContent, + annotations: map[string]string{ + "microshift.io/dns-config-file": cfg.DNS.ConfigFile, }, - } - - configMapsClient := client.CoreV1().ConfigMaps(dnsConfigMapNamespace) - - existing, err := configMapsClient.Get(ctx, dnsConfigMapName, metav1.GetOptions{}) - switch { - case apierrors.IsNotFound(err): - if _, err = configMapsClient.Create(ctx, configMap, metav1.CreateOptions{}); err != nil { - return fmt.Errorf("failed to create ConfigMap: %w", err) - } - klog.Infof("%s created ConfigMap %s/%s", s.Name(), dnsConfigMapNamespace, dnsConfigMapName) - case err != nil: - return fmt.Errorf("failed to get ConfigMap: %w", err) - default: - existing.Data = configMap.Data - if existing.Annotations == nil { - existing.Annotations = map[string]string{} - } - for k, v := range configMap.Annotations { - existing.Annotations[k] = v - } - if _, err = configMapsClient.Update(ctx, existing, metav1.UpdateOptions{}); err != nil { - return fmt.Errorf("failed to update ConfigMap: %w", err) - } - klog.V(2).Infof("%s updated ConfigMap %s/%s", s.Name(), dnsConfigMapNamespace, dnsConfigMapName) - } - - return nil -} - -func (s *DNSConfigurationWatcherManager) getFileHash() (string, error) { - content, err := os.ReadFile(s.file) - if err != nil { - return "", err - } - hash := sha256.Sum256(content) - return fmt.Sprintf("%x", hash), nil + eventMask: fsnotify.Write | fsnotify.Create | fsnotify.Rename | fsnotify.Remove, + reAddOnCreate: true, + mergeAnnotations: true, + deleteOnDisable: false, + }}} } diff --git a/pkg/controllers/filewatcher.go b/pkg/controllers/filewatcher.go new file mode 100644 index 0000000000..be0833a555 --- /dev/null +++ b/pkg/controllers/filewatcher.go @@ -0,0 +1,280 @@ +package controllers + +import ( + "context" + "crypto/sha256" + "fmt" + "os" + "path/filepath" + "time" + + "github.com/fsnotify/fsnotify" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/clientcmd" + "k8s.io/klog/v2" +) + +type fileWatcherConfig struct { + serviceName string + dependencies []string + file string + kubeconfig string + enabled bool + configMapNamespace string + configMapName string + configMapDataKey string + labels map[string]string + annotations map[string]string + eventMask fsnotify.Op + reAddOnCreate bool + mergeAnnotations bool + deleteOnDisable bool +} + +type fileWatcher struct { + cfg fileWatcherConfig +} + +func (fw *fileWatcher) Name() string { return fw.cfg.serviceName } +func (fw *fileWatcher) Dependencies() []string { return fw.cfg.dependencies } + +func (fw *fileWatcher) Run(ctx context.Context, ready chan<- struct{}, stopped chan<- struct{}) error { + defer close(stopped) + + if !fw.cfg.enabled { + klog.Infof("%s is disabled (not configured)", fw.Name()) + fw.cleanupOnDisable(ctx) + close(ready) + return ctx.Err() + } + + kubeClient, err := fw.createKubeClient() + if err != nil { + klog.Errorf("%s failed to create Kubernetes client: %v", fw.Name(), err) + return err + } + + if err := fw.updateConfigMap(ctx, kubeClient); err != nil { + klog.Errorf("%s failed to create initial ConfigMap: %v", fw.Name(), err) + return err + } + + watcher, err := fsnotify.NewWatcher() + if err != nil { + klog.Errorf("%s failed to create file watcher: %v", fw.Name(), err) + return err + } + defer func() { + if cerr := watcher.Close(); cerr != nil { + klog.Errorf("%s failed to close file watcher: %v", fw.Name(), cerr) + } + }() + + if err := fw.setupWatches(watcher); err != nil { + return err + } + close(ready) + + lastHash, err := fw.getFileHash() + if err != nil { + klog.Warningf("%s failed to get initial file hash: %v", fw.Name(), err) + } + + klog.Infof("%s is ready and watching %s", fw.Name(), fw.cfg.file) + + return fw.eventLoop(ctx, watcher, kubeClient, lastHash) +} + +func (fw *fileWatcher) setupWatches(watcher *fsnotify.Watcher) error { + filesToWatch := []string{ + fw.cfg.file, + filepath.Dir(fw.cfg.file), + } + for i, file := range filesToWatch { + if err := watcher.Add(file); err != nil { + if i == 0 { + klog.Errorf("%s failed to watch file %s: %v", fw.Name(), fw.cfg.file, err) + return err + } + klog.Warningf("%s failed to watch directory %s: %v", fw.Name(), file, err) + } + } + return nil +} + +func (fw *fileWatcher) eventLoop(ctx context.Context, watcher *fsnotify.Watcher, kubeClient kubernetes.Interface, initHash string) error { + lastHash := initHash + + for { + select { + case <-ctx.Done(): + klog.Infof("%s stopping", fw.Name()) + return ctx.Err() + + case event, ok := <-watcher.Events: + if !ok { + return fmt.Errorf("%s watcher channel closed", fw.Name()) + } + if fw.isRelevantEvent(event) { + if fw.cfg.reAddOnCreate && event.Op&fsnotify.Create == fsnotify.Create { + _ = watcher.Add(fw.cfg.file) + } + updated, newHash, updateErr := fw.handleChange(ctx, kubeClient, lastHash) + if updateErr != nil { + klog.Errorf("%s failed to process file change: %v", fw.Name(), updateErr) + continue + } + if updated { + lastHash = newHash + } + } + + case err, ok := <-watcher.Errors: + if !ok { + return fmt.Errorf("%s watcher error channel closed", fw.Name()) + } + klog.Errorf("%s watcher error: %v", fw.Name(), err) + } + } +} + +func (fw *fileWatcher) isRelevantEvent(event fsnotify.Event) bool { + if event.Name != fw.cfg.file { + return false + } + return event.Op&fw.cfg.eventMask != 0 +} + +func (fw *fileWatcher) handleChange(ctx context.Context, kubeClient kubernetes.Interface, lastHash string) (bool, string, error) { + klog.Infof("%s detected change in %s", fw.Name(), fw.cfg.file) + currentHash, err := fw.getFileHash() + if err != nil { + klog.Warningf("%s failed to get file hash after change: %v", fw.Name(), err) + return false, lastHash, err + } + if currentHash == lastHash { + klog.V(2).Infof("%s file hash unchanged, skipping update", fw.Name()) + return false, lastHash, nil + } + if err := fw.updateConfigMap(ctx, kubeClient); err != nil { + klog.Errorf("%s failed to update ConfigMap: %v", fw.Name(), err) + return false, lastHash, err + } + klog.Infof("%s successfully updated ConfigMap %s/%s", fw.Name(), fw.cfg.configMapNamespace, fw.cfg.configMapName) + return true, currentHash, nil +} + +func (fw *fileWatcher) createKubeClient() (*kubernetes.Clientset, error) { + cfg, err := clientcmd.BuildConfigFromFlags("", fw.cfg.kubeconfig) + if err != nil { + return nil, fmt.Errorf("failed to build kubeconfig: %w", err) + } + client, err := kubernetes.NewForConfig(cfg) + if err != nil { + return nil, fmt.Errorf("failed to create Kubernetes client: %w", err) + } + return client, nil +} + +func (fw *fileWatcher) updateConfigMap(ctx context.Context, client kubernetes.Interface) error { + content, err := os.ReadFile(fw.cfg.file) + if err != nil { + return fmt.Errorf("failed to read file %s: %w", fw.cfg.file, err) + } + return fw.createOrUpdateConfigMap(ctx, client, string(content)) +} + +func (fw *fileWatcher) createOrUpdateConfigMap(ctx context.Context, client kubernetes.Interface, content string) error { + annotations := fw.buildAnnotations() + configMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: fw.cfg.configMapName, + Namespace: fw.cfg.configMapNamespace, + Labels: fw.cfg.labels, + Annotations: annotations, + }, + Data: map[string]string{ + fw.cfg.configMapDataKey: content, + }, + } + + configMapsClient := client.CoreV1().ConfigMaps(fw.cfg.configMapNamespace) + + existing, err := configMapsClient.Get(ctx, fw.cfg.configMapName, metav1.GetOptions{}) + switch { + case apierrors.IsNotFound(err): + if _, err = configMapsClient.Create(ctx, configMap, metav1.CreateOptions{}); err != nil { + return fmt.Errorf("failed to create ConfigMap: %w", err) + } + klog.Infof("%s created ConfigMap %s/%s", fw.Name(), fw.cfg.configMapNamespace, fw.cfg.configMapName) + case err != nil: + return fmt.Errorf("failed to get ConfigMap: %w", err) + default: + existing.Data = configMap.Data + if fw.cfg.mergeAnnotations { + if existing.Annotations == nil { + existing.Annotations = map[string]string{} + } + for k, v := range annotations { + existing.Annotations[k] = v + } + } else { + existing.Annotations = annotations + } + if _, err = configMapsClient.Update(ctx, existing, metav1.UpdateOptions{}); err != nil { + return fmt.Errorf("failed to update ConfigMap: %w", err) + } + klog.V(2).Infof("%s updated ConfigMap %s/%s", fw.Name(), fw.cfg.configMapNamespace, fw.cfg.configMapName) + } + + return nil +} + +func (fw *fileWatcher) buildAnnotations() map[string]string { + annotations := make(map[string]string, len(fw.cfg.annotations)+1) + for k, v := range fw.cfg.annotations { + annotations[k] = v + } + annotations["microshift.io/last-updated"] = time.Now().Format(time.RFC3339) + return annotations +} + +func (fw *fileWatcher) cleanupOnDisable(ctx context.Context) { + if !fw.cfg.deleteOnDisable { + return + } + kubeClient, err := fw.createKubeClient() + if err != nil { + klog.Warningf("%s could not create Kubernetes client to delete ConfigMap: %v", fw.Name(), err) + return + } + if err := fw.deleteConfigMap(ctx, kubeClient); err != nil { + klog.Warningf("%s failed to delete ConfigMap when disabled: %v", fw.Name(), err) + } +} + +func (fw *fileWatcher) deleteConfigMap(ctx context.Context, client kubernetes.Interface) error { + configMapsClient := client.CoreV1().ConfigMaps(fw.cfg.configMapNamespace) + err := configMapsClient.Delete(ctx, fw.cfg.configMapName, metav1.DeleteOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + klog.V(2).Infof("%s ConfigMap %s/%s does not exist, nothing to delete", fw.Name(), fw.cfg.configMapNamespace, fw.cfg.configMapName) + return nil + } + return fmt.Errorf("failed to delete ConfigMap: %w", err) + } + klog.Infof("%s deleted ConfigMap %s/%s", fw.Name(), fw.cfg.configMapNamespace, fw.cfg.configMapName) + return nil +} + +func (fw *fileWatcher) getFileHash() (string, error) { + content, err := os.ReadFile(fw.cfg.file) + if err != nil { + return "", err + } + hash := sha256.Sum256(content) + return fmt.Sprintf("%x", hash), nil +} diff --git a/pkg/controllers/hostswatcher.go b/pkg/controllers/hostswatcher.go index f666cd3e90..cdf3bd2f16 100644 --- a/pkg/controllers/hostswatcher.go +++ b/pkg/controllers/hostswatcher.go @@ -1,280 +1,36 @@ package controllers import ( - "context" - "crypto/sha256" - "fmt" - "os" - "path/filepath" - "time" - "github.com/fsnotify/fsnotify" "github.com/openshift/microshift/pkg/config" - corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes" - "k8s.io/client-go/tools/clientcmd" - "k8s.io/klog/v2" -) - -const ( - targetNameSpace = "openshift-dns" - configMapName = "hosts-file" ) type HostsWatcherManager struct { - file string - status config.HostsStatusEnum - kubeconfig string + fileWatcher } func NewHostsWatcherManager(cfg *config.Config) *HostsWatcherManager { - return &HostsWatcherManager{ - file: cfg.DNS.Hosts.File, - status: cfg.DNS.Hosts.Status, - kubeconfig: cfg.KubeConfigPath(config.KubeAdmin), - } -} - -func (s *HostsWatcherManager) Name() string { return "hosts-watcher-manager" } -func (s *HostsWatcherManager) Dependencies() []string { return []string{"kube-apiserver"} } - -func (s *HostsWatcherManager) Run(ctx context.Context, ready chan<- struct{}, stopped chan<- struct{}) error { - defer close(stopped) - - if s.status != config.HostsStatusEnabled { - klog.Infof("%s is disabled (not configured)", s.Name()) - // Delete ConfigMap if it exists when service is disabled - if kubeClient, err := s.createKubeClient(); err == nil { - if err := s.deleteConfigMap(ctx, kubeClient); err != nil { - klog.Warningf("%s failed to delete ConfigMap when disabled: %v", s.Name(), err) - } - } else { - klog.Warningf("%s could not create Kubernetes client to delete ConfigMap: %v", s.Name(), err) - } - defer close(ready) - return ctx.Err() - } - - kubeClient, err := s.createKubeClient() - if err != nil { - klog.Errorf("%s failed to create Kubernetes client: %v", s.Name(), err) - return err - } - - if err := s.updateConfigMaps(ctx, kubeClient); err != nil { - klog.Errorf("%s failed to create initial ConfigMaps: %v", s.Name(), err) - return err - } - - watcher, err := fsnotify.NewWatcher() - if err != nil { - klog.Errorf("%s failed to create file watcher: %v", s.Name(), err) - return err - } - defer func() { - if cerr := watcher.Close(); cerr != nil { - klog.Errorf("%s failed to close file watcher: %v", s.Name(), cerr) - } - }() - - if err := s.setupWatches(watcher); err != nil { - return err - } - close(ready) - klog.Infof("%s ready and watching for changes", s.Name()) - - lastHash, err := s.getFileHash(s.file) - if err != nil { - klog.Warningf("%s failed to get initial file hash: %v", s.Name(), err) - } - - klog.Infof("%s is ready", s.Name()) - - return s.eventLoop(ctx, watcher, kubeClient, lastHash) -} - -func (s *HostsWatcherManager) setupWatches(watcher *fsnotify.Watcher) error { - filesToWatch := []string{ - s.file, - filepath.Dir(s.file), - } - for i, file := range filesToWatch { - if err := watcher.Add(file); err != nil { - // Warn if directory, error out if file - if i == 0 { - klog.Errorf("%s failed to watch hosts file %s: %v", s.Name(), s.file, err) - return err - } - klog.Warningf("%s failed to watch hosts directory %s: %v", s.Name(), file, err) - } - } - return nil -} - -func (s *HostsWatcherManager) eventLoop(ctx context.Context, watcher *fsnotify.Watcher, kubeClient kubernetes.Interface, initHash string) error { - lastHash := initHash - - for { - select { - case <-ctx.Done(): - klog.Infof("%s stopping", s.Name()) - return watcher.Close() - - case event, ok := <-watcher.Events: - if !ok { - return fmt.Errorf("%s watcher channel closed", s.Name()) - } - if s.isRelevantHostsEvent(event) { - updated, newHash, updateErr := s.handleHostsChange(ctx, kubeClient, lastHash) - if updateErr != nil { - klog.Errorf("%s failed to process hosts file change: %v", s.Name(), updateErr) - continue - } - if updated { - lastHash = newHash - } - } - - case err, ok := <-watcher.Errors: - if !ok { - return fmt.Errorf("%s watcher error channel closed", s.Name()) - } - klog.Errorf("%s watcher error: %v", s.Name(), err) - } - } -} - -func (s *HostsWatcherManager) isRelevantHostsEvent(event fsnotify.Event) bool { - if event.Name != s.file { - return false - } - return event.Op&fsnotify.Write == fsnotify.Write || event.Op&fsnotify.Create == fsnotify.Create -} - -func (s *HostsWatcherManager) handleHostsChange(ctx context.Context, kubeClient kubernetes.Interface, lastHash string) (bool, string, error) { - klog.Infof("%s detected change in hosts file: %s", s.Name(), s.file) - currentHash, err := s.getFileHash(s.file) - if err != nil { - klog.Warningf("%s failed to get file hash after change: %v", s.Name(), err) - return false, lastHash, err - } - if currentHash == lastHash { - klog.V(2).Infof("%s file hash unchanged, skipping update", s.Name()) - return false, lastHash, nil - } - if err := s.updateConfigMaps(ctx, kubeClient); err != nil { - klog.Errorf("%s failed to update ConfigMaps: %v", s.Name(), err) - return false, currentHash, err - } else { - klog.Infof("%s successfully updated ConfigMaps in namespaces: %v", s.Name(), targetNameSpace) - } - return true, currentHash, nil -} - -func (s *HostsWatcherManager) createKubeClient() (*kubernetes.Clientset, error) { - config, err := clientcmd.BuildConfigFromFlags("", s.kubeconfig) - if err != nil { - return nil, fmt.Errorf("failed to build kubeconfig: %w", err) - } - - client, err := kubernetes.NewForConfig(config) - if err != nil { - return nil, fmt.Errorf("failed to create Kubernetes client: %w", err) - } - - return client, nil -} - -func (s *HostsWatcherManager) updateConfigMaps(ctx context.Context, client kubernetes.Interface) error { - hostsContent, err := s.readHostsFile() - if err != nil { - return fmt.Errorf("failed to read hosts file: %w", err) - } - - if err := s.createOrUpdateConfigMap(ctx, client, targetNameSpace, hostsContent); err != nil { - klog.Errorf("%s failed to update ConfigMap in namespace %s: %v", s.Name(), targetNameSpace, err) - } - - return nil -} - -func (s *HostsWatcherManager) createOrUpdateConfigMap(ctx context.Context, client kubernetes.Interface, namespace string, hostsContent string) error { - configMap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: configMapName, - Namespace: namespace, - Labels: map[string]string{ - "app.kubernetes.io/name": "microshift-hosts-watcher", - "app.kubernetes.io/component": "hosts-file-sync", - "app.kubernetes.io/managed-by": "microshift", - // Restrict access to only CoreDNS pods - "microshift.io/access-restricted": "coredns-only", - }, - Annotations: map[string]string{ - "microshift.io/hosts-file-path": s.file, - "microshift.io/last-updated": time.Now().Format(time.RFC3339), - }, + return &HostsWatcherManager{fileWatcher{cfg: fileWatcherConfig{ + serviceName: "hosts-watcher-manager", + dependencies: []string{"kube-apiserver"}, + file: cfg.DNS.Hosts.File, + kubeconfig: cfg.KubeConfigPath(config.KubeAdmin), + enabled: cfg.DNS.Hosts.Status == config.HostsStatusEnabled, + configMapNamespace: "openshift-dns", + configMapName: "hosts-file", + configMapDataKey: "hosts", + labels: map[string]string{ + "app.kubernetes.io/name": "microshift-hosts-watcher", + "app.kubernetes.io/component": "hosts-file-sync", + "app.kubernetes.io/managed-by": "microshift", + "microshift.io/access-restricted": "coredns-only", }, - Data: map[string]string{ - "hosts": hostsContent, + annotations: map[string]string{ + "microshift.io/hosts-file-path": cfg.DNS.Hosts.File, }, - } - - configMapsClient := client.CoreV1().ConfigMaps(namespace) - - // Try to get existing ConfigMap - existing, err := configMapsClient.Get(ctx, configMapName, metav1.GetOptions{}) - if err != nil { - // ConfigMap doesn't exist, create it - _, err = configMapsClient.Create(ctx, configMap, metav1.CreateOptions{}) - if err != nil { - return fmt.Errorf("failed to create ConfigMap: %w", err) - } - klog.Infof("%s created ConfigMap %s in namespace %s", s.Name(), configMapName, namespace) - } else { - // ConfigMap exists, update it - existing.Data = configMap.Data - existing.Annotations = configMap.Annotations - _, err = configMapsClient.Update(ctx, existing, metav1.UpdateOptions{}) - if err != nil { - return fmt.Errorf("failed to update ConfigMap: %w", err) - } - klog.V(2).Infof("%s updated ConfigMap %s in namespace %s", s.Name(), configMapName, namespace) - } - - return nil -} - -func (s *HostsWatcherManager) readHostsFile() (string, error) { - content, err := os.ReadFile(s.file) - if err != nil { - return "", fmt.Errorf("failed to read hosts file %s: %w", s.file, err) - } - return string(content), nil -} - -func (s *HostsWatcherManager) getFileHash(filePath string) (string, error) { - content, err := os.ReadFile(filePath) - if err != nil { - return "", err - } - hash := sha256.Sum256(content) - return fmt.Sprintf("%x", hash), nil -} - -func (s *HostsWatcherManager) deleteConfigMap(ctx context.Context, client kubernetes.Interface) error { - configMapsClient := client.CoreV1().ConfigMaps(targetNameSpace) - err := configMapsClient.Delete(ctx, configMapName, metav1.DeleteOptions{}) - if err != nil { - // If ConfigMap doesn't exist, that's fine - it's already deleted - if apierrors.IsNotFound(err) { - klog.V(2).Infof("%s ConfigMap %s in namespace %s does not exist, nothing to delete", s.Name(), configMapName, targetNameSpace) - return nil - } - return fmt.Errorf("failed to delete ConfigMap: %w", err) - } - klog.Infof("%s deleted ConfigMap %s in namespace %s", s.Name(), configMapName, targetNameSpace) - return nil + eventMask: fsnotify.Write | fsnotify.Create, + reAddOnCreate: false, + mergeAnnotations: false, + deleteOnDisable: true, + }}} }