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

feat: blocklist-source-range annotation #446

Merged
merged 8 commits into from
May 14, 2021

Conversation

tokers
Copy link
Contributor

@tokers tokers commented May 13, 2021

DO NOT MERGE this PR until #442 is merged.

Please answer these questions before submitting a pull request

  • Why submit this pull request?

  • Bugfix

  • New feature provided

  • Improve performance

  • Backport patches

  • Related issues

#445


Bugfix

  • Description

  • How to fix?


New feature or improvement

  • Describe the details and related test reports.

Backport patches

  • Why need to backport?

  • Source branch

  • Related commits and pull requests

  • Target branch

@tokers tokers linked an issue May 13, 2021 that may be closed by this pull request
@tokers tokers marked this pull request as draft May 13, 2021 07:45
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2021

Codecov Report

Merging #446 (5b6eb88) into master (8824bbd) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
+ Coverage   37.01%   37.04%   +0.03%     
==========================================
  Files          47       47              
  Lines        3839     3841       +2     
==========================================
+ Hits         1421     1423       +2     
  Misses       2233     2233              
  Partials      185      185              
Impacted Files Coverage Δ
pkg/kube/translation/annotations/cors.go 100.00% <ø> (ø)
pkg/kube/translation/annotations/iprestriction.go 100.00% <100.00%> (ø)

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 8824bbd...5b6eb88. Read the comment docs.

@tokers tokers requested a review from gxthrj May 13, 2021 12:45
@tokers
Copy link
Contributor Author

tokers commented May 13, 2021

@tao12345666333 @gxthrj

@tokers tokers marked this pull request as ready for review May 13, 2021 12:46
if am, ok := annotations[_corsAllowMethods]; ok {
cors.Methods = am
// NewCorsHandler creates a handler to convert annotations about
// cors to APISIX cors plugin.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// cors to APISIX cors plugin.
// CORS to APISIX CORS plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use small case to represent a plugin name.

// +k8s:deepcopy-gen=true
type IPRestrictConfig struct {
Whitelist []string `json:"whitelist,omitempty"`
Blacklist []string `json:"blacklist,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Blacklist []string `json:"blacklist,omitempty"`
Blocklist []string `json:"blacklist,omitempty"`

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 think we should keep this name unless APISIX also change it.

// IPRestrictConfig is the rule config for ip-restriction plugin.
// +k8s:deepcopy-gen=true
type IPRestrictConfig struct {
Whitelist []string `json:"whitelist,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

change all Whitelist to Allowlist ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for consistency as terms in APISIX are black, white.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if APISIX will be modified, but these are two projects. I think it makes sense to modify it now. In order to avoid subsequent modifications caused by break changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@tokers
Copy link
Contributor Author

tokers commented May 14, 2021

@tao12345666333 @gxthrj Changed.

@tokers tokers merged commit 5d479ae into apache:master May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: support blocklist-source-range annotation
4 participants