[fix: integration-tests] Ensure Istio gateways are ready #185
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d') by setting ClusterIP service type
guicassolato
added
kind/bug
guicassolato
changed the title
eguzki
approved these changes
May 10, 2023
art-tapin
added a commit
that referenced
this pull request
May 17, 2023
* test: Add unit-tests for UnMarshallLimitNamespace (#167) * test: Add two FAILING unit-tests for UnMarshallLimitNamespace (#167) The two failing test cases include: - when namespace has no gateway name then return an error - when namespace has no domain name then return an error In these test-cases we leave the separator (either "/" or "#"), which causes the function to return an error when the gateway name or domain name is missing. In the last two test cases: - when namespace only has gateway name (missing 'namespace/') and domain then return an error - when namespace only has namespace name (missing '/gwName') and domain then return an error the separator (/) is missing along with the namespace or gateway names, causing the function to return the correct error message. * refactor: Add comment to UnMarshallLimitNamespace (#167) * test: Add unit-tests for MarshallNamespace (#167) * test: Add unit-tests for UnMarshallObjectKey (#167) * test: Add unit-tests for HostnamesToStrings (#167) * [test] Optimizations, improvements, and unit tests for common/common.go (part 2 of 3) (#182) * test: Add unit-tests for Contains (#167) * refactor: Add comment to Contains (#167) * test: Add unit-tests for Map (#167) * refactor: Add comment to Map (#167) * refactor: Add names to Map test cases (#167) * test: Add unit-tests for SliceCopy (#167) * refactor: Add comment to SliceCopy (#167) * test: Add unit-tests for ReverseSlice (#167) * refactor: Add comment to ReverseSlice (#167) * test: Add unit-tests for MergeMapStringString (#167) * refactor: Add missing import 'reflect' * fix: Ensure Istio gateways created in the tests are ready ('Programmed') by setting ClusterIP service type (#185) * changelog (#184) * test: Add two test-cases for UnMarshallObjectKey (#167) 1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only 2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only Changes were discussed here: #187 (comment) * test: Delete test case for UnMarshallLimitNamespace The test case "when namespace has no gateway name then return an error" has been removed from the test suite for UnMarshallLimitNamespace. This test case was tightly connected to the implementation of UnMarshallObjectKey, making it impossible to properly test the desired behavior in the context of UnMarshallLimitNamespace. * refactor: Add clarity to the test name Describing the '/' separator as a default one we show that it is a default constant value. --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
art-tapin
added a commit
that referenced
this pull request
May 18, 2023
* test: Add unit-tests for UnMarshallLimitNamespace (#167) * test: Add two FAILING unit-tests for UnMarshallLimitNamespace (#167) The two failing test cases include: - when namespace has no gateway name then return an error - when namespace has no domain name then return an error In these test-cases we leave the separator (either "/" or "#"), which causes the function to return an error when the gateway name or domain name is missing. In the last two test cases: - when namespace only has gateway name (missing 'namespace/') and domain then return an error - when namespace only has namespace name (missing '/gwName') and domain then return an error the separator (/) is missing along with the namespace or gateway names, causing the function to return the correct error message. * refactor: Add comment to UnMarshallLimitNamespace (#167) * test: Add unit-tests for MarshallNamespace (#167) * test: Add unit-tests for UnMarshallObjectKey (#167) * test: Add unit-tests for HostnamesToStrings (#167) * [test] Optimizations, improvements, and unit tests for common/common.go (part 2 of 3) (#182) * test: Add unit-tests for Contains (#167) * refactor: Add comment to Contains (#167) * test: Add unit-tests for Map (#167) * refactor: Add comment to Map (#167) * refactor: Add names to Map test cases (#167) * test: Add unit-tests for SliceCopy (#167) * refactor: Add comment to SliceCopy (#167) * test: Add unit-tests for ReverseSlice (#167) * refactor: Add comment to ReverseSlice (#167) * test: Add unit-tests for MergeMapStringString (#167) * refactor: Add missing import 'reflect' * fix: Ensure Istio gateways created in the tests are ready ('Programmed') by setting ClusterIP service type (#185) * changelog (#184) * test: Add two test-cases for UnMarshallObjectKey (#167) 1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only 2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only Changes were discussed here: #187 (comment) * test: Delete test case for UnMarshallLimitNamespace The test case "when namespace has no gateway name then return an error" has been removed from the test suite for UnMarshallLimitNamespace. This test case was tightly connected to the implementation of UnMarshallObjectKey, making it impossible to properly test the desired behavior in the context of UnMarshallLimitNamespace. * refactor: Add clarity to the test name Describing the '/' separator as a default one we show that it is a default constant value. --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
didierofrivia
pushed a commit
that referenced
this pull request
May 18, 2023
* optimisation: Improve performance of UnMarshallObjectKey This commit optimizes the UnMarshallObjectKey function by avoiding unnecessary allocations and using a simple string split operation instead of manual string parsing. This results in performance improvements when processing large volumes of Kubernetes objects. Additionally, the function now validates input strings more accurately to prevent errors when working with invalid object keys. * optimisation: Improve performance of UnMarshallLimitNamespace This commit optimizes the UnMarshallLimitNamespace function to improve its runtime performance. The following changes were made: - Used strings.IndexByte instead of strings.Split to search for the index of the '#' delimiter character, reducing the number of iterations and memory allocation overhead. - Extracted the domain directly from the input string instead of creating a temporary slice, reducing the number of string allocations. - Passed only the first half of the input string to UnMarshallObjectKey function, avoiding the creation of unnecessary intermediate strings. * optimisation: improve performance of HostnamesToStrings (#167) - Pre-allocated output slice using make function to reduce append overhead * refactor: Replace errors.New(...) with fmt.Errorf(...) * refactor: Simplify if-condition in UnMarshallObjectKey to handle empty namespace and name values + refactor the error message * fix: Use IndexRune instead of IndexByte to handle non-ascii characters in UnMarshallLimitNamespace and UnMarshallObjectKey Discussion: #187 (comment) Kudos to Eguzki * [test] Unit-tests for common/common.go (part 3 of 3) (#186) * test: Add unit-tests for UnMarshallLimitNamespace (#167) * test: Add two FAILING unit-tests for UnMarshallLimitNamespace (#167) The two failing test cases include: - when namespace has no gateway name then return an error - when namespace has no domain name then return an error In these test-cases we leave the separator (either "/" or "#"), which causes the function to return an error when the gateway name or domain name is missing. In the last two test cases: - when namespace only has gateway name (missing 'namespace/') and domain then return an error - when namespace only has namespace name (missing '/gwName') and domain then return an error the separator (/) is missing along with the namespace or gateway names, causing the function to return the correct error message. * refactor: Add comment to UnMarshallLimitNamespace (#167) * test: Add unit-tests for MarshallNamespace (#167) * test: Add unit-tests for UnMarshallObjectKey (#167) * test: Add unit-tests for HostnamesToStrings (#167) * [test] Optimizations, improvements, and unit tests for common/common.go (part 2 of 3) (#182) * test: Add unit-tests for Contains (#167) * refactor: Add comment to Contains (#167) * test: Add unit-tests for Map (#167) * refactor: Add comment to Map (#167) * refactor: Add names to Map test cases (#167) * test: Add unit-tests for SliceCopy (#167) * refactor: Add comment to SliceCopy (#167) * test: Add unit-tests for ReverseSlice (#167) * refactor: Add comment to ReverseSlice (#167) * test: Add unit-tests for MergeMapStringString (#167) * refactor: Add missing import 'reflect' * fix: Ensure Istio gateways created in the tests are ready ('Programmed') by setting ClusterIP service type (#185) * changelog (#184) * test: Add two test-cases for UnMarshallObjectKey (#167) 1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only 2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only Changes were discussed here: #187 (comment) * test: Delete test case for UnMarshallLimitNamespace The test case "when namespace has no gateway name then return an error" has been removed from the test suite for UnMarshallLimitNamespace. This test case was tightly connected to the implementation of UnMarshallObjectKey, making it impossible to properly test the desired behavior in the context of UnMarshallLimitNamespace. * refactor: Add clarity to the test name Describing the '/' separator as a default one we show that it is a default constant value. --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com> --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
didierofrivia
pushed a commit
that referenced
this pull request
May 24, 2023
* optimisation: Improve performance of UnMarshallObjectKey This commit optimizes the UnMarshallObjectKey function by avoiding unnecessary allocations and using a simple string split operation instead of manual string parsing. This results in performance improvements when processing large volumes of Kubernetes objects. Additionally, the function now validates input strings more accurately to prevent errors when working with invalid object keys. * optimisation: Improve performance of UnMarshallLimitNamespace This commit optimizes the UnMarshallLimitNamespace function to improve its runtime performance. The following changes were made: - Used strings.IndexByte instead of strings.Split to search for the index of the '#' delimiter character, reducing the number of iterations and memory allocation overhead. - Extracted the domain directly from the input string instead of creating a temporary slice, reducing the number of string allocations. - Passed only the first half of the input string to UnMarshallObjectKey function, avoiding the creation of unnecessary intermediate strings. * optimisation: improve performance of HostnamesToStrings (#167) - Pre-allocated output slice using make function to reduce append overhead * refactor: Replace errors.New(...) with fmt.Errorf(...) * refactor: Simplify if-condition in UnMarshallObjectKey to handle empty namespace and name values + refactor the error message * fix: Use IndexRune instead of IndexByte to handle non-ascii characters in UnMarshallLimitNamespace and UnMarshallObjectKey Discussion: #187 (comment) Kudos to Eguzki * [test] Unit-tests for common/common.go (part 3 of 3) (#186) * test: Add unit-tests for UnMarshallLimitNamespace (#167) * test: Add two FAILING unit-tests for UnMarshallLimitNamespace (#167) The two failing test cases include: - when namespace has no gateway name then return an error - when namespace has no domain name then return an error In these test-cases we leave the separator (either "/" or "#"), which causes the function to return an error when the gateway name or domain name is missing. In the last two test cases: - when namespace only has gateway name (missing 'namespace/') and domain then return an error - when namespace only has namespace name (missing '/gwName') and domain then return an error the separator (/) is missing along with the namespace or gateway names, causing the function to return the correct error message. * refactor: Add comment to UnMarshallLimitNamespace (#167) * test: Add unit-tests for MarshallNamespace (#167) * test: Add unit-tests for UnMarshallObjectKey (#167) * test: Add unit-tests for HostnamesToStrings (#167) * [test] Optimizations, improvements, and unit tests for common/common.go (part 2 of 3) (#182) * test: Add unit-tests for Contains (#167) * refactor: Add comment to Contains (#167) * test: Add unit-tests for Map (#167) * refactor: Add comment to Map (#167) * refactor: Add names to Map test cases (#167) * test: Add unit-tests for SliceCopy (#167) * refactor: Add comment to SliceCopy (#167) * test: Add unit-tests for ReverseSlice (#167) * refactor: Add comment to ReverseSlice (#167) * test: Add unit-tests for MergeMapStringString (#167) * refactor: Add missing import 'reflect' * fix: Ensure Istio gateways created in the tests are ready ('Programmed') by setting ClusterIP service type (#185) * changelog (#184) * test: Add two test-cases for UnMarshallObjectKey (#167) 1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only 2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only Changes were discussed here: #187 (comment) * test: Delete test case for UnMarshallLimitNamespace The test case "when namespace has no gateway name then return an error" has been removed from the test suite for UnMarshallLimitNamespace. This test case was tightly connected to the implementation of UnMarshallObjectKey, making it impossible to properly test the desired behavior in the context of UnMarshallLimitNamespace. * refactor: Add clarity to the test name Describing the '/' separator as a default one we show that it is a default constant value. --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com> --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
didierofrivia
pushed a commit
that referenced
this pull request
May 25, 2023
* optimisation: Improve performance of UnMarshallObjectKey This commit optimizes the UnMarshallObjectKey function by avoiding unnecessary allocations and using a simple string split operation instead of manual string parsing. This results in performance improvements when processing large volumes of Kubernetes objects. Additionally, the function now validates input strings more accurately to prevent errors when working with invalid object keys. * optimisation: Improve performance of UnMarshallLimitNamespace This commit optimizes the UnMarshallLimitNamespace function to improve its runtime performance. The following changes were made: - Used strings.IndexByte instead of strings.Split to search for the index of the '#' delimiter character, reducing the number of iterations and memory allocation overhead. - Extracted the domain directly from the input string instead of creating a temporary slice, reducing the number of string allocations. - Passed only the first half of the input string to UnMarshallObjectKey function, avoiding the creation of unnecessary intermediate strings. * optimisation: improve performance of HostnamesToStrings (#167) - Pre-allocated output slice using make function to reduce append overhead * refactor: Replace errors.New(...) with fmt.Errorf(...) * refactor: Simplify if-condition in UnMarshallObjectKey to handle empty namespace and name values + refactor the error message * fix: Use IndexRune instead of IndexByte to handle non-ascii characters in UnMarshallLimitNamespace and UnMarshallObjectKey Discussion: #187 (comment) Kudos to Eguzki * [test] Unit-tests for common/common.go (part 3 of 3) (#186) * test: Add unit-tests for UnMarshallLimitNamespace (#167) * test: Add two FAILING unit-tests for UnMarshallLimitNamespace (#167) The two failing test cases include: - when namespace has no gateway name then return an error - when namespace has no domain name then return an error In these test-cases we leave the separator (either "/" or "#"), which causes the function to return an error when the gateway name or domain name is missing. In the last two test cases: - when namespace only has gateway name (missing 'namespace/') and domain then return an error - when namespace only has namespace name (missing '/gwName') and domain then return an error the separator (/) is missing along with the namespace or gateway names, causing the function to return the correct error message. * refactor: Add comment to UnMarshallLimitNamespace (#167) * test: Add unit-tests for MarshallNamespace (#167) * test: Add unit-tests for UnMarshallObjectKey (#167) * test: Add unit-tests for HostnamesToStrings (#167) * [test] Optimizations, improvements, and unit tests for common/common.go (part 2 of 3) (#182) * test: Add unit-tests for Contains (#167) * refactor: Add comment to Contains (#167) * test: Add unit-tests for Map (#167) * refactor: Add comment to Map (#167) * refactor: Add names to Map test cases (#167) * test: Add unit-tests for SliceCopy (#167) * refactor: Add comment to SliceCopy (#167) * test: Add unit-tests for ReverseSlice (#167) * refactor: Add comment to ReverseSlice (#167) * test: Add unit-tests for MergeMapStringString (#167) * refactor: Add missing import 'reflect' * fix: Ensure Istio gateways created in the tests are ready ('Programmed') by setting ClusterIP service type (#185) * changelog (#184) * test: Add two test-cases for UnMarshallObjectKey (#167) 1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only 2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only Changes were discussed here: #187 (comment) * test: Delete test case for UnMarshallLimitNamespace The test case "when namespace has no gateway name then return an error" has been removed from the test suite for UnMarshallLimitNamespace. This test case was tightly connected to the implementation of UnMarshallObjectKey, making it impossible to properly test the desired behavior in the context of UnMarshallLimitNamespace. * refactor: Add clarity to the test name Describing the '/' separator as a default one we show that it is a default constant value. --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com> --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
didierofrivia
pushed a commit
that referenced
this pull request
May 31, 2023
* optimisation: Improve performance of UnMarshallObjectKey This commit optimizes the UnMarshallObjectKey function by avoiding unnecessary allocations and using a simple string split operation instead of manual string parsing. This results in performance improvements when processing large volumes of Kubernetes objects. Additionally, the function now validates input strings more accurately to prevent errors when working with invalid object keys. * optimisation: Improve performance of UnMarshallLimitNamespace This commit optimizes the UnMarshallLimitNamespace function to improve its runtime performance. The following changes were made: - Used strings.IndexByte instead of strings.Split to search for the index of the '#' delimiter character, reducing the number of iterations and memory allocation overhead. - Extracted the domain directly from the input string instead of creating a temporary slice, reducing the number of string allocations. - Passed only the first half of the input string to UnMarshallObjectKey function, avoiding the creation of unnecessary intermediate strings. * optimisation: improve performance of HostnamesToStrings (#167) - Pre-allocated output slice using make function to reduce append overhead * refactor: Replace errors.New(...) with fmt.Errorf(...) * refactor: Simplify if-condition in UnMarshallObjectKey to handle empty namespace and name values + refactor the error message * fix: Use IndexRune instead of IndexByte to handle non-ascii characters in UnMarshallLimitNamespace and UnMarshallObjectKey Discussion: #187 (comment) Kudos to Eguzki * [test] Unit-tests for common/common.go (part 3 of 3) (#186) * test: Add unit-tests for UnMarshallLimitNamespace (#167) * test: Add two FAILING unit-tests for UnMarshallLimitNamespace (#167) The two failing test cases include: - when namespace has no gateway name then return an error - when namespace has no domain name then return an error In these test-cases we leave the separator (either "/" or "#"), which causes the function to return an error when the gateway name or domain name is missing. In the last two test cases: - when namespace only has gateway name (missing 'namespace/') and domain then return an error - when namespace only has namespace name (missing '/gwName') and domain then return an error the separator (/) is missing along with the namespace or gateway names, causing the function to return the correct error message. * refactor: Add comment to UnMarshallLimitNamespace (#167) * test: Add unit-tests for MarshallNamespace (#167) * test: Add unit-tests for UnMarshallObjectKey (#167) * test: Add unit-tests for HostnamesToStrings (#167) * [test] Optimizations, improvements, and unit tests for common/common.go (part 2 of 3) (#182) * test: Add unit-tests for Contains (#167) * refactor: Add comment to Contains (#167) * test: Add unit-tests for Map (#167) * refactor: Add comment to Map (#167) * refactor: Add names to Map test cases (#167) * test: Add unit-tests for SliceCopy (#167) * refactor: Add comment to SliceCopy (#167) * test: Add unit-tests for ReverseSlice (#167) * refactor: Add comment to ReverseSlice (#167) * test: Add unit-tests for MergeMapStringString (#167) * refactor: Add missing import 'reflect' * fix: Ensure Istio gateways created in the tests are ready ('Programmed') by setting ClusterIP service type (#185) * changelog (#184) * test: Add two test-cases for UnMarshallObjectKey (#167) 1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only 2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only Changes were discussed here: #187 (comment) * test: Delete test case for UnMarshallLimitNamespace The test case "when namespace has no gateway name then return an error" has been removed from the test suite for UnMarshallLimitNamespace. This test case was tightly connected to the implementation of UnMarshallObjectKey, making it impossible to properly test the desired behavior in the context of UnMarshallLimitNamespace. * refactor: Add clarity to the test name Describing the '/' separator as a default one we show that it is a default constant value. --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com> --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ensures Istio gateways created during the integration tests are ready (
Programmed == true), by settingClusterIPservice type instead ofLoadBalancer.This is because in the Kubernetes clusters started with Kind, where no external load balancer has been configured, the
EXTERNAL-IPof theLoadBalancer-type service created by default associated with each Gateway API gateway will remain in<pending>state forever (see istio/istio.io#12630 (comment)). As a consequence, theProgrammedcondition of the gateway resource is invalid, failing with message:Failed to assign to any requested addresses: no instances found for hostname […].Fixes https://github.com/Kuadrant/kuadrant-operator/actions/runs/4932380443/jobs/8815343870.