Create custom annotation for satisfy "value" #3756
Conversation
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
cncf-cla: no
anthonyho007
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
signed CLA |
k8s-ci-robot
added
cncf-cla: yes
test/e2e/annotations/satisfy.go
Outdated
| return Expect(server).Should(ContainSubstring("satisfy any")) | ||
| }) | ||
|
|
||
| resp, body, errs := gorequest.New(). |
There was a problem hiding this comment.
Would it be a good idea to not just see if the line appears in nginx.conf, but also check if it's actually working correctly? Say by enabling both nginx.ingress.kubernetes.io/auth-type and nginx.ingress.kubernetes.io/whitelist-source-range and checking to see if the request succeeds if it satisfies only one, but fails if it satisfies neither.
There was a problem hiding this comment.
I have managed to create a successful e2e test which tests this scenario with auth-type and satisfy any both enabled, and whitelist ip ranges with configuration-snippets: "allow 18.0.0.0/8; allow 56.0.0.0/8; deny all;". However, this test somehow destabilize other e2e tests which causes the e2e integration test to fail with 50% chances. For now, i will just remove this test until I could find a way to stabilize it.
There was a problem hiding this comment.
However, this test somehow destabilize other e2e tests which causes the e2e integration test to fail with 50% chances
We need to refactor some e2e tests to make sure we can predefine valid cidrs in any environment (some test work fine in minikube but not in e2e that uses a different environment to configure the cluster)
|
|
||
| ### Satisfy | ||
|
|
||
| By default, request would need to satisfy all authentications requirements in order to be allow. By using this annotation, it would allow requests to satisfy any or all authentication requirements based on the configuration value. |
There was a problem hiding this comment.
Some grammar/phrasing nitpicks: Say
By default, a request would need to satisfy all authentication requirements in order to be allowed. By using this annotation, requests that satisfy either any or all authentication requirements are allowed, based on the configuration value.
|
@anthonyho007 please squash the commits |
k8s-ci-robot
added
size/XL
k8s-ci-robot
added
size/L
|
@anthonyho007 this is looking 👍 now! Please squash your commits then we can merge this. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anthonyho007, ElvinEfendi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
This PR will add a new annotation for satisfy "value". An example use-case would be any application that wants to have both basic authentication + oauth proxy available to support human + bot authentication, and only requires one of those auth methods to be satisfied to get to the application.
Which issue this PR fixes
fixes # #3677
Special notes for your reviewer: