⬆️ Bump Gateway API version to v0.5.0-rc1 #315

carlisia · 2022-06-28T03:13:06Z

Closes #306.
Closes #292.

Upgraded Gateway API import paths + deps to v0.5.0-rc1
Updated CRDs, config, instructions + point to local CRDs
ParentRef -> ParentReference
ConditionRouteAccepted -> RouteConditionAccepted
Refactored out test configuration for different vendors into a common setup file (setup-and-deploy.sh)
improved README and DEVELOPMENT documentation

Incoming in a separate PR(s):

bump Istio version (gateway-api: upgrade to v0.5.0-rc1 by howardjohn · Pull Request #39506 · istio/istio)
bump Contour version
use Gateway API v1beta1 for Gateway, GatewayClass and HTTPRoute

knative-prow · 2022-06-28T03:13:08Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

carlisia · 2022-06-28T03:59:24Z

/assign @nak3

README.md

go.mod

carlisia · 2022-06-28T19:40:49Z

/retest

codecov · 2022-06-28T21:33:21Z

Codecov Report

Merging #315 (714fff0) into main (a15b90b) will not change coverage.
The diff coverage is 63.93%.

@@           Coverage Diff           @@
##             main     #315   +/-   ##
=======================================
  Coverage   12.91%   12.91%           
=======================================
  Files          20       20           
  Lines        2958     2958           
=======================================
  Hits          382      382           
  Misses       2554     2554           
  Partials       22       22

Impacted Files	Coverage Δ
pkg/reconciler/ingress/ingress.go	`72.00% <0.00%> (ø)`
test/conformance/gateway-api/basic.go	`0.00% <0.00%> (ø)`
test/conformance/gateway-api/grpc.go	`0.00% <0.00%> (ø)`
test/conformance/gateway-api/headers.go	`0.00% <0.00%> (ø)`
test/conformance/gateway-api/hosts.go	`0.00% <0.00%> (ø)`
test/conformance/gateway-api/path.go	`0.00% <0.00%> (ø)`
test/conformance/gateway-api/percentage.go	`0.00% <0.00%> (ø)`
test/conformance/gateway-api/rule.go	`0.00% <0.00%> (ø)`
test/conformance/gateway-api/run.go	`0.00% <ø> (ø)`
test/conformance/gateway-api/timeout.go	`0.00% <0.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a15b90b...714fff0. Read the comment docs.

evankanderson

FWIW, the 100-, 200-, etc prefixes on files in /config are pretty standard elsewhere in Knative (which makes it easier to put in more config files), but I don't have a strong feeling about 01-, 03- and 04- instead.

There are a couple places where I think you ended up replacing /config with /third_party/gateway-api which you didn't mean to, but this otherwise looks as reasonable as can be expected given the changes between 0.4.0 and 0.5.0-rc1.

Thanks!

README.md

hack/update-codegen.sh

test/e2e-common.sh

dprotaso · 2022-06-29T13:18:33Z

Latest gateway-api @ HEAD has the fix (kubernetes-sigs/gateway-api#1239) to unblock injection/code generation - doing a go get sigs.k8s.io/gateway-api@main should pull in the changes

carlisia · 2022-06-29T21:30:14Z

I'm fixing the e2e test scripts so they (hopefully) each contain what's needed for starting the corresponding controller (and waiting for it to be ready), and no more.

evankanderson · 2022-07-01T17:29:06Z

Let me know when this goes in. I think you wanted this before #316, so I'm going to hold that one since it's still open.

carlisia · 2022-07-02T02:16:50Z

Argh, I am sorry that I only saw this now. Wanted to reorg the e2e tests in a separate PR but realized it needs to be done in a way that is compatible with how serving loads tests and artifacts, and so both sides need to be changed.

I will spruce up a couple minor things and have this PR ready tomorrow morning.

Our injection generated code was not being generated correctly anyway.

Pulling from main for now which now contains the correct `ReferencePolicyList.Items.[]ReferencePolicy` spec.

- Reverted: - third_party/gateway-api/ -> config/ - Refactored out test configuration for different vendor into a common setup file - Improved docs for installation (README) (closes knative-extensions#292) and for development/testing (DEVELOPMENT).

carlisia · 2022-07-07T12:46:53Z

I know what's causing these failures, will have a fix soon.

carlisia · 2022-07-07T23:52:18Z

✅ conformance tests
- ~~there's 1 failing but it's the same failure in main: e2e tests (v1.23.6)~~
- ~~Error meeting response expectations: got unexpected status: 503, expected map[200:{}]~~
- ~~considering parity with main a good threshold bar for this PR :) (unless the fix is obvious)~~
✅ e2e tests
integration-tests_net-gateway-api_main

fix for the above coming next..

carlisia · 2022-07-08T23:25:45Z

Tests are passing now.

evankanderson · 2022-07-08T23:28:13Z

(Adding this to the queue of reviews I need to do tonight)

evankanderson

Looks fairly good -- I think only one or two of these are actionable.

/approve

(Will /lgtm if you decide that you want to handle followups as "quick win" subsequent changes.)

evankanderson · 2022-07-09T05:44:46Z

.github/workflows/kind-conformance.yaml

-    - name: Prepare test namespaces
-      run: |
-        kubectl apply -f test/config/
-
-    - name: Upload Test Images
-      run: |
-        # Build and Publish our test images to the docker daemon.
-        ./test/upload-test-images.sh
-
-    - name: Run e2e Tests
+    - name: Run Conformance Tests


As discussed in person, separating the test refactor and the go.mod changes and upgrade fallout probably would make for two easier-to-review PRs that wouldn't get blocked on each other working.

More of an "advice for next time and bystanders" than a direction for this PR.

evankanderson · 2022-07-09T05:45:50Z

.gitignore

+
+tmp


Is this an artifact of your development patterns, or do we now write files to a local tmp?

Not mine. This tmp dir gets created during the code generation. Because it was taking so long/freezing on my machine and I had to kill that process many times, I noticed that depending on how far in the process the dir wouldn't have been cleaned up.

I could have added that to my global gitignore but figured it will be useful in the case when interrupting the codegen before it runs to the end.

evankanderson · 2022-07-09T05:46:17Z

DEVELOPMENT.md

 This doc explains how to setup a development environment so you can get started
-[contributing](https://www.knative.dev/contributing/) to Knative
-`net-gateway-api`. Also take a look at:
+contributing to Knative `net-gateway-api`.


Why cut the link to the other contributing docs?

I moved it to a bullet the very next line, next to another link of similar kind.

evankanderson · 2022-07-09T05:46:47Z

DEVELOPMENT.md

-
-### Checkout your fork
+## Notes
+If you use the konk script to setup your cluster, your cluster will be named `knative`. However, most of the scripts expect it to be the default `kind` name. Set the kind cluster name env `export KIND_CLUSTER_NAME=knative` to point to `knative` cluster. KO requires a registry which if you are developing locally you could use `export KO_DOCKER_REPO=kind.local` to use the local one on kind. Please see official [KO documentation](https://github.com/google/ko#local-publishing-options) for more information.


Link to the konk script?

If you're using a non-standard Kind cluster name with KO_DOCKER_REPO=kind.local, the KIND_CLUSTER_NAME also needs to be set, which isn't clear from these directions. Is KIND_CLUSTER_NAME used by other scripts or tools in this repo?

evankanderson · 2022-07-09T05:50:02Z

DEVELOPMENT.md

+1. A running cluster
+2. [Knative serving installed](README.md#install-knative-serving)
+3. [`ko`](https://github.com/google/ko) (for development and testing)
+4. [`kubectl`](https://kubernetes.io/docs/tasks/tools/install-kubectl/) (for managing development environments)
+5. [`bash`](https://www.gnu.org/software/bash/) v4 or later. On macOS the default bash is too old, you can use [Homebrew](https://brew.sh) to install a later version.


evankanderson · 2022-07-09T05:56:31Z

hack/update-codegen.sh

+  # TODO(carlisia): remove the below two lines in a future release.
+  # Reason: although the `gateway.networking.k8s.io_referencepolicies.yaml` file is included in the `v0.5.0-rc1` version, it is not referenced in the corresponding kustomize file. I'm assuming this will remain the same for the actual release.
+  # The file reference is included in the kustomize file in the `main` branch. With that, if these lines are not removed, there would be a duplicate of this resource.
+	echo "---" >> "${REPO_ROOT_DIR}/config/100-gateway-api.yaml"
+	curl -s "https://raw.githubusercontent.com/kubernetes-sigs/gateway-api/${GATEWAY_API_VERSION}/config/crd/experimental/gateway.networking.k8s.io_referencepolicies.yaml" >> "${REPO_ROOT_DIR}/config/100-gateway-api.yaml"


Is this still accurate? I see:

https://github.com/kubernetes-sigs/gateway-api/blob/v0.5.0-rc1/config/crd/experimental/gateway.networking.k8s.io_referencepolicies.yaml

(and I think you were saying they re-cut rc1 for this)

Yes, unless I am misunderstanding the question.

I think you are remembering the conversation about this fix: fix ReferencePolicyList.Items type by dprotaso · Pull Request #1239 · kubernetes-sigs/gateway-api. This fix is in main and will be included in the final v0.5.0 (I confirmed it).

I adjusted the go mod dep to include what is in main so at least the generated code (on our end) and the deps now match.

evankanderson · 2022-07-09T06:00:26Z

pkg/reconciler/ingress/resources/httproute.go

-	gatewayRef := gatewayv1alpa2.ParentRef{
-		Namespace: namespacePtr(gatewayv1alpa2.Namespace(namespacedNameGateway.Namespace)),
-		Name:      gatewayv1alpa2.ObjectName(namespacedNameGateway.Name),
+	gatewayRef := gatewayv1alpha2.ParentReference{


Reviewing note: Combining the typo fix with the s/ParentRef/ParentReference/ makes it harder to scan n' approve the file -- there are at least two different red/green patterns to look for.

evankanderson · 2022-07-09T06:08:08Z

test/e2e-library.sh

+function cluster_info() {
+    CONTROL_NAMESPACE=knative-serving
+    IPS=( $(kubectl get nodes -lkubernetes.io/hostname!=kind-control-plane -ojsonpath='{.items[*].status.addresses[?(@.type=="InternalIP")].address}') )
+    CLUSTER_SUFFIX=${CLUSTER_SUFFIX:-cluster.local}
+}


cluster_info here feels very similar to export_cluster_info in e2e-common.sh. Combine?

(Feel free to make this a TODO if it feels too fragile to do right now)

evankanderson · 2022-07-09T06:09:44Z

test/e2e-library.sh

+function kind_e2e_istio() {
+    export GATEWAY_OVERRIDE=""
+    export GATEWAY_NAMESPACE_OVERRIDE=""
+
+    cluster_info
+
+    go test -race -count=1 -short -timeout=20m -tags=e2e ./test/conformance \
+        --enable-alpha --enable-beta \
+        --skip-tests="${ISTIO_UNSUPPORTED_E2E_TESTS}" \
+        --ingressendpoint="${IPS[0]}" \
+        --ingressClass=gateway-api.ingress.networking.knative.dev \
+        --cluster-suffix="$CLUSTER_SUFFIX"
+
+    # Give the controller time to sync with the rest of the system components.
+    sleep 30
+
+    echo ">> Scale up controller for HA tests"
+    kubectl -n "${CONTROL_NAMESPACE}" scale deployment net-gateway-api-controller --replicas=2
+
+    go test -count=1 -timeout=15m -failfast -parallel=1 -tags=e2e ./test/ha -spoofinterval="10ms" \
+        --enable-alpha --enable-beta \
+        --ingressendpoint="${IPS[0]}" \
+        --ingressClass=gateway-api.ingress.networking.knative.dev \
+        --cluster-suffix="${CLUSTER_SUFFIX}"
+
+    echo ">> Scale down after HA tests"
+    kubectl -n "${CONTROL_NAMESPACE}" scale deployment net-gateway-api-controller --replicas=1
+}


This feel similar-but-not-identical to both kind_e2e_contour and e2e_istio. Might be worth a TODO to deduplicate more in a future PR.

evankanderson · 2022-07-09T06:11:51Z

test/e2e-tests.sh


-success


We used to call success at the end of this script, and now we don't.

It looks like that function prints a success banner, but then also dumps some metrics from the kubernetes apiserver.

knative-prow · 2022-07-09T06:13:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlisia, evankanderson

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~~ [carlisia,evankanderson]

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

carlisia · 2022-07-11T20:59:49Z

I could compile your comments into a todo list and have a followup PR today. I'm concerned of holding up this PR even longer. Am open to adding anything that is a must include in this PR, just having a hard time distinguishing if anything is. At any rate, thank you so much for the throughout review.

evankanderson · 2022-07-11T21:41:12Z

/lgtm

Notes from slack:

I think the comments on DEVELOPMENT.md, such as this one were the ones I was most concerned about fixing, since they might make it harder for someone else to join in.

I’m also wondering about putting 100-gateway-api.yaml into our release, but it looks like that may be long-standing, so I’d TODO it.

Can you also open an issue for [the addition of ReferencePolicies CRD](https://github.com/knative-sandbox/net-gateway-api/pull/315#discussion_r917226606) to the 100-gateway-api.yamlwhich is marked as temporary inupdate-codegen.sh`?

* Add support for TLS (conformance test passes) * Fix codegen update and run * Fix reconcile on no changes; add reconciliation tests * Update with Dave's feedback. * Fix lint warnings. * Add Finalizer to cleanup Gateway Listeners Added some (defaulted) fields to HTTPRoute to avoid controller looping. * Fix lint complaint. * Fix test breakage after merge post-#315 * Update referencepolicy->referencegrant in roles as well * Switch back to ReferencePolicy from ReferenceGrant, because Istio doesn't support ReferenceGrant yet, and there's no converter.

knative-prow bot assigned nak3 Jun 28, 2022

dprotaso reviewed Jun 28, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

dprotaso reviewed Jun 28, 2022

View reviewed changes

go.mod Outdated Show resolved Hide resolved

carlisia mentioned this pull request Jun 28, 2022

Generated injection code is broken when there's a mismatch in type knative/pkg#2537

Closed

evankanderson reviewed Jun 29, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

hack/update-codegen.sh Outdated Show resolved Hide resolved

test/e2e-common.sh Outdated Show resolved Hide resolved

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2022

carlisia force-pushed the c-v0.5.0-rc1-deps branch from eef0e4f to 1de4442 Compare June 29, 2022 19:22

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2022

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2022

carlisia and others added 10 commits July 2, 2022 16:29

Upgrade Gateway API import paths + deps to v0.5.0-rc1

2221858

ParentRef -> ParentReference

aa48c80

ConditionRouteAccepted -> RouteConditionAccepted

ea093a9

Update CRDs, config, instructions + point to local CRDs

729de13

Fix path to config files

a9adba0

ingressv2 -> gateway-api

72f0558

Fix my dyslexic moment

eb1b989

Remove deprecated type

debf9e0

Our injection generated code was not being generated correctly anyway.

Revert removal of generated injection code + fix

1de3836

Pulling from main for now which now contains the correct `ReferencePolicyList.Items.[]ReferencePolicy` spec.

Carlisia Thompson added 5 commits July 7, 2022 18:14

Fix conformance tests

ee81e32

Unrefactor for now

865623a

Exporting here is a problem, fix

04295eb

Fix conformance tests, round 2

b1c652e

No waiting for conformance tests

b1ca536

Carlisia Thompson added 7 commits July 8, 2022 00:30

Fix e2e tests

6040214

Fix

ad44b1e

Fix?

14c886f

🤞

768f8b0

🤞🤞🤞

53286a2

Test refactor

305843c

Fix bash flag

d350ee4

evankanderson mentioned this pull request Jul 8, 2022

Upgrade to gateway 0.5.0-rc1 **ONLY** #327

Closed

Carlisia Thompson added 2 commits July 8, 2022 22:40

Add ns to istio test

a7f5c7b

Add (maybe) needed source

714fff0

evankanderson reviewed Jul 9, 2022

View reviewed changes

knative-prow bot assigned evankanderson Jul 11, 2022

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2022

knative-prow bot merged commit c82e90c into knative-extensions:main Jul 11, 2022

carlisia deleted the c-v0.5.0-rc1-deps branch July 11, 2022 21:56

evankanderson pushed a commit to evankanderson/net-ingressv2 that referenced this pull request Jul 11, 2022

Fix test breakage after merge post-knative-extensions#315

4045dce

evankanderson mentioned this pull request Jul 12, 2022

Add debug info for prow script #332

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⬆️ Bump Gateway API version to v0.5.0-rc1 #315

⬆️ Bump Gateway API version to v0.5.0-rc1 #315

carlisia commented Jun 28, 2022 •

edited

Loading

knative-prow bot commented Jun 28, 2022

carlisia commented Jun 28, 2022

carlisia commented Jun 28, 2022

codecov bot commented Jun 28, 2022 •

edited

Loading

evankanderson left a comment

dprotaso commented Jun 29, 2022

carlisia commented Jun 29, 2022

evankanderson commented Jul 1, 2022

carlisia commented Jul 2, 2022

carlisia commented Jul 7, 2022

carlisia commented Jul 7, 2022 •

edited

Loading

carlisia commented Jul 8, 2022

evankanderson commented Jul 8, 2022

evankanderson left a comment

evankanderson Jul 9, 2022

evankanderson Jul 9, 2022

carlisia Jul 11, 2022

evankanderson Jul 9, 2022

carlisia Jul 11, 2022

evankanderson Jul 9, 2022

evankanderson Jul 9, 2022

evankanderson Jul 9, 2022

evankanderson Jul 9, 2022

carlisia Jul 11, 2022

evankanderson Jul 9, 2022

evankanderson Jul 9, 2022

evankanderson Jul 9, 2022

evankanderson Jul 9, 2022

evankanderson Jul 9, 2022

knative-prow bot commented Jul 9, 2022

carlisia commented Jul 11, 2022

evankanderson commented Jul 11, 2022

⬆️ Bump Gateway API version to v0.5.0-rc1 #315

⬆️ Bump Gateway API version to v0.5.0-rc1 #315

Conversation

carlisia commented Jun 28, 2022 • edited Loading

knative-prow bot commented Jun 28, 2022

carlisia commented Jun 28, 2022

carlisia commented Jun 28, 2022

codecov bot commented Jun 28, 2022 • edited Loading

Codecov Report

evankanderson left a comment

Choose a reason for hiding this comment

dprotaso commented Jun 29, 2022

carlisia commented Jun 29, 2022

evankanderson commented Jul 1, 2022

carlisia commented Jul 2, 2022

carlisia commented Jul 7, 2022

carlisia commented Jul 7, 2022 • edited Loading

carlisia commented Jul 8, 2022

evankanderson commented Jul 8, 2022

evankanderson 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

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 bot commented Jul 9, 2022

carlisia commented Jul 11, 2022

evankanderson commented Jul 11, 2022

carlisia commented Jun 28, 2022 •

edited

Loading

codecov bot commented Jun 28, 2022 •

edited

Loading

carlisia commented Jul 7, 2022 •

edited

Loading