From ce3c1274b8024ab076078c0e89869092a31fe775 Mon Sep 17 00:00:00 2001 From: Andrei-Alexandru Dobre Date: Tue, 7 Jan 2025 11:59:33 +0100 Subject: [PATCH 1/5] feat: define client interface; feat: implement rate limit decorator; --- .../provider/data_source_all_usergroups.go | 4 +- internal/provider/data_source_all_users.go | 5 +- internal/provider/data_source_user.go | 4 +- internal/provider/provider.go | 8 +-- internal/slackExt/client.go | 18 +++++++ internal/slackExt/client_impl.go | 27 ++++++++++ internal/slackExt/client_rate_limit.go | 49 +++++++++++++++++++ 7 files changed, 108 insertions(+), 7 deletions(-) create mode 100644 internal/slackExt/client.go create mode 100644 internal/slackExt/client_impl.go create mode 100644 internal/slackExt/client_rate_limit.go diff --git a/internal/provider/data_source_all_usergroups.go b/internal/provider/data_source_all_usergroups.go index 4504347..2ce3d85 100644 --- a/internal/provider/data_source_all_usergroups.go +++ b/internal/provider/data_source_all_usergroups.go @@ -7,6 +7,8 @@ import ( "context" "fmt" + "github.com/essent/terraform-provider-slack/internal/slackExt" + "github.com/hashicorp/terraform-plugin-framework/datasource" "github.com/hashicorp/terraform-plugin-framework/datasource/schema" "github.com/hashicorp/terraform-plugin-framework/types" @@ -21,7 +23,7 @@ func NewAllUserGroupsDataSource() datasource.DataSource { } type AllUserGroupsDataSource struct { - client *slack.Client + client slackExt.Client } type AllUserGroupsDataSourceModel struct { diff --git a/internal/provider/data_source_all_users.go b/internal/provider/data_source_all_users.go index c28c0cb..46e3a05 100644 --- a/internal/provider/data_source_all_users.go +++ b/internal/provider/data_source_all_users.go @@ -7,11 +7,12 @@ import ( "context" "fmt" + "github.com/essent/terraform-provider-slack/internal/slackExt" + "github.com/hashicorp/terraform-plugin-framework/datasource" "github.com/hashicorp/terraform-plugin-framework/datasource/schema" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-log/tflog" - "github.com/slack-go/slack" ) var _ datasource.DataSource = &AllUsersDataSource{} @@ -21,7 +22,7 @@ func NewAllUsersDataSource() datasource.DataSource { } type AllUsersDataSource struct { - client *slack.Client + client slackExt.Client } type AllUsersDataSourceModel struct { diff --git a/internal/provider/data_source_user.go b/internal/provider/data_source_user.go index 163fceb..c4a7745 100644 --- a/internal/provider/data_source_user.go +++ b/internal/provider/data_source_user.go @@ -7,6 +7,8 @@ import ( "context" "fmt" + "github.com/essent/terraform-provider-slack/internal/slackExt" + "github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator" "github.com/hashicorp/terraform-plugin-framework/datasource" "github.com/hashicorp/terraform-plugin-framework/datasource/schema" @@ -24,7 +26,7 @@ func NewUserDataSource() datasource.DataSource { } type UserDataSource struct { - client *slack.Client + client slackExt.Client } type UserDataSourceModel struct { diff --git a/internal/provider/provider.go b/internal/provider/provider.go index a7bce2d..17b8304 100644 --- a/internal/provider/provider.go +++ b/internal/provider/provider.go @@ -8,6 +8,8 @@ import ( "fmt" "os" + "github.com/essent/terraform-provider-slack/internal/slackExt" + "github.com/hashicorp/terraform-plugin-framework/datasource" "github.com/hashicorp/terraform-plugin-framework/function" "github.com/hashicorp/terraform-plugin-framework/provider" @@ -30,7 +32,7 @@ type SlackProviderModel struct { } type SlackProviderData struct { - Client *slack.Client + Client slackExt.Client } func (p *SlackProvider) Metadata(ctx context.Context, req provider.MetadataRequest, resp *provider.MetadataResponse) { @@ -83,8 +85,8 @@ func (p *SlackProvider) Configure(ctx context.Context, req provider.ConfigureReq return } - resp.DataSourceData = &SlackProviderData{Client: client} - resp.ResourceData = &SlackProviderData{Client: client} + resp.DataSourceData = &SlackProviderData{Client: slackExt.New(client)} + resp.ResourceData = &SlackProviderData{Client: slackExt.New(client)} } func (p *SlackProvider) Resources(ctx context.Context) []func() resource.Resource { diff --git a/internal/slackExt/client.go b/internal/slackExt/client.go new file mode 100644 index 0000000..1a36623 --- /dev/null +++ b/internal/slackExt/client.go @@ -0,0 +1,18 @@ +package slackExt + +import ( + "context" + + "github.com/slack-go/slack" +) + +type Client interface { + GetUserInfo(user string) (*slack.User, error) + GetUserByEmail(email string) (*slack.User, error) + GetUsersContext(ctx context.Context) ([]slack.User, error) + GetUserGroups(options ...slack.GetUserGroupsOption) ([]slack.UserGroup, error) +} + +func New(base *slack.Client) Client { + return &clientRateLimit{&clientImpl{base}} +} diff --git a/internal/slackExt/client_impl.go b/internal/slackExt/client_impl.go new file mode 100644 index 0000000..291b435 --- /dev/null +++ b/internal/slackExt/client_impl.go @@ -0,0 +1,27 @@ +package slackExt + +import ( + "context" + + "github.com/slack-go/slack" +) + +type clientImpl struct { + base *slack.Client +} + +func (c *clientImpl) GetUserInfo(user string) (*slack.User, error) { + return c.base.GetUserInfo(user) +} + +func (c *clientImpl) GetUserByEmail(email string) (*slack.User, error) { + return c.base.GetUserByEmail(email) +} + +func (c *clientImpl) GetUsersContext(ctx context.Context) ([]slack.User, error) { + return c.base.GetUsersContext(ctx) +} + +func (c *clientImpl) GetUserGroups(options ...slack.GetUserGroupsOption) ([]slack.UserGroup, error) { + return c.base.GetUserGroups(options...) +} diff --git a/internal/slackExt/client_rate_limit.go b/internal/slackExt/client_rate_limit.go new file mode 100644 index 0000000..7f88321 --- /dev/null +++ b/internal/slackExt/client_rate_limit.go @@ -0,0 +1,49 @@ +package slackExt + +import ( + "context" + "time" + + "github.com/slack-go/slack" +) + +type clientRateLimit struct { + base Client +} + +func rateLimit[R any](f func() (R, error)) (R, error) { + for { + result, err := f() + + if err == nil { + return result, nil + } + + if rateLimitedError, ok := err.(*slack.RateLimitedError); ok { + <-time.After(rateLimitedError.RetryAfter) + err = nil + } + } +} + +func (c *clientRateLimit) GetUserInfo(user string) (result *slack.User, err error) { + return rateLimit(func() (*slack.User, error) { + return c.base.GetUserInfo(user) + }) +} + +func (c *clientRateLimit) GetUserByEmail(email string) (*slack.User, error) { + return c.base.GetUserByEmail(email) +} + +func (c *clientRateLimit) GetUsersContext(ctx context.Context) ([]slack.User, error) { + return rateLimit(func() ([]slack.User, error) { + return c.base.GetUsersContext(ctx) + }) +} + +func (c *clientRateLimit) GetUserGroups(options ...slack.GetUserGroupsOption) ([]slack.UserGroup, error) { + return rateLimit(func() ([]slack.UserGroup, error) { + return c.base.GetUserGroups(options...) + }) +} From 7d02c644c26ef2461449b697ee078ba0c9f696bb Mon Sep 17 00:00:00 2001 From: Andrei-Alexandru Dobre Date: Tue, 7 Jan 2025 12:04:19 +0100 Subject: [PATCH 2/5] feat: rate limit; --- internal/slackExt/client_rate_limit.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/slackExt/client_rate_limit.go b/internal/slackExt/client_rate_limit.go index 7f88321..7bd37c9 100644 --- a/internal/slackExt/client_rate_limit.go +++ b/internal/slackExt/client_rate_limit.go @@ -33,13 +33,13 @@ func (c *clientRateLimit) GetUserInfo(user string) (result *slack.User, err erro } func (c *clientRateLimit) GetUserByEmail(email string) (*slack.User, error) { - return c.base.GetUserByEmail(email) + return rateLimit(func() (*slack.User, error) { + return c.base.GetUserByEmail(email) + }) } func (c *clientRateLimit) GetUsersContext(ctx context.Context) ([]slack.User, error) { - return rateLimit(func() ([]slack.User, error) { - return c.base.GetUsersContext(ctx) - }) + return c.base.GetUsersContext(ctx) } func (c *clientRateLimit) GetUserGroups(options ...slack.GetUserGroupsOption) ([]slack.UserGroup, error) { From 12b0d82d58492d4cb126a482cf25baae736e7041 Mon Sep 17 00:00:00 2001 From: Andrei-Alexandru Dobre Date: Tue, 7 Jan 2025 13:41:51 +0100 Subject: [PATCH 3/5] feat: rate limit tracing; fix: rate limit retries; --- .../provider/data_source_all_usergroups.go | 2 +- internal/provider/data_source_user.go | 4 +- internal/slackExt/client.go | 6 +-- internal/slackExt/client_impl.go | 12 ++--- internal/slackExt/client_rate_limit.go | 48 +++++++++++-------- 5 files changed, 39 insertions(+), 33 deletions(-) diff --git a/internal/provider/data_source_all_usergroups.go b/internal/provider/data_source_all_usergroups.go index 2ce3d85..a427c98 100644 --- a/internal/provider/data_source_all_usergroups.go +++ b/internal/provider/data_source_all_usergroups.go @@ -115,7 +115,7 @@ func (d *AllUserGroupsDataSource) Read(ctx context.Context, req datasource.ReadR return } - userGroups, err := d.client.GetUserGroups(slack.GetUserGroupsOptionIncludeUsers(true)) + userGroups, err := d.client.GetUserGroups(ctx, slack.GetUserGroupsOptionIncludeUsers(true)) if err != nil { resp.Diagnostics.AddError( "Client Error", diff --git a/internal/provider/data_source_user.go b/internal/provider/data_source_user.go index c4a7745..5a6c2bc 100644 --- a/internal/provider/data_source_user.go +++ b/internal/provider/data_source_user.go @@ -97,9 +97,9 @@ func (d *UserDataSource) Read(ctx context.Context, req datasource.ReadRequest, r ) if !data.ID.IsNull() { - user, err = d.client.GetUserInfo(data.ID.ValueString()) + user, err = d.client.GetUserInfo(ctx, data.ID.ValueString()) } else { - user, err = d.client.GetUserByEmail(data.Email.ValueString()) + user, err = d.client.GetUserByEmail(ctx, data.Email.ValueString()) } if err != nil { diff --git a/internal/slackExt/client.go b/internal/slackExt/client.go index 1a36623..84170ed 100644 --- a/internal/slackExt/client.go +++ b/internal/slackExt/client.go @@ -7,10 +7,10 @@ import ( ) type Client interface { - GetUserInfo(user string) (*slack.User, error) - GetUserByEmail(email string) (*slack.User, error) + GetUserInfo(ctx context.Context, user string) (*slack.User, error) + GetUserByEmail(ctx context.Context, email string) (*slack.User, error) GetUsersContext(ctx context.Context) ([]slack.User, error) - GetUserGroups(options ...slack.GetUserGroupsOption) ([]slack.UserGroup, error) + GetUserGroups(ctx context.Context, options ...slack.GetUserGroupsOption) ([]slack.UserGroup, error) } func New(base *slack.Client) Client { diff --git a/internal/slackExt/client_impl.go b/internal/slackExt/client_impl.go index 291b435..2e62d9c 100644 --- a/internal/slackExt/client_impl.go +++ b/internal/slackExt/client_impl.go @@ -10,18 +10,18 @@ type clientImpl struct { base *slack.Client } -func (c *clientImpl) GetUserInfo(user string) (*slack.User, error) { - return c.base.GetUserInfo(user) +func (c *clientImpl) GetUserInfo(ctx context.Context, user string) (*slack.User, error) { + return c.base.GetUserInfoContext(ctx, user) } -func (c *clientImpl) GetUserByEmail(email string) (*slack.User, error) { - return c.base.GetUserByEmail(email) +func (c *clientImpl) GetUserByEmail(ctx context.Context, email string) (*slack.User, error) { + return c.base.GetUserByEmailContext(ctx, email) } func (c *clientImpl) GetUsersContext(ctx context.Context) ([]slack.User, error) { return c.base.GetUsersContext(ctx) } -func (c *clientImpl) GetUserGroups(options ...slack.GetUserGroupsOption) ([]slack.UserGroup, error) { - return c.base.GetUserGroups(options...) +func (c *clientImpl) GetUserGroups(ctx context.Context, options ...slack.GetUserGroupsOption) ([]slack.UserGroup, error) { + return c.base.GetUserGroupsContext(ctx, options...) } diff --git a/internal/slackExt/client_rate_limit.go b/internal/slackExt/client_rate_limit.go index 7bd37c9..b5c5ca7 100644 --- a/internal/slackExt/client_rate_limit.go +++ b/internal/slackExt/client_rate_limit.go @@ -2,8 +2,10 @@ package slackExt import ( "context" + "fmt" "time" + "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/slack-go/slack" ) @@ -11,39 +13,43 @@ type clientRateLimit struct { base Client } -func rateLimit[R any](f func() (R, error)) (R, error) { - for { - result, err := f() - - if err == nil { - return result, nil - } +func rateLimit[R any](ctx context.Context, f func() (R, error), getZeroValue func() R) (result R, err error) { + for err != nil { + result, err = f() if rateLimitedError, ok := err.(*slack.RateLimitedError); ok { - <-time.After(rateLimitedError.RetryAfter) - err = nil + tflog.Trace(ctx, fmt.Sprintf("Rate limited, waiting %f", rateLimitedError.RetryAfter.Seconds()), map[string]any{}) + select { + case <-time.After(rateLimitedError.RetryAfter): + tflog.Trace(ctx, "Rate limit wait complete", map[string]any{}) + err = nil + case <-ctx.Done(): + return getZeroValue(), ctx.Err() + } } } + + return result, err } -func (c *clientRateLimit) GetUserInfo(user string) (result *slack.User, err error) { - return rateLimit(func() (*slack.User, error) { - return c.base.GetUserInfo(user) - }) +func (c *clientRateLimit) GetUserInfo(ctx context.Context, user string) (result *slack.User, err error) { + return rateLimit(ctx, func() (*slack.User, error) { + return c.base.GetUserInfo(ctx, user) + }, func() *slack.User { return nil }) } -func (c *clientRateLimit) GetUserByEmail(email string) (*slack.User, error) { - return rateLimit(func() (*slack.User, error) { - return c.base.GetUserByEmail(email) - }) +func (c *clientRateLimit) GetUserByEmail(ctx context.Context, email string) (*slack.User, error) { + return rateLimit(ctx, func() (*slack.User, error) { + return c.base.GetUserByEmail(ctx, email) + }, func() *slack.User { return nil }) } func (c *clientRateLimit) GetUsersContext(ctx context.Context) ([]slack.User, error) { return c.base.GetUsersContext(ctx) } -func (c *clientRateLimit) GetUserGroups(options ...slack.GetUserGroupsOption) ([]slack.UserGroup, error) { - return rateLimit(func() ([]slack.UserGroup, error) { - return c.base.GetUserGroups(options...) - }) +func (c *clientRateLimit) GetUserGroups(ctx context.Context, options ...slack.GetUserGroupsOption) ([]slack.UserGroup, error) { + return rateLimit(ctx, func() ([]slack.UserGroup, error) { + return c.base.GetUserGroups(ctx, options...) + }, func() []slack.UserGroup { return nil }) } From 6ee87bd36738f0c5713e9721f5186a55aadcfb6d Mon Sep 17 00:00:00 2001 From: Andrei-Alexandru Dobre Date: Tue, 7 Jan 2025 13:50:19 +0100 Subject: [PATCH 4/5] fix: rate limit retries; --- internal/slackExt/client_rate_limit.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/internal/slackExt/client_rate_limit.go b/internal/slackExt/client_rate_limit.go index b5c5ca7..68e2ac4 100644 --- a/internal/slackExt/client_rate_limit.go +++ b/internal/slackExt/client_rate_limit.go @@ -14,22 +14,26 @@ type clientRateLimit struct { } func rateLimit[R any](ctx context.Context, f func() (R, error), getZeroValue func() R) (result R, err error) { - for err != nil { + for { result, err = f() + if err == nil { + return result, nil + } + if rateLimitedError, ok := err.(*slack.RateLimitedError); ok { tflog.Trace(ctx, fmt.Sprintf("Rate limited, waiting %f", rateLimitedError.RetryAfter.Seconds()), map[string]any{}) + select { case <-time.After(rateLimitedError.RetryAfter): tflog.Trace(ctx, "Rate limit wait complete", map[string]any{}) - err = nil case <-ctx.Done(): return getZeroValue(), ctx.Err() } + } else { + return getZeroValue(), err } } - - return result, err } func (c *clientRateLimit) GetUserInfo(ctx context.Context, user string) (result *slack.User, err error) { @@ -51,5 +55,5 @@ func (c *clientRateLimit) GetUsersContext(ctx context.Context) ([]slack.User, er func (c *clientRateLimit) GetUserGroups(ctx context.Context, options ...slack.GetUserGroupsOption) ([]slack.UserGroup, error) { return rateLimit(ctx, func() ([]slack.UserGroup, error) { return c.base.GetUserGroups(ctx, options...) - }, func() []slack.UserGroup { return nil }) + }, func() []slack.UserGroup { return []slack.UserGroup{} }) } From ab5d7cdab5edfc7c371d96d7b1da0eed11c49ba9 Mon Sep 17 00:00:00 2001 From: Andrei-Alexandru Dobre Date: Tue, 7 Jan 2025 13:56:01 +0100 Subject: [PATCH 5/5] chore: remove tracing: --- internal/slackExt/client_rate_limit.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/internal/slackExt/client_rate_limit.go b/internal/slackExt/client_rate_limit.go index 68e2ac4..261251b 100644 --- a/internal/slackExt/client_rate_limit.go +++ b/internal/slackExt/client_rate_limit.go @@ -2,10 +2,8 @@ package slackExt import ( "context" - "fmt" "time" - "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/slack-go/slack" ) @@ -22,11 +20,8 @@ func rateLimit[R any](ctx context.Context, f func() (R, error), getZeroValue fun } if rateLimitedError, ok := err.(*slack.RateLimitedError); ok { - tflog.Trace(ctx, fmt.Sprintf("Rate limited, waiting %f", rateLimitedError.RetryAfter.Seconds()), map[string]any{}) - select { case <-time.After(rateLimitedError.RetryAfter): - tflog.Trace(ctx, "Rate limit wait complete", map[string]any{}) case <-ctx.Done(): return getZeroValue(), ctx.Err() }