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.

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

use sts regional endpoint for aws session #283

Closed
wants to merge 2 commits into from

Conversation

jyotimahapatra
Copy link

What type of PR is this?

/kind bug

What this PR does / why we need it:
STS suggests using regional endpoints.

AWS recommends using Regional STS endpoints to reduce latency, build in redundancy, and increase session token validity

The PR creates a way for CCM users to set an env var USE_REGIONAL_STS to use regional endpoints. The default when the flag is unset is false. Only when the flag is explicitly true/TRUE the regional endpoint is chosen.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

CCM users can set an env var `USE_REGIONAL_STS` to use regional endpoints for sts calls. The default when the flag is unset is false. Only when the flag is explicitly `true`/`TRUE` the regional endpoint is chosen.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 4, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jyotimahapatra
To complete the pull request process, please assign wongma7 after the PR has been reviewed.
You can assign the PR to them by writing /assign @wongma7 in a comment when ready.

The full list of commands accepted by this bot can be found 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


// Environment Variable to create sts session with regional endpoint
// If unset or false, uses global sts endpoint
useRegionalSts = "USE_REGIONAL_STS"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the switch should be the opposite because 1. we want everyone to use regional STS endpoints and 2. we are still publishing alpha containers, and we can document in the upgrade notes that if you have STS regional endpoints disabled in your AWS account, you need to change this setting. Also, I think it should be a flag instead of an environment variable (and/or a cloud config option). We can do something more complicated, like a feature gate of sorts that initially is opt-in but when it moves to beta becomes opt-out (I.e. the feature gate is called "UseRegionalSTS" and is false in alpha, but true in beta, and eventually is removed and cannot be opted out of). Whatever we end up with though, we should have users opt-out of regional endpoints, rather than opt-in to them, because STS wants to move traffic off the global endpoint.

Copy link
Author

@jyotimahapatra jyotimahapatra Nov 6, 2021

Choose a reason for hiding this comment

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

i generally agree, but a few things i wanted to discuss about how we make the changes that could affect users.
Since sts has an ability to disable regional endpoints, shifting the default to regional could be a breaking change. I propose that we keep the original behavior intact and deprecate over a release. In the next release of ccm, we could shift the default to regional and put a release note(included in PR description)

From my investigation so far, using a flag is difficult because of the how the cloud provider is instatiated. The kube controller passes no configurations while initializing the cloud provider. Looking at the signature cloudprovider.RegisterCloudProvider(ProviderName, func(config io.Reader) (cloudprovider.Interface, error) { there is no other config apart from CloudConfig file reader. It would take a refactoring in k/k to pass additional flagset. Do you find this explanation correct? If so we'll have to use an env var until flagset can be passed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@k8s-ci-robot
Copy link
Contributor

@jyotimahapatra: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 1, 2022
@k8s-ci-robot
Copy link
Contributor

@jyotimahapatra: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cloud-provider-aws-e2e eae393f link true /test pull-cloud-provider-aws-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@nckturner
Copy link
Contributor

Replaced by #313

/close

@k8s-ci-robot
Copy link
Contributor

@nckturner: Closed this PR.

In response to this:

Replaced by #313

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants