Skip to content

Commit

Permalink
feat(logging/events): add event generation, update log messages, and …
Browse files Browse the repository at this point in the history 
…improve code and tests

Signed-off-by: Emin Aktas <eminaktas34@gmail.com>
  • Loading branch information
@eminaktas
eminaktas committed Sep 23, 2024
1 parent 4599464 commit f196d73
Show file tree
Hide file tree
Showing 8 changed files with 310 additions and 155 deletions.
33 changes: 23 additions & 10 deletions api/v1alpha1/dnsclass_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,25 @@ type DNSClassCustomDefaulter struct {

var _ webhook.CustomDefaulter = &DNSClassCustomDefaulter{}

// Default implements webhook.Defaulter so a webhook will be registered for the type
// Default implements webhook.CustomDefaulter so a webhook will be registered for the Kind DNSClass
func (r *DNSClassCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error {
dnsclass := obj.(*DNSClass)
dnsclasslog.Info("default", "name", dnsclass.Name)
dnsclass, ok := obj.(*DNSClass)
if !ok {
return fmt.Errorf("expected a DNSClass object but got %T", obj)
}

dnsclasslog.Info("Defaulting process started", "name", dnsclass.GetName())

if dnsclass.Spec.AllowedDNSPolicies == nil {
dnsclass.Spec.AllowedDNSPolicies = DefaultDNSPolicies
dnsclasslog.Info("Applied default AllowedDNSPolicies", "name", dnsclass.GetName(), "DefaultPolicies", DefaultDNSPolicies)
}
if dnsclass.Spec.DNSPolicy == "" {
dnsclass.Spec.DNSPolicy = corev1.DNSNone
dnsclasslog.Info("Applied default DNSPolicy", "name", dnsclass.GetName(), "DefaultPolicy", corev1.DNSNone)
}

dnsclasslog.Info("Defaulting process completed", "name", dnsclass.GetName())
return nil
}

Expand All @@ -81,21 +88,27 @@ type DNSClassCustomValidator struct {

var _ webhook.CustomValidator = &DNSClassCustomValidator{}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the Kind DNSClass
func (r *DNSClassCustomValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
dnsclass := obj.(*DNSClass)
dnsclasslog.Info("validate create", "name", dnsclass.Name)
dnsclass, ok := obj.(*DNSClass)
if !ok {
return nil, fmt.Errorf("expected a DNSClass object but got %T", obj)
}
dnsclasslog.Info("Validation for DNSClass upon creation", "name", dnsclass.GetName())
return r.validate(dnsclass)
}

// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the Kind DNSClass
func (r *DNSClassCustomValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
dnsclass := newObj.(*DNSClass)
dnsclasslog.Info("validate update", "name", dnsclass.Name)
dnsclass, ok := newObj.(*DNSClass)
if !ok {
return nil, fmt.Errorf("expected a DNSClass object but got %T", newObj)
}
dnsclasslog.Info("Validation for DNSClass upon update", "name", dnsclass.GetName())
return r.validate(dnsclass)
}

// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the Kind DNSClass
func (r *DNSClassCustomValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
// No validation required on delete for now
return nil, nil
Expand Down
100 changes: 70 additions & 30 deletions api/v1alpha1/dnsclass_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package v1alpha1

import (
"context"
"fmt"

"github.com/eminaktas/kubedns-shepherd/test/utils"
Expand All @@ -28,15 +29,16 @@ import (
var _ = Describe("DNSClass Webhook", Ordered, func() {
var dnsclass *DNSClass

AfterEach(func() {
By("Cleaning up the DNSClass instance")
if dnsclass != nil {
Expect(utils.DeleteDNSClass(ctx, k8sClient, dnsclass)).Should(Succeed())
dnsclass = nil
}
})

Context("When creating DNSClass with Defaulting Webhook", func() {
AfterEach(func() {
By("Cleaning up the DNSClass instance")
if dnsclass != nil {
Expect(utils.DeleteDNSClass(ctx, k8sClient, dnsclass)).Should(Succeed())
// Reset DNSClass variable for the next test
dnsclass = nil
}
})

It("Should apply default values when not provided", func() {
By("Creating DNSClass with no fields set")
dnsclass = &DNSClass{}
Expand All @@ -46,17 +48,41 @@ var _ = Describe("DNSClass Webhook", Ordered, func() {
Expect(dnsclass.Spec.DNSPolicy).Should(Equal(corev1.DNSNone), "DNSPolicy should default to DNSNone")
Expect(dnsclass.Spec.AllowedDNSPolicies).Should(Equal(DefaultDNSPolicies), "AllowedDNSPolicies should default to valid defaults")
})
})

// Helper to create DNSClass and validate reconciliation
createAndValidateDNSClass := func() {
Expect(utils.CreateDNSClass(ctx, k8sClient, dnsclass)).Should(Succeed(), "Failed to create DNSClass")
By("Syncing DNSClass")
Expect(dnsclass.GetName()).Should(BeEmpty())
}
It("Should allow updating DNSClass fields", func() {
By("Creating a valid DNSClass")
dnsclass = &DNSClass{}
Expect(utils.CreateDNSClass(ctx, k8sClient, dnsclass)).Should(Succeed())

By("Updating the AllowedNamespaces field")
updatedAllowedNamespaces := []string{"default", "another-namespace"}
dnsclass.Spec.AllowedNamespaces = updatedAllowedNamespaces
Expect(k8sClient.Update(ctx, dnsclass)).Should(Succeed())

By("Verifying that the DNSClass was updated correctly")
Expect(utils.GetDNSClass(ctx, k8sClient, dnsclass)).Should(Succeed())
Expect(dnsclass.Spec.AllowedNamespaces).Should(Equal(updatedAllowedNamespaces), "AllowedNamespaces should be updated correctly")
})
})

Context("When creating DNSClass under Validating Webhook", func() {
It("Should reject invalid AllowedDNSPolicies", func() {
// Helper to create DNSClass and validate reconciliation for validating
createAndValidateDNSClass := func() {
Expect(utils.CreateDNSClass(ctx, k8sClient, dnsclass)).Should(Succeed(), "Failed to create DNSClass")
By("Syncing DNSClass")
Expect(dnsclass.GetName()).Should(BeEmpty())
}

AfterEach(func() {
By("Cleaning up the DNSClass instance")
if dnsclass != nil {
Expect(utils.DeleteDNSClass(ctx, k8sClient, dnsclass)).Should(Succeed())
// Reset DNSClass variable for the next test
dnsclass = nil
}
})

It("should reject invalid AllowedDNSPolicies", func() {
dnsclass = &DNSClass{
Spec: DNSClassSpec{
AllowedDNSPolicies: []corev1.DNSPolicy{corev1.DNSNone, "InvalidDNSPolicy"},
Expand All @@ -65,7 +91,7 @@ var _ = Describe("DNSClass Webhook", Ordered, func() {
createAndValidateDNSClass()
})

It("Should reject invalid DNSPolicy", func() {
It("should reject invalid DNSPolicy", func() {
By("Creating DNSClass with an invalid DNSPolicy")
dnsclass = &DNSClass{
Spec: DNSClassSpec{
Expand All @@ -75,7 +101,7 @@ var _ = Describe("DNSClass Webhook", Ordered, func() {
createAndValidateDNSClass()
})

It("Should reject DNSConfig when DNSPolicy is not `None`", func() {
It("should reject DNSConfig when DNSPolicy is not `None`", func() {
By("Creating DNSClass with DNSConfig but without DNSPolicy None")
dnsconfig := &corev1.PodDNSConfig{
Nameservers: []string{utils.ClusterDNS},
Expand All @@ -91,7 +117,7 @@ var _ = Describe("DNSClass Webhook", Ordered, func() {
createAndValidateDNSClass()
})

It("Should reject DNSConfig with invalid template keys", func() {
It("should reject DNSConfig with invalid template keys", func() {
By("Creating DNSClass with invalid template keys in DNSConfig.Searches")
dnsconfig := &corev1.PodDNSConfig{
Searches: []string{"svc.{{ .podNamespace }}", "{{ .clusterDomainInvalid }}"},
Expand All @@ -104,20 +130,34 @@ var _ = Describe("DNSClass Webhook", Ordered, func() {
}
createAndValidateDNSClass()
})
})

It("Should allow updating DNSClass fields", func() {
By("Creating a valid DNSClass")
dnsclass = &DNSClass{}
Expect(utils.CreateDNSClass(ctx, k8sClient, dnsclass)).Should(Succeed())
Context("When calling functions directly", func() {
It("should return error when calling Default function with different object", func() {
dnsCustomDefault := &DNSClassCustomDefaulter{}
err := dnsCustomDefault.Default(context.TODO(), &corev1.Pod{})
Expect(err).Should(HaveOccurred())
})

By("Updating the AllowedNamespaces field")
updatedAllowedNamespaces := []string{"default", "another-namespace"}
dnsclass.Spec.AllowedNamespaces = updatedAllowedNamespaces
Expect(k8sClient.Update(ctx, dnsclass)).Should(Succeed())
It("should return error when calling ValidateCreate with different object", func() {
dnsCustomValidator := &DNSClassCustomValidator{}
warn, err := dnsCustomValidator.ValidateCreate(context.TODO(), &corev1.Pod{})
Expect(warn).Should(BeNil())
Expect(err).Should(HaveOccurred())
})

By("Verifying that the DNSClass was updated correctly")
Expect(utils.GetDNSClass(ctx, k8sClient, dnsclass)).Should(Succeed())
Expect(dnsclass.Spec.AllowedNamespaces).Should(Equal(updatedAllowedNamespaces), "AllowedNamespaces should be updated correctly")
It("should return error when calling ValidateUpdate with different object", func() {
dnsCustomValidator := &DNSClassCustomValidator{}
warn, err := dnsCustomValidator.ValidateUpdate(context.TODO(), nil, &corev1.Pod{})
Expect(warn).Should(BeNil())
Expect(err).Should(HaveOccurred())
})

It("should return error when calling ValidateDelete with different object", func() {
dnsCustomValidator := &DNSClassCustomValidator{}
warn, err := dnsCustomValidator.ValidateDelete(context.TODO(), &corev1.Pod{})
Expect(warn).Should(BeNil())
Expect(err).Should(BeNil())
})
})
})
8 changes: 5 additions & 3 deletions cmd/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,8 @@ func main() {
os.Exit(1)
}
if err = (&controller.DNSClassReconciler{
Client: mgr.GetClient(),
Client: mgr.GetClient(),
EventRecorder: mgr.GetEventRecorderFor("dnsclass-controller"),
MaxConcurrentReconcilesForDNSClassReconciler: maxConcurrentReconcilesForDNSClassReconciler,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "DNSClass")
Expand All @@ -211,8 +212,9 @@ func main() {

mgr.GetWebhookServer().Register("/mutate-v1-pod", &webhook.Admission{
Handler: &webhook_controller.PodMutator{
Client: mgr.GetClient(),
Decoder: admission.NewDecoder(mgr.GetScheme()),
Client: mgr.GetClient(),
Decoder: admission.NewDecoder(mgr.GetScheme()),
EventRecorder: mgr.GetEventRecorderFor("pod-mutator-webhook-controller"),
},
})
//+kubebuilder:scaffold:builder
Expand Down
10 changes: 8 additions & 2 deletions internal/controller/dnsclass_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/tools/record"
"k8s.io/client-go/util/retry"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -53,6 +54,7 @@ type Workload struct {
// DNSClassReconciler reconciles a DNSClass object
type DNSClassReconciler struct {
client.Client
record.EventRecorder

MaxConcurrentReconcilesForDNSClassReconciler int
}
Expand All @@ -70,7 +72,7 @@ type DNSClassReconciler struct {
// Reconcile reconciles a DNSClass object
func (r *DNSClassReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
logger := log.FromContext(ctx)
logger.Info("Reconciling for DNSClass")
logger.Info("Reconciling")

dnsclass := &configv1alpha1.DNSClass{}
err := r.Get(ctx, req.NamespacedName, dnsclass)
Expand All @@ -83,6 +85,8 @@ func (r *DNSClassReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
return ctrl.Result{}, err
}

r.EventRecorder.Event(dnsclass, corev1.EventTypeNormal, "Reconciling", "Reconciliation started")

// Set status to Init if it is not set
if dnsclass.Status.State == "" {
dnsclass.Status.State = configv1alpha1.StateInit
Expand All @@ -107,6 +111,7 @@ func (r *DNSClassReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
for _, key := range dnsclass.ExtractTemplateKeysRegex() {
if !slices.Contains(undiscoveredFields, key) {
err = errors.Errorf("failed to discover template keys: %v", strings.Join(undiscoveredFields, ", "))
r.EventRecorder.Event(dnsclass, corev1.EventTypeWarning, "Failed", "DNSClass will be unavailable due to missing required template keys")
return ctrl.Result{}, err
}
}
Expand All @@ -120,7 +125,8 @@ func (r *DNSClassReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
dnsclass.Status.State = configv1alpha1.StateReady
statusMessage = "Ready"

logger.Info("Reconciling completed for DNSClass")
logger.Info("Reconciling successfully completed")
r.EventRecorder.Event(dnsclass, corev1.EventTypeNormal, "Successful", "Reconciliation successfully completed")

return ctrl.Result{}, nil
}
Expand Down
3 changes: 2 additions & 1 deletion internal/controller/suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ var _ = BeforeSuite(func() {
Expect(err).ToNot(HaveOccurred())

err = (&DNSClassReconciler{
Client: k8sManager.GetClient(),
Client: k8sManager.GetClient(),
EventRecorder: k8sManager.GetEventRecorderFor("dnsclass-controller"),
}).SetupWithManager(k8sManager)
Expect(err).ToNot(HaveOccurred())

Expand Down
Loading

0 comments on commit f196d73

Please sign in to comment.