Skip to content

Commit

Permalink
Review fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
savnadya committed Mar 7, 2024
1 parent 9261dc1 commit 2690bae
Show file tree
Hide file tree
Showing 14 changed files with 99 additions and 201 deletions.
5 changes: 4 additions & 1 deletion app.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,13 @@ import (
"k8s.io/utils/clock"
)

type ObjectID = string

type Source interface {
GetUsers() ([]SourceUser, error)
GetGroupsWithMembers() ([]SourceGroupWithMembers, error)
GetSourceType() SourceType
CreateUserFromRaw(raw map[string]any) (SourceUser, error)
CreateGroupFromRaw(raw map[string]any) (SourceGroup, error)
}

type App struct {
Expand Down
44 changes: 15 additions & 29 deletions app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import (
"testing"
"time"

"go.ytsaurus.tech/library/go/ptr"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -100,8 +98,7 @@ var (
}

aliceYtsaurus = YtsaurusUser{
Username: "alice",
SourceType: ptr.String("azure"),
Username: "alice",
SourceRaw: map[string]any{
"id": aliceAzure.AzureID,
"principal_name": aliceAzure.PrincipalName,
Expand All @@ -112,8 +109,7 @@ var (
},
}
bobYtsaurus = YtsaurusUser{
Username: "bob",
SourceType: ptr.String("azure"),
Username: "bob",
SourceRaw: map[string]any{
"id": bobAzure.AzureID,
"principal_name": bobAzure.PrincipalName,
Expand All @@ -124,8 +120,7 @@ var (
},
}
carolYtsaurus = YtsaurusUser{
Username: "carol",
SourceType: ptr.String("azure"),
Username: "carol",
SourceRaw: map[string]any{
"id": carolAzure.AzureID,
"principal_name": carolAzure.PrincipalName,
Expand All @@ -136,8 +131,7 @@ var (
},
}
aliceYtsaurusChangedLastName = YtsaurusUser{
Username: aliceYtsaurus.Username,
SourceType: ptr.String("azure"),
Username: aliceYtsaurus.Username,
SourceRaw: map[string]any{
"id": aliceYtsaurus.SourceRaw["id"],
"principal_name": aliceYtsaurus.SourceRaw["principal_name"],
Expand All @@ -148,8 +142,7 @@ var (
},
}
bobYtsaurusChangedEmail = YtsaurusUser{
Username: "bobby:example.com",
SourceType: ptr.String("azure"),
Username: "bobby:example.com",
SourceRaw: map[string]any{
"id": bobYtsaurus.SourceRaw["id"],
"principal_name": bobAzureChangedEmail.PrincipalName,
Expand All @@ -160,8 +153,7 @@ var (
},
}
bobYtsaurusBanned = YtsaurusUser{
Username: bobYtsaurus.Username,
SourceType: ptr.String("azure"),
Username: bobYtsaurus.Username,
SourceRaw: map[string]any{
"id": bobYtsaurus.SourceRaw["id"],
"principal_name": bobYtsaurus.SourceRaw["principal_name"],
Expand All @@ -173,8 +165,7 @@ var (
BannedSince: initialTestTime,
}
carolYtsaurusBanned = YtsaurusUser{
Username: carolYtsaurus.Username,
SourceType: ptr.String("azure"),
Username: carolYtsaurus.Username,
SourceRaw: map[string]any{
"id": carolYtsaurus.SourceRaw["id"],
"principal_name": carolYtsaurus.SourceRaw["principal_name"],
Expand All @@ -186,44 +177,39 @@ var (
BannedSince: initialTestTime.Add(40 * time.Hour),
}
devsYtsaurusGroup = YtsaurusGroup{
Name: "acme.devs",
SourceType: ptr.String("azure"),
Name: "acme.devs",
SourceRaw: map[string]any{
"id": devsAzureGroup.AzureID,
"display_name": "acme.devs|all",
"identity": "acme.devs|all",
},
}
qaYtsaurusGroup = YtsaurusGroup{
Name: "acme.qa",
SourceType: ptr.String("azure"),
Name: "acme.qa",
SourceRaw: map[string]any{
"id": "fake-az-acme.qa",
"display_name": "acme.qa|all",
"identity": "acme.qa",
},
}
hqYtsaurusGroup = YtsaurusGroup{
Name: "acme.hq",
SourceType: ptr.String("azure"),
Name: "acme.hq",
SourceRaw: map[string]any{
"id": hqAzureGroup.AzureID,
"display_name": "acme.hq",
"identity": "acme.hq",
},
}
devsYtsaurusGroupChangedDisplayName = YtsaurusGroup{
Name: "acme.developers",
SourceType: ptr.String("azure"),
Name: "acme.developers",
SourceRaw: map[string]any{
"id": devsAzureGroup.AzureID,
"display_name": "acme.developers|all",
"identity": "acme.developers|all",
},
}
hqYtsaurusGroupChangedBackwardCompatible = YtsaurusGroup{
Name: "acme.hq",
SourceType: ptr.String("azure"),
Name: "acme.hq",
SourceRaw: map[string]any{
"id": hqAzureGroup.AzureID,
"display_name": "acme.hq|all",
Expand Down Expand Up @@ -497,9 +483,9 @@ var (
),
},
{
// for this group only displayName should be updated
// For this group only displayName should be updated.
SourceGroup: hqAzureGroupChangedBackwardCompatible,
// members also changed
// Members also changed.
Members: NewStringSetFromItems(
aliceAzure.AzureID,
carolAzure.AzureID,
Expand Down Expand Up @@ -583,7 +569,7 @@ func TestAppSync(t *testing.T) {
ApplyGroupChanges: true,
ApplyMemberChanges: true,
LogLevel: "DEBUG",
SourceAttributeName: ptr.String("azure"),
SourceAttributeName: "azure",
},
}, getDevelopmentLogger(),
azure,
Expand Down
8 changes: 6 additions & 2 deletions azure_fake.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@ func (a *AzureFake) setGroups(groups []SourceGroupWithMembers) {
a.groups = groups
}

func (a *AzureFake) GetSourceType() SourceType {
return AzureSourceType
func (a *AzureFake) CreateUserFromRaw(raw map[string]any) (SourceUser, error) {
return NewAzureUser(raw)
}

func (a *AzureFake) CreateGroupFromRaw(raw map[string]any) (SourceGroup, error) {
return NewAzureGroup(raw)
}

func (a *AzureFake) GetUsers() ([]SourceUser, error) {
Expand Down
8 changes: 0 additions & 8 deletions azure_models.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,6 @@ func (au AzureUser) GetName() string {
return au.PrincipalName
}

func (au AzureUser) GetSourceType() SourceType {
return AzureSourceType
}

func (au AzureUser) GetRaw() (map[string]any, error) {
bytes, err := yson.Marshal(au)
if err != nil {
Expand Down Expand Up @@ -87,10 +83,6 @@ func (ag AzureGroup) GetName() string {
return ag.Identity
}

func (ag AzureGroup) GetSourceType() SourceType {
return AzureSourceType
}

func (ag AzureGroup) GetRaw() (map[string]any, error) {
bytes, err := yson.Marshal(ag)
if err != nil {
Expand Down
14 changes: 10 additions & 4 deletions azure_real.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ import (
)

const (
scope = "https://graph.microsoft.com/.default"
msgraphExpandLimit = 20
scope = "https://graph.microsoft.com/.default"
msgraphExpandLimit = 20
defaultAzureTimeout = 3 * time.Second
defaultAzureSecretEnvVar = "AZURE_CLIENT_SECRET"
)

var (
Expand Down Expand Up @@ -98,8 +100,12 @@ func handleNil[T any](s *T) T {
return result
}

func (a *AzureReal) GetSourceType() SourceType {
return AzureSourceType
func (a *AzureReal) CreateUserFromRaw(raw map[string]any) (SourceUser, error) {
return NewAzureUser(raw)
}

func (a *AzureReal) CreateGroupFromRaw(raw map[string]any) (SourceGroup, error) {
return NewAzureGroup(raw)
}

func (a *AzureReal) GetUsers() ([]SourceUser, error) {
Expand Down
4 changes: 2 additions & 2 deletions azure_real_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
//go:embed config.local.yaml
var _localConfig embed.FS

// TestPrintAzureUsersIntegration tests nothing, but can be used to debug Source users retrieved from ms graph api.
// TestPrintAzureUsersIntegration tests nothing, but can be used to debug Azure users retrieved from ms graph api.
// In particular, it can be used to tune userFilter for production use.
// It requires AZURE_CLIENT_SECRET env var and `config.local.yaml` file (which is .gitignored).
func TestPrintAzureUsersIntegration(t *testing.T) {
Expand Down Expand Up @@ -79,7 +79,7 @@ func TestPrintAzureUsersIntegration(t *testing.T) {
require.NotEmpty(t, usersRaw)
}

// TestPrintAzureGroupsIntegration tests nothing, but can be used to debug Source groups retrieved from ms graph api.
// TestPrintAzureGroupsIntegration tests nothing, but can be used to debug Azure groups retrieved from ms graph api.
// In particular, it can be used to tune groupsFilter for production use.
// It requires AZURE_CLIENT_SECRET env var and `config.local.yaml` file (which is .gitignored).
func TestPrintAzureGroupsIntegrationRaw(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions config.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ type YtsaurusConfig struct {
DebugUsernames []string `yaml:"debug_usernames"`
// DebugGroupnames is a list of YTsaurus groupnames for which app will print more debug info in logs.
DebugGroupnames []string `yaml:"debug_groupnames"`

SourceAttributeName *string `yaml:"source_attribute_name"`
// The attribute name of user/group object in YTsaurus.
SourceAttributeName string `yaml:"source_attribute_name"`
}

type LoggingConfig struct {
Expand Down
65 changes: 29 additions & 36 deletions diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,24 @@ import (
"go.uber.org/zap"
)

type SourceUser interface {
GetID() ObjectID
GetName() string
GetRaw() (map[string]any, error)
}

type SourceGroup interface {
GetID() ObjectID
GetName() string
GetRaw() (map[string]any, error)
}

type SourceGroupWithMembers struct {
SourceGroup SourceGroup
// Members is a set of strings, representing users' ObjectID.
Members StringSet
}

func (a *App) syncOnce() {
a.logger.Info("Start syncing")
defer a.logger.Info("Finish syncing")
Expand Down Expand Up @@ -46,7 +64,7 @@ func (a *App) syncUsers() (map[ObjectID]YtsaurusUser, error) {
return nil, errors.Wrap(err, "failed to get Source users")
}

ytUsers, err := a.ytsaurus.GetUsers(a.source.GetSourceType())
ytUsers, err := a.ytsaurus.GetUsers()
if err != nil {
return nil, errors.Wrap(err, "failed to get YTsaurus users")
}
Expand Down Expand Up @@ -106,7 +124,7 @@ func (a *App) syncGroups(usersMap map[ObjectID]YtsaurusUser) error {
if err != nil {
return errors.Wrap(err, "failed to get Source groups")
}
ytGroups, err := a.ytsaurus.GetGroupsWithMembers(a.source.GetSourceType())
ytGroups, err := a.ytsaurus.GetGroupsWithMembers()
if err != nil {
return errors.Wrap(err, "failed to get YTsaurus groups")
}
Expand Down Expand Up @@ -206,10 +224,6 @@ func (a *App) diffGroups(

existedGroupsWithMembersMap := make(map[ObjectID]existedGroup)
for _, group := range ytGroups {
if group.IsManuallyManaged(a.source.GetSourceType()) {
continue
}

sourceGroup, err := a.buildSourceGroup(&group)
if err != nil {
return nil, errors.Wrap(err, "failed to create azure group from source")
Expand Down Expand Up @@ -318,12 +332,9 @@ func (a *App) diffUsers(

existedUsersMap := make(map[ObjectID]existedUser)
for _, user := range ytUsers {
if user.IsManuallyManaged(a.source.GetSourceType()) {
continue
}
sourceUser, err := a.buildSourceUser(&user)
if err != nil {
return nil, errors.Wrap(err, "failed to create azure user from source")
return nil, errors.Wrap(err, "failed to build source user")
}
existedUsersMap[sourceUser.GetID()] = existedUser{user, sourceUser}
resultUsersMap[sourceUser.GetID()] = user
Expand Down Expand Up @@ -390,59 +401,41 @@ func (a *App) buildGroupName(sourceGroup SourceGroup) string {
}

func (a *App) buildSourceUser(ytUser *YtsaurusUser) (SourceUser, error) {
if ytUser.IsManuallyManaged(a.source.GetSourceType()) {
if ytUser.IsManuallyManaged() {
return nil, errors.New("user is manually managed and can't be converted to source user")
}
sourceType := SourceType(*ytUser.SourceType)
switch sourceType {
case AzureSourceType:
{
return NewAzureUser(ytUser.SourceRaw)
}
}
return nil, errors.New("unknown user source type")
return a.source.CreateUserFromRaw(ytUser.SourceRaw)
}

func (a *App) buildSourceGroup(ytGroup *YtsaurusGroupWithMembers) (SourceUser, error) {
if ytGroup.IsManuallyManaged(a.source.GetSourceType()) {
if ytGroup.IsManuallyManaged() {
return nil, errors.New("user is manually managed and can't be converted to source user")
}
sourceType := SourceType(*ytGroup.SourceType)
switch sourceType {
case AzureSourceType:
{
return NewAzureGroup(ytGroup.SourceRaw)
}
}
return nil, errors.New("unknown user source type")
return a.source.CreateGroupFromRaw(ytGroup.SourceRaw)
}

func (a *App) buildYtsaurusUser(sourceUser SourceUser) (YtsaurusUser, error) {
sourceType := string(sourceUser.GetSourceType())
sourceRaw, err := sourceUser.GetRaw()
if err != nil {
return YtsaurusUser{}, err
}
return YtsaurusUser{
Username: a.buildUsername(sourceUser),
SourceType: &sourceType,
SourceRaw: sourceRaw,
Username: a.buildUsername(sourceUser),
SourceRaw: sourceRaw,
// If we have Source user —> he is not banned.
BannedSince: time.Time{},
}, nil
}

func (a *App) buildYtsaurusGroup(sourceGroup SourceGroup) (YtsaurusGroup, error) {
sourceType := string(sourceGroup.GetSourceType())
sourceRaw, err := sourceGroup.GetRaw()
if err != nil {
return YtsaurusGroup{}, err
}

return YtsaurusGroup{
Name: a.buildGroupName(sourceGroup),
SourceType: &sourceType,
SourceRaw: sourceRaw,
Name: a.buildGroupName(sourceGroup),
SourceRaw: sourceRaw,
}, nil
}

Expand Down
Loading

0 comments on commit 2690bae

Please sign in to comment.