feat: always set auth cookie

[nabokihms]

Signed-off-by: m.nabokikh maksim.nabokikh@flant.com

What this PR does / why we need it:

The purpose of this PR is described in detail in the linked issue. This is a naive implementation that changes default behavior.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

Which issue/s this PR fixes

fixes ##8078

How Has This Been Tested?

It was tested manually in a Kubernetes cluster with oauth2 proxy as an authentication provider for the nginx_auth_request module.

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added tests to cover my changes.
• All new and existing tests passed.

[k8s-ci-robot]

@nabokihms: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[k8s-ci-robot]

Hi @nabokihms. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[strongjz]

Thank you for the PR, I am concerned with two things, 1 no new tests for this, and while I agree with your assessment, users probably do not want to continuously login if the back end sends a 404, this changes the behavior and should be configurable, Can we use the annotation instead of a default true?

With the annotations, we can add an e2e test and it allows it to be the users choice to set to true.

-James

/kind bug
/triage accepted
/priority important-longterm

[k8s-ci-robot]

@strongjz: The label(s) triage/accept cannot be applied, because the repository doesn't have them.

In response to this:

Thank you for the PR, I am concerned with two things, 1 no new tests for this, and while I agree with your assessment, users probably do not want to continuously login if the back end sends a 404, this changes the behavior and should be configurable, Can we use the annotation instead of a default true?

With the annotations, we can add an e2e test and it allows it to be the users choice to set to true.

-James

/kind bug
/triage accept
/priority important-longterm

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.

[rikatz]

+1 for James comment, let's not change current behavior. A new annotation is desired and some tests on that :)

[tao12345666333]

+1 for James comment, let's not change current behavior. A new annotation is desired and some tests on that :)

+1 It would be more appropriate to add a new annotation

[nabokihms]

Folks, the code has been updated according to your suggestions. I also wrote e2e tests to support what I described in the issue.

Btw, I noticed that the e2e tests docs generator is broken, so I also fixed it. Because of the links to the code in this doc, no one can write it by hand 😅 . Please let me know if I need to revert these script changes (I suppose that this is a little out of the scope of this PR).

[nabokihms]

@rikatz @strongjz just a friendly reminder about this PR.

[nabokihms]

@rikatz @strongjz and one more. Do I need to add anything, or is this PR ok to be merged in its current state?

[strongjz]

/lgtm

Thank you for the changes and the added tests!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nabokihms, strongjz

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:

• OWNERS [strongjz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[nabokihms]

Thank you so much for the review. Great work 👍