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

Remove deadcode: Interface TokenExchanger, and only implementation (Google STS) #51043

Closed
wants to merge 2 commits into from

Conversation

howardjohn
Copy link
Member

Followup from #50989

@howardjohn howardjohn added the release-notes-none Indicates a PR that does not require release notes. label May 14, 2024
@howardjohn howardjohn requested review from a team as code owners May 14, 2024 18:28
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 14, 2024
Copy link
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

Mostly LG. We haven't absorbed the CA deletion yet and this would require extra additional work to move STS client into CA, so let's wait until we can properly land this removal.

@kyessenov kyessenov added the do-not-merge Block automatic merging of a PR. label May 15, 2024
// TODO extract this logic out to a plugin
if o.CAProviderName == security.GoogleCAProvider || o.CAProviderName == security.GoogleCASProvider {
var err error
o.TokenExchanger, err = stsclient.NewSecureTokenServiceExchanger(o.CredFetcher, o.TrustDomain)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is CredFetcher used anywhere else besides STS?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that allows GCE auth which can be used agent and Istiod I believe. not sure the implementation was ever really completed though...

It was intended for VMs, not Google specific (i.e. there could be an AWS/EC2 one, just no one wrote it)

@howardjohn howardjohn closed this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Block automatic merging of a PR. release-notes-none Indicates a PR that does not require release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants