djpbessems/ContainerImage.Pinniped
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #305 from vmware-tanzu/quiet-secrets-controllers

Browse Source 
Sync Secret-watching controller less often by adjusting their filters to be more specific
This commit is contained in:
Ryan Richard 2020-12-18 18:21:51 -08:00 committed by GitHub
commit 546b8b5d25
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
24 changed files with 437 additions and 156 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
cmd/pinniped-supervisor/main.go
View File

@ -237,7 +237,9 @@ func startControllers(
pinnipedClient,
pinnipedInformers.IDP().V1alpha1().OIDCIdentityProviders(),
secretInformer,
klogr.New()),
klogr.New(),
controllerlib.WithInformer,
),
singletonWorker)
kubeInformers.Start(ctx.Done())

23
internal/controller/supervisorconfig/generator/federation_domain_secrets.go
View File

@ -24,10 +24,6 @@ import (
"go.pinniped.dev/internal/plog"
)
const (
federationDomainKind = "FederationDomain"
)
type federationDomainSecretsController struct {
secretHelper SecretHelper
secretRefFunc func(domain *configv1alpha1.FederationDomain) *corev1.LocalObjectReference
@ -65,16 +61,7 @@ func NewFederationDomainSecretsController(
// should get notified via the corresponding FederationDomain key.
withInformer(
secretInformer,
pinnipedcontroller.SimpleFilter(isFederationDomainControllee, func(obj metav1.Object) controllerlib.Key {
if isFederationDomainControllee(obj) {
controller := metav1.GetControllerOf(obj)
return controllerlib.Key{
Name: controller.Name,
Namespace: obj.GetNamespace(),
}
}
return controllerlib.Key{}
}),
pinnipedcontroller.SimpleFilter(secretHelper.Handles, pinnipedcontroller.SecretIsControlledByParentFunc(secretHelper.Handles)),
controllerlib.InformerOption{},
),
// We want to be notified when anything happens to an FederationDomain.
@ -240,11 +227,3 @@ func (c *federationDomainSecretsController) updateFederationDomain(
return err
})
}
// isFederationDomainControllee returns whether the provided obj is controlled by an FederationDomain.
func isFederationDomainControllee(obj metav1.Object) bool {
controller := metav1.GetControllerOf(obj)
return controller != nil &&
controller.APIVersion == configv1alpha1.SchemeGroupVersion.String() &&
controller.Kind == federationDomainKind
}

79
internal/controller/supervisorconfig/generator/federation_domain_secrets_test.go
View File

@ -5,6 +5,7 @@ package generator
import (
"context"
"crypto/rand"
"errors"
"fmt"
"sync"
@ -35,7 +36,7 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
tests := []struct {
name string
secret corev1.Secret
secret metav1.Object
wantAdd bool
wantUpdate bool
wantDelete bool
@ -43,13 +44,15 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
}{
{
name: "no owner reference",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/federation-domain-token-signing-key",
ObjectMeta: metav1.ObjectMeta{},
},
},
{
name: "owner reference without correct APIVersion",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/federation-domain-token-signing-key",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -64,7 +67,8 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
},
{
name: "owner reference without correct Kind",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/federation-domain-token-signing-key",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -79,7 +83,8 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
},
{
name: "owner reference without controller set to true",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/federation-domain-token-signing-key",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -94,7 +99,8 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
},
{
name: "correct owner reference",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/federation-domain-token-signing-key",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -114,7 +120,8 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
},
{
name: "multiple owner references",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/federation-domain-token-signing-key",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -135,16 +142,44 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
wantDelete: true,
wantParent: controllerlib.Key{Namespace: "some-namespace", Name: "some-name"},
},
{
name: "correct owner reference but wrong secret type",
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/this-is-the-wrong-type",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: configv1alpha1.SchemeGroupVersion.String(),
Kind: "FederationDomain",
Name: "some-name",
Controller: boolPtr(true),
},
},
},
},
},
{
name: "resource of wrong data type",
secret: &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: "some-namespace",
},
},
},
}
for _, test := range tests {
test := test
t.Run(test.name, func(t *testing.T) {
t.Parallel()
ctrl := gomock.NewController(t)
t.Cleanup(ctrl.Finish)
secretHelper := mocksecrethelper.NewMockSecretHelper(ctrl)
secretHelper.EXPECT().NamePrefix().Times(1).Return("some-name")
secretHelper := NewSymmetricSecretHelper(
"some-name",
map[string]string{},
rand.Reader,
SecretUsageTokenSigningKey,
func(cacheKey string, cacheValue []byte) {},
)
secretInformer := kubeinformers.NewSharedInformerFactory(
kubernetesfake.NewSimpleClientset(),
@ -167,11 +202,11 @@ func TestFederationDomainControllerFilterSecret(t *testing.T) {
unrelated := corev1.Secret{}
filter := withInformer.GetFilterForInformer(secretInformer)
require.Equal(t, test.wantAdd, filter.Add(&test.secret))
require.Equal(t, test.wantUpdate, filter.Update(&unrelated, &test.secret))
require.Equal(t, test.wantUpdate, filter.Update(&test.secret, &unrelated))
require.Equal(t, test.wantDelete, filter.Delete(&test.secret))
require.Equal(t, test.wantParent, filter.Parent(&test.secret))
require.Equal(t, test.wantAdd, filter.Add(test.secret))
require.Equal(t, test.wantUpdate, filter.Update(&unrelated, test.secret))
require.Equal(t, test.wantUpdate, filter.Update(test.secret, &unrelated))
require.Equal(t, test.wantDelete, filter.Delete(test.secret))
require.Equal(t, test.wantParent, filter.Parent(test.secret))
})
}
}
@ -201,10 +236,13 @@ func TestNewFederationDomainSecretsControllerFilterFederationDomain(t *testing.T
t.Run(test.name, func(t *testing.T) {
t.Parallel()
ctrl := gomock.NewController(t)
t.Cleanup(ctrl.Finish)
secretHelper := mocksecrethelper.NewMockSecretHelper(ctrl)
secretHelper.EXPECT().NamePrefix().Times(1).Return("some-name")
secretHelper := NewSymmetricSecretHelper(
"some-name",
map[string]string{},
rand.Reader,
SecretUsageTokenSigningKey,
func(cacheKey string, cacheValue []byte) {},
)
secretInformer := kubeinformers.NewSharedInformerFactory(
kubernetesfake.NewSimpleClientset(),
@ -635,6 +673,7 @@ func TestFederationDomainSecretsControllerSync(t *testing.T) {
if test.secretHelper != nil {
test.secretHelper(secretHelper)
}
secretHelper.EXPECT().Handles(gomock.Any()).AnyTimes().Return(true)
c := NewFederationDomainSecretsController(
secretHelper,

28
internal/controller/supervisorconfig/generator/secret_helper.go
View File

@ -23,6 +23,7 @@ type SecretHelper interface {
Generate(*configv1alpha1.FederationDomain) (*corev1.Secret, error)
IsValid(*configv1alpha1.FederationDomain, *corev1.Secret) bool
ObserveActiveSecretAndUpdateParentFederationDomain(*configv1alpha1.FederationDomain, *corev1.Secret) *configv1alpha1.FederationDomain
Handles(metav1.Object) bool
}
const (
@ -38,6 +39,8 @@ const (
// FederationDomainStateEncryptionKeyType for the Secret storing the FederationDomain state encryption key.
FederationDomainStateEncryptionKeyType corev1.SecretType = "secrets.pinniped.dev/federation-domain-state-encryption-key"
federationDomainKind = "FederationDomain"
// symmetricSecretDataKey is the corev1.Secret.Data key for the symmetric key value generated by this helper.
symmetricSecretDataKey = "key"
@ -101,7 +104,7 @@ func (s *symmetricSecretHelper) Generate(parent *configv1alpha1.FederationDomain
*metav1.NewControllerRef(parent, schema.GroupVersionKind{
Group: configv1alpha1.SchemeGroupVersion.Group,
Version: configv1alpha1.SchemeGroupVersion.Version,
Kind: "FederationDomain",
Kind: federationDomainKind,
}),
},
},
@ -166,3 +169,26 @@ func (s *symmetricSecretHelper) secretType() corev1.SecretType {
panic(fmt.Sprintf("unknown secret usage enum value: %d", s.secretUsage))
}
}
func (s *symmetricSecretHelper) Handles(obj metav1.Object) bool {
return IsFederationDomainSecretOfType(obj, s.secretType())
}
func IsFederationDomainSecretOfType(obj metav1.Object, secretType corev1.SecretType) bool {
secret, ok := obj.(*corev1.Secret)
if !ok {
return false
}
if secret.Type != secretType {
return false
}
return isFederationDomainControllee(secret)
}
// isFederationDomainControllee returns whether the provided obj is controlled by an FederationDomain.
func isFederationDomainControllee(obj metav1.Object) bool {
controller := metav1.GetControllerOf(obj)
return controller != nil &&
controller.APIVersion == configv1alpha1.SchemeGroupVersion.String() &&
controller.Kind == federationDomainKind
}

8
internal/controller/supervisorconfig/generator/secret_helper_test.go
View File

@ -108,6 +108,14 @@ func TestSymmetricSecretHelper(t *testing.T) {
require.Equal(t, parent.Spec.Issuer, federationDomainIssuerValue)
require.Equal(t, child.Name, test.wantSetFederationDomainField(parent))
require.Equal(t, child.Data["key"], symmetricKeyValue)
require.True(t, h.Handles(child))
wrongTypedChild := child.DeepCopy()
wrongTypedChild.Type = "the-wrong-type"
require.False(t, h.Handles(wrongTypedChild))
wrongOwnerKindChild := child.DeepCopy()
wrongOwnerKindChild.OwnerReferences[0].Kind = "WrongKind"
require.False(t, h.Handles(wrongOwnerKindChild))
})
}
}

11
internal/controller/supervisorconfig/generator/supervisor_secrets.go
View File

@ -58,8 +58,15 @@ func NewSupervisorSecretsController(
withInformer(
secretInformer,
pinnipedcontroller.SimpleFilter(func(obj metav1.Object) bool {
ownerReferences := obj.GetOwnerReferences()
for i := range obj.GetOwnerReferences() {
secret, ok := obj.(*corev1.Secret)
if !ok {
return false
}
if secret.Type != SupervisorCSRFSigningKeySecretType {
return false
}
ownerReferences := secret.GetOwnerReferences()
for i := range secret.GetOwnerReferences() {
if ownerReferences[i].UID == owner.GetUID() {
return true
}

84
internal/controller/supervisorconfig/generator/supervisor_secrets_test.go
View File

@ -51,14 +51,15 @@ func TestSupervisorSecretsControllerFilterSecret(t *testing.T) {
tests := []struct {
name string
secret corev1.Secret
secret metav1.Object
wantAdd bool
wantUpdate bool
wantDelete bool
}{
{
name: "owner reference is missing",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/supervisor-csrf-signing-key",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
},
@ -66,7 +67,8 @@ func TestSupervisorSecretsControllerFilterSecret(t *testing.T) {
},
{
name: "owner reference with incorrect `APIVersion`",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/supervisor-csrf-signing-key",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -84,7 +86,8 @@ func TestSupervisorSecretsControllerFilterSecret(t *testing.T) {
},
{
name: "owner reference with incorrect `Kind`",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/supervisor-csrf-signing-key",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -101,23 +104,10 @@ func TestSupervisorSecretsControllerFilterSecret(t *testing.T) {
wantUpdate: true,
wantDelete: true,
},
{
name: "owner reference with `Controller`: true",
secret: corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
*metav1.NewControllerRef(owner, ownerGVK),
},
},
},
wantAdd: true,
wantUpdate: true,
wantDelete: true,
},
{
name: "expected owner reference with incorrect `UID`",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/supervisor-csrf-signing-key",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -132,11 +122,15 @@ func TestSupervisorSecretsControllerFilterSecret(t *testing.T) {
},
},
{
name: "expected owner reference - where `Controller`: false",
secret: corev1.Secret{
name: "multiple owner references (expected owner reference, and one more)",
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/supervisor-csrf-signing-key",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
{
Kind: "UnrelatedKind",
},
{
APIVersion: ownerGVK.String(),
Name: owner.GetName(),
@ -151,14 +145,48 @@ func TestSupervisorSecretsControllerFilterSecret(t *testing.T) {
wantDelete: true,
},
{
name: "multiple owner references (expected owner reference, and one more)",
secret: corev1.Secret{
name: "otherwise happy secret but has the wrong Secret type",
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/this-is-not-supervisor-csrf-signing-key-type",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
{
Kind: "UnrelatedKind",
APIVersion: ownerGVK.String(),
Name: owner.GetName(),
Kind: ownerGVK.Kind,
UID: owner.GetUID(),
},
},
},
},
},
{
name: "not a secret",
secret: &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "some-namespace"}},
},
{
name: "owner reference with `Controller`: true",
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/supervisor-csrf-signing-key",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
*metav1.NewControllerRef(owner, ownerGVK),
},
},
},
wantAdd: true,
wantUpdate: true,
wantDelete: true,
},
{
name: "expected owner reference - where `Controller`: false",
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/supervisor-csrf-signing-key",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: ownerGVK.String(),
Name: owner.GetName(),
@ -195,10 +223,10 @@ func TestSupervisorSecretsControllerFilterSecret(t *testing.T) {
unrelated := corev1.Secret{}
filter := withInformer.GetFilterForInformer(secretInformer)
require.Equal(t, test.wantAdd, filter.Add(&test.secret))
require.Equal(t, test.wantUpdate, filter.Update(&unrelated, &test.secret))
require.Equal(t, test.wantUpdate, filter.Update(&test.secret, &unrelated))
require.Equal(t, test.wantDelete, filter.Delete(&test.secret))
require.Equal(t, test.wantAdd, filter.Add(test.secret))
require.Equal(t, test.wantUpdate, filter.Update(&unrelated, test.secret))
require.Equal(t, test.wantUpdate, filter.Update(test.secret, &unrelated))
require.Equal(t, test.wantDelete, filter.Delete(test.secret))
})
}
}

2
internal/controller/supervisorconfig/jwks_observer.go
View File

@ -52,7 +52,7 @@ func NewJWKSObserverController(
},
withInformer(
secretInformer,
pinnipedcontroller.MatchAnythingFilter(nil),
pinnipedcontroller.MatchAnySecretOfTypeFilter(jwksSecretTypeValue, nil),
controllerlib.InformerOption{},
),
withInformer(

21
internal/controller/supervisorconfig/jwks_observer_test.go
View File

@ -52,23 +52,32 @@ func TestJWKSObserverControllerInformerFilters(t *testing.T) {
when("watching Secret objects", func() {
var (
subject controllerlib.Filter
secret, otherSecret *corev1.Secret
secret, otherTypeSecret *corev1.Secret
)
it.Before(func() {
subject = secretsInformerFilter
secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}}
otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}}
secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}, Type: "secrets.pinniped.dev/federation-domain-jwks"}
otherTypeSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}, Type: "other"}
})
when("any Secret changes", func() {
when("any Secret of the JWKS type changes", func() {
it("returns true to trigger the sync method", func() {
r.True(subject.Add(secret))
r.True(subject.Update(secret, otherSecret))
r.True(subject.Update(otherSecret, secret))
r.True(subject.Update(secret, otherTypeSecret))
r.True(subject.Update(otherTypeSecret, secret))
r.True(subject.Delete(secret))
})
})
when("any Secret of some other type changes", func() {
it("returns false to skip the sync method", func() {
r.False(subject.Add(otherTypeSecret))
r.False(subject.Update(otherTypeSecret, otherTypeSecret))
r.False(subject.Update(otherTypeSecret, otherTypeSecret))
r.False(subject.Delete(otherTypeSecret))
})
})
})
when("watching FederationDomain objects", func() {

31
internal/controller/supervisorconfig/jwks_writer.go
View File

@ -26,6 +26,7 @@ import (
pinnipedclientset "go.pinniped.dev/generated/1.19/client/supervisor/clientset/versioned"
configinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions/config/v1alpha1"
pinnipedcontroller "go.pinniped.dev/internal/controller"
"go.pinniped.dev/internal/controller/supervisorconfig/generator"
"go.pinniped.dev/internal/controllerlib"
"go.pinniped.dev/internal/plog"
)
@ -76,6 +77,10 @@ func NewJWKSWriterController(
federationDomainInformer configinformers.FederationDomainInformer,
withInformer pinnipedcontroller.WithInformerOptionFunc,
) controllerlib.Controller {
isSecretToSync := func(obj metav1.Object) bool {
return generator.IsFederationDomainSecretOfType(obj, jwksSecretTypeValue)
}
return controllerlib.New(
controllerlib.Config{
Name: "JWKSController",
@ -91,23 +96,7 @@ func NewJWKSWriterController(
// should get notified via the corresponding FederationDomain key.
withInformer(
secretInformer,
controllerlib.FilterFuncs{
ParentFunc: func(obj metav1.Object) controllerlib.Key {
if isFederationDomainControllee(obj) {
controller := metav1.GetControllerOf(obj)
return controllerlib.Key{
Name: controller.Name,
Namespace: obj.GetNamespace(),
}
}
return controllerlib.Key{}
},
AddFunc: isFederationDomainControllee,
UpdateFunc: func(oldObj, newObj metav1.Object) bool {
return isFederationDomainControllee(oldObj) || isFederationDomainControllee(newObj)
},
DeleteFunc: isFederationDomainControllee,
},
pinnipedcontroller.SimpleFilter(isSecretToSync, pinnipedcontroller.SecretIsControlledByParentFunc(isSecretToSync)),
controllerlib.InformerOption{},
),
// We want to be notified when anything happens to an FederationDomain.
@ -316,14 +305,6 @@ func (c *jwksWriterController) updateFederationDomain(
})
}
// isFederationDomainControlle returns whether the provided obj is controlled by a FederationDomain.
func isFederationDomainControllee(obj metav1.Object) bool {
controller := metav1.GetControllerOf(obj)
return controller != nil &&
controller.APIVersion == configv1alpha1.SchemeGroupVersion.String() &&
controller.Kind == federationDomainKind
}
// isValid returns whether the provided secret contains a valid active JWK and verification JWKS.
func isValid(secret *corev1.Secret) bool {
if secret.Type != jwksSecretTypeValue {

55
internal/controller/supervisorconfig/jwks_writer_test.go
View File

@ -35,7 +35,7 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
tests := []struct {
name string
secret corev1.Secret
secret metav1.Object
wantAdd bool
wantUpdate bool
wantDelete bool
@ -43,13 +43,15 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
}{
{
name: "no owner reference",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/federation-domain-jwks",
ObjectMeta: metav1.ObjectMeta{},
},
},
{
name: "owner reference without correct APIVersion",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/federation-domain-jwks",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -64,7 +66,8 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
},
{
name: "owner reference without correct Kind",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/federation-domain-jwks",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -79,7 +82,8 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
},
{
name: "owner reference without controller set to true",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/federation-domain-jwks",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -94,7 +98,8 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
},
{
name: "correct owner reference",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/federation-domain-jwks",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -114,7 +119,8 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
},
{
name: "multiple owner references",
secret: corev1.Secret{
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/federation-domain-jwks",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
@ -135,6 +141,31 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
wantDelete: true,
wantParent: controllerlib.Key{Namespace: "some-namespace", Name: "some-name"},
},
{
name: "correct owner reference but wrong type",
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/some-other-type",
ObjectMeta: metav1.ObjectMeta{
Namespace: "some-namespace",
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: configv1alpha1.SchemeGroupVersion.String(),
Kind: "FederationDomain",
Name: "some-name",
Controller: boolPtr(true),
},
},
},
},
},
{
name: "resource of wrong data type",
secret: &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: "some-namespace",
},
},
},
}
for _, test := range tests {
test := test
@ -161,11 +192,11 @@ func TestJWKSWriterControllerFilterSecret(t *testing.T) {
unrelated := corev1.Secret{}
filter := withInformer.GetFilterForInformer(secretInformer)
require.Equal(t, test.wantAdd, filter.Add(&test.secret))
require.Equal(t, test.wantUpdate, filter.Update(&unrelated, &test.secret))
require.Equal(t, test.wantUpdate, filter.Update(&test.secret, &unrelated))
require.Equal(t, test.wantDelete, filter.Delete(&test.secret))
require.Equal(t, test.wantParent, filter.Parent(&test.secret))
require.Equal(t, test.wantAdd, filter.Add(test.secret))
require.Equal(t, test.wantUpdate, filter.Update(&unrelated, test.secret))
require.Equal(t, test.wantUpdate, filter.Update(test.secret, &unrelated))
require.Equal(t, test.wantDelete, filter.Delete(test.secret))
require.Equal(t, test.wantParent, filter.Parent(test.secret))
})
}
}

3
internal/controller/supervisorconfig/tls_cert_observer.go
View File

@ -9,6 +9,7 @@ import (
"net/url"
"strings"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/labels"
corev1informers "k8s.io/client-go/informers/core/v1"
@ -49,7 +50,7 @@ func NewTLSCertObserverController(
},
withInformer(
secretInformer,
pinnipedcontroller.MatchAnythingFilter(nil),
pinnipedcontroller.MatchAnySecretOfTypeFilter(v1.SecretTypeTLS, nil),
controllerlib.InformerOption{},
),
withInformer(

14
internal/controller/supervisorconfig/tls_cert_observer_test.go
View File

@ -59,11 +59,11 @@ func TestTLSCertObserverControllerInformerFilters(t *testing.T) {
it.Before(func() {
subject = secretsInformerFilter
secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}}
otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}}
secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}, Type: corev1.SecretTypeTLS}
otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}, Type: "other type"}
})
when("any Secret changes", func() {
when("any Secret of type TLS changes", func() {
it("returns true to trigger the sync method", func() {
r.True(subject.Add(secret))
r.True(subject.Update(secret, otherSecret))
@ -71,6 +71,14 @@ func TestTLSCertObserverControllerInformerFilters(t *testing.T) {
r.True(subject.Delete(secret))
})
})
when("any Secret that is not of type TLS changes", func() {
it("returns false to avoid triggering the sync method", func() {
r.False(subject.Add(otherSecret))
r.False(subject.Update(otherSecret, otherSecret))
r.False(subject.Delete(otherSecret))
})
})
})
when("watching FederationDomain objects", func() {

14
internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go
View File

@ -117,6 +117,7 @@ func New(
oidcIdentityProviderInformer idpinformers.OIDCIdentityProviderInformer,
secretInformer corev1informers.SecretInformer,
log logr.Logger,
withInformer pinnipedcontroller.WithInformerOptionFunc,
) controllerlib.Controller {
c := controller{
cache: idpCache,
@ -126,11 +127,18 @@ func New(
secretInformer: secretInformer,
validatorCache: &lruValidatorCache{cache: cache.NewExpiring()},
}
filter := pinnipedcontroller.MatchAnythingFilter(pinnipedcontroller.SingletonQueue())
return controllerlib.New(
controllerlib.Config{Name: controllerName, Syncer: &c},
controllerlib.WithInformer(oidcIdentityProviderInformer, filter, controllerlib.InformerOption{}),
controllerlib.WithInformer(secretInformer, filter, controllerlib.InformerOption{}),
withInformer(
oidcIdentityProviderInformer,
pinnipedcontroller.MatchAnythingFilter(pinnipedcontroller.SingletonQueue()),
controllerlib.InformerOption{},
),
withInformer(
secretInformer,
pinnipedcontroller.MatchAnySecretOfTypeFilter(oidcClientSecretType, pinnipedcontroller.SingletonQueue()),
controllerlib.InformerOption{},
),
)
}

74
internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go
View File

@ -31,6 +31,76 @@ import (
"go.pinniped.dev/internal/upstreamoidc"
)
func TestControllerFilterSecret(t *testing.T) {
t.Parallel()
tests := []struct {
name string
secret metav1.Object
wantAdd bool
wantUpdate bool
wantDelete bool
}{
{
name: "a secret of the right type",
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/oidc-client",
ObjectMeta: metav1.ObjectMeta{Name: "some-name", Namespace: "some-namespace"},
},
wantAdd: true,
wantUpdate: true,
wantDelete: true,
},
{
name: "a secret of the wrong type",
secret: &corev1.Secret{
Type: "secrets.pinniped.dev/not-the-oidc-client-type",
ObjectMeta: metav1.ObjectMeta{Name: "some-name", Namespace: "some-namespace"},
},
},
{
name: "resource of wrong data type",
secret: &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{Name: "some-name", Namespace: "some-namespace"},
},
},
}
for _, test := range tests {
test := test
t.Run(test.name, func(t *testing.T) {
t.Parallel()
fakePinnipedClient := pinnipedfake.NewSimpleClientset()
pinnipedInformers := pinnipedinformers.NewSharedInformerFactory(fakePinnipedClient, 0)
fakeKubeClient := fake.NewSimpleClientset()
kubeInformers := informers.NewSharedInformerFactory(fakeKubeClient, 0)
testLog := testlogger.New(t)
cache := provider.NewDynamicUpstreamIDPProvider()
cache.SetIDPList([]provider.UpstreamOIDCIdentityProviderI{
&upstreamoidc.ProviderConfig{Name: "initial-entry"},
})
secretInformer := kubeInformers.Core().V1().Secrets()
withInformer := testutil.NewObservableWithInformerOption()
New(
cache,
nil,
pinnipedInformers.IDP().V1alpha1().OIDCIdentityProviders(),
secretInformer,
testLog,
withInformer.WithInformer,
)
unrelated := corev1.Secret{}
filter := withInformer.GetFilterForInformer(secretInformer)
require.Equal(t, test.wantAdd, filter.Add(test.secret))
require.Equal(t, test.wantUpdate, filter.Update(&unrelated, test.secret))
require.Equal(t, test.wantUpdate, filter.Update(test.secret, &unrelated))
require.Equal(t, test.wantDelete, filter.Delete(test.secret))
})
}
}
func TestController(t *testing.T) {
t.Parallel()
now := metav1.NewTime(time.Now().UTC())
@ -550,7 +620,9 @@ func TestController(t *testing.T) {
fakePinnipedClient,
pinnipedInformers.IDP().V1alpha1().OIDCIdentityProviders(),
kubeInformers.Core().V1().Secrets(),
testLog)
testLog,
controllerlib.WithInformer,
)
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()

17
internal/controller/supervisorstorage/garbage_collector.go
View File

@ -34,6 +34,14 @@ func GarbageCollectorController(
secretInformer corev1informers.SecretInformer,
withInformer pinnipedcontroller.WithInformerOptionFunc,
) controllerlib.Controller {
isSecretWithGCAnnotation := func(obj metav1.Object) bool {
secret, ok := obj.(*v1.Secret)
if !ok {
return false
}
_, ok = secret.Annotations[crud.SecretLifetimeAnnotationKey]
return ok
}
return controllerlib.New(
controllerlib.Config{
Name: "garbage-collector-controller",
@ -45,7 +53,14 @@ func GarbageCollectorController(
},
withInformer(
secretInformer,
pinnipedcontroller.MatchAnythingFilter(nil),
controllerlib.FilterFuncs{
AddFunc: isSecretWithGCAnnotation,
UpdateFunc: func(oldObj, newObj metav1.Object) bool {
return isSecretWithGCAnnotation(oldObj) || isSecretWithGCAnnotation(newObj)
},
DeleteFunc: func(obj metav1.Object) bool { return false }, // ignore all deletes
ParentFunc: nil,
},
controllerlib.InformerOption{},
),
)

43
internal/controller/supervisorstorage/garbage_collector_test.go
View File

@ -49,21 +49,46 @@ func TestGarbageCollectorControllerInformerFilters(t *testing.T) {
when("watching Secret objects", func() {
var (
subject controllerlib.Filter
secret, otherSecret *corev1.Secret
secretWithAnnotation, otherSecret *corev1.Secret
)
it.Before(func() {
subject = secretsInformerFilter
secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}}
otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}}
secretWithAnnotation = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace", Annotations: map[string]string{
"storage.pinniped.dev/garbage-collect-after": "some timestamp",
}}}
otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-namespace"}}
})
when("any Secret changes", func() {
it("returns true to trigger the sync function for all secrets", func() {
r.True(subject.Add(secret))
r.True(subject.Update(secret, otherSecret))
r.True(subject.Update(otherSecret, secret))
r.True(subject.Delete(secret))
when("any Secret with the required annotation is added or updated", func() {
it("returns true to trigger the sync function", func() {
r.True(subject.Add(secretWithAnnotation))
r.True(subject.Update(secretWithAnnotation, otherSecret))
r.True(subject.Update(otherSecret, secretWithAnnotation))
})
})
when("any Secret with the required annotation is deleted", func() {
it("returns false to skip the sync function because it does not need to worry about secrets that are already gone", func() {
r.False(subject.Delete(secretWithAnnotation))
})
})
when("any Secret without the required annotation changes", func() {
it("returns false to skip the sync function", func() {
r.False(subject.Add(otherSecret))
r.False(subject.Update(otherSecret, otherSecret))
r.False(subject.Delete(otherSecret))
})
})
when("any other type is passed", func() {
it("returns false to skip the sync function", func() {
wrongType := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "some-ns", Namespace: "some-ns"}}
r.False(subject.Add(wrongType))
r.False(subject.Update(wrongType, wrongType))
r.False(subject.Delete(wrongType))
})
})
})

25
internal/controller/utils.go
View File

@ -4,6 +4,7 @@
package controller
import (
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"go.pinniped.dev/internal/controllerlib"
@ -32,6 +33,30 @@ func SimpleFilter(match func(metav1.Object) bool, parentFunc controllerlib.Paren
}
}
func MatchAnySecretOfTypeFilter(secretType v1.SecretType, parentFunc controllerlib.ParentFunc) controllerlib.Filter {
isSecretOfType := func(obj metav1.Object) bool {
secret, ok := obj.(*v1.Secret)
if !ok {
return false
}
return secret.Type == secretType
}
return SimpleFilter(isSecretOfType, parentFunc)
}
func SecretIsControlledByParentFunc(matchFunc func(obj metav1.Object) bool) func(obj metav1.Object) controllerlib.Key {
return func(obj metav1.Object) controllerlib.Key {
if matchFunc(obj) {
controller := metav1.GetControllerOf(obj)
return controllerlib.Key{
Name: controller.Name,
Namespace: obj.GetNamespace(),
}
}
return controllerlib.Key{}
}
}
// SingletonQueue returns a parent func that treats all events as the same key.
func SingletonQueue() controllerlib.ParentFunc {
return func(_ metav1.Object) controllerlib.Key {

21
internal/mocks/mocksecrethelper/mocksecrethelper.go
View File

@ -9,12 +9,11 @@
package mocksecrethelper
import (
reflect "reflect"
gomock "github.com/golang/mock/gomock"
v1 "k8s.io/api/core/v1"
v1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1"
v1 "k8s.io/api/core/v1"
v10 "k8s.io/apimachinery/pkg/apis/meta/v1"
reflect "reflect"
)
// MockSecretHelper is a mock of SecretHelper interface
@ -55,6 +54,20 @@ func (mr *MockSecretHelperMockRecorder) Generate(arg0 interface{}) *gomock.Call
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Generate", reflect.TypeOf((*MockSecretHelper)(nil).Generate), arg0)
}
// Handles mocks base method
func (m *MockSecretHelper) Handles(arg0 v10.Object) bool {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "Handles", arg0)
ret0, _ := ret[0].(bool)
return ret0
}
// Handles indicates an expected call of Handles
func (mr *MockSecretHelperMockRecorder) Handles(arg0 interface{}) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Handles", reflect.TypeOf((*MockSecretHelper)(nil).Handles), arg0)
}
// IsValid mocks base method
func (m *MockSecretHelper) IsValid(arg0 *v1alpha1.FederationDomain, arg1 *v1.Secret) bool {
m.ctrl.T.Helper()

4
test/integration/e2e_test.go
View File

@ -20,6 +20,8 @@ import (
"testing"
"time"
v1 "k8s.io/api/core/v1"
"github.com/stretchr/testify/require"
rbacv1 "k8s.io/api/rbac/v1"
@ -84,7 +86,7 @@ func TestE2EFullIntegration(t *testing.T) {
certSecret := library.CreateTestSecret(t,
env.SupervisorNamespace,
"oidc-provider-tls",
"kubernetes.io/tls",
v1.SecretTypeTLS,
map[string]string{"tls.crt": string(certPEM), "tls.key": string(keyPEM)},
)

1
test/integration/supervisor_discovery_test.go
View File

@ -287,6 +287,7 @@ func createTLSCertificateSecret(ctx context.Context, t *testing.T, ns string, ho
tlsCertChainPEM, tlsPrivateKeyPEM, err := certauthority.ToPEM(tlsCert)
require.NoError(t, err)
secret := corev1.Secret{
Type: corev1.SecretTypeTLS,
TypeMeta: metav1.TypeMeta{},
ObjectMeta: metav1.ObjectMeta{
Name: secretName,

4
test/integration/supervisor_login_test.go
View File

@ -18,6 +18,8 @@ import (
"testing"
"time"
v1 "k8s.io/api/core/v1"
coreosoidc "github.com/coreos/go-oidc"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@ -95,7 +97,7 @@ func TestSupervisorLogin(t *testing.T) {
certSecret := library.CreateTestSecret(t,
env.SupervisorNamespace,
"oidc-provider-tls",
"kubernetes.io/tls",
v1.SecretTypeTLS,
map[string]string{"tls.crt": string(certPEM), "tls.key": string(keyPEM)},
)

19
test/integration/supervisor_storage_garbage_collection_test.go
View File

@ -44,11 +44,12 @@ func TestStorageGarbageCollection(t *testing.T) {
}
// Start a background goroutine which will end as soon as the test ends.
// Keep updating a secret in the same namespace just to get the controller to respond faster.
// This is just a performance optimization because otherwise this test has to wait
// ~3 minutes for the controller's next full-resync.
// Keep updating a secret which has the "storage.pinniped.dev/garbage-collect-after" annotation
// in the same namespace just to get the controller to respond faster.
// This is just a performance optimization to make this test pass faster because otherwise
// this test has to wait ~3 minutes for the controller's next full-resync.
stopCh := make(chan bool, 1) // It is important that this channel be buffered.
go createAndUpdateSecretEveryTwoSeconds(t, stopCh, secrets)
go updateSecretEveryTwoSeconds(t, stopCh, secrets, secretNotYetExpired)
t.Cleanup(func() {
stopCh <- true
})
@ -68,12 +69,10 @@ func TestStorageGarbageCollection(t *testing.T) {
require.NoError(t, err)
}
func createAndUpdateSecretEveryTwoSeconds(t *testing.T, stopCh chan bool, secrets corev1client.SecretInterface) {
func updateSecretEveryTwoSeconds(t *testing.T, stopCh chan bool, secrets corev1client.SecretInterface, secret *v1.Secret) {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
defer cancel()
unrelatedSecret := createSecret(ctx, t, secrets, "unrelated-to-gc", time.Time{})
i := 0
for {
select {
@ -87,9 +86,9 @@ func createAndUpdateSecretEveryTwoSeconds(t *testing.T, stopCh chan bool, secret
time.Sleep(2 * time.Second)
i++
unrelatedSecret.Data["foo"] = []byte(fmt.Sprintf("bar-%d", i))
secret.Data["foo"] = []byte(fmt.Sprintf("bar-%d", i))
var updateErr error
unrelatedSecret, updateErr = secrets.Update(ctx, unrelatedSecret, metav1.UpdateOptions{})
secret, updateErr = secrets.Update(ctx, secret, metav1.UpdateOptions{})
require.NoError(t, updateErr)
}
}
@ -125,6 +124,6 @@ func newSecret(namePrefix string, expiresAt time.Time) *v1.Secret {
Annotations: annotations,
},
Data: map[string][]byte{"some-key": []byte("fake-data")},
Type: "storage.pinniped.dev/gc-test-integration-test",
Type: "storage.pinniped.dev/gc-test-integration-test", // the garbage collector controller doesn't care about the type
}
}

4
test/library/client.go
View File

@ -314,7 +314,7 @@ func RandHex(t *testing.T, numBytes int) string {
return hex.EncodeToString(buf)
}
func CreateTestSecret(t *testing.T, namespace string, baseName string, secretType string, stringData map[string]string) *corev1.Secret {
func CreateTestSecret(t *testing.T, namespace string, baseName string, secretType corev1.SecretType, stringData map[string]string) *corev1.Secret {
t.Helper()
client := NewClientset(t)
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
@ -322,7 +322,7 @@ func CreateTestSecret(t *testing.T, namespace string, baseName string, secretTyp
created, err := client.CoreV1().Secrets(namespace).Create(ctx, &corev1.Secret{
ObjectMeta: testObjectMeta(t, baseName),
Type: corev1.SecretType(secretType),
Type: secretType,
StringData: stringData,
}, metav1.CreateOptions{})
require.NoError(t, err)