Skip to content

Commit

Permalink
controller: Rework boolean logic for consistency
Browse files Browse the repository at this point in the history 
Switches all `ensureDNS<thing>` functions in `pkg/operator/controller`
to use a `have<thing>` boolean rather than manually checking if
`<thing> != nil`.
  • Loading branch information
@sgreene570
sgreene570 committed Jun 9, 2020
1 parent 0964612 commit f60ef55
Show file tree
Hide file tree
Showing 6 changed files with 65 additions and 64 deletions.
10 changes: 5 additions & 5 deletions pkg/operator/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ func (r *reconciler) ensureDNSNamespace() error {
logrus.Infof("created dns namespace: %s", ns.Name)
}

if _, err := r.ensureDNSClusterRole(); err != nil {
if _, _, err := r.ensureDNSClusterRole(); err != nil {
return fmt.Errorf("failed to ensure dns cluster role for %s: %v", manifests.DNSClusterRole().Name, err)
}

Expand Down Expand Up @@ -329,7 +329,7 @@ func (r *reconciler) ensureMetricsIntegration(dns *operatorv1.DNS, svc *corev1.S
logrus.Infof("created dns metrics role binding %s/%s", mrb.Namespace, mrb.Name)
}

if _, err := r.ensureServiceMonitor(dns, svc, daemonsetRef); err != nil {
if _, _, err := r.ensureServiceMonitor(dns, svc, daemonsetRef); err != nil {
return fmt.Errorf("failed to ensure servicemonitor for %s: %v", dns.Name, err)
}

Expand All @@ -346,7 +346,7 @@ func (r *reconciler) ensureDNS(dns *operatorv1.DNS) error {
}

errs := []error{}
if daemonset, err := r.ensureDNSDaemonSet(dns, clusterIP, clusterDomain); err != nil {
if _, daemonset, err := r.ensureDNSDaemonSet(dns, clusterIP, clusterDomain); err != nil {
errs = append(errs, fmt.Errorf("failed to ensure daemonset for dns %s: %v", dns.Name, err))
} else {
trueVar := true
Expand All @@ -358,10 +358,10 @@ func (r *reconciler) ensureDNS(dns *operatorv1.DNS) error {
Controller: &trueVar,
}

if _, err := r.ensureDNSConfigMap(dns, clusterDomain); err != nil {
if _, _, err := r.ensureDNSConfigMap(dns, clusterDomain); err != nil {
errs = append(errs, fmt.Errorf("failed to create configmap for dns %s: %v", dns.Name, err))
}
if svc, err := r.ensureDNSService(dns, clusterIP, daemonsetRef); err != nil {
if _, svc, err := r.ensureDNSService(dns, clusterIP, daemonsetRef); err != nil {
errs = append(errs, fmt.Errorf("failed to create service for dns %s: %v", dns.Name, err))
} else if err := r.ensureMetricsIntegration(dns, svc, daemonsetRef); err != nil {
errs = append(errs, fmt.Errorf("failed to integrate metrics with openshift-monitoring for dns %s: %v", dns.Name, err))
Expand Down
25 changes: 13 additions & 12 deletions pkg/operator/controller/controller_cluster_role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,37 +15,38 @@ import (
"k8s.io/apimachinery/pkg/types"
)

func (r *reconciler) ensureDNSClusterRole() (*rbacv1.ClusterRole, error) {
current, err := r.currentDNSClusterRole()
func (r *reconciler) ensureDNSClusterRole() (bool, *rbacv1.ClusterRole, error) {
haveCR, current, err := r.currentDNSClusterRole()
if err != nil {
return nil, err
return false, nil, err
}
desired := desiredDNSClusterRole()
// desiredDNSClusterRole() will panic if the desired ClusterRole cannot be created
desired := desiredDNSClus:erRole()

switch {
case desired != nil && current == nil:
case !haveCR:
if err := r.client.Create(context.TODO(), desired); err != nil {
return nil, fmt.Errorf("failed to create dns cluster role: %v", err)
return false, nil, fmt.Errorf("failed to create dns cluster role: %v", err)
}
logrus.Infof("created dns cluster role: %s/%s", desired.Namespace, desired.Name)
case desired != nil && current != nil:
case haveCR:
if err := r.updateDNSClusterRole(current, desired); err != nil {
return nil, err
return true, nil, err
}
}
return r.currentDNSClusterRole()
}

func (r *reconciler) currentDNSClusterRole() (*rbacv1.ClusterRole, error) {
func (r *reconciler) currentDNSClusterRole() (bool, *rbacv1.ClusterRole, error) {
current := &rbacv1.ClusterRole{}
err := r.client.Get(context.TODO(), types.NamespacedName{Name: manifests.DNSClusterRole().Name}, current)
if err != nil {
if errors.IsNotFound(err) {
return nil, nil
return false, nil, nil
}
return nil, err
return false, nil, err
}
return current, nil
return true, current, nil
}

func desiredDNSClusterRole() *rbacv1.ClusterRole {
Expand Down
24 changes: 12 additions & 12 deletions pkg/operator/controller/controller_dns_configmap.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,43 +47,43 @@ var corefileTemplate = template.Must(template.New("Corefile").Parse(`{{range .Se
`))

// ensureDNSConfigMap ensures that a configmap exists for a given DNS.
func (r *reconciler) ensureDNSConfigMap(dns *operatorv1.DNS, clusterDomain string) (*corev1.ConfigMap, error) {
current, err := r.currentDNSConfigMap(dns)
func (r *reconciler) ensureDNSConfigMap(dns *operatorv1.DNS, clusterDomain string) (bool, *corev1.ConfigMap, error) {
haveCM, current, err := r.currentDNSConfigMap(dns)
if err != nil {
return nil, fmt.Errorf("failed to get configmap: %v", err)
return false, nil, fmt.Errorf("failed to get configmap: %v", err)
}
desired, err := desiredDNSConfigMap(dns, clusterDomain)
if err != nil {
return nil, fmt.Errorf("failed to build configmap: %v", err)
return haveCM, nil, fmt.Errorf("failed to build configmap: %v", err)
}

switch {
case desired != nil && current == nil:
case !haveCM:
if err := r.client.Create(context.TODO(), desired); err != nil {
return nil, fmt.Errorf("failed to create configmap: %v", err)
return false, nil, fmt.Errorf("failed to create configmap: %v", err)
}
logrus.Infof("created configmap: %s", desired.Name)
case desired != nil && current != nil:
case haveCM:
if needsUpdate, updated := corefileChanged(current, desired); needsUpdate {
if err := r.client.Update(context.TODO(), updated); err != nil {
return nil, fmt.Errorf("failed to update configmap: %v", err)
return true, nil, fmt.Errorf("failed to update configmap: %v", err)
}
logrus.Infof("updated configmap; old: %#v, new: %#v", current, updated)
}
}
return r.currentDNSConfigMap(dns)
}

func (r *reconciler) currentDNSConfigMap(dns *operatorv1.DNS) (*corev1.ConfigMap, error) {
func (r *reconciler) currentDNSConfigMap(dns *operatorv1.DNS) (bool, *corev1.ConfigMap, error) {
current := &corev1.ConfigMap{}
err := r.client.Get(context.TODO(), DNSConfigMapName(dns), current)
if err != nil {
if errors.IsNotFound(err) {
return nil, nil
return false, nil, nil
}
return nil, err
return false, nil, err
}
return current, nil
return true, current, nil
}

func desiredDNSConfigMap(dns *operatorv1.DNS, clusterDomain string) (*corev1.ConfigMap, error) {
Expand Down
26 changes: 13 additions & 13 deletions pkg/operator/controller/controller_dns_daemonset.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,23 @@ import (
)

// ensureDNSDaemonSet ensures the dns daemonset exists for a given dns.
func (r *reconciler) ensureDNSDaemonSet(dns *operatorv1.DNS, clusterIP, clusterDomain string) (*appsv1.DaemonSet, error) {
desired, err := desiredDNSDaemonSet(dns, clusterIP, clusterDomain, r.CoreDNSImage, r.OpenshiftCLIImage, r.KubeRBACProxyImage)
func (r *reconciler) ensureDNSDaemonSet(dns *operatorv1.DNS, clusterIP, clusterDomain string) (bool, *appsv1.DaemonSet, error) {
haveDS, current, err := r.currentDNSDaemonSet(dns)
if err != nil {
return nil, fmt.Errorf("failed to build dns daemonset: %v", err)
return false, nil, err
}
current, err := r.currentDNSDaemonSet(dns)
desired, err := desiredDNSDaemonSet(dns, clusterIP, clusterDomain, r.CoreDNSImage, r.OpenshiftCLIImage, r.KubeRBACProxyImage)
if err != nil {
return nil, err
return haveDS, nil, fmt.Errorf("failed to build dns daemonset: %v", err)
}
switch {
case desired != nil && current == nil:
case !haveDS:
if err := r.createDNSDaemonSet(desired); err != nil {
return nil, err
return false, nil, err
}
case desired != nil && current != nil:
case haveDS:
if err := r.updateDNSDaemonSet(current, desired); err != nil {
return nil, err
return true, nil, err
}
}
return r.currentDNSDaemonSet(dns)
Expand Down Expand Up @@ -126,15 +126,15 @@ func desiredDNSDaemonSet(dns *operatorv1.DNS, clusterIP, clusterDomain, coreDNSI
}

// currentDNSDaemonSet returns the current dns daemonset.
func (r *reconciler) currentDNSDaemonSet(dns *operatorv1.DNS) (*appsv1.DaemonSet, error) {
func (r *reconciler) currentDNSDaemonSet(dns *operatorv1.DNS) (bool, *appsv1.DaemonSet, error) {
daemonset := &appsv1.DaemonSet{}
if err := r.client.Get(context.TODO(), DNSDaemonSetName(dns), daemonset); err != nil {
if errors.IsNotFound(err) {
return nil, nil
return false, nil, nil
}
return nil, err
return false, nil, err
}
return daemonset, nil
return true, daemonset, nil
}

// createDNSDaemonSet creates a dns daemonset.
Expand Down
22 changes: 11 additions & 11 deletions pkg/operator/controller/controller_dns_service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,37 +18,37 @@ import (
)

// ensureDNSService ensures that a service exists for a given DNS.
func (r *reconciler) ensureDNSService(dns *operatorv1.DNS, clusterIP string, daemonsetRef metav1.OwnerReference) (*corev1.Service, error) {
current, err := r.currentDNSService(dns)
func (r *reconciler) ensureDNSService(dns *operatorv1.DNS, clusterIP string, daemonsetRef metav1.OwnerReference) (bool, *corev1.Service, error) {
haveService, current, err := r.currentDNSService(dns)
if err != nil {
return nil, err
return false, nil, err
}
desired := desiredDNSService(dns, clusterIP, daemonsetRef)

switch {
case desired != nil && current == nil:
case !haveService:
if err := r.client.Create(context.TODO(), desired); err != nil {
return nil, fmt.Errorf("failed to create dns service: %v", err)
return false, nil, fmt.Errorf("failed to create dns service: %v", err)
}
logrus.Infof("created dns service: %s/%s", desired.Namespace, desired.Name)
case desired != nil && current != nil:
case haveService:
if err := r.updateDNSService(current, desired); err != nil {
return nil, err
return true, nil, err
}
}
return r.currentDNSService(dns)
}

func (r *reconciler) currentDNSService(dns *operatorv1.DNS) (*corev1.Service, error) {
func (r *reconciler) currentDNSService(dns *operatorv1.DNS) (bool, *corev1.Service, error) {
current := &corev1.Service{}
err := r.client.Get(context.TODO(), DNSServiceName(dns), current)
if err != nil {
if errors.IsNotFound(err) {
return nil, nil
return false, nil, nil
}
return nil, err
return false, nil, err
}
return current, nil
return true, current, nil
}

func desiredDNSService(dns *operatorv1.DNS, clusterIP string, daemonsetRef metav1.OwnerReference) *corev1.Service {
Expand Down
22 changes: 11 additions & 11 deletions pkg/operator/controller/controller_service_monitor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,23 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
)

func (r *reconciler) ensureServiceMonitor(dns *operatorv1.DNS, svc *corev1.Service, daemonsetRef metav1.OwnerReference) (*unstructured.Unstructured, error) {
func (r *reconciler) ensureServiceMonitor(dns *operatorv1.DNS, svc *corev1.Service, daemonsetRef metav1.OwnerReference) (bool, *unstructured.Unstructured, error) {
desired := desiredServiceMonitor(dns, svc, daemonsetRef)

current, err := r.currentServiceMonitor(dns)
haveSM, current, err := r.currentServiceMonitor(dns)
if err != nil {
return nil, err
return false, nil, err
}

switch {
case desired != nil && current == nil:
case !haveSM:
if err := r.client.Create(context.TODO(), desired); err != nil {
return nil, fmt.Errorf("failed to create servicemonitor %s/%s: %v", desired.GetNamespace(), desired.GetName(), err)
return false, nil, fmt.Errorf("failed to create servicemonitor %s/%s: %v", desired.GetNamespace(), desired.GetName(), err)
}
logrus.Infof("created servicemonitor %s/%s", desired.GetNamespace(), desired.GetName())
case desired != nil && current != nil:
case haveSM:
if err := r.updateDNSServiceMonitor(current, desired); err != nil {
return nil, err
return true, nil, err
}
}
return r.currentServiceMonitor(dns)
Expand Down Expand Up @@ -80,7 +80,7 @@ func desiredServiceMonitor(dns *operatorv1.DNS, svc *corev1.Service, daemonsetRe
return sm
}

func (r *reconciler) currentServiceMonitor(dns *operatorv1.DNS) (*unstructured.Unstructured, error) {
func (r *reconciler) currentServiceMonitor(dns *operatorv1.DNS) (bool, *unstructured.Unstructured, error) {
sm := &unstructured.Unstructured{}
sm.SetGroupVersionKind(schema.GroupVersionKind{
Group: "monitoring.coreos.com",
Expand All @@ -89,11 +89,11 @@ func (r *reconciler) currentServiceMonitor(dns *operatorv1.DNS) (*unstructured.U
})
if err := r.client.Get(context.TODO(), DNSServiceMonitorName(dns), sm); err != nil {
if errors.IsNotFound(err) {
return nil, nil
return false, nil, nil
}
return nil, err
return false, nil, err
}
return sm, nil
return true, sm, nil
}

func (r *reconciler) updateDNSServiceMonitor(current, desired *unstructured.Unstructured) error {
Expand Down

0 comments on commit f60ef55

Please sign in to comment.