Skip to content
This repository was archived by the owner on Jun 11, 2025. It is now read-only.

Enhance logging, error handling, and resource management #381

Merged
nxtcoder17 merged 6 commits into from
Oct 9, 2024
Merged

Enhance logging, error handling, and resource management #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1cbf089
feat: adds gateway configurations, to helm setup instructions
nxtcoder17 Oct 8, 2024
7ce913f
fix: ignores not-found while deleting RoleBinding
nxtcoder17 Oct 8, 2024
274e185
fix: deletes invitation after accept/reject.
nxtcoder17 Oct 8, 2024
e633468
fix: while deleting an environment, also deletes it's resource mapping
nxtcoder17 Oct 8, 2024
12a5968
feat(iam): fixes Access Control check for account member
nxtcoder17 Oct 9, 2024
9ee2ddf
feat: retries on failing apply to edge, and ensures gateway namespace
nxtcoder17 Oct 9, 2024
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
7 changes: 4 additions & 3 deletions apps/accounts/internal/domain/invitations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package domain

import (
"context"

"github.com/kloudlite/api/apps/accounts/internal/entities"
fc "github.com/kloudlite/api/apps/accounts/internal/entities/field-constants"
iamT "github.com/kloudlite/api/apps/iam/types"
Expand Down Expand Up @@ -180,12 +181,12 @@ func (d *domain) AcceptInvitation(ctx UserContext, accountName string, inviteTok
return false, errors.Newf("invitation already accepted or rejected, won't process further")
}

inv.Accepted = fn.New(true)
if _, err := d.invitationRepo.UpdateById(ctx, inv.Id, inv); err != nil {
if err := d.addMembership(ctx, accountName, ctx.UserId, inv.UserRole); err != nil {
return false, errors.NewE(err)
}

if err := d.addMembership(ctx, accountName, ctx.UserId, inv.UserRole); err != nil {
// INFO: invitation accepted, removing invite
if err := d.invitationRepo.DeleteById(ctx, inv.Id); err != nil {
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot Oct 8, 2024

Choose a reason for hiding this comment

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

suggestion: Consider potential inconsistency in invitation acceptance process

The change to delete the invitation after acceptance is good for database cleanliness. However, consider the order of operations and potential failure scenarios. If the system crashes after adding the membership but before deleting the invitation, it could lead to an inconsistent state. Consider using a transactional approach if possible, or implement a mechanism to handle such edge cases.

if err := d.db.Transaction(func(tx *gorm.DB) error {
	if err := d.addMembership(ctx, accountName, ctx.UserId, inv.UserRole); err != nil {
		return err
	}
	return d.invitationRepo.DeleteById(ctx, inv.Id)
}); err != nil {
	return false, errors.NewE(err)
}

return false, errors.NewE(err)
}

Expand Down
4 changes: 2 additions & 2 deletions apps/accounts/internal/domain/memberships.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ package domain

import (
"context"
"github.com/kloudlite/api/apps/accounts/internal/entities"
"strings"

"github.com/kloudlite/api/apps/accounts/internal/entities"

iamT "github.com/kloudlite/api/apps/iam/types"
"github.com/kloudlite/api/grpc-interfaces/kloudlite.io/rpc/iam"
"github.com/kloudlite/api/pkg/errors"
Expand Down Expand Up @@ -69,7 +70,6 @@ func (d *domain) UpdateAccountMembership(ctx UserContext, accountName string, me
ResourceRef: iamT.NewResourceRef(accountName, iamT.ResourceAccount, accountName),
Role: string(role),
})

if err != nil {
return false, errors.NewE(err)
}
Expand Down
42 changes: 23 additions & 19 deletions apps/accounts/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package main
import (
"context"
"flag"
"log/slog"
"os"
"time"

Expand All @@ -20,37 +19,42 @@ import (
)

func main() {
start := time.Now()

var isDev bool
flag.BoolVar(&isDev, "dev", false, "--dev")

var debug bool
flag.BoolVar(&debug, "debug", false, "--debug")

flag.Parse()

logger, err := logging.New(&logging.Options{Name: "accounts", Dev: isDev})
if err != nil {
panic(err)
if isDev {
debug = true
}

logger := logging.NewSlogLogger(logging.SlogOptions{
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot Oct 8, 2024

Choose a reason for hiding this comment

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

issue (complexity): Consider consolidating logger initialization to reduce code duplication and complexity.

The changes introduce useful new functionality, but the logger initialization process has become overly complex. We suggest unifying the logger creation to reduce duplication while maintaining the new debugging options. Here's a proposed simplification:

func main() {
    start := time.Now()
    var isDev, debug bool
    flag.BoolVar(&isDev, "dev", false, "--dev")
    flag.BoolVar(&debug, "debug", false, "--debug")
    flag.Parse()

    if isDev {
        debug = true
    }

    logger, err := logging.New(&logging.Options{
        Name: "accounts-api",
        Dev: isDev,
        ShowDebugLog: debug,
        SlogOptions: logging.SlogOptions{
            ShowCaller:         true,
            ShowDebugLogs:      debug,
            SetAsDefaultLogger: true,
        },
    })
    if err != nil {
        panic(err)
    }

    app := fx.New(
        fx.NopLogger,
        fx.Supply(logger),
        // ... rest of the providers
    )

    // ... rest of the main function
}

This approach:

  1. Unifies logger creation, eliminating the need for two separate logger initializations.
  2. Maintains the new debug flag functionality.
  3. Keeps the performance measurement feature.
  4. Simplifies the fx.Provide for the logger by directly supplying it.

These changes reduce complexity while preserving the new features, making the code more maintainable and easier to understand.

ShowCaller: true,
ShowDebugLogs: debug,
SetAsDefaultLogger: true,
})

app := fx.New(
fx.NopLogger,

fx.Provide(func() logging.Logger {
return logger
fx.Provide(func() (logging.Logger, error) {
return logging.New(&logging.Options{Name: "accounts-api", Dev: isDev, ShowDebugLog: debug})
}),

fx.Provide(func() *slog.Logger {
return logging.NewSlogLogger(logging.SlogOptions{
ShowCaller: true,
ShowDebugLogs: isDev,
SetAsDefaultLogger: true,
})
}),
fx.Supply(logger),

fx.Provide(func() (*env.Env, error) {
if e, err := env.LoadEnv(); err != nil {
e, err := env.LoadEnv()
if err != nil {
return nil, errors.NewE(err)
} else {
e.IsDev = isDev
return e, nil
}
e.IsDev = isDev
return e, nil
}),

fx.Provide(func(e *env.Env) (*rest.Config, error) {
Expand Down Expand Up @@ -79,10 +83,10 @@ func main() {
defer cancelFunc()

if err := app.Start(ctx); err != nil {
logger.Errorf(err, "error starting accounts app")
logger.Error("failed to start accounts api, got", "err", err)
os.Exit(1)
}

common.PrintReadyBanner()
common.PrintReadyBanner2(time.Since(start))
<-app.Done()
}
31 changes: 14 additions & 17 deletions apps/console/internal/domain/environment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,13 +666,17 @@ func (d *domain) OnEnvironmentDeleteMessage(ctx ConsoleContext, env entities.Env
return errors.NewE(err)
}

if err := d.environmentRepo.DeleteOne(
ctx,
repos.Filter{
fields.AccountName: ctx.AccountName,
fields.MetadataName: env.Name,
},
); err != nil {
if err := d.environmentRepo.DeleteOne(ctx, repos.Filter{
fields.AccountName: ctx.AccountName,
fields.MetadataName: env.Name,
}); err != nil {
return errors.NewE(err)
}

if err := d.resourceMappingRepo.DeleteMany(ctx, repos.Filter{
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot Oct 8, 2024

Choose a reason for hiding this comment

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

suggestion: Consider handling partial failure scenarios

The addition of resource mapping deletion is good for data integrity. However, consider wrapping this operation along with the environment deletion in a transaction, or implement a rollback mechanism to handle scenarios where one operation succeeds and the other fails.

tx, err := d.db.BeginTx(ctx, nil)
if err != nil {
    return errors.NewE(err)
}
defer tx.Rollback()

if err := d.resourceMappingRepo.DeleteMany(ctx, repos.Filter{
    fc.ResourceMappingResourceHeirarchy: entities.ResourceHeirarchyEnvironment,
    fc.EnvironmentName:                  env.Name,
}, tx); err != nil {
    return errors.NewE(err)
}

fc.ResourceMappingResourceHeirarchy: entities.ResourceHeirarchyEnvironment,
fc.EnvironmentName: env.Name,
}); err != nil {
return errors.NewE(err)
}

Expand Down Expand Up @@ -701,16 +705,9 @@ func (d *domain) OnEnvironmentUpdateMessage(ctx ConsoleContext, env entities.Env
return d.resyncK8sResource(ctx, xenv.Name, xenv.SyncStatus.Action, &xenv.Environment, xenv.RecordVersion)
}

uenv, err := d.environmentRepo.PatchById(
ctx,
xenv.Id,
common.PatchForSyncFromAgent(
&env,
recordVersion,
status,
common.PatchOpts{
MessageTimestamp: opts.MessageTimestamp,
}))
uenv, err := d.environmentRepo.PatchById(ctx, xenv.Id, common.PatchForSyncFromAgent(
&env, recordVersion, status, common.PatchOpts{MessageTimestamp: opts.MessageTimestamp}),
)
if err != nil {
return err
}
Expand Down
153 changes: 108 additions & 45 deletions apps/iam/internal/app/grpc-server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
t "github.com/kloudlite/api/apps/iam/types"
"github.com/kloudlite/api/grpc-interfaces/kloudlite.io/rpc/iam"
"github.com/kloudlite/api/pkg/errors"
fn "github.com/kloudlite/api/pkg/functions"
"github.com/kloudlite/api/pkg/logging"
"github.com/kloudlite/api/pkg/repos"
)
Expand Down Expand Up @@ -52,6 +51,8 @@ func (s *GrpcService) UpdateMembership(ctx context.Context, in *iam.UpdateMember
}, nil
}

var ErrRoleBindingNotFound error = fmt.Errorf("role binding not found")

func (s *GrpcService) findRoleBinding(ctx context.Context, userId repos.ID, resourceRef string) (*entities.RoleBinding, error) {
rb, err := s.rbRepo.FindOne(
ctx, repos.Filter{
Expand All @@ -63,7 +64,7 @@ func (s *GrpcService) findRoleBinding(ctx context.Context, userId repos.ID, reso
return nil, errors.NewE(err)
}
if rb == nil {
return nil, errors.Newf("role binding for (userId=%s, ResourceRef=%s) not found", userId, resourceRef)
return nil, ErrRoleBindingNotFound
}
return rb, nil
}
Expand Down Expand Up @@ -121,45 +122,99 @@ func (s *GrpcService) Can(ctx context.Context, in *iam.CanIn) (*iam.CanOut, erro
return &iam.CanOut{Status: false}, nil
}

var hasAccountMemberRole bool

canFilter := repos.Filter{
"resource_ref": map[string]any{"$in": in.ResourceRefs},
"user_id": in.UserId,
arb := make([]any, len(rb))
for i := range rb {
arb = append(arb, rb[i])
}

for i := range rb {
if rb[i] == t.RoleAccountMember {
hasAccountMemberRole = true

rr := make([]map[string]any, 0, len(in.ResourceRefs))

for i := range in.ResourceRefs {
accountName, _, _, err := t.ParseResourceRef(in.ResourceRefs[i])
if err != nil {
return nil, err
}

if strings.TrimSpace(accountName) == "" {
return nil, fmt.Errorf("accountName must be provided")
}

nf := s.rbRepo.MergeMatchFilters(repos.Filter{}, map[string]repos.MatchFilter{
"resource_ref": {
MatchType: repos.MatchTypeRegex,
Regex: fn.New(t.NewResourceRef(accountName, "*", "*")),
},
})
rr = append(rr, map[string]any{"resource_ref": nf["resource_ref"]})
}

delete(canFilter, "resource_ref")
canFilter["$or"] = rr
accountName := ""
for i := range in.ResourceRefs {
acc, _, _, err := t.ParseResourceRef(in.ResourceRefs[i])
if err != nil {
return nil, err
}
accountName = acc
}

rbs, err := s.rbRepo.Find(
ctx, repos.Query{Filter: canFilter},
// var hasAccountMemberRole bool

// resourceFilter := repos.Filter{
// "resource_ref": map[string]any{"$in": in.ResourceRefs},
// "user_id": in.UserId,
// }

// resourceFilter = s.rbRepo.MergeMatchFilters(resourceFilter, map[string]repos.MatchFilter{
// "role": {
// MatchType: repos.MatchTypeArray,
// Array: arb,
// },
// })

accountLevelFilter := s.rbRepo.MergeMatchFilters(repos.Filter{}, map[string]repos.MatchFilter{
"user_id": {
MatchType: repos.MatchTypeExact,
Exact: in.UserId,
},
"resource_ref": {
MatchType: repos.MatchTypeExact,
Exact: fmt.Sprintf("%s/account/%s", accountName, accountName),
},
"role": {
MatchType: repos.MatchTypeArray,
Array: []any{t.RoleAccountOwner, t.RoleAccountAdmin, t.RoleAccountMember},
},
})

// for i := range rb {
// if rb[i] == t.RoleAccountMember {
// hasAccountMemberRole = true
//
// rr := make([]map[string]any, 0, len(in.ResourceRefs))
//
// for i := range in.ResourceRefs {
// accountName, _, _, err := t.ParseResourceRef(in.ResourceRefs[i])
// if err != nil {
// return nil, err
// }
//
// if strings.TrimSpace(accountName) == "" {
// return nil, fmt.Errorf("accountName must be provided")
// }
//
// nf := s.rbRepo.MergeMatchFilters(repos.Filter{}, map[string]repos.MatchFilter{
// "resource_ref": {
// MatchType: repos.MatchTypeRegex,
// // FIXME: HERE
// Regex: fn.New(t.NewResourceRef(accountName, "*", "*")),
// },
// })
// rr = append(rr, map[string]any{
// "resource_ref": nf["resource_ref"],
// })
// }
//
// // FIXME: error HERE
// delete(canFilter, "resource_ref")
// canFilter["$or"] = rr
// }
// }

// accountMemberFilter := repos.Filter{
// "resource_ref":
// }
//
// resourceFilter = s.rbRepo.MergeMatchFilters(resourceFilter, map[string]repos.MatchFilter{
// "role": {
// MatchType: repos.MatchTypeArray,
// Array: []any{t.RoleAccountOwner, t.RoleAccountAdmin, t.RoleAccountMember},
// },
// })

rbs, err := s.rbRepo.Find(ctx, repos.Query{Filter: repos.Filter{
"$and": []map[string]any{
accountLevelFilter,
},
}},
)
if err != nil {
return nil, errors.NewEf(err, "could not find rolebindings for (resourceRefs=%s)", strings.Join(in.ResourceRefs, ","))
Expand All @@ -169,18 +224,22 @@ func (s *GrpcService) Can(ctx context.Context, in *iam.CanIn) (*iam.CanOut, erro
return nil, errors.Newf("no rolebinding found for (userId=%s, resourceRefs=%s)", in.UserId, strings.Join(in.ResourceRefs, ","))
}

if hasAccountMemberRole && len(rbs) > 0 {
if len(rbs) > 0 {
return &iam.CanOut{Status: true}, nil
}

for i := range rbs {
// 2nd loop, but very small length (always < #roles), so it's not exactly O(n^2), much like XO(n)
for _, role := range s.roleBindingMap[t.Action(in.Action)] {
if role == rbs[i].Role {
return &iam.CanOut{Status: true}, nil
}
}
}
// if hasAccountMemberRole && len(rbs) > 0 {
// return &iam.CanOut{Status: true}, nil
// }
//
// for i := range rbs {
// // 2nd loop, but very small length (always < #roles), so it's not exactly O(n^2), much like XO(n)
// for _, role := range s.roleBindingMap[t.Action(in.Action)] {
// if role == rbs[i].Role {
// return &iam.CanOut{Status: true}, nil
// }
// }
// }

return &iam.CanOut{Status: false}, nil
}
Expand Down Expand Up @@ -236,6 +295,10 @@ func (s *GrpcService) RemoveMembership(ctx context.Context, in *iam.RemoveMember

rb, err := s.findRoleBinding(ctx, repos.ID(in.UserId), in.ResourceRef)
if err != nil {
if errors.Is(err, ErrRoleBindingNotFound) {
s.logger.WithKV("userID", in.UserId, "resourceRef", in.ResourceRef).Infof("role binding might already have been deleted")
return &iam.RemoveMembershipOut{Result: true}, nil
}
return nil, errors.NewE(err)
}

Expand Down
3 changes: 2 additions & 1 deletion apps/iam/internal/app/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@ package app

import (
"encoding/json"
"os"

"github.com/kloudlite/api/apps/iam/internal/entities"
"github.com/kloudlite/api/pkg/errors"
"github.com/kloudlite/api/pkg/logging"
"os"

"github.com/kloudlite/api/apps/iam/internal/env"
"github.com/kloudlite/api/grpc-interfaces/kloudlite.io/rpc/iam"
Expand Down
2 changes: 2 additions & 0 deletions apps/iam/internal/env/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ type Env struct {
MongoDbName string `env:"MONGO_DB_NAME" required:"true"`

ActionRoleMapFile string `env:"ACTION_ROLE_MAP_FILE" required:"false"`

ShowGRPCLogs bool `env:"SHOW_GRPC_LOGS" default:"false"`
}

func LoadEnv() (*Env, error) {
Expand Down
Loading