fix: ensure VS is updated when delegation is enabled #1818
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.
This PR fixes a bug where Flagger failed to remove
gateways
andhosts
from an IstioVirtualService
when the Canary was updated to enable the delegation feature.The Problem
When a user enables the istio route delegation by setting
spec.service.delegation: true
and removinggateways
andhosts
from theCanary
spec, the reconciliation logic did not properly update the managedVirtualService
.See #1464 for more details.
The Solution
The reconciliation logic has been updated to explicitly trigger a update on the VS removing the
gateways
andhosts
when delegation is enabled. This ensures the managedVirtualService
is correctly synced.I'm not sure if this is the best solution since at the core the problem seems to be that the reconciliation logic is doing a diff but because the
virtualService.Spec
(VS that is read from the cluster) is updated on the fly with empty ([]
) hosts and gateways, the removal ofhosts
andgateways
are never detected by the diff.I thought the best option would be to simply NOT modify the
virtualService
variable, but this seemed to generate other bugs where aflagger.kubernetes.io/original-configuration
annotation was added on the VS and the canary got stuck in progressing at 10% traffic, which I couldn't fully understand how to fix.Testing
To validate the fix I have done the following:
Additional notes
This issue has some quirky workarounds that actually triggers the diff and in-turn causes the VS to be updated, for example, with the E2E I found out that setting
portDiscovery: true
causes the diff logic to trigger and update the VS as expected.Fixes #1464