From 3a539254c23ec31892064b9fc71893cf995cc5dd Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Thu, 5 Mar 2020 14:06:50 +1100 Subject: [PATCH] Ensure charts imported per namespace when using MongoDB. (#1551) * Ensure charts imported per namespace when using MongoDB. * Include removal of old charts * Update other calls in asset-syncer and start assetsvc. * Ensure IRL works. Fix mock tests. * Add index to files collection (takes import of stable from 14m -> 2m) --- chart/kubeapps/templates/apprepositories.yaml | 3 +- .../apprepository-jobs-preupgrade.yaml | 7 +- cmd/asset-syncer/mongodb_db_test.go | 120 +++++++++++++++++- cmd/asset-syncer/mongodb_utils.go | 57 +++++---- cmd/asset-syncer/mongodb_utils_test.go | 26 ++-- cmd/asset-syncer/utils_test.go | 24 ++-- cmd/assetsvc/mongodb_utils.go | 6 +- go.mod | 2 +- go.sum | 8 ++ pkg/chart/models/chart.go | 4 +- pkg/dbutils/dbutilstest/mgtest/mgtest.go | 4 +- pkg/dbutils/mongodb_utils.go | 34 ++++- 12 files changed, 228 insertions(+), 67 deletions(-) diff --git a/chart/kubeapps/templates/apprepositories.yaml b/chart/kubeapps/templates/apprepositories.yaml index 3f70f86278b..9a732a06c8a 100644 --- a/chart/kubeapps/templates/apprepositories.yaml +++ b/chart/kubeapps/templates/apprepositories.yaml @@ -4,7 +4,8 @@ kind: AppRepository metadata: name: {{ .name }} annotations: - "helm.sh/hook": pre-install + "helm.sh/hook": post-install + "helm.sh/hook-weight": "10" labels: app: {{ template "kubeapps.apprepository.fullname" $ }} chart: {{ template "kubeapps.chart" $ }} diff --git a/chart/kubeapps/templates/apprepository-jobs-preupgrade.yaml b/chart/kubeapps/templates/apprepository-jobs-preupgrade.yaml index bca978c3029..324e5dd9adc 100644 --- a/chart/kubeapps/templates/apprepository-jobs-preupgrade.yaml +++ b/chart/kubeapps/templates/apprepository-jobs-preupgrade.yaml @@ -1,11 +1,11 @@ -{{- if .Values.featureFlags.reposPerNamespace -}} -# Invalidate the chart cache during upgrade. +# Ensure db indexes are set and invalidate the chart cache during both install and upgrade. apiVersion: batch/v1 kind: Job metadata: name: {{ template "kubeapps.apprepository-jobs-preupgrade.fullname" . }} annotations: - helm.sh/hook: pre-upgrade + helm.sh/hook: pre-upgrade,post-install + helm.sh/hook-weight: "0" helm.sh/hook-delete-policy: hook-succeeded labels: app: {{ template "kubeapps.apprepository-jobs-preupgrade.fullname" . }} @@ -66,4 +66,3 @@ spec: key: postgresql-password name: {{ .Values.postgresql.existingSecret }} {{- end }} -{{- end -}} diff --git a/cmd/asset-syncer/mongodb_db_test.go b/cmd/asset-syncer/mongodb_db_test.go index ad980487e2c..c0cfc416803 100644 --- a/cmd/asset-syncer/mongodb_db_test.go +++ b/cmd/asset-syncer/mongodb_db_test.go @@ -22,9 +22,13 @@ limitations under the License. package main import ( + "errors" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/kubeapps/kubeapps/pkg/chart/models" + "github.com/kubeapps/kubeapps/pkg/dbutils" "github.com/kubeapps/kubeapps/pkg/dbutils/dbutilstest/mgtest" ) @@ -40,10 +44,17 @@ func TestMongoImportCharts(t *testing.T) { Name: "repo-name", Namespace: "repo-namespace", } + repoSameNameOtherNamespace := models.Repo{ + Name: "repo-name", + Namespace: "other-namespace", + } testCases := []struct { - name string - charts []models.Chart + name string + existingCharts []models.Chart + charts []models.Chart + expectedCharts []models.Chart + expectedError error }{ { name: "it inserts the charts", @@ -51,6 +62,77 @@ func TestMongoImportCharts(t *testing.T) { models.Chart{Name: "my-chart1", Repo: &repo, ID: "foo/bar:123"}, models.Chart{Name: "my-chart2", Repo: &repo, ID: "foo/bar:456"}, }, + expectedCharts: []models.Chart{ + models.Chart{Name: "my-chart1", Repo: &repo, ID: "foo/bar:123"}, + models.Chart{Name: "my-chart2", Repo: &repo, ID: "foo/bar:456"}, + }, + }, + { + name: "it errors if asked to insert a chart in a different namespace", + charts: []models.Chart{ + models.Chart{Name: "my-chart1", Repo: &repo, ID: "foo/bar:123"}, + models.Chart{Name: "my-chart2", Repo: &repo, ID: "foo/bar:456"}, + models.Chart{Name: "my-chart1", Repo: &repoSameNameOtherNamespace, ID: "foo/bar:123"}, + }, + expectedError: ErrRepoMismatch, + }, + { + name: "it updates existing charts in the chart namespace", + existingCharts: []models.Chart{ + models.Chart{Name: "my-chart1", Repo: &repo, ID: "foo/bar:123", Description: "Old description"}, + }, + charts: []models.Chart{ + models.Chart{Name: "my-chart1", Repo: &repo, ID: "foo/bar:123", Description: "New description"}, + models.Chart{Name: "my-chart2", Repo: &repo, ID: "foo/bar:456"}, + }, + expectedCharts: []models.Chart{ + models.Chart{Name: "my-chart1", Repo: &repo, ID: "foo/bar:123", Description: "New description"}, + models.Chart{Name: "my-chart2", Repo: &repo, ID: "foo/bar:456"}, + }, + }, + { + name: "it removes charts that are not included in the import", + existingCharts: []models.Chart{ + models.Chart{Name: "my-chart-old", Repo: &repo, ID: "foo/old:123"}, + }, + charts: []models.Chart{ + models.Chart{Name: "my-chart1", Repo: &repo, ID: "foo/bar:123", Description: "New description"}, + models.Chart{Name: "my-chart2", Repo: &repo, ID: "foo/bar:456"}, + }, + expectedCharts: []models.Chart{ + models.Chart{Name: "my-chart1", Repo: &repo, ID: "foo/bar:123", Description: "New description"}, + models.Chart{Name: "my-chart2", Repo: &repo, ID: "foo/bar:456"}, + }, + }, + { + name: "it does not remove charts from other namespaces", + existingCharts: []models.Chart{ + models.Chart{Name: "my-chart-old", Repo: &repoSameNameOtherNamespace, ID: "foo/other:123"}, + }, + charts: []models.Chart{ + models.Chart{Name: "my-chart1", Repo: &repo, ID: "foo/bar:123", Description: "New description"}, + models.Chart{Name: "my-chart2", Repo: &repo, ID: "foo/bar:456"}, + }, + expectedCharts: []models.Chart{ + models.Chart{Name: "my-chart-old", Repo: &repoSameNameOtherNamespace, ID: "foo/other:123"}, + models.Chart{Name: "my-chart1", Repo: &repo, ID: "foo/bar:123", Description: "New description"}, + models.Chart{Name: "my-chart2", Repo: &repo, ID: "foo/bar:456"}, + }, + }, + { + name: "it does not remove charts from other namespaces even if they have the same repo name", + existingCharts: []models.Chart{ + models.Chart{Name: "my-chart-old", Repo: &repoSameNameOtherNamespace, ID: "foo/bar:123"}, + }, + charts: []models.Chart{ + models.Chart{Name: "my-chart1", Repo: &repo, ID: "foo/bar:123", Description: "New description"}, + models.Chart{Name: "my-chart2", Repo: &repo, ID: "foo/bar:456"}, + }, + expectedCharts: []models.Chart{ + models.Chart{Name: "my-chart-old", Repo: &repoSameNameOtherNamespace, ID: "foo/bar:123"}, + models.Chart{Name: "my-chart1", Repo: &repo, ID: "foo/bar:123", Description: "New description"}, + models.Chart{Name: "my-chart2", Repo: &repo, ID: "foo/bar:456"}, + }, }, } @@ -58,13 +140,39 @@ func TestMongoImportCharts(t *testing.T) { t.Run(tc.name, func(t *testing.T) { manager, cleanup := getInitializedMongoManager(t) defer cleanup() + if len(tc.existingCharts) > 0 { + err := manager.importCharts(tc.existingCharts, *tc.existingCharts[0].Repo) + if err != nil { + t.Fatalf("%+v", err) + } + } - err := manager.importCharts(tc.charts) - if err != nil { - t.Errorf("%+v", err) + err := manager.importCharts(tc.charts, repo) + if tc.expectedError != nil { + if got, want := err, tc.expectedError; !errors.Is(got, want) { + t.Fatalf("got: %+v, want: %+v", got, want) + } + } else if err != nil { + t.Fatalf("%+v", err) } - // TODO: Add actual assertions on remaining charts etc. + opts := cmpopts.EquateEmpty() + if got, want := getAllCharts(t, manager), tc.expectedCharts; !cmp.Equal(want, got, opts) { + t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(want, got, opts)) + } }) } } + +func getAllCharts(t *testing.T, manager *mongodbAssetManager) []models.Chart { + var result []models.Chart + db, closer := manager.DBSession.DB() + defer closer() + + coll := db.C(dbutils.ChartCollection) + err := coll.Find(nil).Sort("repo.name", "repo.namespace", "id").All(&result) + if err != nil { + t.Fatalf("%+v", err) + } + return result +} diff --git a/cmd/asset-syncer/mongodb_utils.go b/cmd/asset-syncer/mongodb_utils.go index 466c166616c..1432a8e9887 100644 --- a/cmd/asset-syncer/mongodb_utils.go +++ b/cmd/asset-syncer/mongodb_utils.go @@ -17,6 +17,7 @@ limitations under the License. package main import ( + "fmt" "time" "github.com/globalsign/mgo/bson" @@ -25,11 +26,7 @@ import ( "github.com/kubeapps/kubeapps/pkg/dbutils" ) -const ( - chartCollection = "charts" - repositoryCollection = "repos" - chartFilesCollection = "files" -) +var ErrRepoMismatch = fmt.Errorf("chart repository did not match import repository") type mongodbAssetManager struct { *dbutils.MongodbAssetManager @@ -50,69 +47,77 @@ func newMongoDBManager(config datastore.Config) assetManager { // imported into the database as fast as possible. E.g. we want all icons for // charts before fetching readmes for each chart and version pair. func (m *mongodbAssetManager) Sync(repo models.Repo, charts []models.Chart) error { - return m.importCharts(charts) + return m.importCharts(charts, repo) } func (m *mongodbAssetManager) RepoAlreadyProcessed(repo models.Repo, checksum string) bool { db, closer := m.DBSession.DB() defer closer() lastCheck := &models.RepoCheck{} - err := db.C(repositoryCollection).Find(bson.M{"_id": repo.Name}).One(lastCheck) + err := db.C(dbutils.RepositoryCollection).Find(bson.M{"name": repo.Name, "namespace": repo.Namespace}).One(lastCheck) return err == nil && checksum == lastCheck.Checksum } func (m *mongodbAssetManager) UpdateLastCheck(repoNamespace, repoName, checksum string, now time.Time) error { db, closer := m.DBSession.DB() defer closer() - _, err := db.C(repositoryCollection).UpsertId(repoName, bson.M{"$set": bson.M{"last_update": now, "checksum": checksum}}) + _, err := db.C(dbutils.RepositoryCollection).Upsert(bson.M{"name": repoName, "namespace": repoNamespace}, bson.M{"$set": bson.M{"last_update": now, "checksum": checksum}}) return err } func (m *mongodbAssetManager) Delete(repo models.Repo) error { db, closer := m.DBSession.DB() defer closer() - _, err := db.C(chartCollection).RemoveAll(bson.M{ - "repo.name": repo.Name, + _, err := db.C(dbutils.ChartCollection).RemoveAll(bson.M{ + "repo.name": repo.Name, + "repo.namespace": repo.Namespace, }) if err != nil { return err } - _, err = db.C(chartFilesCollection).RemoveAll(bson.M{ - "repo.name": repo.Name, + _, err = db.C(dbutils.ChartFilesCollection).RemoveAll(bson.M{ + "repo.name": repo.Name, + "repo.namespace": repo.Namespace, }) if err != nil { return err } - _, err = db.C(repositoryCollection).RemoveAll(bson.M{ - "_id": repo.Name, + _, err = db.C(dbutils.RepositoryCollection).RemoveAll(bson.M{ + "name": repo.Name, + "namespace": repo.Namespace, }) return err } -func (m *mongodbAssetManager) importCharts(charts []models.Chart) error { +func (m *mongodbAssetManager) importCharts(charts []models.Chart, repo models.Repo) error { var pairs []interface{} var chartIDs []string for _, c := range charts { + if c.Repo == nil || c.Repo.Namespace != repo.Namespace || c.Repo.Name != repo.Name { + return fmt.Errorf("%w: chart repo: %+v, import repo: %+v", ErrRepoMismatch, c.Repo, repo) + } chartIDs = append(chartIDs, c.ID) // charts to upsert - pair of selector, chart - pairs = append(pairs, bson.M{"_id": c.ID}, c) + // Mongodb generates the unique _id, we rely on the compound unique index on chart_id and repo. + pairs = append(pairs, bson.M{"chart_id": c.ID, "repo.name": repo.Name, "repo.namespace": repo.Namespace}, bson.M{"$set": c}) } db, closer := m.DBSession.DB() defer closer() - bulk := db.C(chartCollection).Bulk() + bulk := db.C(dbutils.ChartCollection).Bulk() // Upsert pairs of selectors, charts bulk.Upsert(pairs...) // Remove charts no longer existing in index bulk.RemoveAll(bson.M{ - "_id": bson.M{ + "chart_id": bson.M{ "$nin": chartIDs, }, - "repo.name": charts[0].Repo.Name, + "repo.name": repo.Name, + "repo.namespace": repo.Namespace, }) _, err := bulk.Run() @@ -122,25 +127,21 @@ func (m *mongodbAssetManager) importCharts(charts []models.Chart) error { func (m *mongodbAssetManager) updateIcon(repo models.Repo, data []byte, contentType, ID string) error { db, closer := m.DBSession.DB() defer closer() - return db.C(chartCollection).UpdateId(ID, bson.M{"$set": bson.M{"raw_icon": data, "icon_content_type": contentType}}) + _, err := db.C(dbutils.ChartCollection).Upsert(bson.M{"chart_id": ID, "repo.name": repo.Name, "repo.namespace": repo.Namespace}, bson.M{"$set": bson.M{"raw_icon": data, "icon_content_type": contentType}}) + return err } func (m *mongodbAssetManager) filesExist(repo models.Repo, chartFilesID, digest string) bool { db, closer := m.DBSession.DB() defer closer() - err := db.C(chartFilesCollection).Find(bson.M{"_id": chartFilesID, "digest": digest}).One(&models.ChartFiles{}) + err := db.C(dbutils.ChartFilesCollection).Find(bson.M{"file_id": chartFilesID, "repo.name": repo.Name, "repo.namespace": repo.Namespace, "digest": digest}).One(&models.ChartFiles{}) return err == nil } func (m *mongodbAssetManager) insertFiles(chartId string, files models.ChartFiles) error { db, closer := m.DBSession.DB() defer closer() - _, err := db.C(chartFilesCollection).UpsertId(files.ID, files) - return err -} -// InvalidateCache for mongodb currently is a noop to fulfil the interface. -func (m *mongodbAssetManager) InvalidateCache() error { - // TODO: implement a cache invalidation - return nil + _, err := db.C(dbutils.ChartFilesCollection).Upsert(bson.M{"file_id": files.ID, "repo.name": files.Repo.Name, "repo.namespace": files.Repo.Namespace}, files) + return err } diff --git a/cmd/asset-syncer/mongodb_utils_test.go b/cmd/asset-syncer/mongodb_utils_test.go index 8e2bd302c97..a5333969f18 100644 --- a/cmd/asset-syncer/mongodb_utils_test.go +++ b/cmd/asset-syncer/mongodb_utils_test.go @@ -42,9 +42,14 @@ func Test_importCharts(t *testing.T) { m.On("Upsert", mock.Anything) m.On("RemoveAll", mock.Anything) index, _ := parseRepoIndex([]byte(validRepoIndexYAML)) - charts := chartsFromIndex(index, &models.Repo{Name: "test", URL: "http://testrepo.com"}) + repo := models.Repo{ + Name: "repo-name", + Namespace: "repo-namespace", + URL: "http://testrepo.example.com", + } + charts := chartsFromIndex(index, &repo) manager := getMockManager(m) - manager.importCharts(charts) + manager.importCharts(charts, repo) m.AssertExpectations(t) // The Bulk Upsert method takes an array that consists of a selector followed by an interface to upsert. @@ -53,21 +58,25 @@ func Test_importCharts(t *testing.T) { args := m.Calls[0].Arguments.Get(0).([]interface{}) assert.Equal(t, len(args), len(charts)*2, "number of selector, chart pairs to upsert") for i := 0; i < len(args); i += 2 { - c := args[i+1].(models.Chart) - assert.Equal(t, args[i], bson.M{"_id": "test/" + c.Name}, "selector") + m := args[i+1].(bson.M) + c := m["$set"].(models.Chart) + assert.Equal(t, args[i], bson.M{"chart_id": "repo-name/" + c.Name, "repo.name": "repo-name", "repo.namespace": "repo-namespace"}, "selector") } } func Test_DeleteRepo(t *testing.T) { m := &mock.Mock{} + repo := models.Repo{Name: "repo-name", Namespace: "repo-namespace"} m.On("RemoveAll", bson.M{ - "repo.name": "test", + "repo.name": repo.Name, + "repo.namespace": repo.Namespace, }) m.On("RemoveAll", bson.M{ - "_id": "test", + "name": repo.Name, + "namespace": repo.Namespace, }) manager := getMockManager(m) - err := manager.Delete(models.Repo{Name: "test"}) + err := manager.Delete(repo) if err != nil { t.Errorf("failed to delete chart repo test: %v", err) } @@ -117,9 +126,10 @@ func Test_updateLastCheck(t *testing.T) { checksum = "bar" ) now := time.Now() - m.On("UpsertId", repoName, bson.M{"$set": bson.M{"last_update": now, "checksum": checksum}}).Return(nil) + m.On("Upsert", bson.M{"name": repoName, "namespace": repoNamespace}, bson.M{"$set": bson.M{"last_update": now, "checksum": checksum}}).Return(nil) manager := getMockManager(m) err := manager.UpdateLastCheck(repoNamespace, repoName, checksum, now) + m.AssertExpectations(t) if err != nil { t.Errorf("Unexpected error %v", err) } diff --git a/cmd/asset-syncer/utils_test.go b/cmd/asset-syncer/utils_test.go index 44d3b83aa28..dcf92dca679 100644 --- a/cmd/asset-syncer/utils_test.go +++ b/cmd/asset-syncer/utils_test.go @@ -494,7 +494,7 @@ func Test_newManager(t *testing.T) { } func Test_fetchAndImportIcon(t *testing.T) { - r := &models.RepoInternal{Name: "test"} + r := &models.RepoInternal{Name: "test", Namespace: "repo-namespace"} t.Run("no icon", func(t *testing.T) { m := &mock.Mock{} c := models.Chart{ID: "test/acs-engine-autoscaler"} @@ -504,7 +504,7 @@ func Test_fetchAndImportIcon(t *testing.T) { }) index, _ := parseRepoIndex([]byte(validRepoIndexYAML)) - charts := chartsFromIndex(index, &models.Repo{Name: "test", URL: "http://testrepo.com"}) + charts := chartsFromIndex(index, &models.Repo{Name: "test", Namespace: "repo-namespace", URL: "http://testrepo.com"}) t.Run("failed download", func(t *testing.T) { netClient = &badHTTPClient{} @@ -528,7 +528,7 @@ func Test_fetchAndImportIcon(t *testing.T) { netClient = &goodIconClient{} c := charts[0] m := &mock.Mock{} - m.On("UpdateId", c.ID, bson.M{"$set": bson.M{"raw_icon": iconBytes(), "icon_content_type": "image/png"}}).Return(nil) + m.On("Upsert", bson.M{"chart_id": c.ID, "repo.name": c.Repo.Name, "repo.namespace": c.Repo.Namespace}, bson.M{"$set": bson.M{"raw_icon": iconBytes(), "icon_content_type": "image/png"}}).Return(nil) manager := getMockManager(m) fImporter := fileImporter{manager} assert.NoErr(t, fImporter.fetchAndImportIcon(c, r)) @@ -540,10 +540,11 @@ func Test_fetchAndImportIcon(t *testing.T) { c := models.Chart{ ID: "foo", Icon: "https://foo/bar/logo.svg", - Repo: &models.Repo{}, + Repo: &models.Repo{Name: r.Name, Namespace: r.Namespace}, } m := &mock.Mock{} - m.On("UpdateId", c.ID, bson.M{"$set": bson.M{"raw_icon": []byte("foo"), "icon_content_type": "image/svg"}}).Return(nil) + m.On("Upsert", bson.M{"chart_id": c.ID, "repo.name": c.Repo.Name, "repo.namespace": c.Repo.Namespace}, bson.M{"$set": bson.M{"raw_icon": []byte("foo"), "icon_content_type": "image/svg"}}).Return(nil) + manager := getMockManager(m) fImporter := fileImporter{manager} assert.NoErr(t, fImporter.fetchAndImportIcon(c, r)) @@ -553,8 +554,8 @@ func Test_fetchAndImportIcon(t *testing.T) { func Test_fetchAndImportFiles(t *testing.T) { index, _ := parseRepoIndex([]byte(validRepoIndexYAML)) - repo := &models.RepoInternal{Name: "test", URL: "http://testrepo.com"} - charts := chartsFromIndex(index, &models.Repo{Name: repo.Name, URL: repo.URL}) + repo := &models.RepoInternal{Name: "test", Namespace: "repo-namespace", URL: "http://testrepo.com"} + charts := chartsFromIndex(index, &models.Repo{Name: repo.Name, Namespace: repo.Namespace, URL: repo.URL}) cv := charts[0].ChartVersions[0] t.Run("http error", func(t *testing.T) { @@ -571,7 +572,7 @@ func Test_fetchAndImportFiles(t *testing.T) { m := mock.Mock{} m.On("One", mock.Anything).Return(errors.New("return an error when checking if files already exists to force fetching")) chartFilesID := fmt.Sprintf("%s/%s-%s", charts[0].Repo.Name, charts[0].Name, cv.Version) - m.On("UpsertId", chartFilesID, models.ChartFiles{ + m.On("Upsert", bson.M{"file_id": chartFilesID, "repo.name": repo.Name, "repo.namespace": repo.Namespace}, models.ChartFiles{ ID: chartFilesID, Readme: "", Values: "", @@ -579,6 +580,7 @@ func Test_fetchAndImportFiles(t *testing.T) { Repo: charts[0].Repo, Digest: cv.Digest, }) + manager := getMockManager(&m) fImporter := fileImporter{manager} err := fImporter.fetchAndImportFiles(charts[0].Name, repo, cv) @@ -591,7 +593,7 @@ func Test_fetchAndImportFiles(t *testing.T) { m := mock.Mock{} m.On("One", mock.Anything).Return(errors.New("return an error when checking if files already exists to force fetching")) chartFilesID := fmt.Sprintf("%s/%s-%s", charts[0].Repo.Name, charts[0].Name, cv.Version) - m.On("UpsertId", chartFilesID, models.ChartFiles{ + m.On("Upsert", bson.M{"file_id": chartFilesID, "repo.name": repo.Name, "repo.namespace": repo.Namespace}, models.ChartFiles{ ID: chartFilesID, Readme: testChartReadme, Values: testChartValues, @@ -601,7 +603,7 @@ func Test_fetchAndImportFiles(t *testing.T) { }) manager := getMockManager(&m) fImporter := fileImporter{manager} - r := &models.RepoInternal{Name: repo.Name, URL: repo.URL, AuthorizationHeader: "Bearer ThisSecretAccessTokenAuthenticatesTheClient"} + r := &models.RepoInternal{Name: repo.Name, Namespace: repo.Namespace, URL: repo.URL, AuthorizationHeader: "Bearer ThisSecretAccessTokenAuthenticatesTheClient"} err := fImporter.fetchAndImportFiles(charts[0].Name, r, cv) assert.NoErr(t, err) m.AssertExpectations(t) @@ -612,7 +614,7 @@ func Test_fetchAndImportFiles(t *testing.T) { m := mock.Mock{} m.On("One", mock.Anything).Return(errors.New("return an error when checking if files already exists to force fetching")) chartFilesID := fmt.Sprintf("%s/%s-%s", charts[0].Repo.Name, charts[0].Name, cv.Version) - m.On("UpsertId", chartFilesID, models.ChartFiles{ + m.On("Upsert", bson.M{"file_id": chartFilesID, "repo.name": repo.Name, "repo.namespace": repo.Namespace}, models.ChartFiles{ ID: chartFilesID, Readme: testChartReadme, Values: testChartValues, diff --git a/cmd/assetsvc/mongodb_utils.go b/cmd/assetsvc/mongodb_utils.go index ad1d16e552b..7b060efb3cc 100644 --- a/cmd/assetsvc/mongodb_utils.go +++ b/cmd/assetsvc/mongodb_utils.go @@ -94,7 +94,7 @@ func (m *mongodbAssetManager) getChart(chartID string) (models.Chart, error) { db, closer := m.DBSession.DB() defer closer() var chart models.Chart - err := db.C(chartCollection).FindId(chartID).One(&chart) + err := db.C(chartCollection).Find(bson.M{"chart_id": chartID}).One(&chart) return chart, err } @@ -103,7 +103,7 @@ func (m *mongodbAssetManager) getChartVersion(chartID, version string) (models.C defer closer() var chart models.Chart err := db.C(chartCollection).Find(bson.M{ - "_id": chartID, + "chart_id": chartID, "chartversions": bson.M{"$elemMatch": bson.M{"version": version}}, }).Select(bson.M{ "name": 1, "repo": 1, "description": 1, "home": 1, "keywords": 1, "maintainers": 1, "sources": 1, @@ -116,7 +116,7 @@ func (m *mongodbAssetManager) getChartFiles(filesID string) (models.ChartFiles, db, closer := m.DBSession.DB() defer closer() var files models.ChartFiles - err := db.C(filesCollection).FindId(filesID).One(&files) + err := db.C(filesCollection).Find(bson.M{"file_id": filesID}).One(&files) return files, err } diff --git a/go.mod b/go.mod index 7a0451902d3..84d330fec09 100644 --- a/go.mod +++ b/go.mod @@ -18,7 +18,7 @@ require ( github.com/gorilla/mux v1.7.3 github.com/heptiolabs/healthcheck v0.0.0-20180807145615-6ff867650f40 github.com/jinzhu/copier v0.0.0-20190924061706-b57f9002281a - github.com/kubeapps/common v0.0.0-20190508164739-10b110436c1a + github.com/kubeapps/common v0.0.0-20200304064434-f6ba82e79f47 github.com/lib/pq v1.2.0 github.com/pkg/errors v0.8.1 github.com/sirupsen/logrus v1.4.2 diff --git a/go.sum b/go.sum index 429b06fd01e..c45f6ecce8d 100644 --- a/go.sum +++ b/go.sum @@ -287,6 +287,14 @@ github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kubeapps/common v0.0.0-20190508164739-10b110436c1a h1:VqeX/fehAB6FtBox0TVYcjOMXGE56INQIfbXegditX4= github.com/kubeapps/common v0.0.0-20190508164739-10b110436c1a/go.mod h1:TsgmjeDpbftqhwPKInJ3v+l+xbHs4goiB6DFb2WqY9c= +github.com/kubeapps/common v0.0.0-20200302011928-2d5128279fd5 h1:XuApD0+DYpyPrOZCpQBdyxHEBLxzpq6oNSUqK8oQ6ss= +github.com/kubeapps/common v0.0.0-20200302011928-2d5128279fd5/go.mod h1:TsgmjeDpbftqhwPKInJ3v+l+xbHs4goiB6DFb2WqY9c= +github.com/kubeapps/common v0.0.0-20200303230740-24b02ef65caf h1:JQI3IKAxj2fTPg5Zuwgx30j9VGLnFJSKAnyJZZ131y8= +github.com/kubeapps/common v0.0.0-20200303230740-24b02ef65caf/go.mod h1:TsgmjeDpbftqhwPKInJ3v+l+xbHs4goiB6DFb2WqY9c= +github.com/kubeapps/common v0.0.0-20200304054735-9a4709593163 h1:PqdyxAPCi/ZWljQ6pg4ix83pqel7nee9ZTwmp1eMQ7g= +github.com/kubeapps/common v0.0.0-20200304054735-9a4709593163/go.mod h1:TsgmjeDpbftqhwPKInJ3v+l+xbHs4goiB6DFb2WqY9c= +github.com/kubeapps/common v0.0.0-20200304064434-f6ba82e79f47 h1:Bi+dW7HktNkUkXkMB/cil7+BAbPYNW82hWJ73UV42Z4= +github.com/kubeapps/common v0.0.0-20200304064434-f6ba82e79f47/go.mod h1:TsgmjeDpbftqhwPKInJ3v+l+xbHs4goiB6DFb2WqY9c= github.com/lib/pq v1.2.0 h1:LXpIM/LZ5xGFhOpXAQUIMM1HdyqzVYM13zNdjCEEcA0= github.com/lib/pq v1.2.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo= github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de/go.mod h1:zAbeS9B/r2mtpb6U+EI2rYA5OAXxsYw6wTamcNW+zcE= diff --git a/pkg/chart/models/chart.go b/pkg/chart/models/chart.go index b94346d5d85..d0625ba8b81 100644 --- a/pkg/chart/models/chart.go +++ b/pkg/chart/models/chart.go @@ -43,7 +43,7 @@ type RepoInternal struct { // Chart is a higher-level representation of a chart package type Chart struct { - ID string `json:"ID" bson:"_id"` + ID string `json:"ID" bson:"chart_id"` Name string `json:"name"` Repo *Repo `json:"repo"` Description string `json:"description"` @@ -81,7 +81,7 @@ type ChartVersion struct { // ChartFiles holds the README and values for a given chart version type ChartFiles struct { - ID string `bson:"_id"` + ID string `bson:"file_id"` Readme string Values string Schema string diff --git a/pkg/dbutils/dbutilstest/mgtest/mgtest.go b/pkg/dbutils/dbutilstest/mgtest/mgtest.go index cff0c8312b5..9bdfe524744 100644 --- a/pkg/dbutils/dbutilstest/mgtest/mgtest.go +++ b/pkg/dbutils/dbutilstest/mgtest/mgtest.go @@ -35,7 +35,7 @@ func SkipIfNoDB(t *testing.T) { } } -func openTestManager(t *testing.T) *dbutils.MongodbAssetManager { +func OpenTestManager(t *testing.T) *dbutils.MongodbAssetManager { manager := dbutils.NewMongoDBManager(datastore.Config{ URL: "localhost:27017", Username: "root", @@ -51,7 +51,7 @@ func openTestManager(t *testing.T) *dbutils.MongodbAssetManager { // GetInitializedManager returns an initialized mongodb manager ready for testing. func GetInitializedManager(t *testing.T) (*dbutils.MongodbAssetManager, func()) { - manager := openTestManager(t) + manager := OpenTestManager(t) cleanup := func() { manager.Close() } err := manager.InvalidateCache() diff --git a/pkg/dbutils/mongodb_utils.go b/pkg/dbutils/mongodb_utils.go index 6a7c9b88484..fe460ef3a9e 100644 --- a/pkg/dbutils/mongodb_utils.go +++ b/pkg/dbutils/mongodb_utils.go @@ -19,9 +19,16 @@ package dbutils import ( "fmt" + "github.com/globalsign/mgo" "github.com/kubeapps/common/datastore" ) +const ( + ChartCollection = "charts" + RepositoryCollection = "repos" + ChartFilesCollection = "files" +) + // MongodbAssetManager struct containing mongodb info type MongodbAssetManager struct { mongoConfig datastore.Config @@ -49,5 +56,30 @@ func (m *MongodbAssetManager) Close() error { } func (m *MongodbAssetManager) InvalidateCache() error { - return nil + db, closer := m.DBSession.DB() + defer closer() + + err := db.C(ChartCollection).DropCollection() + // We ignore "ns not found" which relates to an operation on a non-existent collection. + if err != nil && err.Error() != "ns not found" { + return err + } + + err = db.C(ChartCollection).EnsureIndex(mgo.Index{ + Key: []string{"chart_id", "repo.namespace", "repo.name"}, + Unique: true, + DropDups: true, + Background: false, + }) + if err != nil { + return err + } + err = db.C(ChartFilesCollection).EnsureIndex(mgo.Index{ + Key: []string{"file_id", "repo.namespace", "repo.name"}, + Background: false, + }) + if err != nil { + return err + } + return m.DBSession.Fsync(false) }