Skip to content

Commit daf5ac2

Browse files
dinhxuanvudinhxuanvu
committed
fix(packagemanifests): Fix index key func to avoid key collision
Currently, packagemanifests cache uses packagemanifest name and ns as a key which is not unique as different catalogsources can have the same package in the same namespace. As a result, key collision happens in cache (map) which in turn leads to missing packagemanifests when users query for available packageminifests in the namespace. This PR adds new key function to use the combination of catalogsource name, namespace and packagemanifest name to ensure the key uniqueness to prevent collision. Signed-off-by: Vu Dinh <[email protected]>
1 parent 8e442f2 commit daf5ac2

File tree

2 files changed

+176
-4
lines changed

2 files changed

+176
-4
lines changed

‎pkg/package-server/provider/registry.go

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"encoding/json"
66
"fmt"
77
"io"
8+
"strings"
89
"sync"
910
"time"
1011

@@ -57,6 +58,30 @@ func catalogIndexFunc(obj interface{}) ([]string, error) {
5758
return []string{getSourceKey(pkg).String()}, nil
5859
}
5960

61+
func PackageManifestKeyFunc(obj interface{}) (string, error) {
62+
if key, ok := obj.(string); ok {
63+
return string(key), nil
64+
}
65+
66+
pkg, ok := obj.(*operators.PackageManifest)
67+
if !ok {
68+
return "", fmt.Errorf("obj is not a packagemanifest %v", obj)
69+
}
70+
71+
return pkg.Status.CatalogSource + "/" + pkg.GetNamespace() + "/" + pkg.GetName(), nil
72+
}
73+
74+
func SplitPackageManifestKey(key string) (catsrcname, namespace, name string, err error) {
75+
parts := strings.Split(key, "/")
76+
switch len(parts) {
77+
case 3:
78+
// catalogsource name, namespace and packagemanifest name
79+
return parts[0], parts[1], parts[2], nil
80+
}
81+
82+
return "", "", "", fmt.Errorf("unexpected key format: %q", key)
83+
}
84+
6085
type registryClient struct {
6186
api.RegistryClient
6287
catsrc *operatorsv1alpha1.CatalogSource
@@ -104,7 +129,7 @@ func NewRegistryProvider(ctx context.Context, crClient versioned.Interface, oper
104129
Operator: operator,
105130

106131
globalNamespace: globalNamespace,
107-
cache: cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{
132+
cache: cache.NewIndexer(PackageManifestKeyFunc, cache.Indexers{
108133
cache.NamespaceIndex: cache.MetaNamespaceIndexFunc,
109134
catalogIndex: catalogIndexFunc,
110135
}),
@@ -311,14 +336,13 @@ func (p *RegistryProvider) gcPackages(key resolver.CatalogKey, keep map[string]s
311336

312337
var errs []error
313338
for _, storedPkgKey := range storedPkgKeys {
314-
_, name, _ := cache.SplitMetaNamespaceKey(storedPkgKey)
339+
_, _, name, _ := SplitPackageManifestKey(storedPkgKey)
315340
if keep != nil {
316341
if _, ok := keep[name]; ok {
317342
continue
318343
}
319344
}
320-
321-
if err := p.cache.Delete(cache.ExplicitKey(storedPkgKey)); err != nil {
345+
if err := p.cache.Delete(string(storedPkgKey)); err != nil {
322346
logger.WithField("pkg", name).WithError(err).Warn("failed to delete cache entry")
323347
errs = append(errs, err)
324348
}

‎pkg/package-server/provider/registry_test.go

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -812,6 +812,154 @@ func TestRegistryProviderList(t *testing.T) {
812812
},
813813
}},
814814
},
815+
{
816+
name: "TwoCatalogs/SameNamespace/DuplicatePackages/PackagesFound",
817+
globalNS: "ns",
818+
registryClients: []*registryClient{
819+
newTestRegistryClient(t, withRegistryServiceStatus(catalogSource("cool-operators", "ns"), "grpc", "cool-operators", "ns", port, metav1.NewTime(time.Now()))),
820+
newTestRegistryClient(t, withRegistryServiceStatus(catalogSource("cool-operators-2", "ns"), "grpc", "cool-operators-2", "ns", port, metav1.NewTime(time.Now()))),
821+
},
822+
requestNamespace: "ns",
823+
expectedErr: "",
824+
expected: &operators.PackageManifestList{Items: []operators.PackageManifest{
825+
{
826+
ObjectMeta: metav1.ObjectMeta{
827+
Name: "prometheus",
828+
Namespace: "ns",
829+
Labels: labels.Set{
830+
"catalog": "cool-operators",
831+
"catalog-namespace": "ns",
832+
"provider": "Red Hat",
833+
"provider-url": "",
834+
"operatorframework.io/arch.amd64": "supported",
835+
"operatorframework.io/os.linux": "supported",
836+
},
837+
},
838+
Status: operators.PackageManifestStatus{
839+
CatalogSource: "cool-operators",
840+
CatalogSourceNamespace: "ns",
841+
PackageName: "prometheus",
842+
Provider: operators.AppLink{
843+
Name: "Red Hat",
844+
},
845+
DefaultChannel: "preview",
846+
Channels: []operators.PackageChannel{
847+
{
848+
Name: "preview",
849+
CurrentCSV: "prometheusoperator.0.22.2",
850+
CurrentCSVDesc: func() operators.CSVDescription {
851+
csv := operatorsv1alpha1.ClusterServiceVersion{}
852+
require.NoError(t, json.Unmarshal([]byte(prometheusCSVJSON), &csv))
853+
return operators.CreateCSVDescription(&csv)
854+
}(),
855+
},
856+
},
857+
},
858+
},
859+
{
860+
ObjectMeta: metav1.ObjectMeta{
861+
Name: "etcd",
862+
Namespace: "ns",
863+
Labels: labels.Set{
864+
"catalog": "cool-operators",
865+
"catalog-namespace": "ns",
866+
"provider": "CoreOS, Inc",
867+
"provider-url": "",
868+
"operatorframework.io/arch.amd64": "supported",
869+
"operatorframework.io/os.linux": "supported",
870+
},
871+
},
872+
Status: operators.PackageManifestStatus{
873+
CatalogSource: "cool-operators",
874+
CatalogSourceNamespace: "ns",
875+
PackageName: "etcd",
876+
Provider: operators.AppLink{
877+
Name: "CoreOS, Inc",
878+
},
879+
DefaultChannel: "alpha",
880+
Channels: []operators.PackageChannel{
881+
{
882+
Name: "alpha",
883+
CurrentCSV: "etcdoperator.v0.9.2",
884+
CurrentCSVDesc: func() operators.CSVDescription {
885+
csv := operatorsv1alpha1.ClusterServiceVersion{}
886+
require.NoError(t, json.Unmarshal([]byte(etcdCSVJSON), &csv))
887+
return operators.CreateCSVDescription(&csv)
888+
}(),
889+
},
890+
},
891+
},
892+
},
893+
{
894+
ObjectMeta: metav1.ObjectMeta{
895+
Name: "prometheus",
896+
Namespace: "ns",
897+
Labels: labels.Set{
898+
"catalog": "cool-operators-2",
899+
"catalog-namespace": "ns",
900+
"provider": "Red Hat",
901+
"provider-url": "",
902+
"operatorframework.io/arch.amd64": "supported",
903+
"operatorframework.io/os.linux": "supported",
904+
},
905+
},
906+
Status: operators.PackageManifestStatus{
907+
CatalogSource: "cool-operators-2",
908+
CatalogSourceNamespace: "ns",
909+
PackageName: "prometheus",
910+
Provider: operators.AppLink{
911+
Name: "Red Hat",
912+
},
913+
DefaultChannel: "preview",
914+
Channels: []operators.PackageChannel{
915+
{
916+
Name: "preview",
917+
CurrentCSV: "prometheusoperator.0.22.2",
918+
CurrentCSVDesc: func() operators.CSVDescription {
919+
csv := operatorsv1alpha1.ClusterServiceVersion{}
920+
require.NoError(t, json.Unmarshal([]byte(prometheusCSVJSON), &csv))
921+
return operators.CreateCSVDescription(&csv)
922+
}(),
923+
},
924+
},
925+
},
926+
},
927+
{
928+
ObjectMeta: metav1.ObjectMeta{
929+
Name: "etcd",
930+
Namespace: "ns",
931+
Labels: labels.Set{
932+
"catalog": "cool-operators-2",
933+
"catalog-namespace": "ns",
934+
"provider": "CoreOS, Inc",
935+
"provider-url": "",
936+
"operatorframework.io/arch.amd64": "supported",
937+
"operatorframework.io/os.linux": "supported",
938+
},
939+
},
940+
Status: operators.PackageManifestStatus{
941+
CatalogSource: "cool-operators-2",
942+
CatalogSourceNamespace: "ns",
943+
PackageName: "etcd",
944+
Provider: operators.AppLink{
945+
Name: "CoreOS, Inc",
946+
},
947+
DefaultChannel: "alpha",
948+
Channels: []operators.PackageChannel{
949+
{
950+
Name: "alpha",
951+
CurrentCSV: "etcdoperator.v0.9.2",
952+
CurrentCSVDesc: func() operators.CSVDescription {
953+
csv := operatorsv1alpha1.ClusterServiceVersion{}
954+
require.NoError(t, json.Unmarshal([]byte(etcdCSVJSON), &csv))
955+
return operators.CreateCSVDescription(&csv)
956+
}(),
957+
},
958+
},
959+
},
960+
},
961+
}},
962+
},
815963
{
816964
name: "OneCatalog/ManyPackages/OneMissingBundle/Elided",
817965
globalNS: "ns",

0 commit comments

Comments
 (0)