fix: regulation-worker changes for oauth destinations #2730

sanpj2292 · 2022-11-24T03:43:17Z

Description

Initially in regulation-worker for OAuth destinations as soon as the refresh token request is complete, we would retry the regulation worker job recursively. It is possible that the recursion happens for infinite number of times(according to source-code but in actual case it can be rarely seen).
To prevent the risk of recursive calls happening infinite number of times for a job. We want to restrict it to only once
This PR includes this change.

Several other things included

We have included a configurable maxRetryAttempts for OAuth case
Included a stat to help us understand if the request to control plane(as part of the OAuth flow) is timing out

Notion Ticket

Notion

Security

The code changed/added as part of this pull request won't create any security issues with how the software is being used.

…resh failure

codecov · 2022-11-24T10:15:35Z

Codecov Report

❗ No coverage uploaded for pull request base (master@9f006e5). Click here to learn what that means.
Patch coverage: 75.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #2730   +/-   ##
=========================================
  Coverage          ?   46.74%           
=========================================
  Files             ?      298           
  Lines             ?    48951           
  Branches          ?        0           
=========================================
  Hits              ?    22881           
  Misses            ?    24605           
  Partials          ?     1465

Impacted Files	Coverage Δ
regulation-worker/internal/delete/api/api.go	`67.53% <57.14%> (ø)`
services/oauth/oauth.go	`53.88% <76.92%> (ø)`
regulation-worker/cmd/main.go	`68.25% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

regulation-worker/internal/delete/api/api.go

- update error message to include actual response - update naming - making maxRetryAttempts as configurable - include jobId in logs

koladilip · 2022-11-25T04:09:46Z

regulation-worker/internal/delete/api/api.go

-// prepares payload based on (job,destDetail) & make an API call to transformer.
-// gets (status, failure_reason) which is converted to appropriate model.Error & returned to caller.
-func (api *APIManager) Delete(ctx context.Context, job model.Job, destination model.Destination) model.JobStatus {
+func (api *APIManager) deleteWithRetry(ctx context.Context, job model.Job, destination model.Destination, retryAttempts int) model.JobStatus {


retryAttempts -> retryAttempt or retryAttemptCount or something like makes more sense as it is indicating, up till how many attempts happened right?

It can go either way:

count how many retries happened until now

current retry number

I'd prefer the latter.

saikumarrs · 2022-11-25T04:30:40Z

regulation-worker/cmd/main.go

+				OAuth:                        OAuth,
+				MaxOAuthRefreshRetryAttempts: config.GetInt("RegulationWorker.oauth.maxRefreshRetryAttempts", 1),


I'd not tie this variable to OAuth as it's a delete retry we are doing rather than a "refresh retry".
Moreover, we might retry a delete request due to other failure scenarios that could possibly succeed in the next retry.

The retry only is required for the OAuth case in order to prevent the case where in a failed job is retried after the token's validity period.

atzoum · 2022-11-25T08:31:32Z

regulation-worker/internal/delete/api/api.go


-	if isOAuthEnabled && isTokenExpired(jobResp) {
+	if isOAuthEnabled && isTokenExpired(jobResp) && currentOauthRetryAttempt < api.MaxOAuthRefreshRetryAttempts {


what the jobStatus will be when isOAuthEnabled && isTokenExpired && currentOauthRetryAttempt >= api.MaxOAuthRefreshRetryAttempts?

Whatever is calculated as part of getJobStatus() that we got in the last retry(when currentOauthRetry = api.MaxOAuthRefreshRetryAttempts - 1)

There is already a retry mechanism such that regulation-manager would provide the already failed jobs to regulation-worker to send it to destinaiton, so that way I feel it should be ok. Let me know if you think otherwise

I will try to rephrase the question:
What will the transformer's HTTP response code be whenever it returns in its response body at least 1 entry with authErrorCategory=REFRESH_TOKEN? Is it deterministic or not?

Whenever atleast one entry of authErrorCategory = REFRESH_TOKEN is present in response body, we will return a status-code of 500 as transformer http status-code as of today.

When you say deterministic, are you asking about how do we identify perpetual refresh token failures, I mean the scenario wherein even after multiple retries(when api.MaxOAuthRefreshRetryAttempts > 1), we are still getting refresh_token error from transformer or may be it is something else ?

Just trying to make sure that the jobStatus will be model.JobStatusFailed if the token refresh fails or we have reached to maximum refresh attempts :)

fix: oauth retry restricted to only one re-try attempt in case of ref…

0d80f96

…resh failure

sanpj2292 requested review from atzoum, krishna2020, saurav-malani, koladilip and saikumarrs November 24, 2022 03:43

sanpj2292 self-assigned this Nov 24, 2022

github-actions bot added the server-team label Nov 24, 2022

sanpj2292 requested a review from chandumlg November 24, 2022 04:22

sanpj2292 added the integrations-team label Nov 24, 2022

Merge remote-tracking branch 'origin/master' into fix.delete-users-oauth

4b92595

saikumarrs reviewed Nov 24, 2022

View reviewed changes

koladilip reviewed Nov 24, 2022

View reviewed changes

regulation-worker/internal/delete/api/api.go Outdated Show resolved Hide resolved

regulation-worker/internal/delete/api/api.go Show resolved Hide resolved

- add control-plane request timeout stat

435cced

- update error message to include actual response - update naming - making maxRetryAttempts as configurable - include jobId in logs

koladilip reviewed Nov 25, 2022

View reviewed changes

saikumarrs reviewed Nov 25, 2022

View reviewed changes

saikumarrs approved these changes Nov 25, 2022

View reviewed changes

rename the attempts argument

c050764

koladilip approved these changes Nov 25, 2022

View reviewed changes

atzoum reviewed Nov 25, 2022

View reviewed changes

sanpj2292 requested a review from atzoum November 25, 2022 09:26

atzoum approved these changes Nov 28, 2022

View reviewed changes

Sai Sankeerth added 5 commits November 28, 2022 13:45

fix: info logs to debug deployment issue in dev

35e9f07

Add logs

c8c0060

revert logs

1d6810a

revert to Debug logs

24d6535

Merge remote-tracking branch 'origin/master' into fix.delete-users-oauth

0ce1737

sanpj2292 merged commit 0ed5a82 into master Nov 28, 2022

rudder-server-bot mentioned this pull request Dec 1, 2022

chore: release 1.3.0 #2749

Closed

This was referenced Dec 1, 2022

chore: prerelease 1.4.0-rc.1 #2750

Merged

chore: release 1.4.0 #2752

Merged

chore: prerelease 1.4.0-rc.1 #2757

Closed

chore: prerelease 1.4.0-rc.2 #2759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: regulation-worker changes for oauth destinations #2730

fix: regulation-worker changes for oauth destinations #2730

sanpj2292 commented Nov 24, 2022 •

edited

Loading

codecov bot commented Nov 24, 2022 •

edited

Loading

koladilip Nov 25, 2022

saikumarrs Nov 25, 2022

saikumarrs Nov 25, 2022

sanpj2292 Nov 25, 2022

atzoum Nov 25, 2022

sanpj2292 Nov 25, 2022

sanpj2292 Nov 25, 2022

atzoum Nov 27, 2022

sanpj2292 Nov 28, 2022

sanpj2292 Nov 28, 2022

atzoum Nov 28, 2022

		OAuth: OAuth,
		MaxOAuthRefreshRetryAttempts: config.GetInt("RegulationWorker.oauth.maxRefreshRetryAttempts", 1),


		if isOAuthEnabled && isTokenExpired(jobResp) {
		if isOAuthEnabled && isTokenExpired(jobResp) && currentOauthRetryAttempt < api.MaxOAuthRefreshRetryAttempts {

fix: regulation-worker changes for oauth destinations #2730

fix: regulation-worker changes for oauth destinations #2730

Conversation

sanpj2292 commented Nov 24, 2022 • edited Loading

Description

Notion Ticket

Security

codecov bot commented Nov 24, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanpj2292 commented Nov 24, 2022 •

edited

Loading

codecov bot commented Nov 24, 2022 •

edited

Loading