bump controller-runtime and k8s deps

[tsaarni]

This pull request contains following changes:

• Consolidates the previously separate PRs to bump k8s dependencies together with controller runtime.
  • build(deps): bump the k8s-dependencies group with 5 updates #6623
  • build(deps): bump the k8s-dependencies group with 5 updates #6696
  • build(deps): bump sigs.k8s.io/controller-runtime from 0.18.4 to 0.19.0 #6624
• Removes +required from ExtensionServiceReference.Name
  • See bump controller-runtime and k8s deps #6634 (comment) for reasoning.

Background:

sigs.k8s.io/controller-runtime v0.19.0 includes a breaking change that necessitates updating it alongside k8s.io/* v1.31.

sigs.k8s.io/controller-tools v0.16.x needs to be updated for k8s.io/* v1.31 compatibility, which also prompted the re-generation of CRDs.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.01%. Comparing base (e5a25e2) to head (2c1946c).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6634      +/-   ##
==========================================
- Coverage   81.76%   81.01%   -0.76%     
==========================================
  Files         133      133              
  Lines       15944    19996    +4052     
==========================================
+ Hits        13037    16200    +3163     
- Misses       2614     3503     +889     
  Partials      293      293              

see 127 files with indirect coverage changes

[tsaarni]

I believe the RBAC rules were changed by make generate because of deduplication done by controller-tools v0.16.0 kubernetes-sigs/controller-tools#937.

[tsaarni]

There is still an additional hurdle with the version bump. Following change in controller-tools v0.16.0

✨ add support for kubernetes +required annotations kubernetes-sigs/controller-tools#944

This change appropriately triggers the generation of new "required" entries in CRDs when +required is specified in the type definition.

In our API, we have a +required field in ExtensionServiceReference

contour/apis/projectcontour/v1/httpproxy.go

Lines 238 to 240 in 20e57df

	// +required
	// +kubebuilder:validation:MinLength=1
	Name string `json:"name,omitempty" protobuf:"bytes,3,opt,name=name"`

ExtensionServiceReference is used in AuthorizationServer as an +optional field but it is embedded as a struct rather than a pointer

contour/apis/projectcontour/v1/httpproxy.go

Lines 249 to 250 in 20e57df

	// +optional
	ExtensionServiceRef ExtensionServiceReference `json:"extensionRef,omitempty"`

Now consider how AuthorizationServer is used in the Go code

contour/test/e2e/httpproxy/global_external_auth_test.go

Lines 234 to 238 in 20e57df

	Authorization: &contour_v1.AuthorizationServer{
	AuthPolicy: &contour_v1.AuthorizationPolicy{
	Disabled: true,
	},
	},

Even though the optional HTTPProxy.Spec.VirtualHost.Authorization.ExtensionServiceRef is never explicitly set, it gets included in the request since it's embedded in the AuthorizationServer struct. The omitempty tag does not prevent this since the field should have been a pointer for omitempty to work as intended. As a result, ExtensionServiceReference.Name is initialized with a nil value, and the newly introduced +required validation triggers, causing configuration to fail.

https://github.com/projectcontour/contour/actions/runs/10536218743/job/29196551673?pr=6634

  	Error:      	Received unexpected error:
  	            	HTTPProxy.projectcontour.io "external-auth" is invalid: [spec.virtualhost.authorization.extensionRef.name: Required value, <nil>: Invalid value: "null": some validation rules were not checked because the object was invalid; correct the existing errors to complete validation]
  	Test:       	HTTPProxy global external auth with namespace: httpproxy-global-ext-auth-non-tls-disabled with global external auth service global external auth can be disabled on a non TLS HTTPProxy

[tsaarni]

There are several other types that also use +required, but only ExtensionServiceReference.Name is causing the test suite to fail.

I believe backwards compatible fix would be to remove the +required from ExtensionServiceReference.Name. Although it seems illogical to reference to a non-existing resource, this approach shouldn't be any worse than previous state, since +required has never actually been enforced by the CRD.

Another option would be to change HTTPProxy.Spec.VirtualHost.Authorization.ExtensionServiceRef to a pointer, but that could potentially break external Go code relying on the API.

Any thoughts?

[sunjayBhatia]

There are several other types that also use +required, but only ExtensionServiceReference.Name is causing the test suite to fail.

I believe backwards compatible fix would be to remove the +required from ExtensionServiceReference.Name. Although it seems illogical to reference to a non-existing resource, this approach shouldn't be any worse than previous state, since +required has never actually been enforced by the CRD.

Another option would be to change HTTPProxy.Spec.VirtualHost.Authorization.ExtensionServiceRef to a pointer, but that could potentially break external Go code relying on the API.

Any thoughts?

Looks like we missed out making the change to a pointer in #4994

Since it is a v1 type seems like we should go the conservative approach here and do the former

[tsaarni]

Since it is a v1 type seems like we should go the conservative approach here and do the former

I agree. I've removed the +required annotation from ExtensionServiceReference.Name so that it will be treated as optional again, like it was before controller-tools v0.16.0.