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

carlisia · 2022-06-25T03:07:01Z

This is work in preparation for upgrading to v0.5.0, so we can start running and fixing/adding tests.

Tackling Contour first. Istio would be next.

Changes

Upgrades to Go v1.17
Upgrades to Gateway API v0.5.0-rc1 + generates the associated CRDs
Upgrades the Contour operator to a commit that includes the Gateway API v0.5.0-rc1 (which is not included in the latest release)

/kind

Fixes #
#306

Release Note

Docs

knative-prow · 2022-06-25T03:07:02Z

@carlisia: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them.

In response to this:

This is work in preparation for upgrading to v0.5.0, so we can start running and fixing/adding tests.

Changes

Upgrades to Go v1.17

Upgrades to Gateway API v0.5.0-rc1 + generates the associated CRDs

Upgrades the Contour operator to a commit that includes the Gateway API v0.5.0-rc1 (which is not included in the latest release)

/kind

Fixes #
#306

Release Note
Docs

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.

knative-prow · 2022-06-25T03:07:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlisia

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]

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

carlisia · 2022-06-25T03:07:48Z

c/c @nak3 @evankanderson

codecov · 2022-06-25T03:10:11Z

Codecov Report

Merging #311 (f9c37f8) into main (207051d) will not change coverage.
The diff coverage is n/a.

❗ Current head f9c37f8 differs from pull request most recent head 2f8ed93. Consider uploading reports for the commit 2f8ed93 to get more accurate results

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

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 207051d...2f8ed93. Read the comment docs.

carlisia · 2022-06-25T03:20:29Z

As we are focusing on tackling Contour first, I wonder if I could comment out the Istio tests that are failing so we could lean in on CI and automation to run the Contour tests, for now?

nak3 · 2022-06-25T04:48:02Z

go.mod

+	k8s.io/apimachinery v0.24.1 => k8s.io/apimachinery v0.23.8
+	k8s.io/client-go v0.24.1 => k8s.io/client-go v0.23.8
+	k8s.io/code-generator v0.24.1 => k8s.io/code-generator v0.23.8
+	sigs.k8s.io/gateway-api v0.5.0-rc1 => sigs.k8s.io/gateway-api v0.4.0


I think we should not close #306 yet (or track it by another issue) if we keep using v0.4.0.

nak3 · 2022-06-25T04:50:10Z

One quick Q, as commented #311 (comment), this PR bumps CRD files from 0.4.0's to 0.5.0-rc1 but does not bump the libs yet. Will you update the deps in this PR or the follow up PR?

nak3 · 2022-06-25T05:02:19Z

As we are focusing on tackling Contour first, I wonder if I could comment out the Istio tests that are failing so we could lean in on CI and automation to run the Contour tests, for now?

I assume that "the failing Istio tests" means this issue in KinD conformance test. If so, I think we should add it to the skip test or just ignore it. I tracked the issue #185 but no progress yet.

Actually for Contour, it looks like stable than Istio, but Contour just skips the tests (or you can see the doc).

carlisia · 2022-06-25T05:15:39Z

I assume that "the failing Istio tests" means this issue in KinD conformance test. If so, I think we should add it to the skip test or just ignore it. I tracked the issue #185 but no progress yet.

Ah great that there's an issue for that. I'll skip it then.

Actually for Contour, it looks like stable than Istio, but Contour just skips the tests (or you can see the doc).

Yes. There is a "Create tracking issues for knative conformance tests that are failing" in the Gateway API Roadmap. After this upgrade gets merged I'll be commenting out those tests and creating issues for the ones that fail. But glad you pointed that out.

carlisia · 2022-06-25T05:20:57Z

One quick Q, as commented #311 (comment), this PR bumps CRD files from 0.4.0's to 0.5.0-rc1 but does not bump the libs yet. Will you update the deps in this PR or the follow up PR?

There is an issue (#307) to track upgrading to the actual v0.5.0 release, which will need to have both the corresponding CRDs and the deps.

I wanted to upgrade the deps on this PR. But due to the deps on 0.5.0-rc1 it upgrades our k8s.io stuff to v0.24.1 and causes errors like the one shown below. I haven't figured this out yet. Or maybe pkg will need to update to v0.24.1 for this to work?

./hack/update-deps.sh  
=== Update Deps for Golang
--- Go mod tidy and vendor
go: finding module for package sigs.k8s.io/gateway-api/pkg/client/clientset/gateway/versioned
go: finding module for package sigs.k8s.io/gateway-api/pkg/client/clientset/gateway/versioned/typed/apis/v1alpha2
knative.dev/net-gateway-api/test imports
        sigs.k8s.io/gateway-api/pkg/client/clientset/gateway/versioned: package sigs.k8s.io/gateway-api/pkg/client/clientset/gateway/versioned p
rovided by sigs.k8s.io/gateway-api at latest version v0.4.3 but not at required version v0.5.0-rc1

Update on 6/25: I found what I was doing wrong (not just inverting the replace values) 🤦‍♀️. Will push the fix tomorrow.

evankanderson

I wonder if it would be simpler to review as "bump to Go 1.17" and "use gateway-api 0.5.0-rc1". I think the first would be trivial to review, but I'm not sure we have the second quite right yet.

evankanderson · 2022-06-25T20:20:07Z

DEVELOPMENT.md

@@ -94,7 +94,7 @@ source ./hack/test-env.sh
 #### Install Gateway API CRDs

 ```
-kubectl apply -k "github.com/kubernetes-sigs/gateway-api/config/crd?ref=${GATEWAY_API_VERSION}"
+kubectl apply -f config/100-gateway-api.yaml


It feels strange to be copying the definitions into our repo, rather than referencing some upstream artifact (even if it's a raw or nightly reference rather than a release).

It feels practical to me rather than strange. If I'm using artifacts to change/test the code, I'd rather have it sitting on my machine rather than going over the network. And in this case I can control/change the version I want by changing the version variable the codegen uses. Fetch once/run locally with the same copy. Is this an anti-pattern?

evankanderson · 2022-06-25T20:27:16Z

go.mod

+	k8s.io/api v0.24.1 => k8s.io/api v0.23.8
+	k8s.io/apimachinery v0.24.1 => k8s.io/apimachinery v0.23.8
+	k8s.io/client-go v0.24.1 => k8s.io/client-go v0.23.8
+	k8s.io/code-generator v0.24.1 => k8s.io/code-generator v0.23.8
+	sigs.k8s.io/gateway-api v0.5.0-rc1 => sigs.k8s.io/gateway-api v0.4.0


Aren't these replace lines backwards? (As evidenced by lack of changes to go.mod)

Likely. I did it both ways just to be sure to attain the most suffering, ended up picking the wrong one evidently. Will fix, thanks for pointing it out.

evankanderson · 2022-06-25T20:28:07Z

go.mod

@@ -18,3 +27,78 @@ require (
 	sigs.k8s.io/gateway-api v0.4.0
 	sigs.k8s.io/yaml v1.3.0
 )
+
+require (


What added all these indirect definitions? The switch to Go 1.16?

Yes. Nit correction: switch to Go 1.17 (I know that's what you meant).

evankanderson · 2022-06-25T20:29:34Z

hack/update-codegen.sh

-  echo "# Generated with \"kubectl kustomize https://github.com/kubernetes-sigs/gateway-api/config/crd?ref=${GATEWAY_API_VERSION}\"" > ${REPO_ROOT_DIR}/config/100-gateway-api.yaml
-  kubectl kustomize "https://github.com/kubernetes-sigs/gateway-api/config/crd?ref=${GATEWAY_API_VERSION}" >> ${REPO_ROOT_DIR}/config/100-gateway-api.yaml
+ echo "# Generated with \"kubectl kustomize github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=${GATEWAY_API_VERSION}" > "${REPO_ROOT_DIR}/config/100-gateway-api.yaml"
+	kubectl kustomize "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=${GATEWAY_API_VERSION}" >> "${REPO_ROOT_DIR}/config/100-gateway-api.yaml"


It looks like the 100-gateway-api.yaml is usually auto-created. Maybe we should .gitignore it?

I guess i want to roll back and understand what this file is intended for. Would like to get a read from @nak3, since he paved the way here and might have some thoughts.

To recap:

What is in main now:

this file is generated by codegen but not being used anywhere as far as I can tell. I assumed it was serving as a local example of what this component is working with for any given release of the Gateway API that we use.

What changes in this PR:

codegen is updated to generate a new set of CRDs based on the upcoming v0.5.0 version (changes a bit).

since this file was there anyway, I changed whatever was pointing at the the upstream (DEVELOPMENT.md and the Istio conformance test) to point at the local file instead. Might be my beginner roots speaking from the era when network was sparse and expensive to nonexisting (and the dinosaurs roamed the earth!) but I still think it's more practical for development.

If pointing to upstream to get a copy of these resources has an advantage over pointing at a local copy (which can be refreshed, to be sure) that I'm not seeing, I can change those references to point upstream.

If this file ends up having no solid purpose for our use at the moment, I would say maybe not even keep maintaining this code generation and the overhead in time that it adds to running that script?

I'm concerned that having this file checked in to config (vs third_party or somewhere else) will cause it to be collected and bundled into the release.

I can understand wanting to vendor this somewhere rather than fetch it from the Internet; my primary concern is putting it next to the configs we release with ko resolve -f $DIR.

Sorry yes we should not add the gateway-api CRD into config directory but should add it into third_party as Evan mentioned.

The reason why I added it into config (actually release net-gateway-api.yaml) was that we must install the GatewayAPI CRD before the net-gateway-api controller and Istio/Contour. So, I struggled to modify the ytt & kapp in serving repo and in the end, I added it into the head of net-gateway-api.yaml but I am sure that is not ideal.

Probably you have knowledge for the two vmware CLI tools, so I would like you to move the file into third_party and modify the test script in serving repo.

That makes sense. And yes, I can tackle the ytt stuff. Will move the file and add an issue for this. Thanks for the clarification.

nak3 · 2022-06-27T02:32:12Z

Or maybe pkg will need to update to v0.24.1 for this to work?

I usually waited for the k8s libs in pkg repo was updated to the gateway-api repo's version like:

knative/pkg#1881
knative/pkg#2111

Do you need v0.5.0-rc1 urgently? If not urgent, I think we just wait that knative/pkg updates to the k8s lib to 0.24.1+.

carlisia · 2022-06-27T14:14:06Z

I got the deps updated. 🎉

I can't get the codegen to regenerate the client code

I noticed that running ./hack/update-codegen.sh was not generating any code related to the new type ReferenceGrant. I ran a few experiments to try to force it (for example, replaced v1alpha2 with v1beta1) and nothing. I did a similar experiment with serving and nothing changed either so I think I'm missing some magical incantation to run this.

I tried:

passing --output-base
adding these to the script:
- export PATH="$GOBIN:$PATH"
- export GOFLAGS=-mod=
- both things above at the same time

knative-prow · 2022-06-27T14:18:17Z

@carlisia: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
unit-tests_net-gateway-api_main	`2f8ed93`	link	true	`/test unit-tests_net-gateway-api_main`
build-tests_net-gateway-api_main	`2f8ed93`	link	true	`/test build-tests_net-gateway-api_main`
integration-tests_net-gateway-api_main	`2f8ed93`	link	true	`/test integration-tests_net-gateway-api_main`

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.

carlisia · 2022-06-27T14:24:24Z

PS: I haven't updated the code (tests) to point to the new changes in the Gateway API spec yet, want to resolve the above issue first.

dprotaso · 2022-06-27T14:57:18Z

This worked for me - make sure you run update-deps.sh prior to codegen

diff --git a/hack/update-codegen.sh b/hack/update-codegen.sh
index e1fa3cbd..ae365d94 100755
--- a/hack/update-codegen.sh
+++ b/hack/update-codegen.sh
@@ -29,7 +29,7 @@ group "Gateway API Codegen"
 # Gateway API
 ${CODEGEN_PKG}/generate-groups.sh "client,informer,lister" \
   knative.dev/net-gateway-api/pkg/client/gatewayapi sigs.k8s.io/gateway-api \
-  "apis:v1alpha2" \
+  "apis:v1alpha2 apis:v1beta1" \
   --go-header-file ${REPO_ROOT_DIR}/hack/boilerplate/boilerplate.go.txt

 ## Gateway API

carlisia · 2022-06-27T18:52:04Z

Thanks for all the feedback so far. Closing this in favor of braking this into smaller PRs. First one:

Upgrade to Go v1.17 #313

carlisia added 4 commits June 24, 2022 16:44

Update Go to v1.17

c1c5b78

Upgrade to Gateway API v0.5.0-rc1

117e8ea

Update Gateway API CRDs to v0.5.0-rc1

9178e17

Update to Contour that includes v0.5.0-rc1

f9c37f8

knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 25, 2022

nak3 reviewed Jun 25, 2022

View reviewed changes

evankanderson reviewed Jun 25, 2022

View reviewed changes

Fix deps replacement (≧︿≦)

ade6dd3

carlisia added 2 commits June 26, 2022 19:37

Upgrade Gateway API v0.5.0-rc1 deps, for real this time

e913cac

Update modules

2f8ed93

carlisia closed this Jun 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

carlisia commented Jun 25, 2022 •

edited

Loading

knative-prow bot commented Jun 25, 2022

Changes

knative-prow bot commented Jun 25, 2022

carlisia commented Jun 25, 2022

codecov bot commented Jun 25, 2022 •

edited

Loading

carlisia commented Jun 25, 2022 •

edited

Loading

nak3 Jun 25, 2022

nak3 commented Jun 25, 2022

nak3 commented Jun 25, 2022 •

edited

Loading

carlisia commented Jun 25, 2022

carlisia commented Jun 25, 2022 •

edited

Loading

evankanderson left a comment

evankanderson Jun 25, 2022

carlisia Jun 25, 2022

evankanderson Jun 25, 2022

carlisia Jun 25, 2022

evankanderson Jun 25, 2022

carlisia Jun 25, 2022

evankanderson Jun 25, 2022

carlisia Jun 25, 2022

evankanderson Jun 26, 2022

nak3 Jun 27, 2022

carlisia Jun 27, 2022

nak3 commented Jun 27, 2022

carlisia commented Jun 27, 2022

knative-prow bot commented Jun 27, 2022

carlisia commented Jun 27, 2022

dprotaso commented Jun 27, 2022 •

edited

Loading

carlisia commented Jun 27, 2022

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

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

Conversation

carlisia commented Jun 25, 2022 • edited Loading

Changes

knative-prow bot commented Jun 25, 2022

Changes

knative-prow bot commented Jun 25, 2022

carlisia commented Jun 25, 2022

codecov bot commented Jun 25, 2022 • edited Loading

Codecov Report

carlisia commented Jun 25, 2022 • edited Loading

Choose a reason for hiding this comment

nak3 commented Jun 25, 2022

nak3 commented Jun 25, 2022 • edited Loading

carlisia commented Jun 25, 2022

carlisia commented Jun 25, 2022 • edited Loading

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

nak3 commented Jun 27, 2022

carlisia commented Jun 27, 2022

knative-prow bot commented Jun 27, 2022

carlisia commented Jun 27, 2022

dprotaso commented Jun 27, 2022 • edited Loading

carlisia commented Jun 27, 2022

carlisia commented Jun 25, 2022 •

edited

Loading

codecov bot commented Jun 25, 2022 •

edited

Loading

carlisia commented Jun 25, 2022 •

edited

Loading

nak3 commented Jun 25, 2022 •

edited

Loading

carlisia commented Jun 25, 2022 •

edited

Loading

dprotaso commented Jun 27, 2022 •

edited

Loading