Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ref(server): Re-authenticate at regular intervals #731

Merged
merged 8 commits into from
Aug 27, 2020
Merged

ref(server): Re-authenticate at regular intervals #731

merged 8 commits into from
Aug 27, 2020

Conversation

RaduW
Copy link
Contributor

@RaduW RaduW commented Aug 24, 2020

Relay now retries to authenticate at regular intervals instead
of only at startup.

@RaduW RaduW requested a review from a team August 24, 2020 13:41
@RaduW
Copy link
Contributor Author

RaduW commented Aug 25, 2020

Abandoned splitting UpstreamRelay actor in two.

jan-auer
jan-auer requested changes Aug 26, 2020
View reviewed changes
relay-config/src/config.rs Outdated Show resolved Hide resolved
relay-config/src/config.rs Outdated Show resolved Hide resolved
relay-server/src/actors/upstream.rs Show resolved Hide resolved
@jan-auer
Copy link
Member

@RaduW Updated the PR as per our offline discussion:

  • When we receive a network error, we start the grace period and only after the grace period expires the state is set to errored.
  • On all other errors, we apply the error state immediately.

There's now a TODO for the follow-up PR to start authenticating on network errors if authentication is not already in progress.

@jan-auer jan-auer self-requested a review August 26, 2020 13:01
untitaker
untitaker approved these changes Aug 26, 2020
View reviewed changes
relay-server/src/actors/upstream.rs Outdated Show resolved Hide resolved
relay-config/src/config.rs
@@ -533,6 +539,8 @@ impl Default for Http {
connection_timeout: 3,
max_retry_interval: 60,
host_header: None,
auth_interval: 60,
Copy link
Member

Choose a reason for hiding this comment

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

As long as we don't have all PRs impld and merged I would make the entire new behavior opt-in.

This comment was marked as spam.

Sign in to view

jan-auer
jan-auer approved these changes Aug 27, 2020
View reviewed changes
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

@tonyo I think we need to reconsider the default values for reauthenticating. At the moment it is 60s with a 10s grace period. The grace period is fine, but I think we should increase the interval.

@untitaker untitaker mentioned this pull request Aug 27, 2020
Closed
@RaduW RaduW merged commit d3db077 into master Aug 27, 2020
@RaduW RaduW deleted the ref/upstream-authentication branch August 27, 2020 08:24
jan-auer added a commit that referenced this pull request Aug 27, 2020
@jan-auer
Merge branch 'master' into feature/unreal-crash-properties
87606a3 
* master:
  fix: Avoid rust-analyzer recursion bug (#734)
  ref(server): Re-authenticate at regular intervals (#731)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hughes123b hughes123b hughes123b left review comments

@untitaker untitaker untitaker approved these changes

@jan-auer jan-auer jan-auer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@RaduW @jan-auer @untitaker @hughes123b