internal/dag: Initial implementation for gateway-api #3278

stevesloka · 2021-01-26T21:13:32Z

Super rough initial implementation for gateway-api.

Needs a lot of love, but at least has started to work while still behind the feature flag in Contour. Using this sample yaml I can get things to route properly:

kind: Gateway
apiVersion: networking.x-k8s.io/v1alpha1
metadata:
  name: contour-gateway
spec:
  gatewayClassName: contour-class
  listeners:
    - protocol: HTTP
      port: 80
      routes:
        kind: HTTPRoute
        selector:
          matchLabels:
            app: filter
        namespaces:
          from: "All"
---
kind: HTTPRoute
apiVersion: networking.x-k8s.io/v1alpha1
metadata:
  name: http-filter-1
  namespace: projectcontour-roots 
  labels:
    app: filter
spec:
  hostnames:
    - local.projectcontour.io
  rules:
    - matches:
      - path:
          type: Prefix
          value: /
      forwardTo:
      - serviceName: rootapp
        port: 80

Updates #2287

Signed-off-by: Steve Sloka slokas@vmware.com

codecov · 2021-01-26T21:17:15Z

Codecov Report

Merging #3278 (f9fa493) into main (1035ddb) will increase coverage by 0.10%.
The diff coverage is 77.27%.

@@            Coverage Diff             @@
##             main    #3278      +/-   ##
==========================================
+ Coverage   75.60%   75.70%   +0.10%     
==========================================
  Files          98       98              
  Lines        6353     6396      +43     
==========================================
+ Hits         4803     4842      +39     
- Misses       1444     1446       +2     
- Partials      106      108       +2

Impacted Files	Coverage Δ
cmd/contour/serve.go	`0.00% <0.00%> (ø)`
internal/dag/serviceapis_processor.go	`83.72% <82.92%> (+83.72%)`	⬆️
internal/k8s/log.go	`69.56% <0.00%> (+6.52%)`	⬆️

stevesloka · 2021-01-27T21:18:08Z

Ok this is a bit better with tests, but I think a reasonable start. It doesn't have any status information which I think would be a good next step to get integrated, but want to limit the changes since it's turning into a large PR.

internal/dag/serviceapis_processor.go

danehans · 2021-02-10T23:49:21Z

internal/dag/serviceapis_processor.go

+	// on the HTTPRoute is set to "FromList".
+	for _, route := range p.source.httproutes {
+		switch route.Spec.Gateways.Allow {
+		case serviceapis.GatewayAllowFromList:


Should case statements be added for "All" and "SameNamespace" allow types? I would expect a route to be precluded from valid if allow: All and no gateways exist in any namespace. I would also expect a route to be precluded from valid if allow: SameNamespace and no gateways exist in the route's namespace.

Let me take this logic out all together, then reference resolution in a new issue? I think that would be best if you're ok with that. (#3351)

internal/dag/serviceapis_processor.go

danehans · 2021-02-11T00:26:02Z

internal/dag/serviceapis_processor.go

+	if len(route.Spec.Hostnames) == 0 {
+		hosts = append(hosts, "*")
+	} else {
+		for _, host := range route.Spec.Hostnames {


I suggest validating each hostname. The first phase of hostname validation can be:

Ensure the hostname is not an IP address. xref: https://golang.org/pkg/net/#ParseIP

Ensure the hostname does not contain a port. xref: https://golang.org/pkg/net/#SplitHostPort

As a second phase, the hostname spec states "Hostname is the fully qualified domain name of a network host, as defined by RFC 3986." We should be able to use https://golang.org/pkg/net/?m=all#isDomainName (verify that it supports wildcard prefixes or augment to support wildcards).

danehans · 2021-02-11T05:37:16Z

internal/dag/serviceapis_processor.go

+			meta := types.NamespacedName{Name: *forward.ServiceName, Namespace: route.Namespace}
+
+			// TODO: Refactor EnsureService to take an int32 so conversion to intstr is not needed.
+			service, err := p.dag.EnsureService(meta, intstr.FromInt(int(forward.Port)), p.source)


Note that if kubernetes-sigs/gateway-api#538 merges, port will be optional. With this PR, when port is unspecified, the implementation should forward the request with the port unmodified. Take a look and comment if this is an issue.

skriss

Handful of initial comments. This looks like a good start on MVP functionality. In general, I'd say let's just be explicit about which subset of functionality we're supporting as we go, and logging errors/not processing config that we can't support yet.

skriss · 2021-02-11T18:44:32Z

internal/dag/serviceapis_processor.go

+			case serviceapis.PathMatchPrefix:
+				pathPrefixes = append(pathPrefixes, stringOrDefault(match.Path.Value, "/"))
+			default:
+				p.Error("NOT IMPLEMENTED: Only PathMatchPrefix is currently implemented.")


should we skip the entire rule if we can't parse one of the matches? (I'm not totally sure)

There's an exact match as well which Contour doesn't have support for yet. I was trying to not get backed into the current HTTPProxy issue of one error makes the entire resource invalid.

These are places where we need to raise status condition errors. I think we should try and not throw away the entire resource for things that are either not implemented yet or are in error (ref #3145).

internal/dag/serviceapis_processor.go

skriss · 2021-02-11T18:55:54Z

internal/dag/serviceapis_processor.go

+}
+
+func (p *ServiceAPIsProcessor) computeHTTPRoute(route *serviceapis.HTTPRoute) {
+


we should probably check for TLS config and error/not process the route if it's set, since we don't support it yet.

internal/dag/serviceapis_processor.go

skriss · 2021-02-11T19:00:49Z

internal/dag/serviceapis_processor.go

+// httpRoutes returns a slice of *serviceapis.HTTPRoute objects.
+// invalid HTTPRoute objects are excluded from the slice and their status
+// updated accordingly.
+func (p *ServiceAPIsProcessor) validHTTPRoutes() []*serviceapis.HTTPRoute {


I think limiting the routes we process to only those that map to this Contour's gateway is probably essential functionality for an MVP here - we have the config option in the config file, so should be able to wire that through.

I opened #3351 to track this. Do you think it's ok to merge this as-is, then follow up with that? I do understand that right now we'll process ALL gateways. =)

#3278 (comment)

youngnick

Couple of small things from me, but this looks okay as a first pass. Definitely plenty of work to do still though.

youngnick · 2021-02-15T02:22:30Z

internal/dag/serviceapis_processor.go

+
+	for _, rule := range route.Spec.Rules {
+
+		// Paths are a set of paths that are


Comment needs finishing up?

youngnick · 2021-02-15T02:26:21Z

internal/dag/builder_test.go

 )

+func TestDAGInsertServiceAPIs(t *testing.T) {
+
+	svc1 := &v1.Service{


I'd recommend calling these kuardSvc and blogSvc respectively, that saves one lookup when you're writing more tests.

jmprusi · 2021-02-15T10:49:57Z

internal/dag/serviceapis_processor.go

 // ServiceAPIsProcessor translates Service API types into DAG
 // objects and adds them to the DAG.
 type ServiceAPIsProcessor struct {
+	logrus.FieldLogger


All the usages of the FieldLogger (p.Error,p.Errorf...) panic, as it seems to not be initialized (nil).

👍 I added this to the init of the processor.

sunjayBhatia

Looks good to me!

On testing, left a comment about adding a new issue to track the test coverage we will need

Also, since the conformance suite is not ready, should we add some basic integration tests that we can throw away later once we can start contributing upstream? Worth adding an issue for that?

sunjayBhatia · 2021-02-17T00:40:00Z

internal/dag/builder_test.go

 )

+func TestDAGInsertServiceAPIs(t *testing.T) {


Worth adding tests (or maybe a tracking issue for adding tests) around the interactions between different resources (HTTPProxy, Ingress, Gateway APIs etc.)

sunjayBhatia · 2021-02-17T00:47:41Z

internal/dag/serviceapis_processor.go

+		for _, forward := range rule.ForwardTo {
+			// Verify the service is valid
+			if forward.ServiceName == nil {
+				p.Error("ServiceName must not specified and is currently only implemented!")


nit: Should this error be reworded?

haha yup! 👍

Super rough initial implementation for service-apis. Updates projectcontour#2287 Signed-off-by: Steve Sloka <slokas@vmware.com>

…#3278) Super rough initial implementation for service-apis. Updates projectcontour#2287 Signed-off-by: Steve Sloka <slokas@vmware.com> Signed-off-by: iyacontrol <gaohj2015@yeah.net>

stevesloka force-pushed the HTTPRoutes branch from f0ed4bc to 976a877 Compare January 27, 2021 21:16

stevesloka marked this pull request as ready for review January 27, 2021 21:16

stevesloka requested a review from a team as a code owner January 27, 2021 21:16

stevesloka requested review from jpeach, sunjayBhatia, skriss and youngnick and removed request for a team January 27, 2021 21:16

youngnick added this to the 1.13.0 milestone Feb 1, 2021

danehans reviewed Feb 10, 2021

View reviewed changes

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

danehans reviewed Feb 10, 2021

View reviewed changes

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

danehans reviewed Feb 11, 2021

View reviewed changes

youngnick assigned stevesloka Feb 11, 2021

danehans reviewed Feb 11, 2021

View reviewed changes

sunjayBhatia added this to In progress in Contour Project Board via automation Feb 11, 2021

sunjayBhatia moved this from In progress to 1.13 Release in Contour Project Board Feb 11, 2021

skriss requested changes Feb 11, 2021

View reviewed changes

This was referenced Feb 11, 2021

Implement Gateway.AllowType in Processor #3351

Closed

Validation for hostnames #3353

Closed

stevesloka force-pushed the HTTPRoutes branch from ef62b34 to d643389 Compare February 11, 2021 20:58

youngnick approved these changes Feb 15, 2021

View reviewed changes

jmprusi reviewed Feb 15, 2021

View reviewed changes

stevesloka force-pushed the HTTPRoutes branch 2 times, most recently from ffe5e62 to 95ac280 Compare February 16, 2021 18:26

stevesloka requested review from skriss and danehans February 16, 2021 18:38

sunjayBhatia approved these changes Feb 17, 2021

View reviewed changes

stevesloka mentioned this pull request Feb 17, 2021

Tests the interactions between different resources (HTTPProxy, Ingress, Gateway APIs etc.) #3367

Open

internal/dag: Initial implementation for service-apis

7a7b12c

Super rough initial implementation for service-apis. Updates projectcontour#2287 Signed-off-by: Steve Sloka <slokas@vmware.com>

stevesloka force-pushed the HTTPRoutes branch from fa07e05 to 7a7b12c Compare February 17, 2021 15:38

stevesloka merged commit 1afeff9 into projectcontour:main Feb 17, 2021

stevesloka deleted the HTTPRoutes branch February 17, 2021 16:19

stevesloka changed the title ~~internal/dag: Initial implementation for service-apis~~ internal/dag: Initial implementation for gateway-api Feb 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/dag: Initial implementation for gateway-api #3278

internal/dag: Initial implementation for gateway-api #3278

stevesloka commented Jan 26, 2021 •

edited

codecov bot commented Jan 26, 2021 •

edited

stevesloka commented Jan 27, 2021

danehans Feb 10, 2021

stevesloka Feb 11, 2021

danehans Feb 11, 2021

stevesloka Feb 11, 2021

danehans Feb 11, 2021

stevesloka Feb 11, 2021

skriss left a comment •

edited

skriss Feb 11, 2021

stevesloka Feb 11, 2021

stevesloka Feb 11, 2021

skriss Feb 11, 2021

stevesloka Feb 11, 2021

skriss Feb 11, 2021

stevesloka Feb 11, 2021

stevesloka Feb 11, 2021

youngnick left a comment

youngnick Feb 15, 2021

youngnick Feb 15, 2021

jmprusi Feb 15, 2021 •

edited

stevesloka Feb 16, 2021

sunjayBhatia left a comment

sunjayBhatia Feb 17, 2021

stevesloka Feb 17, 2021

sunjayBhatia Feb 17, 2021

stevesloka Feb 17, 2021

		}

		func (p ServiceAPIsProcessor) computeHTTPRoute(route serviceapis.HTTPRoute) {


		for _, rule := range route.Spec.Rules {

		// Paths are a set of paths that are

internal/dag: Initial implementation for gateway-api #3278

internal/dag: Initial implementation for gateway-api #3278

Conversation

stevesloka commented Jan 26, 2021 • edited

codecov bot commented Jan 26, 2021 • edited

Codecov Report

stevesloka commented Jan 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmprusi Feb 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevesloka commented Jan 26, 2021 •

edited

codecov bot commented Jan 26, 2021 •

edited

skriss left a comment •

edited

jmprusi Feb 15, 2021 •

edited