fix #6617 and #6659

[SamMHD]

closes #6617 and #6659

---

In order to fix #6617 I have changed how dagRoute.DisableAuth is caclulated.

Also, to fix #6659 I thought it is needed to change behavior when we are upgrading a request to HTTPS (when permitInsecure is not set and we are redirecting HTTP to HTTPS). Thereby, when dagRoute.AuthContext/dagRoute.AuthDisabled is configured it will affect redirection as well -- before these changes, redirection won't care about them.

---

Changes:

• use dagRoute's AuthContext and AuthDisabled in HTTPS-Upgrade to fix 6659
• Use globalExtAuth.AuthPolicy.Disabled to calculate dagRoute.AuthDisabled
• Fix Tests

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 81.05%. Comparing base (db432cc) to head (d818a8c). Report is 62 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6661      +/-   ##
==========================================
- Coverage   81.76%   81.05%   -0.72%     
==========================================
  Files         133      133              
  Lines       15944    20026    +4082     
==========================================
+ Hits        13037    16232    +3195     
- Misses       2614     3500     +886     
- Partials      293      294       +1     

Files with missing lines	Coverage Δ
internal/dag/httpproxy_processor.go	91.00% <100.00%> (-0.41%)	:arrow_down:
internal/envoy/v3/route.go	81.00% <100.00%> (+0.97%)	:arrow_up:
internal/featuretests/v3/envoy.go	99.13% <100.00%> (-0.16%)	:arrow_down:

... and 124 files with indirect coverage changes

---- 🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[SamMHD]

@sunjayBhatia can you please add required labels to this one? cc: @skriss @rajatvig @davinci26

[SamMHD]

Thank you @tsaarni for the label. When can we proceed to merge?

[tsaarni]

@SamMHD I will try to arrange time for myself to review ASAP today or tomorrow!

[SamMHD]

Thank you so much @tsaarni

[SamMHD]

Thank you for your feedback; I'll add the tests as suggested.

I previously attempted to write tests for this scenario but found the existing test functions a bit incompatible to implement disabled: true for GlobalAuth, which led me to initially omit them. Now that you've requested it, I'll revisit this and include the necessary tests.

[tsaarni]

I previously attempted to write tests for this scenario but found the existing test functions a bit incompatible to implement disabled: true for GlobalAuth, which led me to initially omit them. Now that you've requested it, I'll revisit this and include the necessary tests.

Exactly, the existing test functions make no sense for disabled: true. There could be a new one to test this behaviour specifically.

[SamMHD]

Exactly, the existing test functions make no sense for disabled: true. There could be a new one to test this behaviour specifically.

I think I have implemented a workaround to this by making the test function more flexible. I'll push the changes now, hope you find time to review this, too.

Btw, Do you think we still need to add this to e2e tests?

[SamMHD]

@tsaarni Do have time to review the changes again?

[SamMHD]

@tsaarni @sunjayBhatia @skriss is there any update on this?

[SamMHD]

Why nobody answer us? :((( @tsaarni @sunjayBhatia @skriss

[tsaarni]

Sorry @SamMHD I haven’t had a chance to review yet, I've been swamped 😅

[SamMHD]

thank you @tsaarni , when do you think you can give it a look?

[SamMHD]

Hello again, Is there any update on this PR? @tsaarni

[tsaarni]

Hi @SamMHD and apologies once more for the delays! I've now created an environment to test this PR. It works well, however it seems override can only be done at the route level and not at the virtual host level. This example does not work:

apiVersion: projectcontour.io/v1
kind: HTTPProxy
metadata:
  name: echoserver
spec:
  virtualhost:
    fqdn: protected.127-0-0-101.nip.io
    tls:
      secretName: ingress
    authorization:
      authPolicy:
        disabled: false
  routes:
    - services:
        - name: echoserver
          port: 80

It seems that with this scenario we end up in this code branch where it will try use the uninitialized extension name from HTTPProxy.spec.virtualhost.authorization.extensionRef for the virtualhost and fail.

[SamMHD]

Very good point dear @tsaarni , I have visited that code branch and fixed it, other than this one more thing came to my attention, and it was that "If we disable global auth in vhost configuration then we can not enable it back again in a route" so I have fixed it in "else clause" of same code branch.

Can you give it a try or help me setup your test environment for myself? I think it should be fixed now.

[github-actions[bot]]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[SamMHD]

dear @tsaarni , do you have any update on this PR?

[izturn]

/lgtm

[SamMHD]

Thank you so much @izturn 🙌 Are we going to merge or we need additional approvals?

[SamMHD]

Thank you dear @tsaarni for the changes 🙏 Applied all of your suggestions ✅

[SamMHD]

Will we proceed to merge or we need more approvals?

[sunjayBhatia]

Will we proceed to merge or we need more approvals?

Could you add a more descriptive PR title please? thanks!

[SamMHD]

Will we proceed to merge or we need more approvals?

Could you add a more descriptive PR title please? thanks!

@sunjayBhatia , I have changed PR title. Is it good to go or you recommend another title?

[SamMHD]

btw, Sorry @tsaarni for that force push, I had to sign-off to pass DCO

[tsaarni]

btw, Sorry @tsaarni for that force push, I had to sign-off to pass DCO

No problem @SamMHD, github luckily shows the commit hash before and after force push so it becomes easy to spot the changes :)

[SamMHD]

Thank you @tsaarni @sunjayBhatia @izturn for the efforts and review 🙏