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

Update list to filter resources using label selectors #488

Merged
merged 3 commits into from
Nov 8, 2023

Conversation

jtucci
Copy link
Contributor

@jtucci jtucci commented Sep 28, 2023

What this PR does / why we need it:

Currently, If no name is specified in an assert resource, the CheckResource and CheckResourceAbsent will find all resources of with the same gvk and assert against one of the resources returned in the list. This PR is a proposal to enhance how resources are matched by filtering the resources by any included labels in the case where no name is specified.

Example Use Case / Scenario
We are using KUTTL for e2e testing for the crossplane compositions we are developing. The resources we are asserting against (managed resources) are created in a similar way to pods being created by a replica set, in that the metadata.name will contain a random uuid. In the case where there are several resources of the same gvk, we are unable to guarantee that the correct resource is asserted against which causes unexpected failures within the tests. The only other workaround is running script commands such as:

test "$(kubectl get virtualnetworkpeering.network.azure.upbound.io -o json | jq -r '.items[] |
      select(.metadata.labels.name=="virtual-network-peering-to-target") |
      .metadata.annotations["crossplane.io/external-name"]')" = "to-shoot--test--test-dev-us"`

This is very tedious (we have to do this for every field in the resource(s)). It also makes understanding any test failures difficult since no diff is shown, just that a command failed.

This enhancement would give us the ability to instead assert against the yaml definition of the resource like any other test would because we can specify unique labels to identify the resources. For example, the following two resources with no name and the same gvk would be asserted against the correct resource with the matching labels

---
apiVersion: network.azure.upbound.io/v1beta1
kind: VirtualNetworkPeering
metadata:
  annotations:
    crossplane.io/external-name: to-shoot--test--test-dev-us
  labels:
    name: virtual-network-peering-to-target
spec:
  deletionPolicy: Delete
  forProvider:
    remoteVirtualNetworkId: >-
      /subscriptions/xxxxxxxxx/resourceGroups/shoot--test--test-dev-us/providers/Microsoft.Network/virtualNetworks/shoot--test--test-dev-us
    resourceGroupNameSelector:
      matchControllerRef: true
      policy:
        resolution: Required
        resolve: Always
    virtualNetworkNameSelector:
      matchControllerRef: true
      policy:
        resolution: Required
        resolve: Always
---
apiVersion: network.azure.upbound.io/v1beta1
kind: VirtualNetworkPeering
metadata:
  annotations:
    crossplane.io/composition-resource-name: virtual-network-peering-from-target
    crossplane.io/external-name: from-shoot--test--test-dev-us
  labels:
    name: virtual-network-peering-from-target
spec:
  deletionPolicy: Delete
  forProvider:
    remoteVirtualNetworkIdSelector:
      matchControllerRef: true
      policy:
        resolution: Required
        resolve: Always
    resourceGroupName: shoot--test--test-dev-us
    virtualNetworkName: shoot--test--test-dev-us
---

Fixes #480

Copy link
Member

@porridge porridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome! Would you mind adding some tests?

pkg/test/step.go Outdated Show resolved Hide resolved
@jtucci
Copy link
Contributor Author

jtucci commented Oct 4, 2023

This looks awesome! Would you mind adding some tests?

@porridge Sure thing! I just updated the test cases to allow for several 'actual' resources to be created and added a case to test the label matching.

pkg/test/step.go Outdated Show resolved Hide resolved
Signed-off-by: jtucci <jonytucci01@gmail.com>
Signed-off-by: jtucci <jonytucci01@gmail.com>
Copy link
Member

@porridge porridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error returning fix LGTM overall, but I have one worry, see inline.
Also, please take a look at the TestCheckResourceIntegration/match_object_by_labels,_field_mismatch that started failing.

pkg/test/step.go Show resolved Hide resolved
@porridge
Copy link
Member

[...] please take a look at the TestCheckResourceIntegration/match_object_by_labels,_field_mismatch that started failing.

Any ideas about this one, @jtucci ?

@jtucci
Copy link
Contributor Author

jtucci commented Nov 1, 2023

[...] please take a look at the TestCheckResourceIntegration/match_object_by_labels,_field_mismatch that started failing.

Any ideas about this one, @jtucci ?

I just pushed the fix, the integration tests are passing locally for me now.

Signed-off-by: jtucci <jonytucci01@gmail.com>
@jtucci jtucci requested a review from porridge November 3, 2023 17:35
Copy link
Member

@porridge porridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@porridge porridge merged commit 46a545d into kudobuilder:main Nov 8, 2023
4 checks passed
porridge added a commit that referenced this pull request Jan 31, 2024
See PR #488.

Signed-off-by: Marcin Owsiany <porridge@redhat.com>
porridge added a commit that referenced this pull request Feb 7, 2024
See PR #488.

Signed-off-by: Marcin Owsiany <porridge@redhat.com>
This pull request was closed.
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.

Kuttl should be able to match resource on label if metadata.name is not provided
2 participants