Skip to content
This repository was archived by the owner on Sep 26, 2023. It is now read-only.

feat: add api key support via client settings #1880

Closed
arithmetic1728 wants to merge 3 commits into from
Closed

feat: add api key support via client settings #1880

arithmetic1728 wants to merge 3 commits into from

Conversation

@arithmetic1728
Copy link
Contributor

@arithmetic1728 arithmetic1728 commented Nov 15, 2022 •
edited
Loading

This PR supports API key via client settings. When an API key is provided via client settings, its value will be passed to API call request via x-goog-api-key header and other types of credential is no longer needed. API key is useful for accessing public data anonymously, for instance google maps APIs, see https://cloud.google.com/docs/authentication/api-keys

This feature has been tested with language client, see arithmetic1728/java-language#1

@arithmetic1728 arithmetic1728 added kokoro:force-run Add this label to force Kokoro to re-run the tests. downstream-check:run labels Nov 18, 2022
@arithmetic1728 arithmetic1728 marked this pull request as ready for review November 18, 2022 23:02
@arithmetic1728 arithmetic1728 requested a review from a team November 18, 2022 23:02
@arithmetic1728 arithmetic1728 requested review from a team as code owners November 18, 2022 23:02
@arithmetic1728 arithmetic1728 changed the title feat: add api key support [draft] feat: add api key support Nov 18, 2022
@arithmetic1728 arithmetic1728 removed kokoro:force-run Add this label to force Kokoro to re-run the tests. downstream-check:run labels Nov 18, 2022
blakeli0
blakeli0 reviewed Nov 28, 2022
View reviewed changes
Copy link
Contributor

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

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

LGTM on the surface, do you mind providing more info on this api key support? Is there a design doc? In what scenario we would want customers to set it? Is it going to be use for anything else other than setting a new header?

@arithmetic1728
Copy link
Contributor Author

arithmetic1728 commented Nov 28, 2022

LGTM on the surface, do you mind providing more info on this api key support? Is there a design doc? In what scenario we would want customers to set it? Is it going to be use for anything else other than setting a new header?

Here is the public doc: https://cloud.google.com/docs/authentication/api-keys
Here is the old design doc: go/gapic-api-key-support, it includes API key support via client options and env var, but we now only support it via client options.

@blakeli0
Copy link
Contributor

blakeli0 commented Nov 28, 2022

LGTM on the surface, do you mind providing more info on this api key support? Is there a design doc? In what scenario we would want customers to set it? Is it going to be use for anything else other than setting a new header?

Here is the public doc: https://cloud.google.com/docs/authentication/api-keys Here is the old design doc: go/gapic-api-key-support, it includes API key support via client options and env var, but we now only support it via client options.

Thanks, can you please add these info to the description of the PR and maybe explain the new auth flow if API key is set? Also please add some java doc to explain what this attribute is and when/how developers should use it? Like this

blakeli0
blakeli0 reviewed Nov 28, 2022
View reviewed changes
gax/src/main/java/com/google/api/gax/rpc/ClientSettings.java
stubSettings.setQuotaProjectId(quotaProjectId);
return self();
}

Copy link
Contributor

@blakeli0 blakeli0 Nov 28, 2022

Choose a reason for hiding this comment

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

Please add Javadoc here.

Copy link
Member

@meltsufin meltsufin Nov 29, 2022

Choose a reason for hiding this comment

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

Can we use checkstyle to automatically check for javadoc on public methods?

Copy link
Contributor

@blakeli0 blakeli0 Nov 29, 2022

Choose a reason for hiding this comment

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

Not sure how flexible the checkstyle plugin is, as we would need to exclude a lot of classes marked with InternalApi, but we can give it a try.

Copy link
Contributor Author

@arithmetic1728 arithmetic1728 Nov 29, 2022

Choose a reason for hiding this comment

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

added javadoc

@arithmetic1728 arithmetic1728 changed the title feat: add api key support feat: add api key support via client settings Nov 29, 2022
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 29, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

91.7% 91.7% Coverage
0.0% 0.0% Duplication

@arithmetic1728
Copy link
Contributor Author

arithmetic1728 commented Nov 29, 2022

Thanks, can you please add these info to the description of the PR and maybe explain the new auth flow if API key is set? Also please add some java doc to explain what this attribute is and when/how developers should use it? Like this

Done.

TimurSadykov
TimurSadykov approved these changes Nov 29, 2022
View reviewed changes
Copy link

@TimurSadykov TimurSadykov left a comment

Choose a reason for hiding this comment

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

auth bits looks good

gax/src/main/java/com/google/api/gax/rpc/ClientSettings.java
}

public final String getApiKey() {
return stubSettings.getApiKey();
Copy link

@TimurSadykov TimurSadykov Nov 29, 2022

Choose a reason for hiding this comment

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

exclude empty values and return null, to avoid extra checks everywhere?

@arithmetic1728 arithmetic1728 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 3, 2022
@arithmetic1728 arithmetic1728 marked this pull request as draft December 3, 2022 00:53
suztomo added a commit to suztomo/gax-java that referenced this pull request Dec 9, 2022
@suztomo
chore: testing pull request copy of googleapis#1880
d22152a
@suztomo suztomo mentioned this pull request Dec 9, 2022
Closed
@suztomo
Copy link
Member

suztomo commented Dec 21, 2022

@arithmetic1728 gax-java has been moved to gapic-generator-java/gax-java. Blake notified me that this PR 1880 is important one worth moving. Would you move this pull request? Here is the steps:

git clone https://github.com/googleapis/gapic-generator-java
cd gapic-generator-java/gax-java
curl https://patch-diff.githubusercontent.com/raw/googleapis/gax-java/pull/1880.diff | patch  --strip=1
git commit -a

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Neenu1995 Neenu1995 Awaiting requested review from Neenu1995

3 more reviewers

@blakeli0 blakeli0 blakeli0 left review comments

@meltsufin meltsufin meltsufin left review comments

@TimurSadykov TimurSadykov TimurSadykov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

do not merge Indicates a pull request not ready for merge, due to either quality or timing.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@arithmetic1728 @blakeli0 @suztomo @TimurSadykov @meltsufin