Skip to content
This repository has been archived by the owner on Feb 11, 2022. It is now read-only.

Fix missed error check in API call #54

Merged
merged 4 commits into from
Nov 13, 2020
Merged

Fix missed error check in API call #54

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
79cb4a1
Fix missed error check in API call
vasilevp Nov 6, 2020
05ce92d
Remove unused Context
vasilevp Nov 9, 2020
98a291b
Fix context, error wrapping & reformat code
vasilevp Nov 11, 2020
05de62c
Merge branch 'master' into fix-statestorage
leo-ri Nov 13, 2020
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
4 changes: 2 additions & 2 deletions pkg/broker/broker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,13 +223,13 @@ func (b *Broker) getClient(ctx context.Context, instanceID string, planID string
return
}

func (b *Broker) getState(orgID string) (*statestorage.RealmStateStorage, error) {
func (b *Broker) getState(ctx context.Context, orgID string) (*statestorage.RealmStateStorage, error) {
key, err := b.credentials.ByOrg(orgID)
if err != nil {
return nil, err
}

return statestorage.Get(key, b.cfg.AtlasURL, b.cfg.RealmURL, b.logger)
return statestorage.Get(ctx, key, b.cfg.AtlasURL, b.cfg.RealmURL, b.logger)
}

func (b *Broker) AuthMiddleware() mux.MiddlewareFunc {
Expand Down
10 changes: 5 additions & 5 deletions pkg/broker/instance_operations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,12 @@ func (b Broker) Provision(ctx context.Context, instanceID string, details domain
Parameters: planEnc,
}

state, err := b.getState(dp.Project.OrgID)
state, err := b.getState(ctx, dp.Project.OrgID)
if err != nil {
return
}

v, err := state.Put(context.Background(), instanceID, &s)
v, err := state.Put(ctx, instanceID, &s)
if err != nil {
logger.Errorw("Error during provision, broker maintenance:", "err", err)
return
Expand Down Expand Up @@ -275,7 +275,7 @@ func (b Broker) Update(ctx context.Context, instanceID string, details domain.Up
Parameters: planEnc,
}

state, err := b.getState(oldPlan.Project.OrgID)
state, err := b.getState(ctx, oldPlan.Project.OrgID)
if err != nil {
return
}
Expand Down Expand Up @@ -359,7 +359,7 @@ func (b Broker) getInstance(ctx context.Context, instanceID string) (spec domain
for k, v := range b.credentials.Keys() {
logger = logger.With("orgID", k)

state, err := statestorage.Get(v, b.cfg.AtlasURL, b.cfg.RealmURL, b.logger)
state, err := statestorage.Get(ctx, v, b.cfg.AtlasURL, b.cfg.RealmURL, b.logger)
if err != nil {
logger.Errorw("Cannot get state storage for org", "error", err)
continue
Expand Down Expand Up @@ -457,7 +457,7 @@ func (b Broker) LastOperation(ctx context.Context, instanceID string, details do
err = nil
}

state, errDel := b.getState(p.Project.OrgID)
state, errDel := b.getState(ctx, p.Project.OrgID)
if errDel != nil {
logger.Errorw("Failed to get state storage", "error", errDel)
break
Expand Down
36 changes: 21 additions & 15 deletions pkg/broker/statestorage/statestorage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,12 @@ func client(baseURL string, k credentials.APIKey) (*mongodbatlas.Client, error)
return mongodbatlas.New(hc, mongodbatlas.SetBaseURL(baseURL))
}

func Get(key credentials.APIKey, atlasURL string, realmURL string, logger *zap.SugaredLogger) (*RealmStateStorage, error) {
func Get(ctx context.Context, key credentials.APIKey, atlasURL string, realmURL string, logger *zap.SugaredLogger) (*RealmStateStorage, error) {
realmClient, err := mongodbrealm.New(
context.TODO(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[q] I see that context parameter is not used in New function - is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - probably left over from an older version

ctx,
nil,
mongodbrealm.SetBaseURL(realmURL),
mongodbrealm.SetAPIAuth(context.TODO(), key.PublicKey, key.PrivateKey),
mongodbrealm.SetAPIAuth(ctx, key.PublicKey, key.PrivateKey),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct me if I'm wrong but the context parameter which is passed everywhere is not used? mongodbrealm.NewRequest doesn't use the parameter passed. Is it planned for future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding context everywhere (except mongodbrealm.New above) right now - not sure if I should do it in this PR or a separate one, as it's a big change and not necessarily related to this error

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! 👍

)
if err != nil {
return nil, err
Expand All @@ -75,16 +75,16 @@ func Get(key credentials.APIKey, atlasURL string, realmURL string, logger *zap.S
return nil, errors.Wrap(err, "cannot create Atlas client")
}

mainPrj, err := getOrCreateBrokerMaintentaceGroup(key.OrgID, client, logger)
mainPrj, err := getOrCreateBrokerMaintentaceGroup(ctx, key.OrgID, client, logger)
if err != nil {
return nil, err
}

logger.Infow("Found mainteneance project", "mainPrj", mainPrj)
realmApp, err := getOrCreateRealmAppForOrg(mainPrj.ID, realmClient, logger)
realmApp, err := getOrCreateRealmAppForOrg(ctx, mainPrj.ID, realmClient, logger)
if err != nil {
logger.Errorw("Error getOrCreateRealmAppForOrg", "err", err)
return nil, err
return nil, errors.Wrap(err, "cannot get/create Realm app")
}

rss := &RealmStateStorage{
Expand All @@ -97,16 +97,16 @@ func Get(key credentials.APIKey, atlasURL string, realmURL string, logger *zap.S
return rss, nil
}

func getOrCreateBrokerMaintentaceGroup(orgID string, client *mongodbatlas.Client, logger *zap.SugaredLogger) (*mongodbatlas.Project, error) {
project, _, err := client.Projects.GetOneProjectByName(context.Background(), maintenanceProjectName)
func getOrCreateBrokerMaintentaceGroup(ctx context.Context, orgID string, client *mongodbatlas.Client, logger *zap.SugaredLogger) (*mongodbatlas.Project, error) {
project, _, err := client.Projects.GetOneProjectByName(ctx, maintenanceProjectName)
if err != nil {
logger.Infow("getOrCreateBrokerMaintentaceGroup", "err", err)
prj := mongodbatlas.Project{
Name: maintenanceProjectName,
OrgID: orgID,
}

project, _, err = client.Projects.Create(context.Background(), &prj)
project, _, err = client.Projects.Create(ctx, &prj)
if err != nil {
return nil, errors.Wrap(err, "cannot create project")
}
Expand All @@ -117,29 +117,35 @@ func getOrCreateBrokerMaintentaceGroup(orgID string, client *mongodbatlas.Client
return project, nil
}

func getOrCreateRealmAppForOrg(groupID string, realmClient *mongodbrealm.Client, logger *zap.SugaredLogger) (*mongodbrealm.RealmApp, error) {
func getOrCreateRealmAppForOrg(ctx context.Context, groupID string, realmClient *mongodbrealm.Client, logger *zap.SugaredLogger) (*mongodbrealm.RealmApp, error) {
app := mongodbrealm.RealmAppInput{
Name: realmAppName,
ClientAppID: "atlas-osb",
Location: "US-VA",
/* [US-VA, AU, US-OR, IE] */
}

apps, _, err := realmClient.RealmApps.List(context.Background(), groupID, nil)
var realmApp *mongodbrealm.RealmApp
apps, _, err := realmClient.RealmApps.List(ctx, groupID, nil)
if err != nil {
return nil, errors.Wrapf(err, "cannot list Realm apps for project %s", groupID)
}

var realmApp *mongodbrealm.RealmApp
for _, ra := range apps {
ra := ra
logger.Infow("Found realm app", "ra", ra)
if ra.Name == app.Name {
if realmApp != nil {
// for existing issue: don't start up until it's fixed - also helps to catch this in future
return nil, fmt.Errorf("multiple %q apps found in maintenance project %s - not supported", realmAppName, groupID)
}
realmApp = &ra
}
}

if realmApp == nil {
logger.Infow("Error fetching maintenance realm app", "err", err)
logger.Infow("Attempt create", "app", app)
realmApp, _, err := realmClient.RealmApps.Create(context.Background(), groupID, &app)
logger.Infow("Could not find Realm app for State Storage. Creating...", "app", app)
realmApp, _, err := realmClient.RealmApps.Create(ctx, groupID, &app)
if err != nil {
return nil, errors.Wrap(err, "cannot create Realm app")
}
Expand Down