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.

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

Add support for "projectcontour.io/tls-cert-namespace" annotation #4271

Merged
merged 7 commits into from
Jan 19, 2022

Conversation

pablo-ruth
Copy link
Contributor

Hello,

Following the issue raised in #3544 we are maintaining an internal fork of Contour to be able to use TLS Delegation with Ingress v1. After a discussion with our Tanzu Solution Egineer @lzetea we decided that it could be interesting to propose our changes upstream.

We described our context here to @xaleeks and it seems to be similar to @ghouscht here.

@youngnick said that using an annotation could be the only way to go so I used this approach. I tried to keep the code simple and the change very small. It's backward compatible with "namespace/secret" syntax because the annotation is used only if namespace is blank. It doesn't cover the case where you have multiple TLS secrets from different namespaces.

@pablo-ruth pablo-ruth requested a review from a team as a code owner January 14, 2022 16:45
@pablo-ruth pablo-ruth requested review from tsaarni and skriss and removed request for a team January 14, 2022 16:45
@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #4271 (b145460) into main (49e3dcc) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4271      +/-   ##
==========================================
+ Coverage   77.74%   77.82%   +0.08%     
==========================================
  Files         112      112              
  Lines       10014    10020       +6     
==========================================
+ Hits         7785     7798      +13     
+ Misses       2046     2040       -6     
+ Partials      183      182       -1     
Impacted Files Coverage Δ
internal/annotation/annotations.go 100.00% <100.00%> (ø)
internal/dag/ingress_processor.go 97.53% <100.00%> (+4.32%) ⬆️
internal/k8s/objectmeta.go 93.75% <100.00%> (+0.89%) ⬆️

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @pablo-ruth! I think this is a reasonable approach here for giving folks an option to keep using this functionality when moving to Ingress v1, but others with more of the backstory here can chime in as well.

Assuming we go forward with this, I'd also like to see some test cases added to:

  • internal/dag/builder_test.go
  • internal/featuretests/v3/tlscertificatedelegation_test.go

And some docs updates:

internal/annotation/annotations.go Outdated Show resolved Hide resolved
@pablo-ruth pablo-ruth force-pushed the TLSDelegationAnnotation branch 3 times, most recently from 0cc4420 to 865a179 Compare January 18, 2022 16:46
@youngnick youngnick added this to the 1.20.0 milestone Jan 18, 2022
@xaleeks xaleeks added this to 1.20 release (candidates) in Contour Project Board Jan 18, 2022
@sunjayBhatia sunjayBhatia added the release-note/small A small change that needs one line of explanation in the release notes. label Jan 18, 2022
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

i think one unit test to fix, included an Ingress with namespace in the secret name

@ghouscht
Copy link

Looks like a good solution to me. However we went "all-in" with GatewayAPI together with contour in order to be able to update to k8s 1.22. Nevertheless I'd like to see this merged too 👍🏻

pablo-ruth and others added 6 commits January 19, 2022 14:31
Signed-off-by: Pablo Ruth <contact@pablo-ruth.fr>
Signed-off-by: Pablo RUTH <contact@pablo-ruth.fr>
Signed-off-by: Pablo RUTH <contact@pablo-ruth.fr>
Signed-off-by: Pablo RUTH <contact@pablo-ruth.fr>
Signed-off-by: Pablo RUTH <contact@pablo-ruth.fr>
Co-authored-by: Sunjay Bhatia <5337253+sunjayBhatia@users.noreply.github.com>
Signed-off-by: Pablo RUTH <contact@pablo-ruth.fr>
Signed-off-by: Pablo RUTH <contact@pablo-ruth.fr>
@pablo-ruth
Copy link
Contributor Author

Thanks @sunjayBhatia for the fix on the test, I merged it.

As discussed @skriss I added tests on both test files and updated the docs.

@pablo-ruth pablo-ruth changed the title Add support for "projectcontour.io/tls-delegation-namespace" annotation Add support for "projectcontour.io/tls-cert-namespace" annotation Jan 19, 2022
@skriss
Copy link
Member

skriss commented Jan 19, 2022

@pablo-ruth thanks for those updates, they all LGTM. The last thing I see to do here is to add a changelog file, changelogs/unreleased/4271-pablo-ruth-small.md, with a one line description of the change (see https://github.com/projectcontour/contour/blob/main/changelogs/unreleased/small-sample.md). Or if you prefer to write a short paragraph that could include an example, use https://github.com/projectcontour/contour/blob/main/changelogs/unreleased/minor-sample.md and name your file 4271-pablo-ruth-minor.md. Up to you.

Signed-off-by: Pablo RUTH <contact@pablo-ruth.fr>
@skriss skriss merged commit f83a023 into projectcontour:main Jan 19, 2022
@skriss
Copy link
Member

skriss commented Jan 19, 2022

Thanks again @pablo-ruth for contributing this change!

@pablo-ruth pablo-ruth deleted the TLSDelegationAnnotation branch January 19, 2022 17:31
@pablo-ruth
Copy link
Contributor Author

Thanks for your guidance !

skriss added a commit to skriss/contour that referenced this pull request Jan 19, 2022
…tation

Follow-up to projectcontour#4271, updates the ingress.md page
to also describe the new annotation.

Closes projectcontour#4282.

Signed-off-by: Steve Kriss <krisss@vmware.com>
skriss added a commit to skriss/contour that referenced this pull request Jan 19, 2022
Follow-up to projectcontour#4271, updates the ingress.md page
to also describe the new annotation.

Closes projectcontour#4282.

Signed-off-by: Steve Kriss <krisss@vmware.com>
sunjayBhatia pushed a commit that referenced this pull request Jan 20, 2022
Follow-up to #4271, updates the ingress.md page
to also describe the new annotation.

Closes #4282.

Signed-off-by: Steve Kriss <krisss@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
No open projects
Contour Project Board
  
1.20 release (completed)
Development

Successfully merging this pull request may close these issues.

None yet

5 participants