Skip to content

fix(google-cloud-logging): add missing config#9622

Merged
juststillthinking merged 7 commits into
apache:masterfrom
kindomLee:docs-google-cloud-logging
Jul 18, 2023
Merged

fix(google-cloud-logging): add missing config#9622
juststillthinking merged 7 commits into
apache:masterfrom
kindomLee:docs-google-cloud-logging

Conversation

@kindomLee

@kindomLee kindomLee commented Jun 8, 2023

Copy link
Copy Markdown
Contributor

Description

Add missing config field for google cloud logging plugin

  • docs: add client_email field for google-cloud-logging plugin
  • docs: add client_email field and log_format default value for google-cloud-logging plugin in zh

Fixes # (issue)

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

kindomLee added 2 commits June 8, 2023 11:14
@kindomLee
Update google-cloud-logging.md
73eb2d5 
docs: add client_email field  for google-cloud-logging plugin
@kindomLee
Update google-cloud-logging.md
565d1d2 
docs: add client_email field and log_format default value for google-cloud-logging plugin in zh
@kindomLee kindomLee changed the title Docs google cloud logging docs: add missing config in google cloud logging plugin Jun 8, 2023
@juststillthinking

juststillthinking commented Jun 12, 2023

Copy link
Copy Markdown
Contributor

hi @kindomLee, you'd better add the missing config in schema too:
https://github.com/apache/apisix/blob/master/apisix/plugins/google-cloud-logging.lua#L38

@juststillthinking juststillthinking added the wait for update wait for the author's response in this issue/PR label Jun 12, 2023
@kindomLee
feat(google-cloud-logging): add client_email to auth_config schema
6566d47 
This commit adds the client_email field to the auth_config schema of the google-cloud-logging plugin. This field is required for the Google OAuth2 authentication process.
@juststillthinking juststillthinking changed the title docs: add missing config in google cloud logging plugin fix: add missing config in google cloud logging plugin Jun 12, 2023
@kindomLee

kindomLee commented Jun 12, 2023

Copy link
Copy Markdown
Contributor Author

Thank you @monkeyDluffy6017 for your suggestion. I have added the missing config in schema as you advised. Could you please review my PR again and approve it if everything looks good? I hope we can merge this soon.

juststillthinking
juststillthinking reviewed Jun 12, 2023
View reviewed changes
Comment thread apisix/plugins/google-cloud-logging.lua
@kindomLee

kindomLee commented Jun 14, 2023

Copy link
Copy Markdown
Contributor Author

We need to set up Google authentication to authenticate to Logging, according to the existing code [1], we are currently using OAuth2.0 as our authentication method, and this method requires us to provide the client_email parameter [2].

[1] https://github.com/apache/apisix/blob/7ec3c0f583510ebcdf9c414f83792e1ac247a5b9/apisix/plugins/google-cloud-logging/oauth.lua#LL103C15-L103C15
[2] https://developers.google.com/identity/protocols/oauth2/service-account?hl=en#authorizingrequests

juststillthinking
juststillthinking reviewed Jun 15, 2023
View reviewed changes
Comment thread docs/zh/latest/plugins/google-cloud-logging.md
@juststillthinking

juststillthinking commented Jun 15, 2023

Copy link
Copy Markdown
Contributor

Could you add client_email to the all test file, google-cloud-logging.t, google-cloud-logging2.t?

@juststillthinking

juststillthinking commented Jun 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

We need to set up Google authentication to authenticate to Logging

Could you create a issue about this?

@kindomLee

kindomLee commented Jun 17, 2023

Copy link
Copy Markdown
Contributor Author

Could you add client_email to the all test file, google-cloud-logging.t, google-cloud-logging2.t?

Sorry, I'm not very familiar with the lua testing framework, this part might need some other partners to help

We need to set up Google authentication to authenticate to Logging

Could you create a issue about this?

I should say this, we have already provided two ways to read Google authentication when using the google cloud logging plugin, which are calling and passing in auth_config and reading the file auth_file[1]. When reading auth_file, the google client_email field is there so it won't cause any problems; but when authenticating through auth_config, because the default documentation description skips the client_mail setting, that's why it will cause an error. I don't think we need to raise another authentication issue

[1]

apisix/apisix/plugins/google-cloud-logging.lua

Line 138 in 7ec3c0f

local function fetch_oauth_conf(conf)

@juststillthinking

juststillthinking commented Jun 21, 2023

Copy link
Copy Markdown
Contributor

@kindomLee just add client_email in auth_config in test cases, or they will fail because this change:
image

@juststillthinking

juststillthinking commented Jun 21, 2023

Copy link
Copy Markdown
Contributor

I should say this, we have already provided two ways to read Google authentication when using the google cloud logging plugin, which are calling and passing in auth_config and reading the file auth_file[1]. When reading auth_file, the google client_email field is there so it won't cause any problems; but when authenticating through auth_config, because the default documentation description skips the client_mail setting, that's why it will cause an error. I don't think we need to raise another authentication issue

I misunderstood you, I thought you wanted to add actual test cases

@shreemaan-abhishek

shreemaan-abhishek commented Jul 14, 2023

Copy link
Copy Markdown
Contributor

I will take over the PR and add test cases. Thanks for the initial contribution @kindomLee.

@shreemaan-abhishek shreemaan-abhishek mentioned this pull request Jul 14, 2023
Closed
5 tasks
@shreemaan-abhishek

shreemaan-abhishek commented Jul 14, 2023

Copy link
Copy Markdown
Contributor

Raised #9836

@moonming

moonming commented Jul 14, 2023

Copy link
Copy Markdown
Member

@shreemaan-abhishek you can add test cases in this PR

@juststillthinking juststillthinking added approved and removed wait for update wait for the author's response in this issue/PR labels Jul 17, 2023
@juststillthinking juststillthinking changed the title fix: add missing config in google cloud logging plugin fix(google-cloud-logging): add missing config in google cloud logging plugin Jul 17, 2023
@juststillthinking juststillthinking changed the title fix(google-cloud-logging): add missing config in google cloud logging plugin fix(google-cloud-logging): add missing config Jul 17, 2023
@juststillthinking
juststillthinking merged commit 5d418b4 into apache:master Jul 18, 2023
membphis added a commit that referenced this pull request May 12, 2026
@membphis
docs(changelog): fix typos, version number, PR reference, and heading…
ba6035a 
… levels in v3.x sections (#13360)

- Replace `## Doc improvements` (3.13.0 / 3.14.0 / 3.14.1) and
  `## Developer productivity` (3.13.0) with H3 so they nest under the
  release header instead of starting a new top-level section.
- 3.12.0: correct OpenResty version typo `v1.27.11` -> `1.27.1.1` (#11936).
- 3.11.0: fix plugin name typo `attach-consmer-label` -> `attach-consumer-label` (#11604).
- 3.5.0: the `google-cloud-logging` `client_email` entry referenced #9813
  (proxy-rewrite); the actual PR is #9622.
- 3.0.0: fix `APSISIX` typo in the `enable_cpu_affinity` entry.

Refs #13359.
wistefan pushed a commit to wistefan/apisix that referenced this pull request Jun 16, 2026
@membphis @wistefan
docs(changelog): fix typos, version number, PR reference, and heading…
b2f4cd1 
… levels in v3.x sections (apache#13360)

- Replace `## Doc improvements` (3.13.0 / 3.14.0 / 3.14.1) and
  `## Developer productivity` (3.13.0) with H3 so they nest under the
  release header instead of starting a new top-level section.
- 3.12.0: correct OpenResty version typo `v1.27.11` -> `1.27.1.1` (apache#11936).
- 3.11.0: fix plugin name typo `attach-consmer-label` -> `attach-consumer-label` (apache#11604).
- 3.5.0: the `google-cloud-logging` `client_email` entry referenced apache#9813
  (proxy-rewrite); the actual PR is apache#9622.
- 3.0.0: fix `APSISIX` typo in the `enable_cpu_affinity` entry.

Refs apache#13359.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlinsRan AlinsRan AlinsRan approved these changes
+1 more reviewer
@juststillthinking juststillthinking juststillthinking approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kindomLee @juststillthinking @shreemaan-abhishek @moonming @AlinsRan