internal: Add support for watching ingress/v1 resources #3266

stevesloka · 2021-01-20T19:10:33Z

Add support for watching networking.k8s.io/v1 Ingress resources and add/remove
from internal Cache later allowing these objects to be implemented.

Signed-off-by: Steve Sloka [email protected]

codecov · 2021-01-20T19:14:38Z

Codecov Report

Merging #3266 (037a964) into main (721652f) will decrease coverage by 0.12%.
The diff coverage is 73.52%.

@@            Coverage Diff             @@
##             main    #3266      +/-   ##
==========================================
- Coverage   75.43%   75.30%   -0.13%     
==========================================
  Files          98       98              
  Lines        6162     6249      +87     
==========================================
+ Hits         4648     4706      +58     
- Misses       1409     1436      +27     
- Partials      105      107       +2

Impacted Files	Coverage Δ
cmd/contour/serve.go	`0.00% <0.00%> (ø)`
internal/annotation/annotations.go	`100.00% <ø> (ø)`
internal/dag/policy.go	`96.50% <ø> (ø)`
internal/k8s/informers.go	`0.00% <0.00%> (ø)`
internal/dag/cache.go	`90.96% <77.33%> (-4.24%)`	⬇️
internal/dag/ingress_processor.go	`93.85% <100.00%> (+0.40%)`	⬆️
internal/k8s/log.go	`63.04% <0.00%> (-6.53%)`	⬇️

stevesloka · 2021-01-20T19:46:35Z

Please hold on this for a second, I'm going to add IngressClass as well which is needed along with ingress/v1.

internal/k8s/informers.go

stevesloka · 2021-01-21T16:05:50Z

Integration tests are failing because of RBAC. =)

cmd/contour/serve.go

internal/dag/cache.go

internal/dag/ingress_processor.go

stevesloka · 2021-01-22T14:59:46Z

Ok turns out testing against a kind v1.16.4 cluster (image: kindest/node:v1.16.4@sha256:b91a2c2317a000f3a783489dfb755064177dbc3a0b2f4147d50f04825d016f55) doesn't let Envoy connect to Contour, but using a v1.16.9 image (kindest/node:v1.16.9@sha256:7175872357bc85847ec4b1aba46ed1d12fa054c83ac7a8a11f5c268957fd5765) does.

I was down a rabbit hole chasing the wrong issue. =)

youngnick

This is looking good! I like explicitly translating v1beta1 Ingress objects into v1 objects internally, that makes things easier.

stevesloka · 2021-01-25T15:47:53Z

@skriss I updated with your comments in the initial review. The method now is I have a converter which turns all v1beta1 ingress objects into v1 ingress types. From that point, all the processing is the same.

skriss

@stevesloka I think this is looking pretty good. Looks like it'll address a few of the sub-issues listed in #2135 (comment), and then we'll likely have to follow up on a few more.

internal/dag/cache.go

internal/dag/ingress_processor.go

internal/dag/cache.go

skriss

couple more comments on the converter function

internal/dag/cache.go

skriss · 2021-01-25T20:47:05Z

internal/dag/builder_test.go

@@ -104,154 +105,185 @@ func TestDAGInsert(t *testing.T) {
 		},
 	}

-	i1 := &v1beta1.Ingress{
+	i1V1 := &networking_v1.Ingress{


this file's diff is pretty impossible to review - can you describe the changes? I think you said you were creating a second copy of all the Ingress tests, that used a v1 Ingress instead of a v1beta1, so we'd have tests for both the v1beta1 and the v1 representations.

Yeah it's unfortunate that the diff is so hard to read, but I copied all the v1beta1 ingress resources into networking_v1 ingress resources and corresponding tests so that we could test the v1beta1 resources along with the networking_v1 resources.

In the next set of PRs, we should expand these tests with the new features in v1 (e.g. ingressclass, path type, etc)

skriss

This looks pretty good to me. Do you want to hold it until after we release 1.12, so we can get a few extra test cycles in on it (along with the additional items for full v1 support)?

stevesloka · 2021-01-25T21:20:55Z

This looks pretty good to me. Do you want to hold it until after we release 1.12, so we can get a few extra test cycles in on it (along with the additional items for full v1 support)?

Yup we can hold, this PR doesn't add anything to the release at the moment.

internal/dag/cache.go

sunjayBhatia · 2021-01-25T22:03:11Z

This looks pretty good to me. Do you want to hold it until after we release 1.12, so we can get a few extra test cycles in on it (along with the additional items for full v1 support)?

+1 just a small comment

youngnick

This LGTM, but agree that we should hold until after 1.12 releases.

skriss · 2021-02-03T02:14:23Z

ready for a rebase & merge

skriss · 2021-02-03T15:45:23Z

Needs a make generate.

stevesloka · 2021-02-03T15:58:18Z

Needs a make generate.

Yup! Just did that and pushed, should work now.

Add support for watching networking.k8s.io/v1 Ingress & IngressClass resources and add/remove from internal cache later allowing these objects to be implemented. Signed-off-by: Steve Sloka <[email protected]>

sunjayBhatia

I might be missing where the logic for this is, but do we have coverage for when an IngressClass object is removed? Should we be removing all the Ingress objects that reference that IngressClass from the cache etc.?

I was initially going to say the above but read through #2146 before clicking submit. Given we agreed to add all Ingress objects to the cache and only add them to the DAG if they apply, this LGTM

stevesloka · 2021-02-03T16:12:58Z

I'm trying to make smaller commits for this stuff so it's easier to read. Now that we have the plumbing to get the objects, we can now operate against them. Thanks for the reviews @skriss & @sunjayBhatia! 🎉

stevesloka · 2021-02-03T16:14:03Z

@skriss are you good with the converter bits? There were changes requested so wanted to double-check.

skriss

LGTM!

stevesloka requested a review from a team as a code owner January 20, 2021 19:10

stevesloka requested review from sunjayBhatia, youngnick and skriss and removed request for a team January 20, 2021 19:10

skriss reviewed Jan 20, 2021

View reviewed changes

internal/k8s/informers.go Show resolved Hide resolved

stevesloka force-pushed the watchIngressv1 branch from 39e64ba to f40e557 Compare January 21, 2021 03:29

stevesloka added this to the 1.12.0 milestone Jan 21, 2021

skriss reviewed Jan 21, 2021

View reviewed changes

stevesloka force-pushed the watchIngressv1 branch from e278946 to 01e89be Compare January 22, 2021 17:33

stevesloka marked this pull request as draft January 25, 2021 01:39

youngnick reviewed Jan 25, 2021

View reviewed changes

stevesloka force-pushed the watchIngressv1 branch from 0d29616 to 1168405 Compare January 25, 2021 15:41

stevesloka force-pushed the watchIngressv1 branch 2 times, most recently from d71bf23 to 744112c Compare January 25, 2021 15:56

stevesloka marked this pull request as ready for review January 25, 2021 15:56

skriss reviewed Jan 25, 2021

View reviewed changes

internal/dag/cache.go Show resolved Hide resolved

internal/dag/ingress_processor.go Show resolved Hide resolved

internal/dag/cache.go Show resolved Hide resolved

skriss requested changes Jan 25, 2021

View reviewed changes

internal/dag/cache.go Show resolved Hide resolved

internal/dag/cache.go Outdated Show resolved Hide resolved

internal/dag/cache.go Outdated Show resolved Hide resolved

internal/dag/cache.go Show resolved Hide resolved

stevesloka force-pushed the watchIngressv1 branch from 231c9f5 to 42307d6 Compare January 25, 2021 20:23

stevesloka requested a review from skriss January 25, 2021 20:33

skriss reviewed Jan 25, 2021

View reviewed changes

sunjayBhatia requested changes Jan 25, 2021

View reviewed changes

internal/dag/cache.go Show resolved Hide resolved

stevesloka force-pushed the watchIngressv1 branch from 42307d6 to 83db520 Compare January 26, 2021 16:19

youngnick approved these changes Jan 26, 2021

View reviewed changes

youngnick modified the milestones: 1.12.0, 1.13.0 Jan 29, 2021

stevesloka force-pushed the watchIngressv1 branch from 83db520 to 9f6d94e Compare February 3, 2021 13:22

stevesloka requested review from skriss, youngnick and sunjayBhatia February 3, 2021 13:25

internal: Add support for watching ingress/v1 resources

82a6973

Add support for watching networking.k8s.io/v1 Ingress & IngressClass resources and add/remove from internal cache later allowing these objects to be implemented. Signed-off-by: Steve Sloka <[email protected]>

stevesloka force-pushed the watchIngressv1 branch 2 times, most recently from 9f6d94e to 556d138 Compare February 3, 2021 16:04

sunjayBhatia approved these changes Feb 3, 2021

View reviewed changes

skriss approved these changes Feb 3, 2021

View reviewed changes

skriss merged commit 6a288ab into projectcontour:main Feb 3, 2021

stevesloka deleted the watchIngressv1 branch February 3, 2021 16:57

danehans mentioned this pull request Feb 3, 2021

Updates RABC for IngressClass and Service APIs projectcontour/contour-operator#193

Merged

sunjayBhatia mentioned this pull request Feb 10, 2021

Ingress V1: support for service backends #2137

Closed

skriss mentioned this pull request Feb 10, 2021

Ingress V1: support spec.backend field rename #2136

Closed

enyo mentioned this pull request Mar 17, 2021

Contour helm chart needs to include ingressclasses bitnami/charts#5824

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal: Add support for watching ingress/v1 resources #3266

internal: Add support for watching ingress/v1 resources #3266

stevesloka commented Jan 20, 2021

codecov bot commented Jan 20, 2021 •

edited

Loading

stevesloka commented Jan 20, 2021

stevesloka commented Jan 21, 2021

stevesloka commented Jan 22, 2021

youngnick left a comment

stevesloka commented Jan 25, 2021

skriss left a comment

skriss left a comment

skriss Jan 25, 2021

stevesloka Jan 25, 2021

skriss left a comment

stevesloka commented Jan 25, 2021

sunjayBhatia commented Jan 25, 2021

youngnick left a comment

skriss commented Feb 3, 2021

skriss commented Feb 3, 2021

stevesloka commented Feb 3, 2021

sunjayBhatia left a comment

stevesloka commented Feb 3, 2021

stevesloka commented Feb 3, 2021

skriss left a comment

internal: Add support for watching ingress/v1 resources #3266

internal: Add support for watching ingress/v1 resources #3266

Conversation

stevesloka commented Jan 20, 2021

codecov bot commented Jan 20, 2021 • edited Loading

Codecov Report

stevesloka commented Jan 20, 2021

stevesloka commented Jan 21, 2021

stevesloka commented Jan 22, 2021

youngnick left a comment

Choose a reason for hiding this comment

stevesloka commented Jan 25, 2021

skriss left a comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

skriss Jan 25, 2021

Choose a reason for hiding this comment

stevesloka Jan 25, 2021

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

stevesloka commented Jan 25, 2021

sunjayBhatia commented Jan 25, 2021

youngnick left a comment

Choose a reason for hiding this comment

skriss commented Feb 3, 2021

skriss commented Feb 3, 2021

stevesloka commented Feb 3, 2021

sunjayBhatia left a comment

Choose a reason for hiding this comment

stevesloka commented Feb 3, 2021

stevesloka commented Feb 3, 2021

skriss left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 20, 2021 •

edited

Loading