Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump k8s libs and controller runtime #2005

Closed
wants to merge 2 commits into from
Closed

Conversation

kieron-dev
Copy link
Contributor

@kieron-dev kieron-dev commented Dec 12, 2022

Is there a related GitHub Issue?

No

What is this change about?

Draft PR as we require feedback on including controller-runtime@master

Controller-runtime does not yet have a release that builds with v0.26.0 of the client-go library.

We are getting dependabot requests to update the k8s libraries to v0.26.0, so here we bump those libraries and take the latest version of controller-runtime so that the code builds.

Note that there are some struct/interface renames that have caused changes in our client library fakes. StatusWriter -> SubResourceWriter, for example.

Does this PR introduce a breaking change?

Maybe

Acceptance Steps

Tag your pair, your PM, and/or team

@cloudfoundry/wg-cf-on-k8s-korifi-approvers

Things to remember

@kieron-dev
Copy link
Contributor Author

We're not seeing the controllers tests fail locally strangely

@davewalter davewalter force-pushed the bump-k8s-libs-0.26.0 branch 2 times, most recently from 33a39e4 to de224fd Compare December 15, 2022 12:59
@davewalter davewalter marked this pull request as ready for review December 15, 2022 13:12
@davewalter davewalter marked this pull request as draft December 15, 2022 13:13
@davewalter
Copy link
Member

The failing controller tests appear to be caused by a new Claims field that was added to the ResourceRequirements type for Kubernetes v1.26. The comments for this field include the listType Kubebuilder marker with a value of set, which results in the x-kubernetes-list-type key being included in the CRD definition for AppWorkload and TaskWorkload. This in turn requires the corresponding x-kubernetes-map-type key to be included in the items subkey of the new claims property in the CRD, but there is no corresponding mapType Kubebuilder marker in the definition for the ResourceClaim type.

This results in the following error when Kubernetes tries to install the generated CRD:

The CustomResourceDefinition "appworkloads.korifi.cloudfoundry.org" is invalid: spec.validation.openAPIV3Schema.properties[spec].properties[resources].properties[claims].items.x-kubernetes-map-type: Invalid value: "null": must be atomic as item of a list with x-kubernetes-list-type=set

I can fix this by:

  1. remove the x-kubernetes-list-type key from the CRD, or
  2. add the x-kubernetes-map-type key to the items key under claims with a value of atomic

Both of these changes require a change to the api/core/v1 library code; either to add a comment to the ResourceClaim type, or to remove the comment from the Claims field in the ResourceRequirements type. The alternative is for us to copy the ResourceRequirements type into the Korifi codebase so that we can remove it ourselves.

@davewalter
Copy link
Member

This has been fixed in kubernetes/kubernetes#114585 by changing the ResourceClaim's listType comment from set to map and adding a listMapKey=name comment. The change is being back-ported to Kubernetes v1.26 in kubernetes/kubernetes#114617 (which has not been merged as of the time of writing).

Kieron Browne and others added 2 commits January 13, 2023 16:04
Note that there are some struct/interface renames that have caused
changes in our client library fakes. StatusWriter -> SubResourceWriter,
for example.

Co-authored-by: Dave Walter <walterda@vmware.com>
@kieron-dev
Copy link
Contributor Author

Closing, as all the dependencies have now caught up and work together in published releases

@kieron-dev kieron-dev closed this Jan 20, 2023
@davewalter davewalter deleted the bump-k8s-libs-0.26.0 branch January 20, 2023 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants