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

managed-by label checks for spc, placeholder pods and ingress backend #111

Merged
merged 21 commits into from
Oct 26, 2023
Merged

managed-by label checks for spc, placeholder pods and ingress backend #111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
d2b0b32
initial commit
aamgayle Oct 5, 2023
1d93845
Added label checks before deleting spc for ingress_secret_provider_cl…
aamgayle Oct 6, 2023
5420943
Added label check for osm ingress_backend_reconciler and moved HasTop…
aamgayle Oct 10, 2023
3f1656a
Merge branch 'main' into aamgayle/managedBySPCLabels
aamgayle Oct 11, 2023
7331620
Merge main
aamgayle Oct 11, 2023
3afb354
Addressing comments
aamgayle Oct 16, 2023
30c1162
ignore not found error on successful cluster delete
aamgayle Oct 16, 2023
8a75055
Merge branch 'main' into aamgayle/managedBySPCLabels
aamgayle Oct 16, 2023
ecdce88
returned ingress_manager back to main
aamgayle Oct 16, 2023
b232c42
Unit test updates and refactors
aamgayle Oct 19, 2023
60b6925
Merge branch 'main' into aamgayle/managedBySPCLabels
aamgayle Oct 19, 2023
9369c4c
addressing comments for ingress_secret_provider_class
aamgayle Oct 19, 2023
82996d4
spc class test changes and typo fix for placeholder pod test
aamgayle Oct 19, 2023
fe08d34
Refactors and addressing comments
aamgayle Oct 24, 2023
724ddaa
fixing comment
aamgayle Oct 24, 2023
c3b2dbf
Removing len check for labels
aamgayle Oct 24, 2023
27b9a29
Making variable names more generic for HasTopLevelLabels
aamgayle Oct 24, 2023
c2c4563
additional obj specfically for deletion
aamgayle Oct 25, 2023
d848536
small backend reconcile refactor
aamgayle Oct 25, 2023
afe2b98
change dep to obj literal
aamgayle Oct 25, 2023
1ffab9d
Requested changes
aamgayle Oct 25, 2023
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
19 changes: 14 additions & 5 deletions pkg/controller/keyvault/ingress_secret_provider_class.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"net/url"
"strings"

"github.com/Azure/aks-app-routing-operator/pkg/controller/controllername"
"github.com/go-logr/logr"
netv1 "k8s.io/api/networking/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -20,7 +19,9 @@ import (
secv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1"

"github.com/Azure/aks-app-routing-operator/pkg/config"
"github.com/Azure/aks-app-routing-operator/pkg/controller/controllername"
"github.com/Azure/aks-app-routing-operator/pkg/controller/metrics"
"github.com/Azure/aks-app-routing-operator/pkg/manifests"
"github.com/Azure/aks-app-routing-operator/pkg/util"
kvcsi "github.com/Azure/secrets-store-csi-driver-provider-azure/pkg/provider/types"
)
Expand Down Expand Up @@ -91,6 +92,7 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("keyvault-%s", ing.Name),
Namespace: ing.Namespace,
Labels: manifests.GetTopLevelLabels(),
OwnerReferences: []metav1.OwnerReference{{
APIVersion: ing.APIVersion,
Controller: util.BoolPtr(true),
Expand All @@ -115,14 +117,21 @@ func (i *IngressSecretProviderClassReconciler) Reconcile(ctx context.Context, re

logger.Info("cleaning unused managed spc for ingress")
logger.Info("getting secret provider class for ingress")
err = i.client.Get(ctx, client.ObjectKeyFromObject(spc), spc)

toCleanSPC := &secv1.SecretProviderClass{}

err = i.client.Get(ctx, client.ObjectKeyFromObject(spc), toCleanSPC)
if err != nil {
return result, client.IgnoreNotFound(err)
aamgayle marked this conversation as resolved.
Show resolved Hide resolved
}

logger.Info("removing secret provider class for ingress")
err = i.client.Delete(ctx, spc)
return result, err
if manifests.HasTopLevelLabels(toCleanSPC.Labels) {
logger.Info("removing secret provider class for ingress")
aamgayle marked this conversation as resolved.
Show resolved Hide resolved
err = i.client.Delete(ctx, toCleanSPC)
return result, client.IgnoreNotFound(err)
}

return result, nil
}

func (i *IngressSecretProviderClassReconciler) buildSPC(ing *netv1.Ingress, spc *secv1.SecretProviderClass) (bool, error) {
Expand Down
186 changes: 147 additions & 39 deletions pkg/controller/keyvault/ingress_secret_provider_class_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import (
"net/url"
"testing"

"github.com/Azure/aks-app-routing-operator/pkg/controller/testutils"

"github.com/go-logr/logr"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand All @@ -26,18 +24,31 @@ import (

"github.com/Azure/aks-app-routing-operator/pkg/config"
"github.com/Azure/aks-app-routing-operator/pkg/controller/metrics"
"github.com/Azure/aks-app-routing-operator/pkg/controller/testutils"
"github.com/Azure/aks-app-routing-operator/pkg/manifests"
"github.com/Azure/aks-app-routing-operator/pkg/util"
kvcsi "github.com/Azure/secrets-store-csi-driver-provider-azure/pkg/provider/types"
)

func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) {
ing := &netv1.Ingress{}
ing.Name = "test-ingress"
ing.Namespace = "default"
ingressClass := "webapprouting.kubernetes.azure.com"
ing.Spec.IngressClassName = &ingressClass
ing.Annotations = map[string]string{
"kubernetes.azure.com/tls-cert-keyvault-uri": "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34",
var (
spcTestIngressClassName = "webapprouting.kubernetes.azure.com"
spcTestIngress = &netv1.Ingress{
ObjectMeta: metav1.ObjectMeta{
Name: "test-ingress",
Namespace: "default",
Annotations: map[string]string{
"kubernetes.azure.com/tls-cert-keyvault-uri": "https://testvault.vault.azure.net/certificates/testcert/f8982febc6894c0697b884f946fb1a34",
},
},
Spec: netv1.IngressSpec{
IngressClassName: &spcTestIngressClassName,
},
}
)

func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) {
// Create the ingress
ing := spcTestIngress.DeepCopy()

c := fake.NewClientBuilder().WithObjects(ing).Build()
require.NoError(t, secv1.AddToScheme(c.Scheme()))
Expand All @@ -47,7 +58,7 @@ func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) {
TenantID: "test-tenant-id",
MSIClientID: "test-msi-client-id",
},
ingressManager: NewIngressManager(map[string]struct{}{ingressClass: {}}),
ingressManager: NewIngressManager(map[string]struct{}{spcTestIngressClassName: {}}),
}

ctx := context.Background()
Expand All @@ -67,6 +78,7 @@ func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) {
spc := &secv1.SecretProviderClass{}
spc.Name = "keyvault-" + ing.Name
spc.Namespace = ing.Namespace
spc.Labels = manifests.GetTopLevelLabels()
require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(spc), spc))

expected := &secv1.SecretProviderClass{
Expand Down Expand Up @@ -138,12 +150,114 @@ func TestIngressSecretProviderClassReconcilerIntegration(t *testing.T) {
require.Greater(t, testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount)
aamgayle marked this conversation as resolved.
Show resolved Hide resolved
}

func TestIngressSecretProviderClassReconcilerIntegrationWithoutSPCLabels(t *testing.T) {
// Create the ingress
ing := spcTestIngress.DeepCopy()

c := fake.NewClientBuilder().WithObjects(ing).Build()
require.NoError(t, secv1.AddToScheme(c.Scheme()))
i := &IngressSecretProviderClassReconciler{
client: c,
config: &config.Config{
TenantID: "test-tenant-id",
MSIClientID: "test-msi-client-id",
},
ingressManager: NewIngressManager(map[string]struct{}{spcTestIngressClassName: {}}),
}

ctx := context.Background()
ctx = logr.NewContext(ctx, logr.Discard())

// Create the secret provider class
req := ctrl.Request{NamespacedName: types.NamespacedName{Namespace: ing.Namespace, Name: ing.Name}}
beforeErrCount := testutils.GetErrMetricCount(t, ingressSecretProviderControllerName)
beforeRequestCount := testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess)
_, err := i.Reconcile(ctx, req)
require.NoError(t, err)

require.Equal(t, testutils.GetErrMetricCount(t, ingressSecretProviderControllerName), beforeErrCount)
require.Greater(t, testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount)

spc := &secv1.SecretProviderClass{
TypeMeta: metav1.TypeMeta{
APIVersion: "secrets-store.csi.x-k8s.io/v1",
Kind: "SecretProviderClass",
},
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("keyvault-%s", ing.Name),
Namespace: ing.Namespace,
Labels: manifests.GetTopLevelLabels(),
OwnerReferences: []metav1.OwnerReference{{
APIVersion: ing.APIVersion,
Controller: util.BoolPtr(true),
Kind: ing.Kind,
Name: ing.Name,
UID: ing.UID,
}},
},
}

// Get secret provider class
require.False(t, errors.IsNotFound(c.Get(ctx, client.ObjectKeyFromObject(spc), spc)))
assert.Equal(t, len(manifests.GetTopLevelLabels()), len(spc.Labels))

// Remove the labels
spc.Labels = map[string]string{}
require.NoError(t, i.client.Update(ctx, spc))
assert.Equal(t, 0, len(spc.Labels))

// Remove the cert annotation from the ingress
ing.Annotations = map[string]string{}
require.NoError(t, i.client.Update(ctx, ing))

// Reconcile both changes
beforeErrCount = testutils.GetErrMetricCount(t, ingressSecretProviderControllerName)
beforeRequestCount = testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess)
_, err = i.Reconcile(ctx, req)
require.NoError(t, err)
require.Equal(t, testutils.GetErrMetricCount(t, ingressSecretProviderControllerName), beforeErrCount)
require.Greater(t, testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount)

// Prove secret class was not removed
require.False(t, errors.IsNotFound(c.Get(ctx, client.ObjectKeyFromObject(spc), spc)))
assert.Equal(t, 0, len(spc.Labels))
require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(spc), spc))

// Check secret provider class Spec after Reconcile
expected := &secv1.SecretProviderClass{
Spec: secv1.SecretProviderClassSpec{
Provider: "azure",
Parameters: map[string]string{
"keyvaultName": "testvault",
"objects": "{\"array\":[\"{\\\"objectName\\\":\\\"testcert\\\",\\\"objectType\\\":\\\"secret\\\",\\\"objectVersion\\\":\\\"f8982febc6894c0697b884f946fb1a34\\\"}\"]}",
"tenantId": i.config.TenantID,
"useVMManagedIdentity": "true",
"userAssignedIdentityID": i.config.MSIClientID,
},
SecretObjects: []*secv1.SecretObject{{
SecretName: spc.Name,
Type: "kubernetes.io/tls",
Data: []*secv1.SecretObjectData{
{ObjectName: "testcert", Key: "tls.key"},
{ObjectName: "testcert", Key: "tls.crt"},
},
}},
},
}
assert.Equal(t, expected.Spec, spc.Spec)

// Check for idempotence
beforeErrCount = testutils.GetErrMetricCount(t, ingressSecretProviderControllerName)
beforeRequestCount = testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess)
_, err = i.Reconcile(ctx, req)
require.NoError(t, err)
require.Equal(t, testutils.GetErrMetricCount(t, ingressSecretProviderControllerName), beforeErrCount)
require.Greater(t, testutils.GetReconcileMetricCount(t, ingressSecretProviderControllerName, metrics.LabelSuccess), beforeRequestCount)
}

func TestIngressSecretProviderClassReconcilerInvalidURL(t *testing.T) {
ing := &netv1.Ingress{}
ing.Name = "test-ingress"
ing.Namespace = "default"
ingressClass := "webapprouting.kubernetes.azure.com"
ing.Spec.IngressClassName = &ingressClass
// Create the ingress
ing := spcTestIngress.DeepCopy()
ing.Annotations = map[string]string{
"kubernetes.azure.com/tls-cert-keyvault-uri": "inv@lid URL",
}
Expand All @@ -158,7 +272,7 @@ func TestIngressSecretProviderClassReconcilerInvalidURL(t *testing.T) {
MSIClientID: "test-msi-client-id",
},
events: recorder,
ingressManager: NewIngressManager(map[string]struct{}{ingressClass: {}}),
ingressManager: NewIngressManager(map[string]struct{}{spcTestIngressClassName: {}}),
}

ctx := context.Background()
Expand All @@ -184,17 +298,18 @@ func TestIngressSecretProviderClassReconcilerInvalidURL(t *testing.T) {
}

func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) {
ingressClass := "webapprouting.kubernetes.azure.com"

i := &IngressSecretProviderClassReconciler{
ingressManager: NewIngressManager(map[string]struct{}{ingressClass: {}}),
ingressManager: NewIngressManager(map[string]struct{}{spcTestIngressClassName: {}}),
}

ing := &netv1.Ingress{}
ing.Spec.IngressClassName = &ingressClass
invalidURLIng := &netv1.Ingress{
Spec: netv1.IngressSpec{
IngressClassName: &spcTestIngressClassName,
},
}

t.Run("missing ingress class", func(t *testing.T) {
ing := ing.DeepCopy()
ing := invalidURLIng.DeepCopy()
ing.Spec.IngressClassName = nil
ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": "inv@lid URL"}

Expand All @@ -204,7 +319,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) {
})

t.Run("incorrect ingress class", func(t *testing.T) {
ing := ing.DeepCopy()
ing := invalidURLIng.DeepCopy()
incorrect := "some-other-ingress-class"
ing.Spec.IngressClassName = &incorrect
ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": "inv@lid URL"}
Expand All @@ -215,15 +330,15 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) {
})

t.Run("nil annotations", func(t *testing.T) {
ing := ing.DeepCopy()
ing := invalidURLIng.DeepCopy()

ok, err := i.buildSPC(ing, &secv1.SecretProviderClass{})
assert.False(t, ok)
require.NoError(t, err)
})

t.Run("empty url", func(t *testing.T) {
ing := ing.DeepCopy()
ing := invalidURLIng.DeepCopy()
ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": ""}

ok, err := i.buildSPC(ing, &secv1.SecretProviderClass{})
Expand All @@ -232,7 +347,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) {
})

t.Run("url with control character", func(t *testing.T) {
ing := ing.DeepCopy()
ing := invalidURLIng.DeepCopy()
cc := string([]byte{0x7f})
ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": cc}

Expand All @@ -243,7 +358,7 @@ func TestIngressSecretProviderClassReconcilerBuildSPCInvalidURLs(t *testing.T) {
})

t.Run("url with one path segment", func(t *testing.T) {
ing := ing.DeepCopy()
ing := invalidURLIng.DeepCopy()
ing.Annotations = map[string]string{"kubernetes.azure.com/tls-cert-keyvault-uri": "http://test.com/foo"}

ok, err := i.buildSPC(ing, &secv1.SecretProviderClass{})
Expand Down Expand Up @@ -278,23 +393,16 @@ func TestIngressSecretProviderClassReconcilerBuildSPCCloud(t *testing.T) {

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
ingressClass := "webapprouting.kubernetes.azure.com"
i := &IngressSecretProviderClassReconciler{
config: &config.Config{
Cloud: c.configCloud,
},
ingressManager: NewIngressManager(map[string]struct{}{ingressClass: {}}),
ingressManager: NewIngressManager(map[string]struct{}{spcTestIngressClassName: {}}),
}

ing := &netv1.Ingress{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
"kubernetes.azure.com/tls-cert-keyvault-uri": "https://test.vault.azure.net/secrets/test-secret",
},
},
Spec: netv1.IngressSpec{
IngressClassName: &ingressClass,
},
ing := spcTestIngress.DeepCopy()
ing.Annotations = map[string]string{
"kubernetes.azure.com/tls-cert-keyvault-uri": "https://test.vault.azure.net/secrets/test-secret",
}

spc := &secv1.SecretProviderClass{}
Expand Down
Loading