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

return 429 for STS throttling #630

Merged
merged 1 commit into from
Sep 21, 2023
Merged

return 429 for STS throttling #630

merged 1 commit into from
Sep 21, 2023

Conversation

nnmin-aws
Copy link
Contributor

@nnmin-aws nnmin-aws commented Sep 18, 2023 •
edited
Loading

What this PR does / why we need it:
currently authenticator only returns 401 and it is hard to know if the request is really unauthorized or it is due to aws sts throttle.
This PR will create a new metric in authenticator to record the STS throttling count.
This PR will return 429 to api-server when authenticator got STS throttling from AWS STS service

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 18, 2023
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 18, 2023
@nnmin-aws
Copy link
Contributor Author

/retest

@nnmin-aws nnmin-aws force-pushed the nnmin-rel5 branch 2 times, most recently from 4de7547 to db531b9 Compare September 19, 2023 18:11
@nnmin-aws
Copy link
Contributor Author

/retest

@nnmin-aws
Copy link
Contributor Author

/retest

@dims
Copy link
Member

dims commented Sep 19, 2023

/assign @nckturner

nckturner
nckturner reviewed Sep 19, 2023
View reviewed changes
pkg/token/token.go Outdated Show resolved Hide resolved
pkg/token/token.go Show resolved Hide resolved
@mmerkes
Copy link

mmerkes commented Sep 19, 2023

Can you add more context to the PR description? Would be good for people to understand why we're making this change.

@mmerkes
Copy link

mmerkes commented Sep 19, 2023

What testing have you done?

mmerkes
mmerkes reviewed Sep 19, 2023
View reviewed changes
pkg/token/token.go Outdated Show resolved Hide resolved
pkg/token/token.go Outdated Show resolved Hide resolved
@nnmin-aws
Copy link
Contributor Author

Can you add more context to the PR description? Would be good for people to understand why we're making this change.

thank you! updated the PR description

@nnmin-aws
Copy link
Contributor Author

nnmin-aws commented Sep 20, 2023 •
edited
Loading

What testing have you done?

The unit test has been included in the PR, server_test.go and token_test.go.

Also integration and e2e test ran for each PR

@nnmin-aws nnmin-aws force-pushed the nnmin-rel5 branch 2 times, most recently from 9e5bee9 to fec7b95 Compare September 20, 2023 17:18
@nnmin-aws
Copy link
Contributor Author

/retest

@nnmin-aws nnmin-aws force-pushed the nnmin-rel5 branch 2 times, most recently from ad14a2f to e700330 Compare September 20, 2023 17:46
@nnmin-aws
Copy link
Contributor Author

/retest

mmerkes
mmerkes reviewed Sep 20, 2023
View reviewed changes
pkg/token/token.go Outdated Show resolved Hide resolved
mmerkes
mmerkes approved these changes Sep 20, 2023
View reviewed changes
Copy link

@mmerkes mmerkes left a comment

Choose a reason for hiding this comment

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

LGTM

@nnmin-aws
Copy link
Contributor Author

retest

@nnmin-aws
Copy link
Contributor Author

/retest

@nnmin-aws
Copy link
Contributor Author

/retest

@nckturner
Copy link
Contributor

/lgtm
/approved

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2023
@dims dims added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2023
@dims
Copy link
Member

dims commented Sep 21, 2023

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: dims, mmerkes, nnmin-aws

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit e847a7b into kubernetes-sigs:master Sep 21, 2023
@nnmin-aws nnmin-aws deleted the nnmin-rel5 branch December 10, 2023 03:52
@sushanth0910 sushanth0910 mentioned this pull request Nov 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nckturner nckturner nckturner left review comments

@mmerkes mmerkes mmerkes approved these changes

@jaypipes jaypipes Awaiting requested review from jaypipes

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@dims dims

@nckturner nckturner

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nnmin-aws @dims @mmerkes @nckturner @k8s-ci-robot