-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
✨ Add indexes to fake client to mimic field selectors #2025
✨ Add indexes to fake client to mimic field selectors #2025
Conversation
Hi @matteoolivi. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Allow registration of indexes into the fake client to allow usage of field selectors when doing a List. The indexing is done lazily by doing a normal list and then filtering the results by computing the index value for each list item. To enable the main change for this commit, some refactorings are performed: an internal and unexported function that checks whether a field selector is in the form key=val or key==val is made internal and exported to be shared between real and faked code to ensure loyalty. Unit tests for it are added. Co-authored-by: Paul Eichler <[email protected]>
c9fa5e7
to
c8aad1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
pkg/client/fake/client.go
Outdated
// API objects of GroupVersionResource `gvr` in the fake client. | ||
// It can be invoked multiple times, both with different GroupVersionResource or the same one. | ||
// Invoking WithIndex twice with the same `name` and `gvr` will panic. | ||
func (f *ClientBuilder) WithIndex(gvr schema.GroupVersionResource, name string, indexer client.IndexerFunc) *ClientBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than taking an gvr, could we take a client.Object
instead? And maybe stick with the argument naming used in the FieldIndexer
(i.E. obj Object, field string, extractValue IndexerFunc
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 5db9d19
pkg/client/fake/client.go
Outdated
|
||
// Field selection is mimicked via indexes, so there's no sane answer this function can give | ||
// if there are no indexes registered for the GroupVersionResource of the objects in the list. | ||
indexes, listGVRHasIndexes := c.indexes[gvr] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should return the same error for this and the second error condition, for the user of this it is the same mistake so getting one or the other might be confusing because what they have to change is the same in both cases
indexes, listGVRHasIndexes := c.indexes[gvr] | |
if indexes := c.indexes[gvr]; indexers == nil || indexers[fieldKey] == nil { | |
return nil, fmt.Errorf(... | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 5db9d19
pkg/client/fake/client_test.go
Outdated
@@ -86,7 +100,7 @@ var _ = Describe("Fake client", func() { | |||
} | |||
}) | |||
|
|||
AssertClientBehavior := func() { | |||
AssertClientWOIndexBehavior := func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AssertClientWOIndexBehavior := func() { | |
AssertClientWithoutIndexBehavior := func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 5db9d19
@alvaroaleman I pushed a separate commit to address your change requests. |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, matteoolivi 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:
Approvers can indicate their approval by writing |
looks like we have some flaky tests /retest |
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime#2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime#2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime#2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime#2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime#2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime#2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime#2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime#2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime#2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime#2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime#2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime/pull/2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime/pull/2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime/pull/2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime/pull/2025
See the unintentional breaking change in: kubernetes-sigs/controller-runtime#2025 Signed-off-by: Mike Beaumont <[email protected]>
See the unintentional breaking change in: kubernetes-sigs/controller-runtime#2025 There's now an error thrown if a field selector is used but an index wasn't registered but previously it was simply ignored. Signed-off-by: Mike Beaumont <[email protected]>
…5541) * chore(deps): bump sigs.k8s.io/controller-tools from 0.10.0 to 0.11.1 Bumps [sigs.k8s.io/controller-tools](https://github.com/kubernetes-sigs/controller-tools) from 0.10.0 to 0.11.1. - [Release notes](https://github.com/kubernetes-sigs/controller-tools/releases) - [Changelog](https://github.com/kubernetes-sigs/controller-tools/blob/master/RELEASE.md) - [Commits](kubernetes-sigs/controller-tools@v0.10.0...v0.11.1) --- updated-dependencies: - dependency-name: sigs.k8s.io/controller-tools dependency-type: direct:production update-type: version-update:semver-minor ... * chore(deps): bump sigs.k8s.io/controller-runtime from 0.13.1 to 0.14.1 Bumps [sigs.k8s.io/controller-runtime](https://github.com/kubernetes-sigs/controller-runtime) from 0.13.1 to 0.14.1. - [Release notes](https://github.com/kubernetes-sigs/controller-runtime/releases) - [Changelog](https://github.com/kubernetes-sigs/controller-runtime/blob/master/RELEASE.md) - [Commits](kubernetes-sigs/[email protected]) --- updated-dependencies: - dependency-name: sigs.k8s.io/controller-runtime dependency-type: direct:production update-type: version-update:semver-minor ... * fix: check AddEventHandler error * fix: deprecated functions * chore: update CRDs * chore: update golden files * fix: use index in test See the unintentional breaking change in: kubernetes-sigs/controller-runtime#2025 There's now an error thrown if a field selector is used but an index wasn't registered but previously it was simply ignored. Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Mike Beaumont <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Mike Beaumont <[email protected]>
…troller-runtime controller-runtime 0.14 includes kubernetes-sigs/controller-runtime#2025 which turned out to be a breaking change for any tests that use fake controller-runtime client for list operations against the index. This commit addresses that, by extracting the indexer func out of the reconciler setup function so that both the reconciler setup func and the set-only fake client setup func can use the same indexer func. This fixes integration errors like the below example: ``` --- FAIL: TestWebhookWorkflowJobWithSelfHostedLabel (0.00s) --- FAIL: TestWebhookWorkflowJobWithSelfHostedLabel/Successful (0.00s) horizontal_runner_autoscaler_webhook_test.go:256: status: 500 horizontal_runner_autoscaler_webhook_test.go:256: body: List on GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler specifies selector on field scaleTarget, but no index with name scaleTarget has been registered for GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler horizontal_runner_autoscaler_webhook_test.go:440: diagnostics: testlog] finding repository-wide runnerrepository.name=MYREPO repository.owner.login=MYORG repository.owner.type=Organization action=queued delivery= workflowJob.labels=[self-hosted label1] workflowJob.status=queued enterprise.slug= workflowJob.runID=1234567890 workflowJob.ID=1234567890 event=workflow_job hookID= repository=MYORG/MYREPO error=List on GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler specifies selector on field scaleTarget, but no index with name scaleTarget has been registered for GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler testlog] handling check_run eventevent=workflow_job hookID= workflowJob.status=queued enterprise.slug= workflowJob.runID=1234567890 workflowJob.ID=1234567890 delivery= workflowJob.labels=[self-hosted label1] repository.name=MYREPO repository.owner.login=MYORG repository.owner.type=Organization action=queued error=List on GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler specifies selector on field scaleTarget, but no index with name scaleTarget has been registered for GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler ```
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime/pull/2025
…troller-runtime controller-runtime 0.14 includes kubernetes-sigs/controller-runtime#2025 which turned out to be a breaking change for any tests that use fake controller-runtime client for list operations against the index. This commit addresses that, by extracting the indexer func out of the reconciler setup function so that both the reconciler setup func and the set-only fake client setup func can use the same indexer func. This fixes integration errors like the below example: ``` --- FAIL: TestWebhookWorkflowJobWithSelfHostedLabel (0.00s) --- FAIL: TestWebhookWorkflowJobWithSelfHostedLabel/Successful (0.00s) horizontal_runner_autoscaler_webhook_test.go:256: status: 500 horizontal_runner_autoscaler_webhook_test.go:256: body: List on GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler specifies selector on field scaleTarget, but no index with name scaleTarget has been registered for GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler horizontal_runner_autoscaler_webhook_test.go:440: diagnostics: testlog] finding repository-wide runnerrepository.name=MYREPO repository.owner.login=MYORG repository.owner.type=Organization action=queued delivery= workflowJob.labels=[self-hosted label1] workflowJob.status=queued enterprise.slug= workflowJob.runID=1234567890 workflowJob.ID=1234567890 event=workflow_job hookID= repository=MYORG/MYREPO error=List on GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler specifies selector on field scaleTarget, but no index with name scaleTarget has been registered for GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler testlog] handling check_run eventevent=workflow_job hookID= workflowJob.status=queued enterprise.slug= workflowJob.runID=1234567890 workflowJob.ID=1234567890 delivery= workflowJob.labels=[self-hosted label1] repository.name=MYREPO repository.owner.login=MYORG repository.owner.type=Organization action=queued error=List on GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler specifies selector on field scaleTarget, but no index with name scaleTarget has been registered for GroupVersionKind actions.summerwind.dev/v1alpha1, Kind=HorizontalRunnerAutoscaler ``` Signed-off-by: Yusuke Kuoka <[email protected]>
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime/pull/2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime/pull/2025
…ler-runtime `WithIndex` function kubernetes-sigs/controller-runtime/pull/2025
…`v0.14.1` (#7248) * Update `k8s.io/*` to `v0.26.0` * Update `sigs.k8s.io/controller-runtime` to `v0.14.1` * Set `RecoverPanic` globally for the manager of `controller-manager` controllers * Set `RecoverPanic` globally for the manager of `gardenlet` controllers * Set `RecoverPanic` globally for the manager of `operator` controllers * Set `RecoverPanic` globally for the manager of `resource-manager` controllers * Set `RecoverPanic` globally for the manager of `scheduler` controllers * Set `RecoverPanic` globally in manager options for extension controllers * Change RecoverPanic bool to bool pointer for other controllers * Run `make generate` * Set `AllowInvalidLabelValueInSelector` to true in LabelSelectorValidationOptions for backwards compatibility See kubernetes/kubernetes#113699 for more details * Drop `Not` predicate util function in favor of controller-runtime `predicate.Not` * Drop `NewClientWithFieldSelectorSupport` function in favor of controller-runtime `WithIndex` function kubernetes-sigs/controller-runtime/pull/2025 * Use `Build()` function for all controllers * Use Subresource client for `shoots/binding` and drop generated clientset * Use Subresource client for `shoots/adminkubeconfig` and drop generated clientset * Use Status() client for Status Ref: kubernetes-sigs/controller-runtime#2072 * Adapt unit tests with mock StatusWriter * Add "ValidatingAdmissionPolicy" to default admission plugins * Adapt changes related to removed fields in kube-proxy config * Add unit test cases for "#IsAdmissionPluginSupported" function * Update envtest version * Return error from `informer.AddEventHandler` * Copy onsi/gomega/format package also to gomegacheck testdata * Address PR review feedback * Vendor current `etcd-druid` master * Use `builder.Watches()` wherever possible * Call `etcdOptions.Complete()` for gardener apiserver config etcd-encryption is supported out-of-the-box now. ref: kubernetes/kubernetes#112789 * Use Subresource client for `serviceaccounts/token` wherever possible * Update `ahmetb/gen-crd-api-reference-docs` to `0.3.0` * Update `sigs.k8s.io/controller-tools` to `v0.11.0` Update to `v0.11.1` once kubernetes/kubernetes/pull/114617 is released. In k8s v0.26.0 the CRD generation is broken. * Remove unneeded dependencies for `gardener-scheduler` from skaffold.yaml * Hardcode `RecoverPanic` to true for extensions controller * Use `k8s.io/apimachinery/pkg/util/sets` and drop copied packages * Drop ready check for garden informer sync for gardenlet Adding ready check to manager is not possible after it's started, since controller-runtime, `v0.11.0`. However the check was broken, and it is now fixed in kubernetes-sigs/controller-runtime#2090, so we have to drop this. * Adapt `provider-local` webhook * Address PR review feedback * Fix panic in ManagedSeed controller Contexts: from this PR, we're not setting RecoverPanic to true in tests * Vendor `etcd-druid` `v0.15.3` * Update `k8s.io/*` and `controller-runtime` k8s.io/* - 0.26.0=>0.26.1 controller-tools - 0.11.0=>0.11.1 * Run `make generate` * Rebase * Address PR review feedback * Fix failing test * Adapt `highavailabilityconfig` webhook integration test `autoscaling/v2beta2.HorizontalPodAutoscaler` is removed in v1.26. Ref: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-26 * Use `apiutil.NewDynamicRESTMapper` for Manager cache in tests Co-authored-by: Rafael Franzke <[email protected]> * Don't set `RecoverPanic` for os extensions in AddToManager This is not required since we call `mgrOpts.Options()` here : https://github.com/gardener/gardener/blob/c9cb564d1adad0a1ecf9d44f23fb249bcf946a79/extensions/pkg/controller/operatingsystemconfig/oscommon/app/app.go#L88 * Truncate time in test to microsecond precision Ref: kubernetes/kubernetes#111936 * Rebase --------- Co-authored-by: Rafael Franzke <[email protected]>
controller-runtime deprecated the envtest/printer package in v0.14.0. They don't needed anymore since they upgraded to ginko v2. In order to keep the scope contained, I just copied the code we needed, it's quite simple. This allow us to maintain the same version of ginko while bumping controller-runtime. The fake client provided by controller-runtime was updated to allow for field selectors in List operations. This requires to define the index prior to its use. Since the Job controller uses ".metadata.controller" for a List query, we now set this index in the tests client. kubernetes-sigs/controller-runtime#2025 Signed-off-by: Guillermo Gaston <[email protected]>
…`v0.14.1` (gardener#7248) * Update `k8s.io/*` to `v0.26.0` * Update `sigs.k8s.io/controller-runtime` to `v0.14.1` * Set `RecoverPanic` globally for the manager of `controller-manager` controllers * Set `RecoverPanic` globally for the manager of `gardenlet` controllers * Set `RecoverPanic` globally for the manager of `operator` controllers * Set `RecoverPanic` globally for the manager of `resource-manager` controllers * Set `RecoverPanic` globally for the manager of `scheduler` controllers * Set `RecoverPanic` globally in manager options for extension controllers * Change RecoverPanic bool to bool pointer for other controllers * Run `make generate` * Set `AllowInvalidLabelValueInSelector` to true in LabelSelectorValidationOptions for backwards compatibility See kubernetes/kubernetes#113699 for more details * Drop `Not` predicate util function in favor of controller-runtime `predicate.Not` * Drop `NewClientWithFieldSelectorSupport` function in favor of controller-runtime `WithIndex` function kubernetes-sigs/controller-runtime/pull/2025 * Use `Build()` function for all controllers * Use Subresource client for `shoots/binding` and drop generated clientset * Use Subresource client for `shoots/adminkubeconfig` and drop generated clientset * Use Status() client for Status Ref: kubernetes-sigs/controller-runtime#2072 * Adapt unit tests with mock StatusWriter * Add "ValidatingAdmissionPolicy" to default admission plugins * Adapt changes related to removed fields in kube-proxy config * Add unit test cases for "#IsAdmissionPluginSupported" function * Update envtest version * Return error from `informer.AddEventHandler` * Copy onsi/gomega/format package also to gomegacheck testdata * Address PR review feedback * Vendor current `etcd-druid` master * Use `builder.Watches()` wherever possible * Call `etcdOptions.Complete()` for gardener apiserver config etcd-encryption is supported out-of-the-box now. ref: kubernetes/kubernetes#112789 * Use Subresource client for `serviceaccounts/token` wherever possible * Update `ahmetb/gen-crd-api-reference-docs` to `0.3.0` * Update `sigs.k8s.io/controller-tools` to `v0.11.0` Update to `v0.11.1` once kubernetes/kubernetes/pull/114617 is released. In k8s v0.26.0 the CRD generation is broken. * Remove unneeded dependencies for `gardener-scheduler` from skaffold.yaml * Hardcode `RecoverPanic` to true for extensions controller * Use `k8s.io/apimachinery/pkg/util/sets` and drop copied packages * Drop ready check for garden informer sync for gardenlet Adding ready check to manager is not possible after it's started, since controller-runtime, `v0.11.0`. However the check was broken, and it is now fixed in kubernetes-sigs/controller-runtime#2090, so we have to drop this. * Adapt `provider-local` webhook * Address PR review feedback * Fix panic in ManagedSeed controller Contexts: from this PR, we're not setting RecoverPanic to true in tests * Vendor `etcd-druid` `v0.15.3` * Update `k8s.io/*` and `controller-runtime` k8s.io/* - 0.26.0=>0.26.1 controller-tools - 0.11.0=>0.11.1 * Run `make generate` * Rebase * Address PR review feedback * Fix failing test * Adapt `highavailabilityconfig` webhook integration test `autoscaling/v2beta2.HorizontalPodAutoscaler` is removed in v1.26. Ref: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-26 * Use `apiutil.NewDynamicRESTMapper` for Manager cache in tests Co-authored-by: Rafael Franzke <[email protected]> * Don't set `RecoverPanic` for os extensions in AddToManager This is not required since we call `mgrOpts.Options()` here : https://github.com/gardener/gardener/blob/c9cb564d1adad0a1ecf9d44f23fb249bcf946a79/extensions/pkg/controller/operatingsystemconfig/oscommon/app/app.go#L88 * Truncate time in test to microsecond precision Ref: kubernetes/kubernetes#111936 * Rebase --------- Co-authored-by: Rafael Franzke <[email protected]>
Allow registration of indexes into the fake client to allow usage of field selectors when doing a List. The indexing is done lazily by doing a normal list and then filtering the results by computing the index value for each list item.
To enable the main change for this commit, some refactorings are performed: an internal and unexported function that checks whether a field selector is in the form key=val or key==val is made internal and exported to be shared between real and faked code to ensure loyalty. Unit tests for it are added.
Co-authored-by: Paul Eichler [email protected]
This PR addresses #1948 (comment) and is a reincarnation of #800