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

feat: support forward-auth plugin #937

Merged
merged 3 commits into from
Apr 15, 2022
Merged

feat: support forward-auth plugin #937

merged 3 commits into from
Apr 15, 2022

Conversation

nayihz
Copy link
Contributor

@nayihz nayihz commented Mar 25, 2022

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

@tao12345666333 tao12345666333 linked an issue Mar 26, 2022 that may be closed by this pull request
Feature suggestion: support forward auth for Ingress resource #767
Closed
@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2022 •
edited

Codecov Report

Merging #937 (a3d43d2) into master (4da91b7) will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #937      +/-   ##
==========================================
+ Coverage   32.89%   33.05%   +0.16%     
==========================================
  Files          72       74       +2     
  Lines        7661     7680      +19     
==========================================
+ Hits         2520     2539      +19     
  Misses       4865     4865              
  Partials      276      276
Impacted Files Coverage Δ
pkg/kube/translation/annotations.go 61.53% <ø> (ø)
pkg/kube/translation/annotations/forward_auth.go 100.00% <100.00%> (ø)
test/e2e/e2e.go 100.00% <0.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 4da91b7...a3d43d2. Read the comment docs.

@nayihz nayihz marked this pull request as draft March 27, 2022 02:50
@nayihz
Copy link
Contributor Author

nayihz commented Mar 27, 2022

How do I test extensions/v1beta1 ingress in my local environment? My local k8s cluster(v1.23.3) do not support this version.

@tao12345666333
Copy link
Member

You can use kind with v1.21 kubernetes

@nayihz nayihz force-pushed the feat_forward_auth branch 4 times, most recently from 6b21a53 to a3d43d2 Compare March 27, 2022 13:45
@nayihz nayihz marked this pull request as ready for review March 27, 2022 14:54
@tao12345666333
Copy link
Member

re-run CI

@tao12345666333 tao12345666333 self-assigned this Mar 28, 2022
gxthrj
gxthrj approved these changes Apr 6, 2022
View reviewed changes
Copy link
Contributor

@gxthrj gxthrj left a comment

Choose a reason for hiding this comment

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

LGTM

tao12345666333
tao12345666333 approved these changes Apr 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

But due to the recent merge of #949, I think in e2e we need to tweak a little bit

@tao12345666333
Copy link
Member

@cmssczy Can you pull the latest code and make some adjustments to the test case names in e2e? so that they can be more uniform

tokers
tokers reviewed Apr 7, 2022
View reviewed changes
test/e2e/scaffold/k8s.go
return s.ensureHTTPDeleteSuccess(u.String())
}

func (s *Scaffold) ensureHTTPDeleteSuccess(url string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can encapsulate a method ensureAdminOperationIsSuccessful, and pass the request, so that we can reuse the code base of ensureHTTPPutSuccess.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. We can optimize in subsequent PRs. @cmssczy

Copy link
Member

Choose a reason for hiding this comment

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

I agree. We can optimize in subsequent PRs. @cmssczy

Copy link
Member

Choose a reason for hiding this comment

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

Let's move forward.

Copy link
Member

Choose a reason for hiding this comment

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

Let's move forward.

@nayihz nayihz force-pushed the feat_forward_auth branch 2 times, most recently from 543fd4b to 0c9008e Compare April 7, 2022 11:51
@tao12345666333 tao12345666333 merged commit eb02429 into apache:master Apr 15, 2022
@tao12345666333 tao12345666333 mentioned this pull request Apr 15, 2022
Closed
@nayihz nayihz deleted the feat_forward_auth branch April 15, 2022 11:02
AlinsRan pushed a commit to AlinsRan/apisix-ingress-controller that referenced this pull request May 9, 2022
feat: support forward-auth plugin (apache#937)
dbd9ed3
@tao12345666333 tao12345666333 mentioned this pull request Jul 13, 2022
Closed
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

@tokers tokers tokers left review comments

@gxthrj gxthrj gxthrj approved these changes

@lingsamuel lingsamuel lingsamuel approved these changes

@nic-6443 nic-6443 Awaiting requested review from nic-6443

Assignees

@tao12345666333 tao12345666333

Labels
area/controller enhancement New feature or request
Projects
Apache APISIX Ingress controller
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature suggestion: support forward auth for Ingress resource
6 participants
@nayihz @codecov-commenter @tao12345666333 @gxthrj @tokers @lingsamuel