8000 Update VirtualServer to ignore CRL for EgressMTLS by shaun-nx · Pull Request #3737 · nginx/kubernetes-ingress · GitHub
[go: up one dir, main page]

Skip to content

Conversation

shaun-nx
Copy link
Contributor
@shaun-nx shaun-nx commented Apr 7, 2023

Proposed changes

This change updates the logic which applies policies to VirtualServer to ignore CRL for EgressMTLS.
Resolves #3732

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@shaun-nx shaun-nx requested a review from a team as a code owner April 7, 2023 17:21
@github-actions github-actions bot added bug An issue reporting a potential bug tests Pull requests that update tests labels Apr 7, 2023
@codecov
Copy link
codecov bot commented Apr 11, 2023

Codecov Report

Merging #3737 (4c36d81) into main (38ae409) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3737      +/-   ##
==========================================
+ Coverage   52.37%   52.41%   +0.03%     
==========================================
  Files          59       59              
  Lines       16898    16902       +4     
==========================================
+ Hits         8851     8859       +8     
+ Misses       7750     7748       -2     
+ Partials      297      295       -2     
Impacted Files Coverage Δ
internal/configs/virtualserver.go 95.17% <100.00%> (+0.01%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@brianehlert brianehlert added this to the v3.1.1 milestone Apr 17, 2023
Copy link
@lucacome lucacome left a comment

Choose a reason for hiding this comment

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

I don't think we want to ignore the CRL. We should use the right directive for it proxy_ssl_crl.

@brianehlert
Copy link
Collaborator

I don't think we want to ignore the CRL. We should use the right directive for it proxy_ssl_crl.

That would be the right thing to do.

@shaun-nx
Copy link
Contributor Author

@brianehlert @lucacome
The aim of this PR isn't to support using CRLs with EgressMTLs. This only addresses the issue posted in #3732

We only added support for CRLs with IngressMTLs but not EgressMTLs.
The PR for IngressMTLs update is here for reference: #3632

@brianehlert
Copy link
Collaborator

To close this conversation, we will be adding proper EgressMTLS support for CRL to match IngressMTLS behavior.
That is not work contained in this PR.

@shaun-nx shaun-nx requested a review from lucacome April 25, 2023 05:12
@lucacome lucacome dismissed their stale review April 26, 2023 02:10

avoid blocking PR

@shaun-nx shaun-nx merged commit 87b8a58 into main Apr 26, 2023
@shaun-nx shaun-nx deleted the fix/caSecret branch April 26, 2023 08:58
lucacome pushed a commit that referenced this pull request May 4, 2023
* Update VirtualServer to ignore CRL for EgressMTLS

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Un-comment tests

* Fix crt and crl path in test and fix nill slice reference

* Update data files for egress MTLS tests

* Remove VSR python test

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add new app.yaml file for EgressMTLS tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
(cherry picked from commit 87b8a58)
lucacome added a commit that referenced this pull request May 4, 2023
Update VirtualServer to ignore CRL for EgressMTLS (#3737)

* Update VirtualServer to ignore CRL for EgressMTLS

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Un-comment tests

* Fix crt and crl path in test and fix nill slice reference

* Update data files for egress MTLS tests

* Remove VSR python test

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add new app.yaml file for EgressMTLS tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
(cherry picked from commit 87b8a58)

Co-authored-by: Shaun <s.odonovan@f5.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a potential bug tests Pull requests that update tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proxy_ssl_trusted_certificate getting cert and then crl on the same line
6 participants
0