weiguoqiang/cert-manager
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

Code review feedback

Browse Source 
Signed-off-by: irbekrm <irbekrm@gmail.com>
Co-authored-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: irbekrm <irbekrm@gmail.com>
This commit is contained in:
irbekrm 2023-03-28 17:52:33 +01:00
parent 729d358cd2
commit 85c766a082
3 changed files with 70 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
internal/informers/core_basic.go
View File

@ -43,7 +43,7 @@ type baseFactory struct {
func NewBaseKubeInformerFactory(client kubernetes.Interface, resync time.Duration, namespace string) KubeInformerFactory {
return &baseFactory{
f: kubeinformers.NewSharedInformerFactory(client, resync),
f: kubeinformers.NewSharedInformerFactoryWithOptions(client, resync, kubeinformers.WithNamespace(namespace)),
// namespace is set to a non-empty value if cert-manager
// controller is scoped to a single namespace via --namespace
// flag

84
internal/informers/core_filteredsecrets.go
View File

@ -24,9 +24,8 @@ import (
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/selection"
"k8s.io/apimachinery/pkg/types"
kubeinformers "k8s.io/client-go/informers"
@ -60,7 +59,7 @@ func init() {
}
isCertManageSecretLabelSelector = labels.NewSelector().Add(*r)
r, err = labels.NewRequirement(cmapi.PartOfCertManagerControllerLabelKey, selection.DoesNotExist, make([]string, 0))
r, err = labels.NewRequirement(cmapi.PartOfCertManagerControllerLabelKey, selection.DoesNotExist, nil)
if err != nil {
panic(fmt.Errorf("internal error: failed to build label selector to filter non-cert-manager secrets: %w", err))
}
@ -77,7 +76,7 @@ type filteredSecretsFactory struct {
func NewFilteredSecretsKubeInformerFactory(ctx context.Context, typedClient kubernetes.Interface, metadataClient metadata.Interface, resync time.Duration, namespace string) KubeInformerFactory {
return &filteredSecretsFactory{
typedInformerFactory: kubeinformers.NewSharedInformerFactory(typedClient, resync),
typedInformerFactory: kubeinformers.NewSharedInformerFactoryWithOptions(typedClient, resync, kubeinformers.WithNamespace(namespace)),
metadataInformerFactory: metadatainformer.NewFilteredSharedInformerFactory(metadataClient, resync, namespace, func(listOptions *metav1.ListOptions) {
listOptions.LabelSelector = isNotCertManagerSecretLabelSelector.String()
@ -251,15 +250,16 @@ type secretNamespaceLister struct {
func (snl *secretNamespaceLister) Get(name string) (*corev1.Secret, error) {
log := logf.FromContext(snl.ctx)
log = log.WithValues("secret", name, "namespace", snl.namespace)
var secretFoundInTypedCache, secretFoundInMetadataCache bool
secret, err := snl.typedLister.Secrets(snl.namespace).Get(name)
if err == nil {
secret, typedCacheErr := snl.typedLister.Secrets(snl.namespace).Get(name)
if typedCacheErr == nil {
secretFoundInTypedCache = true
}
if err != nil && !apierrors.IsNotFound(err) {
log.Error(err, "error getting secret from typed cache")
return nil, fmt.Errorf("error retrieving secret from the typed cache: %w", err)
if typedCacheErr != nil && !apierrors.IsNotFound(typedCacheErr) {
log.Error(typedCacheErr, "error getting secret from typed cache")
return nil, fmt.Errorf("error retrieving secret from the typed cache: %w", typedCacheErr)
}
_, partialMetadataGetErr := snl.partialMetadataLister.Namespace(snl.namespace).Get(name)
if partialMetadataGetErr == nil {
@ -267,11 +267,16 @@ func (snl *secretNamespaceLister) Get(name string) (*corev1.Secret, error) {
}
if partialMetadataGetErr != nil && !apierrors.IsNotFound(partialMetadataGetErr) {
return nil, fmt.Errorf("error retrieving object from partial object metadata cache: %w", err)
log.Error(partialMetadataGetErr, "error getting secret from metadata cache")
return nil, fmt.Errorf("error retrieving object from partial object metadata cache: %w", partialMetadataGetErr)
}
if secretFoundInMetadataCache && secretFoundInTypedCache {
log.Info(fmt.Sprintf("warning: possible internal error: stale cache: secret found both in typed cache and in partial cache: %s", pleaseOpenIssue), "secret name")
if secretFoundInMetadataCache {
// if secret is found in both caches log an error and return the version from kube apiserver
if secretFoundInTypedCache {
key := types.NamespacedName{Namespace: snl.namespace, Name: name}
log.Info(fmt.Sprintf("warning: possible internal error: stale cache: secret found both in typed cache and in partial cache: %s", pleaseOpenIssue), "secret", key)
}
return snl.typedClient.Secrets(snl.namespace).Get(snl.ctx, name, metav1.GetOptions{})
}
@ -279,47 +284,51 @@ func (snl *secretNamespaceLister) Get(name string) (*corev1.Secret, error) {
return secret, nil
}
if secretFoundInMetadataCache {
return snl.typedClient.Secrets(snl.namespace).Get(snl.ctx, name, metav1.GetOptions{})
}
return nil, partialMetadataGetErr
// If we get here it is because secret was found neither in typed cache
// nor partial metadata cache
return nil, apierrors.NewNotFound(schema.GroupResource{Group: corev1.GroupName, Resource: corev1.ResourceSecrets.String()}, name)
}
func (snl *secretNamespaceLister) List(selector labels.Selector) ([]*corev1.Secret, error) {
log := logf.FromContext(snl.ctx)
log = log.WithValues("secrets namespace", snl.namespace, "secrets selector", selector.String())
matchingSecretsMap := make(map[string]*corev1.Secret)
matchingSecretsMap := make(map[types.NamespacedName]*corev1.Secret)
typedSecrets, err := snl.typedLister.List(selector)
if err != nil {
log.Error(err, "error listing Secrets from typed cache")
return nil, err
return nil, fmt.Errorf("error listing Secrets from typed cache: %w", err)
}
for _, secret := range typedSecrets {
key := types.NamespacedName{Namespace: secret.Namespace, Name: secret.Name}.String()
key := types.NamespacedName{Namespace: secret.Namespace, Name: secret.Name}
matchingSecretsMap[key] = secret
}
metadataSecrets, err := snl.partialMetadataLister.List(selector)
if err != nil {
log.Error(err, "error listing Secrets from metadata only cache")
return nil, err
return nil, fmt.Errorf("error listing Secrets from metadata only cache: %w", err)
}
if len(metadataSecrets) > 0 {
// We currently do not LIST unlabelled Secrets. This log line is
// here in case we do it sometime in the future at which point
// we can see whether the metadata functionality is performant
// enough.
log.V(logf.InfoLevel).Info("unexpected behaviour: secrets LISTed from metadata cache. Please open an isue")
}
// In practice this section will never be used. The only place
// where we LIST Secrets is in keymanager controller where we list
// temporary Certificate Secrets which are all labelled.
// It is unlikely that we will every list unlabelled Secrets.
for _, secretMeta := range metadataSecrets {
unstructuredObj, err := objectToUnstructured(secretMeta)
if err != nil {
log.Error(err, "error converting runtime object to unstructured")
return nil, err
}
name := unstructuredObj.GetName()
key := types.NamespacedName{Namespace: snl.namespace, Name: name}.String()
key := types.NamespacedName{Namespace: secretMeta.Namespace, Name: secretMeta.Name}
if _, ok := matchingSecretsMap[key]; ok {
log.Info(fmt.Sprintf("warning: possible internal error: stale cache: secret found both in typed cache and in partial cache: %s", pleaseOpenIssue), "secret name", name)
log.Info(fmt.Sprintf("warning: possible internal error: stale cache: secret found both in typed cache and in partial cache: %s", pleaseOpenIssue), "secret name", secretMeta.Name)
// in case of duplicates, return the version from kube apiserver
}
secret, err := snl.typedClient.Secrets(snl.namespace).Get(snl.ctx, name, metav1.GetOptions{})
secret, err := snl.typedClient.Secrets(snl.namespace).Get(snl.ctx, secretMeta.Name, metav1.GetOptions{})
if err != nil {
log.Error(err, "error retrieving secret from kube apiserver", "secret name", name)
return nil, err
log.Error(err, "error retrieving secret from kube apiserver", "secret name", secretMeta.Name)
return nil, fmt.Errorf("error retrieving Secret from kube apiserver: %w", err)
}
matchingSecretsMap[key] = secret
}
@ -330,14 +339,3 @@ func (snl *secretNamespaceLister) List(selector labels.Selector) ([]*corev1.Secr
}
return matchingSecrets, nil
}
func objectToUnstructured(obj runtime.Object) (*unstructured.Unstructured, error) {
unstructuredContent, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
if err != nil {
return nil, err
}
u := &unstructured.Unstructured{}
u.SetUnstructuredContent(unstructuredContent)
u.SetGroupVersionKind(obj.GetObjectKind().GroupVersionKind())
return u, nil
}

28
internal/informers/core_filteredsecrets_test.go
View File

@ -188,6 +188,34 @@ func Test_secretNamespaceLister_Get(t *testing.T) {
},
wantErr: true,
},
"if Secret found not found in either cache return not found error": {
namespace: "foo",
name: "foo",
typedLister: FakeSecretLister{
NamespaceLister: FakeSecretNamespaceLister{
FakeGet: func(string) (*corev1.Secret, error) {
return nil, apierrors.NewNotFound(schema.GroupResource{}, "foo")
},
},
},
partialMetadataLister: FakeMetadataLister{
NamespaceLister: FakeMetadataNamespaceLister{
FakeGet: func(string) (*metav1.PartialObjectMetadata, error) {
return &metav1.PartialObjectMetadata{}, apierrors.NewNotFound(schema.GroupResource{}, "foo")
},
},
},
typedClient: FakeSecretsGetter{
FakeSecrets: func(string) typedcorev1.SecretInterface {
return FakeSecretInterface{
FakeGet: func(context.Context, string, metav1.GetOptions) (*corev1.Secret, error) {
return nil, errors.New("some error")
},
}
},
},
wantErr: true,
},
}
for name, scenario := range tests {
t.Run(name, func(t *testing.T) {