Add gloo tests #3087

ilackarms · 2019-02-04T15:41:12Z

For context, Gloo, the Envoy-based Kubernetes Ingress / API Gateway can now serve as a clusteringress controller for knative-serving.

Proposed Changes

Add a set of configuration manifests for knative that support running without Istio
Add a deployment manifest for Gloo 0.6.16 to third_party resources
Add a path in the e2e setup scripts to install Gloo instead of Istio before running tests
Allow hard-coded references to the Istio gateway to be overridden using an environment variable

Release Note

NONE

knative-prow-robot · 2019-02-04T15:41:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ilackarms
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mdemirhan

If they are not already assigned, you can assign the PR to them by writing /assign @mdemirhan in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow-robot

@ilackarms: 0 warnings.

In response to this:

For context, Gloo, the Envoy-based Kubernetes Ingress / API Gateway can now serve as a clusteringress controller for knative-serving.

Proposed Changes

Add a set of configuration manifests for knative that support running without Istio

Add a deployment manifest for Gloo 0.6.16 to third_party resources

Add a path in the e2e setup scripts to install Gloo instead of Istio before running tests

Allow hard-coded references to the Istio gateway to be overridden using an environment variable

Release Note
NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ilackarms · 2019-02-04T15:48:29Z

this pr also depends on knative/pkg#260

vagababov · 2019-02-04T17:54:55Z

/ok-to-test

vagababov · 2019-02-04T17:52:51Z

config-no-istio/100-namespace.yaml

@@ -0,0 +1,20 @@
+# Copyright 2018 The Knative Authors


Suggested change

# Copyright 2018 The Knative Authors

# Copyright 2019 The Knative Authors

Same in other places.

tcnghia · 2019-02-06T18:12:36Z

/test pull-knative-serving-unit-tests

ilackarms · 2019-02-08T19:36:19Z

@tcnghia i'm seeeing test failures, one seems due to a flake but the other seems like an issue with a vendored dependency:

github.com/knative/serving/vendor/k8s.io/client-go/plugin/pkg/client/auth/gcp
# github.com/knative/serving/cmd/queue
cmd/queue/main.go:283:50: not enough arguments in call to websocket.NewDurableSendingConnection
 	have (string)
 	want (string, *zap.SugaredLogger)

ilackarms · 2019-02-08T20:15:11Z

compile err was caused by the new version of knative/pkg. fixed the compile issue by updating main

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

ilackarms · 2019-02-13T16:29:00Z

/retest

markusthoemmes

Suprisingly simple I have to say! I don't like all the duplication of our config YAMLs though, can we do better there? These will eventually rot I fear.

/assign @adrcunha
For some opinions on how to structure this in the tests.

markusthoemmes · 2019-02-26T07:18:28Z

config-no-istio/100-namespace.yaml

@@ -0,0 +1,20 @@
+# Copyright 2019 The Knative Authors


Can you comment on the diff of all of these config-no-istio files with their config counterparts? This seems like a lot of duplication to me. Also config-no-istio should be config-gloo, shouldn't it?

I agree that there's a lot of duplication in config-no-istio (which i'll rename as suggested). The challenge here is how to modify the existing config dir which has dependencies on Istio that aren't needed for the Gloo tests. the changes in the config-no-istio dir include:

removing any references to istio crds

removing any references to the istio-system namespace

Without some kind of chart templating system like Helm, it's hard to reuse the original manifest. I agree that a modified duplicate of the config dir is unsuitable here.

One possibility is to run a simple script that will parse the finished serving.yaml manifest and spit out a serving-gloo.yaml with all Istio references removed.

Another is, rather than removing references to Istio from the default serving manifest, instead, simply pre-create the istio-system namespace and register Istio crds before applying the serving.yaml in the test setup.

Other than these two options, I'm not sure the best way to move forward with this part of the PR.

a third option is to remove the alternate config path for Knative. always install Knative-Serving with Istio. Enabling Gloo for the e2e tests will not avoid deploying Istio, but instead simply ignore the fact that Istio is deployed. This will obviate the need to have an alternate deployment manifest for knative and remove all this duplication.

@ilackarms after #2445 checks in you can install Knative without any Istio support by ignoring components with the label networking.knative.dev/ingress-provider: istio.

markusthoemmes · 2019-02-26T07:21:42Z

test/e2e-tests.sh

+  export GATEWAY_NAMESPACE_OVERRIDE="gloo-system"
+else
+      install_knative_serving || fail_test "Knative Serving installation failed"
+fi


Fix formatting please.

markusthoemmes · 2019-02-26T07:24:12Z

test/e2e/helloworld_shell_test.go

+	}
+	if gatewayNsOverride := os.Getenv("GATEWAY_NAMESPACE_OVERRIDE"); gatewayNsOverride != "" {
+		gatewayNamespace = gatewayNsOverride
+	}


Can we make these test flags to pass to the tests? We use flags to override values elsewhere and we could just add -gateway and -gatewayNamespace (without override) and default them to the istio ones for now. WDYT?

fixed, but note that https://github.com/knative/pkg/blob/master/test/spoof/spoof.go#L133 still requires these env vars to be set

adrcunha · 2019-02-26T07:35:58Z

test/e2e/helloworld_shell_test.go

+	}
+	if gatewayNsOverride := os.Getenv("GATEWAY_NAMESPACE_OVERRIDE"); gatewayNsOverride != "" {
+		gatewayNamespace = gatewayNsOverride
+	}


adrcunha · 2019-02-26T07:39:26Z

test/e2e-common.sh

@@ -209,4 +281,5 @@ function publish_test_images() {
 # Deletes everything created on the cluster including all knative and istio components.
 function teardown() {
  uninstall_knative_serving
+  uninstall_knative_serving_gloo_version


Remove (see my comment below).

adrcunha · 2019-02-26T07:40:10Z

test/e2e-tests.sh

@@ -57,7 +57,15 @@ initialize $@

 header "Setting up environment"

-install_knative_serving || fail_test "Knative Serving installation failed"
+if [[ "${RUN_GLOO_TESTS}" -eq "1" ]]; then


Use a flag instead of an environment variable, it's better. Update parse_flags() in e2e-common.sh.

install_knative_serving_gloo_version looks like a copy of install_knative_serving with a few changes. It seems to me that you could call install_knative_serving with a few changes or parameters and make it install Gloo instead of Istio. Same comment about the uninstall function. They are already complex enough, so I want to avoid duplicating them as much as possible.

Signed-off-by: soloio-bot <sdw35@cornell.edu>

adrcunha · 2019-03-20T16:43:40Z

Please fix the conflicts and address the bot's comments.

Co-Authored-By: ilackarms <sdw35@cornell.edu>

mattmoor-sockpuppet · 2019-03-21T12:13:07Z

test/config-gloo/400-settings-crd.yaml

@@ -0,0 +1,30 @@
+# Copyright 2018 The Knative Authors


Suggested change

# Copyright 2018 The Knative Authors

# Copyright 2019 The Knative Authors

mattmoor-sockpuppet · 2019-03-21T12:13:14Z

config-no-istio/config-logging.yaml

+        "callerEncoder": ""
+      }
+    }
+


Suggested change

mattmoor-sockpuppet · 2019-03-21T12:13:14Z

config-no-istio/config-logging.yaml

+    }
+
+  # Log level overrides
+  # For all components except the autoscaler and queue proxy, 


Suggested change

# For all components except the autoscaler and queue proxy,

# For all components except the autoscaler and queue proxy,

mattmoor-sockpuppet · 2019-03-21T12:13:14Z

config-no-istio/config-network.yaml

+  # Replace this with the IP ranges of your cluster (see below for some examples).
+  # Separate multiple entries with a comma.
+  # Example: "10.4.0.0/14,10.7.240.0/20"
+  # 


Suggested change

#

#

mattmoor-sockpuppet · 2019-03-21T12:13:14Z

config-no-istio/config-network.yaml

+  # deployment, global.proxy.includeIPRanges value is set to "*".
+  #
+  # If an invalid value is passed, "" is used instead.
+  # 


Suggested change

#

#

mattmoor-sockpuppet · 2019-03-21T12:13:15Z

config-no-istio/config-network.yaml

+  # Istio will no longer intercept traffic going to IP addresses
+  # outside the provided ranges and there is no need to specify
+  # egress rules.
+  # 


Suggested change

#

#

mattmoor-sockpuppet · 2019-03-21T12:13:15Z

config-no-istio/config-network.yaml

+  #   Azure Container Service (ACS; deprecated): "10.244.0.0/16,10.240.0.0/16"
+  #   Azure Container Service Engine (ACS-Engine; OSS): Configurable, but defaults to "10.0.0.0/16"
+  #   Minikube: "10.0.0.1/24"
+  # 


Suggested change

#

#

mattmoor-sockpuppet · 2019-03-21T12:13:15Z

config-no-istio/config-observability.yaml

+
+  # metrics.backend-destination field specifies the system metrics destination.
+  # It defaults to prometheus. If this is stackdriver, the metrics will be sent
+  # to stackdriver. "prometheus" and "stackdriver" are supported. This field 


Suggested change

# to stackdriver. "prometheus" and "stackdriver" are supported. This field

# to stackdriver. "prometheus" and "stackdriver" are supported. This field

mattmoor-sockpuppet · 2019-03-21T12:13:15Z

config-no-istio/config-observability.yaml

+  # field is optional. When running on GKE, application default credentials will be
+  # used if this field is not provided.
+  # Note: Using stackdriver will incur additional charges
+  # metrics.stackdriver-project-id: "<your stackdriver project id>" 


Suggested change

# metrics.stackdriver-project-id: "<your stackdriver project id>"

# metrics.stackdriver-project-id: "<your stackdriver project id>"

mattmoor-sockpuppet · 2019-03-21T12:13:15Z

test/config-gloo/gloo-settings.yaml

+  - knative-testing
+  - kube-public
+  - kube-system
+  - serving-tests


Suggested change

- serving-tests

- serving-tests

mattmoor-sockpuppet · 2019-03-21T12:13:20Z

config-no-istio/config-logging.yaml

+        "callerEncoder": ""
+      }
+    }
+


Suggested change

mattmoor-sockpuppet · 2019-03-21T12:13:20Z

config-no-istio/config-logging.yaml

+    }
+
+  # Log level overrides
+  # For all components except the autoscaler and queue proxy, 


Suggested change

# For all components except the autoscaler and queue proxy,

# For all components except the autoscaler and queue proxy,

mattmoor-sockpuppet · 2019-03-21T12:13:20Z

config-no-istio/config-network.yaml

+  # Replace this with the IP ranges of your cluster (see below for some examples).
+  # Separate multiple entries with a comma.
+  # Example: "10.4.0.0/14,10.7.240.0/20"
+  # 


Suggested change

#

#

mattmoor-sockpuppet · 2019-03-21T12:13:20Z

config-no-istio/config-network.yaml

+  # deployment, global.proxy.includeIPRanges value is set to "*".
+  #
+  # If an invalid value is passed, "" is used instead.
+  # 


Suggested change

#

#

mattmoor-sockpuppet · 2019-03-21T12:13:20Z

config-no-istio/config-network.yaml

+  # Istio will no longer intercept traffic going to IP addresses
+  # outside the provided ranges and there is no need to specify
+  # egress rules.
+  # 


Suggested change

#

#

mattmoor-sockpuppet · 2019-03-21T12:13:21Z

config-no-istio/config-network.yaml

+  #   Azure Container Service (ACS; deprecated): "10.244.0.0/16,10.240.0.0/16"
+  #   Azure Container Service Engine (ACS-Engine; OSS): Configurable, but defaults to "10.0.0.0/16"
+  #   Minikube: "10.0.0.1/24"
+  # 


Suggested change

#

#

mattmoor-sockpuppet · 2019-03-21T12:13:21Z

config-no-istio/config-observability.yaml

+
+  # metrics.backend-destination field specifies the system metrics destination.
+  # It defaults to prometheus. If this is stackdriver, the metrics will be sent
+  # to stackdriver. "prometheus" and "stackdriver" are supported. This field 


Suggested change

# to stackdriver. "prometheus" and "stackdriver" are supported. This field

# to stackdriver. "prometheus" and "stackdriver" are supported. This field

mattmoor-sockpuppet · 2019-03-21T12:13:21Z

config-no-istio/config-observability.yaml

+  # field is optional. When running on GKE, application default credentials will be
+  # used if this field is not provided.
+  # Note: Using stackdriver will incur additional charges
+  # metrics.stackdriver-project-id: "<your stackdriver project id>" 


Suggested change

# metrics.stackdriver-project-id: "<your stackdriver project id>"

# metrics.stackdriver-project-id: "<your stackdriver project id>"

mattmoor-sockpuppet · 2019-03-21T12:13:21Z

test/config-gloo/gloo-settings.yaml

+  - knative-testing
+  - kube-public
+  - kube-system
+  - serving-tests


Suggested change

- serving-tests

- serving-tests

knative-prow-robot · 2019-03-21T12:14:33Z

@ilackarms: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-serving-go-coverage	`d209014`	link	`/test pull-knative-serving-go-coverage`
pull-knative-serving-build-tests	`d209014`	link	`/test pull-knative-serving-build-tests`
pull-knative-serving-unit-tests	`d209014`	link	`/test pull-knative-serving-unit-tests`
pull-knative-serving-integration-tests	`d209014`	link	`/test pull-knative-serving-integration-tests`
pull-knative-serving-upgrade-tests	`d209014`	link	`/test pull-knative-serving-upgrade-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

markusthoemmes · 2019-07-01T07:48:36Z

This has become vastly outdated. @ilackarms is this still a valid PR? Are you planning to pick it up again?

ilackarms · 2019-07-01T16:47:37Z

@markusthoemmes i'm closing this one in favor of a new one i've been working on. will be opening that shortly

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 4, 2019

knative-prow-robot requested review from ericKlawitter, markusthoemmes and srinivashegde86 February 4, 2019 15:41

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 4, 2019

knative-prow-robot reviewed Feb 4, 2019

View reviewed changes

ilackarms force-pushed the add-gloo-tests branch from b7fb5eb to dc856ac Compare February 4, 2019 15:53

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 4, 2019

vagababov reviewed Feb 4, 2019

View reviewed changes

ilackarms force-pushed the add-gloo-tests branch from a0fb396 to 963c763 Compare February 6, 2019 15:46

ilackarms force-pushed the add-gloo-tests branch from 3c532a4 to ec0ef17 Compare February 8, 2019 19:19

ilackarms force-pushed the add-gloo-tests branch from a5f7847 to 794a013 Compare February 8, 2019 20:13

ilackarms added 12 commits February 12, 2019 10:39

add setup function for gloo style test

d228e7d

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

new config dir for gloo-style istio deploy

82d4abc

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

fix scripts

1097615

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

fix e2e test to work with both istio and gloo

7bd69d5

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

use override from env

4326c00

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

add necessary config yamls for test and no-istio release

2792ef9

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

add thirdparty yaml for gloo

b490cc9

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

use official image versions

2acf42d

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

update copyright date

70ef82f

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

uppdate dep with fork

00e4a11

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

run ./hack/update-codegen.sh

8aafe02

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

dep

d51ad49

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

ilackarms added 5 commits February 12, 2019 10:40

update codegen.sh

7e4bbb2

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

separate out gloo configs

d5af843

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

dep

128cc35

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

fix compile err

f393b99

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

re-dep

6e55850

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

ilackarms force-pushed the add-gloo-tests branch from ce52313 to 6e55850 Compare February 12, 2019 15:41

update codegen

d177aef

Signed-off-by: Scott Weiss <sdw35@cornell.edu>

markusthoemmes reviewed Feb 26, 2019

View reviewed changes

knative-prow-robot assigned adrcunha Feb 26, 2019

adrcunha suggested changes Feb 26, 2019

View reviewed changes

adrcunha removed request for srinivashegde86 and ericKlawitter February 26, 2019 07:40

fix formatting, replace env var with flag

20dd925

Signed-off-by: soloio-bot <sdw35@cornell.edu>

mattmoor-sockpuppet reviewed Feb 28, 2019

View reviewed changes

mattmoor-sockpuppet and others added 2 commits March 21, 2019 08:12

Update config-no-istio/config-network.yaml

5682e70

Co-Authored-By: ilackarms <sdw35@cornell.edu>

Update config-no-istio/config-network.yaml

d209014

Co-Authored-By: ilackarms <sdw35@cornell.edu>

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Mar 21, 2019

mattmoor-sockpuppet reviewed Mar 21, 2019

View reviewed changes

ilackarms closed this Jul 1, 2019

ilackarms mentioned this pull request Jul 10, 2019

Gloo e2e tests #4704

Merged

	# Copyright 2018 The Knative Authors
	# Copyright 2019 The Knative Authors

	# For all components except the autoscaler and queue proxy,
	# For all components except the autoscaler and queue proxy,

	# to stackdriver. "prometheus" and "stackdriver" are supported. This field
	# to stackdriver. "prometheus" and "stackdriver" are supported. This field

	# metrics.stackdriver-project-id: "<your stackdriver project id>"
	# metrics.stackdriver-project-id: "<your stackdriver project id>"

Add gloo tests #3087

Add gloo tests #3087

Conversation

ilackarms commented Feb 4, 2019

Proposed Changes

knative-prow-robot commented Feb 4, 2019

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

ilackarms commented Feb 4, 2019

vagababov commented Feb 4, 2019

Choose a reason for hiding this comment

tcnghia commented Feb 6, 2019

ilackarms commented Feb 8, 2019

ilackarms commented Feb 8, 2019

ilackarms commented Feb 13, 2019

markusthoemmes 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

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

adrcunha commented Mar 20, 2019

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

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

Choose a reason for hiding this comment

knative-prow-robot commented Mar 21, 2019

markusthoemmes commented Jul 1, 2019

ilackarms commented Jul 1, 2019