Skip to content

Commit

Permalink
added owner reference to webhook config (#129)
Browse files Browse the repository at this point in the history
  • Loading branch information
bosesuneha authored Nov 9, 2023
1 parent d972a1b commit 5dc7a12
Show file tree
Hide file tree
Showing 7 changed files with 42 additions and 14 deletions.
2 changes: 1 addition & 1 deletion pkg/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func NewManagerForRestConfig(conf *config.Config, rc *rest.Config) (ctrl.Manager
return nil, fmt.Errorf("creating webhook config: %w", err)
}

if err := webhookCfg.EnsureWebhookConfigurations(context.Background(), cl); err != nil {
if err := webhookCfg.EnsureWebhookConfigurations(context.Background(), cl, conf); err != nil {
setupLog.Error(err, "unable to ensure webhook configurations")
return nil, fmt.Errorf("ensuring webhook configurations: %w", err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/manifests/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func GetOwnerRefs(owner client.Object, controller bool) []metav1.OwnerReference
}}
}

func namespace(conf *config.Config) *corev1.Namespace {
func Namespace(conf *config.Config) *corev1.Namespace {
ns := &corev1.Namespace{
TypeMeta: metav1.TypeMeta{
Kind: "Namespace",
Expand Down
2 changes: 1 addition & 1 deletion pkg/manifests/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ var (

func TestNamespaceResources(t *testing.T) {
for _, tc := range namespaceTestCases {
objs := namespace(tc.Config)
objs := Namespace(tc.Config)
fixture := path.Join("fixtures", "common", tc.Name) + ".json"
AssertFixture(t, fixture, []client.Object{objs})
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/manifests/external_dns.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func ExternalDnsResources(conf *config.Config, externalDnsConfigs []*ExternalDns

// Can safely assume the namespace exists if using kube-system
if conf.NS != "kube-system" {
objs = append(objs, namespace(conf))
objs = append(objs, Namespace(conf))
}

for _, dnsConfig := range externalDnsConfigs {
Expand Down
2 changes: 1 addition & 1 deletion pkg/manifests/nginx.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ func GetNginxResources(conf *config.Config, ingressConfig *NginxIngressConfig) *
// Can safely assume the namespace exists if using kube-system.
// Purposefully do this after applying the labels, namespace isn't an Nginx-specific resource
if conf.NS != "kube-system" {
res.Namespace = namespace(conf)
res.Namespace = Namespace(conf)
}

return res
Expand Down
14 changes: 12 additions & 2 deletions pkg/webhook/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@ import (
"fmt"

globalCfg "github.com/Azure/aks-app-routing-operator/pkg/config"
"github.com/Azure/aks-app-routing-operator/pkg/manifests"
"github.com/Azure/aks-app-routing-operator/pkg/util"
"github.com/open-policy-agent/cert-controller/pkg/rotator"
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/log"
Expand Down Expand Up @@ -62,7 +64,7 @@ func New(globalCfg *globalCfg.Config) (*config, error) {
}

// EnsureWebhookConfigurations ensures the webhook configurations exist in the cluster in the desired state
func (c *config) EnsureWebhookConfigurations(ctx context.Context, cl client.Client) error {
func (c *config) EnsureWebhookConfigurations(ctx context.Context, cl client.Client, globalCfg *globalCfg.Config) error {
lgr := log.FromContext(ctx).WithName("webhooks")

lgr.Info("calculating ValidatingWebhookConfiguration")
Expand Down Expand Up @@ -117,11 +119,19 @@ func (c *config) EnsureWebhookConfigurations(ctx context.Context, cl client.Clie
Webhooks: mutatingWhs,
}

// todo: add ownership references to app-routing-system ns
lgr.Info("ensuring namespace exists")
appRoutingNamespace := &corev1.Namespace{}
appRoutingNamespace = manifests.Namespace(globalCfg)
if err := util.Upsert(ctx, cl, appRoutingNamespace); err != nil {
return fmt.Errorf("upserting namespace: %w", err)
}

ownerRef := manifests.GetOwnerRefs(appRoutingNamespace, false)

lgr.Info("ensuring webhook configuration")
whs := []client.Object{validatingWhc, mutatingWhc}
for _, wh := range whs {
wh.SetOwnerReferences(ownerRef)
copy := wh.DeepCopyObject().(client.Object)
lgr := lgr.WithValues("resource", wh.GetName(), "resourceKind", wh.GetObjectKind().GroupVersionKind().Kind)
lgr.Info("upserting resource")
Expand Down
32 changes: 25 additions & 7 deletions pkg/webhook/webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ func TestNew(t *testing.T) {

func TestEnsureWebhookConfigurations(t *testing.T) {
t.Run("valid webhooks", func(t *testing.T) {
globalCfg := &globalCfg.Config{
NS: "app-routing-system",
}
c := &config{
validatingWebhookConfigName: "app-routing-validating",
mutatingWebhookConfigName: "app-routing-mutating",
Expand All @@ -94,17 +97,32 @@ func TestEnsureWebhookConfigurations(t *testing.T) {
}

cl := fake.NewClientBuilder().Build()
var validatingWhCfg admissionregistrationv1.ValidatingWebhookConfiguration
var mutatingWhCfg admissionregistrationv1.MutatingWebhookConfiguration
// prove webhook configurations don't exist
require.True(t, k8serrors.IsNotFound(cl.Get(context.Background(), types.NamespacedName{Name: c.validatingWebhookConfigName}, &admissionregistrationv1.ValidatingWebhookConfiguration{})), "expected not to find validating webhook config")
require.True(t, k8serrors.IsNotFound(cl.Get(context.Background(), types.NamespacedName{Name: c.mutatingWebhookConfigName}, &admissionregistrationv1.MutatingWebhookConfiguration{})), "expected not to find mutating webhook config")
require.True(t, k8serrors.IsNotFound(cl.Get(context.Background(), types.NamespacedName{Name: c.validatingWebhookConfigName}, &validatingWhCfg)), "expected not to find validating webhook config")
require.True(t, k8serrors.IsNotFound(cl.Get(context.Background(), types.NamespacedName{Name: c.mutatingWebhookConfigName}, &mutatingWhCfg)), "expected not to find mutating webhook config")

// prove webhook configurations exist after ensuring
require.NoError(t, c.EnsureWebhookConfigurations(context.Background(), cl), "unexpected error")
require.NoError(t, cl.Get(context.Background(), types.NamespacedName{Name: c.validatingWebhookConfigName}, &admissionregistrationv1.ValidatingWebhookConfiguration{}), "unexpected error getting validating webhook config")
require.NoError(t, cl.Get(context.Background(), types.NamespacedName{Name: c.mutatingWebhookConfigName}, &admissionregistrationv1.MutatingWebhookConfiguration{}), "unexpected error getting mutating webhook config")
require.NoError(t, c.EnsureWebhookConfigurations(context.Background(), cl, globalCfg), "unexpected error")
require.NoError(t, cl.Get(context.Background(), types.NamespacedName{Name: c.validatingWebhookConfigName}, &validatingWhCfg), "unexpected error getting validating webhook config")
require.NoError(t, cl.Get(context.Background(), types.NamespacedName{Name: c.mutatingWebhookConfigName}, &mutatingWhCfg), "unexpected error getting mutating webhook config")

// prove ownerReferences are set on webhook configurations
require.True(t, len(validatingWhCfg.OwnerReferences) == 1, "expected 1 owner reference")
require.True(t, validatingWhCfg.OwnerReferences[0].Name == "app-routing-system", "expected owner reference name to be app-routing-system")
require.True(t, validatingWhCfg.OwnerReferences[0].Kind == "Namespace", "expected owner reference kind to be Namespace")
require.True(t, validatingWhCfg.OwnerReferences[0].APIVersion == "v1", "expected owner reference api version to be v1")
require.True(t, len(mutatingWhCfg.OwnerReferences) == 1, "expected 1 owner reference")
require.True(t, mutatingWhCfg.OwnerReferences[0].Name == "app-routing-system", "expected owner reference name to be app-routing-system")
require.True(t, mutatingWhCfg.OwnerReferences[0].Kind == "Namespace", "expected owner reference kind to be Namespace")
require.True(t, mutatingWhCfg.OwnerReferences[0].APIVersion == "v1", "expected owner reference api version to be v1")

})

t.Run("invalid webhooks", func(t *testing.T) {

globalCfg := &globalCfg.Config{}
c := &config{
validatingWebhooks: []Webhook[admissionregistrationv1.ValidatingWebhook]{
{
Expand All @@ -123,10 +141,10 @@ func TestEnsureWebhookConfigurations(t *testing.T) {
}

cl := fake.NewClientBuilder().Build()
require.True(t, c.EnsureWebhookConfigurations(context.Background(), cl).Error() == "getting webhook definition: invalid webhook", "expected error")
require.True(t, c.EnsureWebhookConfigurations(context.Background(), cl, globalCfg).Error() == "getting webhook definition: invalid webhook", "expected error")

c.validatingWebhooks = nil
require.True(t, c.EnsureWebhookConfigurations(context.Background(), cl).Error() == "getting webhook definition: invalid webhook", "expected error")
require.True(t, c.EnsureWebhookConfigurations(context.Background(), cl, globalCfg).Error() == "getting webhook definition: invalid webhook", "expected error")
})
}

Expand Down

0 comments on commit 5dc7a12

Please sign in to comment.