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

doc(APISIX-Ingress): add how to change Admin API key of Apache APISIX… #1031

Merged
merged 4 commits into from
Jun 8, 2022
Merged

doc(APISIX-Ingress): add how to change Admin API key of Apache APISIX… #1031

merged 4 commits into from
Jun 8, 2022

Conversation

Chever-John
Copy link
Contributor

@Chever-John Chever-John commented May 20, 2022 •
edited
Loading

… Ingress

mention #1021

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What this PR does / why we need it:

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

juzhiyuan
juzhiyuan reviewed May 21, 2022
View reviewed changes
Copy link
Member

@juzhiyuan juzhiyuan left a comment

Choose a reason for hiding this comment

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

Hi, please update this PR's checklist

docs/en/latest/FAQ.md Outdated

### 9. Modify the Admin API key in APISIX-Ingress

Usually, you need to modify the Admin API key to protect Apache APISIX and APISIX Ingress. Please refer to this [link](https://apisix.apache.org/zh/docs/apisix/how-to-build/#:~:text=%E5%8F%82%E8%A7%81%20%E6%B5%8B%E8%AF%95%E6%A1%86%E6%9E%B6-,%E6%AD%A5%E9%AA%A45%EF%BC%9A%E4%BF%AE%E6%94%B9%20Admin%20API%20key,-%23) to simply change Apache APISIX.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Usually, you need to modify the Admin API key to protect Apache APISIX and APISIX Ingress. Please refer to this [link](https://apisix.apache.org/zh/docs/apisix/how-to-build/#:~:text=%E5%8F%82%E8%A7%81%20%E6%B5%8B%E8%AF%95%E6%A1%86%E6%9E%B6-,%E6%AD%A5%E9%AA%A45%EF%BC%9A%E4%BF%AE%E6%94%B9%20Admin%20API%20key,-%23) to simply change Apache APISIX.
Usually, you need to modify the Admin API key to protect Apache APISIX and APISIX Ingress. Please refer to this [link](https://apisix.apache.org/zh/docs/apisix/how-to-build/) to simply change Apache APISIX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved~

tokers
tokers reviewed May 23, 2022
View reviewed changes
docs/en/latest/FAQ.md Outdated

However, in apisix-ingress-controller, if we need to change the Admin API key, we also need to change the Admin API key in apisix-ingress-controller. There are two ways to implement the requirements here.

In one method, we need to change the values of the two values.yaml files in the helm chart We need to change the Admin API key values in both helm chart packages.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence makes me confused. Which values should we modify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me refine it a little～

Copy link
Member

Choose a reason for hiding this comment

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

This sentence makes me confused. Which values should we modify?

+1.

If the description is not clear, please paste the specific code directly, it will be clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the description of the first method.
Please review it, sir.

@tao12345666333 tao12345666333 linked an issue May 25, 2022 that may be closed by this pull request
doc: add the way of changing the admin key into FAQ #1021
Closed
@tao12345666333 tao12345666333 added the documentation Improvements or additions to documentation label May 25, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1031 (c4e6835) into master (3cccd56) will increase coverage by 0.17%.
The diff coverage is 34.72%.

❗ Current head c4e6835 differs from pull request most recent head 17cfb9a. Consider uploading reports for the commit 17cfb9a to get more accurate results

@@            Coverage Diff             @@
##           master    #1031      +/-   ##
==========================================
+ Coverage   30.87%   31.04%   +0.17%     
==========================================
  Files          74       77       +3     
  Lines        8373     9070     +697     
==========================================
+ Hits         2585     2816     +231     
- Misses       5511     5951     +440     
- Partials      277      303      +26
Impacted Files Coverage Δ
pkg/ingress/apisix_consumer.go 0.00% <0.00%> (ø)
pkg/ingress/compare.go 0.00% <0.00%> (ø)
pkg/ingress/controller.go 0.86% <0.00%> (-0.04%) ⬇️
pkg/ingress/gateway_httproute.go 0.00% <0.00%> (ø)
pkg/ingress/status.go 0.00% <0.00%> (ø)
pkg/kube/translation/annotations.go 61.53% <ø> (ø)
pkg/kube/translation/annotations/csrf.go 0.00% <0.00%> (ø)
pkg/kube/translation/apisix_route.go 20.60% <0.00%> (-0.55%) ⬇️
pkg/kube/translation/translator.go 46.07% <ø> (ø)
pkg/types/event.go 100.00% <ø> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cccd56...17cfb9a. Read the comment docs.

@tao12345666333
Copy link
Member

Please merge latest code, and resolve the conflicts. Thanks

@tao12345666333
Copy link
Member

If you need any help, please ping me directly

@Chever-John
Copy link
Contributor Author

@tao12345666333 Got it~

tao12345666333
tao12345666333 requested changes Jun 2, 2022
View reviewed changes
docs/en/latest/FAQ.md Outdated Show resolved Hide resolved
docs/en/latest/FAQ.md Outdated Show resolved Hide resolved
docs/en/latest/FAQ.md Outdated Show resolved Hide resolved
tao12345666333
tao12345666333 approved these changes Jun 7, 2022
View reviewed changes
Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

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

LGTM

let's move forward

@tao12345666333 tao12345666333 merged commit 2af39c9 into apache:master Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tao12345666333 tao12345666333 tao12345666333 approved these changes

@juzhiyuan juzhiyuan juzhiyuan approved these changes

@tokers tokers Awaiting requested review from tokers

@gxthrj gxthrj Awaiting requested review from gxthrj

@bzp2010 bzp2010 Awaiting requested review from bzp2010

@lingsamuel lingsamuel Awaiting requested review from lingsamuel

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
Apache APISIX Ingress controller
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

doc: add the way of changing the admin key into FAQ
5 participants
@Chever-John @codecov-commenter @tao12345666333 @juzhiyuan @tokers