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.

Sign up for GitHub

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

Handle non-standard coredns ConfigMap name in SD controller #3295

Merged
merged 4 commits into from
Dec 9, 2024
Merged

Handle non-standard coredns ConfigMap name in SD controller #3295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8043f4d
Define and reuse constants in servicediscovery module
tpantelis Dec 4, 2024
d045c3e
Add unit test to cover microshift DNS ConfigMap
tpantelis Dec 4, 2024
9e5a83b
Handle non-standard coredns ConfigMap name in SD controller
tpantelis Dec 5, 2024
89ba5ec
Merge branch 'devel' into coredns_cm
tpantelis Dec 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ config/bundle/patches/submariner.csv.config.yaml
package/.image.*
output/
vendor/
*.coverprofile
*coverprofile*
.idea
.shflags
junit.xml
Expand Down
4 changes: 2 additions & 2 deletions controllers/servicediscovery/cleanup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func (r *Reconciler) doCleanup(ctx context.Context, instance *operatorv1alpha1.S
if instance.Spec.CoreDNSCustomConfig != nil && instance.Spec.CoreDNSCustomConfig.ConfigMapName != "" {
err = r.removeLighthouseConfigFromCustomDNSConfigMap(ctx, instance.Spec.CoreDNSCustomConfig)
} else {
err = r.updateLighthouseConfigInConfigMap(ctx, instance, defaultCoreDNSNamespace, coreDNSName, "")
err = r.updateLighthouseConfigInConfigMap(ctx, instance, DefaultCoreDNSNamespace, CoreDNSName, "")
}

if apierrors.IsNotFound(err) {
Expand Down Expand Up @@ -81,7 +81,7 @@ func (r *Reconciler) doCleanup(ctx context.Context, instance *operatorv1alpha1.S
Client: r.ScopedClient,
Components: components,
StartTime: instance.DeletionTimestamp.Time,
Log: log,
Log: log.Logger,
GetImageInfo: func(imageName, componentName string) (string, corev1.PullPolicy) {
return getImagePath(instance, imageName, componentName),
images.GetPullPolicy(instance.Spec.Version, instance.Spec.ImageOverrides[componentName])
Expand Down
101 changes: 55 additions & 46 deletions controllers/servicediscovery/servicediscovery_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
operatorv1 "github.com/openshift/api/operator/v1"
"github.com/pkg/errors"
"github.com/submariner-io/admiral/pkg/finalizer"
logw "github.com/submariner-io/admiral/pkg/log"
"github.com/submariner-io/admiral/pkg/names"
"github.com/submariner-io/admiral/pkg/resource"
"github.com/submariner-io/admiral/pkg/syncer/broker"
Expand Down Expand Up @@ -59,16 +60,17 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"
)

var log = logf.Log.WithName("controller_servicediscovery")
var log = logw.Logger{Logger: logf.Log.WithName("controller_servicediscovery")}

const (
componentName = "submariner-lighthouse"
defaultOpenShiftDNSController = "default"
lighthouseForwardPluginName = "lighthouse"
defaultCoreDNSNamespace = "kube-system"
coreDNSName = "coredns"
microshiftDNSNamespace = "openshift-dns"
microshiftDNSConfigMap = "dns-default"
Corefile = "Corefile"
DefaultOpenShiftDNSController = "default"
LighthouseForwardPluginName = "lighthouse"
DefaultCoreDNSNamespace = "kube-system"
CoreDNSName = "coredns"
MicroshiftDNSNamespace = "openshift-dns"
MicroshiftDNSConfigMap = "dns-default"
coreDNSDefaultPort = "53"
)

Expand Down Expand Up @@ -150,12 +152,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
return reconcile.Result{}, err
}
} else {
err = r.configureDNSConfigMap(ctx, instance, defaultCoreDNSNamespace, coreDNSName)
}

if apierrors.IsNotFound(err) {
// Try to update Openshift-DNS
return reconcile.Result{}, r.configureOpenshiftClusterDNSOperator(ctx, instance)
err = r.updateDNSConfig(ctx, instance)
}

return reconcile.Result{}, err
Expand Down Expand Up @@ -290,7 +287,7 @@ prometheus :9153
Labels: labels,
},
Data: map[string]string{
"Corefile": expectedCorefile,
Corefile: expectedCorefile,
},
}
}
Expand Down Expand Up @@ -363,7 +360,7 @@ func newLighthouseCoreDNSDeployment(cr *submarinerv1alpha1.ServiceDiscovery) *ap
{Name: "config-volume", VolumeSource: corev1.VolumeSource{ConfigMap: &corev1.ConfigMapVolumeSource{
LocalObjectReference: corev1.LocalObjectReference{Name: names.LighthouseCoreDNSComponent},
Items: []corev1.KeyToPath{
{Key: "Corefile", Path: "Corefile"},
{Key: Corefile, Path: Corefile},
},
DefaultMode: ptr.To(int32(0o644)),
}}},
Expand Down Expand Up @@ -420,7 +417,7 @@ func getCustomCoreDNSNamespace(config *submarinerv1alpha1.CoreDNSCustomConfig) s
return config.Namespace
}

return defaultCoreDNSNamespace
return DefaultCoreDNSNamespace
}

func (r *Reconciler) updateDNSCustomConfigMap(ctx context.Context, cr *submarinerv1alpha1.ServiceDiscovery,
Expand Down Expand Up @@ -462,9 +459,7 @@ func (r *Reconciler) updateDNSCustomConfigMap(ctx context.Context, cr *submarine
return errors.Wrap(err, "error updating DNS custom ConfigMap")
}

func (r *Reconciler) configureDNSConfigMap(ctx context.Context, cr *submarinerv1alpha1.ServiceDiscovery, configMapNamespace,
configMapName string,
) error {
func (r *Reconciler) updateDNSConfig(ctx context.Context, cr *submarinerv1alpha1.ServiceDiscovery) error {
lighthouseDNSService := &corev1.Service{}

err := r.ScopedClient.Get(ctx, types.NamespacedName{Name: names.LighthouseCoreDNSComponent, Namespace: cr.Namespace},
Expand All @@ -477,7 +472,35 @@ func (r *Reconciler) configureDNSConfigMap(ctx context.Context, cr *submarinerv1
return goerrors.New("the lighthouse DNS Service ClusterIP is not set")
}

return r.updateLighthouseConfigInConfigMap(ctx, cr, configMapNamespace, configMapName, lighthouseDNSService.Spec.ClusterIP)
err = r.updateLighthouseConfigInConfigMap(ctx, cr, DefaultCoreDNSNamespace, CoreDNSName, lighthouseDNSService.Spec.ClusterIP)

if apierrors.IsNotFound(err) {
// Some providers may not use the exact "coredns" name but use it as a suffix, eg RKE "rke2-coredns".
configMaps := &corev1.ConfigMapList{}

listErr := r.GeneralClient.List(ctx, configMaps, controllerClient.InNamespace(DefaultCoreDNSNamespace))
if listErr != nil {
return errors.Wrapf(err, "error listing ConfigMaps in %q", DefaultCoreDNSNamespace)
}

suffix := "-" + CoreDNSName

for i := range configMaps.Items {
cm := &configMaps.Items[i]

_, hasCorefile := cm.Data[Corefile]
if strings.HasSuffix(cm.Name, suffix) && hasCorefile {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on this comment, we could we just check for the existence of the Corefile data entry. @vthapar @aswinsuryan WDYT?

Copy link
Contributor

@aswinsuryan aswinsuryan Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this looks good , since it is listing all the configmaps in DefaultCoreDNSNamespace , we should be safe as there should be only one coredns corefile normally in this namespace.

return r.updateLighthouseConfigInConfigMap(ctx, cr, cm.Namespace, cm.Name, lighthouseDNSService.Spec.ClusterIP)
}
}
}

if apierrors.IsNotFound(err) {
// Try to update Openshift-DNS
return r.updateLighthouseConfigInOpenshiftDNSOperator(ctx, cr, lighthouseDNSService.Spec.ClusterIP)
}

return err
}

func (r *Reconciler) updateLighthouseConfigInConfigMap(ctx context.Context, cr *submarinerv1alpha1.ServiceDiscovery,
Expand All @@ -486,13 +509,13 @@ func (r *Reconciler) updateLighthouseConfigInConfigMap(ctx context.Context, cr *
configMap := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: configMapNamespace, Name: configMapName}}
err := util.MustUpdate[*corev1.ConfigMap](ctx, resource.ForControllerClient(r.GeneralClient, configMap.Namespace, configMap), configMap,
func(existing *corev1.ConfigMap) (*corev1.ConfigMap, error) {
coreFile := existing.Data["Corefile"]
coreFile := existing.Data[Corefile]
if strings.Contains(coreFile, "lighthouse-start") {
// Assume this means we've already set the ConfigMap up, first remove existing lighthouse config
newCoreStr := ""
skip := false

log.Info("coredns configmap has lighthouse configuration hence updating")
log.Infof("Coredns ConfigMap \"%s/%s\" has lighthouse configuration - updating it", configMapNamespace, configMapName)

lines := strings.Split(coreFile, "\n")
for _, line := range lines {
Expand All @@ -512,7 +535,8 @@ func (r *Reconciler) updateLighthouseConfigInConfigMap(ctx context.Context, cr *

coreFile = newCoreStr
} else {
log.Info("coredns configmap does not have lighthouse configuration hence creating")
log.Infof("Coredns ConfigMap \"%s/%s\" does not have lighthouse configuration - adding it",
configMapNamespace, configMapName)
}

if clusterIP != "" {
Expand All @@ -527,8 +551,9 @@ func (r *Reconciler) updateLighthouseConfigInConfigMap(ctx context.Context, cr *
coreFile = expectedCorefile + "#lighthouse-end\n" + coreFile
}

log.Info("Updated coredns ConfigMap " + coreFile)
existing.Data["Corefile"] = coreFile
log.Infof("Updated coredns ConfigMap \"%s/%s\": %s", configMapNamespace, configMapName, coreFile)

existing.Data[Corefile] = coreFile

return existing, nil
})
Expand All @@ -548,33 +573,17 @@ func findCoreDNSListeningPort(coreFile string) string {
return coreDNSPort
}

func (r *Reconciler) configureOpenshiftClusterDNSOperator(ctx context.Context, instance *submarinerv1alpha1.ServiceDiscovery) error {
lighthouseDNSService := &corev1.Service{}

err := r.ScopedClient.Get(ctx, types.NamespacedName{Name: names.LighthouseCoreDNSComponent, Namespace: instance.Namespace},
lighthouseDNSService)
if err != nil {
return errors.Wrap(err, "error retrieving lighthouse DNS Service")
}

if lighthouseDNSService.Spec.ClusterIP == "" {
return goerrors.New("the lighthouse DNS Service ClusterIP is not set")
}

return r.updateLighthouseConfigInOpenshiftDNSOperator(ctx, instance, lighthouseDNSService.Spec.ClusterIP)
}

func (r *Reconciler) updateLighthouseConfigInOpenshiftDNSOperator(ctx context.Context, instance *submarinerv1alpha1.ServiceDiscovery,
clusterIP string,
) error {
retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
dnsOperator := &operatorv1.DNS{}
if err := r.GeneralClient.Get(ctx, types.NamespacedName{Name: defaultOpenShiftDNSController}, dnsOperator); err != nil {
if err := r.GeneralClient.Get(ctx, types.NamespacedName{Name: DefaultOpenShiftDNSController}, dnsOperator); err != nil {
// microshift uses the coredns image, but the DNS operator and CRDs are off
if resource.IsNotFoundErr(err) {
err = r.configureDNSConfigMap(ctx, instance, microshiftDNSNamespace, microshiftDNSConfigMap)
err = r.updateLighthouseConfigInConfigMap(ctx, instance, MicroshiftDNSNamespace, MicroshiftDNSConfigMap, clusterIP)
return errors.Wrapf(err, "error trying to update microshift coredns configmap %q in namespace %q",
microshiftDNSNamespace, microshiftDNSNamespace)
MicroshiftDNSNamespace, MicroshiftDNSNamespace)
}

return err
Expand Down Expand Up @@ -618,7 +627,7 @@ func getUpdatedForwardServers(instance *submarinerv1alpha1.ServiceDiscovery, dns
lighthouseDomains := buildDomains(instance)

for _, forwardServer := range dnsOperator.Spec.Servers {
if forwardServer.Name == lighthouseForwardPluginName {
if forwardServer.Name == LighthouseForwardPluginName {
containsLighthouse = true

existingDomains = append(existingDomains, forwardServer.Zones...)
Expand Down Expand Up @@ -655,7 +664,7 @@ func getUpdatedForwardServers(instance *submarinerv1alpha1.ServiceDiscovery, dns

for _, domain := range lighthouseDomains {
lighthouseServer := operatorv1.Server{
Name: lighthouseForwardPluginName,
Name: LighthouseForwardPluginName,
Zones: []string{domain},
ForwardPlugin: operatorv1.ForwardPlugin{
Upstreams: []string{clusterIP},
Expand Down
43 changes: 38 additions & 5 deletions controllers/servicediscovery/servicediscovery_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
. "github.com/onsi/gomega"
"github.com/submariner-io/admiral/pkg/names"
submariner_v1 "github.com/submariner-io/submariner-operator/api/v1alpha1"
"github.com/submariner-io/submariner-operator/controllers/servicediscovery"
opnames "github.com/submariner-io/submariner-operator/pkg/names"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -92,7 +93,7 @@ func testReconciliation() {
})
})

When("the coredns ConfigMap exists", func() {
When("the default coredns ConfigMap exists", func() {
Context("and the lighthouse config isn't present", func() {
BeforeEach(func() {
t.InitScopedClientObjs = append(t.InitScopedClientObjs, newDNSService(clusterIP))
Expand All @@ -102,7 +103,7 @@ func testReconciliation() {
It("should add it", func(ctx SpecContext) {
t.AssertReconcileSuccess(ctx)

Expect(strings.TrimSpace(t.assertCoreDNSConfigMap(ctx).Data["Corefile"])).To(Equal(coreDNSCorefileData(clusterIP)))
Expect(getCorefileData(t.assertCoreDNSConfigMap(ctx))).To(Equal(coreDNSCorefileData(clusterIP)))
})
})

Expand All @@ -117,7 +118,7 @@ func testReconciliation() {
It("should update the lighthouse config", func(ctx SpecContext) {
t.AssertReconcileSuccess(ctx)

Expect(strings.TrimSpace(t.assertCoreDNSConfigMap(ctx).Data["Corefile"])).To(Equal(coreDNSCorefileData(updatedClusterIP)))
Expect(getCorefileData(t.assertCoreDNSConfigMap(ctx))).To(Equal(coreDNSCorefileData(updatedClusterIP)))
})
})

Expand All @@ -133,11 +134,28 @@ func testReconciliation() {

t.AssertReconcileSuccess(ctx)

Expect(strings.TrimSpace(t.assertCoreDNSConfigMap(ctx).Data["Corefile"])).To(Equal(coreDNSCorefileData(clusterIP)))
Expect(getCorefileData(t.assertCoreDNSConfigMap(ctx))).To(Equal(coreDNSCorefileData(clusterIP)))
})
})
})

When("a ConfigMap exists with a non-standard coredns name", func() {
nonStandardName := "rke2-coredns-rke2-coredns"

BeforeEach(func() {
t.InitScopedClientObjs = append(t.InitScopedClientObjs, newDNSService(clusterIP))
t.InitGeneralClientObjs = append(t.InitGeneralClientObjs, newDNSConfigMap(
nonStandardName, servicediscovery.DefaultCoreDNSNamespace, coreDNSCorefileData("")))
})

It("should update it with the lighthouse config", func(ctx SpecContext) {
t.AssertReconcileSuccess(ctx)

Expect(getCorefileData(t.assertConfigMap(ctx, nonStandardName,
servicediscovery.DefaultCoreDNSNamespace))).To(Equal(coreDNSCorefileData(clusterIP)))
})
})

When("a custom coredns config is specified", func() {
BeforeEach(func() {
t.serviceDiscovery.Spec.CoreDNSCustomConfig = &submariner_v1.CoreDNSCustomConfig{
Expand Down Expand Up @@ -196,6 +214,21 @@ func testReconciliation() {
})
})
})

When("the microshift DNS ConfigMap exists", func() {
BeforeEach(func() {
t.InitScopedClientObjs = append(t.InitScopedClientObjs, newDNSService(clusterIP))
t.InitGeneralClientObjs = append(t.InitGeneralClientObjs, newDNSConfigMap(
servicediscovery.MicroshiftDNSConfigMap, servicediscovery.MicroshiftDNSNamespace, coreDNSCorefileData("")))
})

It("should update it with the lighthouse config", func(ctx SpecContext) {
t.AssertReconcileSuccess(ctx)

Expect(getCorefileData(t.assertConfigMap(ctx, servicediscovery.MicroshiftDNSConfigMap,
servicediscovery.MicroshiftDNSNamespace))).To(Equal(coreDNSCorefileData(clusterIP)))
})
})
}

func testCoreDNSCleanup() {
Expand Down Expand Up @@ -226,7 +259,7 @@ func testCoreDNSCleanup() {
})

It("should remove the lighthouse config section", func(ctx SpecContext) {
Expect(strings.TrimSpace(t.assertCoreDNSConfigMap(ctx).Data["Corefile"])).To(Equal(coreDNSCorefileData("")))
Expect(getCorefileData(t.assertCoreDNSConfigMap(ctx))).To(Equal(coreDNSCorefileData("")))
})

t.testServiceDiscoveryDeleted()
Expand Down
Loading
Loading