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

Deployment traffic filters are computed to not conflict with a traffic filter association resource #632

Merged
merged 6 commits into from
May 15, 2023

Conversation

tobio
Copy link
Member

@tobio tobio commented Apr 30, 2023

Description

Marks the ec_deployment traffic_filter attribute as computed, since it can also be managed via a ec_deployment_traffic_filter_association resource.

Related Issues

Fixes #621
Fixes #419

How Has This Been Tested?

Manually, acceptance tests

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improves code quality but has no user-facing effect)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation

Readiness Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@tobio tobio requested a review from dimuon April 30, 2023 12:32
@tobio tobio requested a review from a team as a code owner April 30, 2023 12:32
@tobio tobio self-assigned this Apr 30, 2023
Copy link

@webfella webfella left a comment

Choose a reason for hiding this comment

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

LGTM once the test is updated 👍

@dimuon
Copy link
Contributor

dimuon commented May 1, 2023

@tobio , I'm curious why the test fails. Did we come across a defect?

@tobio
Copy link
Member Author

tobio commented May 1, 2023

Did we come across a defect?

Previously the failing test was expecting that removing the traffic_filter attribute from a deployment should also remove the traffic filters from ESS. This PR breaks that, since it would then treat the attribute as Unknown and use the value from state in the plan, i.e leave the traffic filter present.

It's a legitimate failure, I should push a fix for it shortly.

@dimuon
Copy link
Contributor

dimuon commented May 2, 2023

@tobio , will it work in case of imported resource?

@dimuon
Copy link
Contributor

dimuon commented May 2, 2023

I'm curious whether the plan modifier can lead to the inconsistent result errors. E.g. once the modifier set new value to traffic_filter and subsequent read returns another value from backend, e.g. a rule that is added by traffic filter assoc resource or UI.
Can we just fix the test by explicitly setting traffic_filter = [] instead of relying on the plan modifier?

@tobio
Copy link
Member Author

tobio commented May 3, 2023

I'm curious whether the plan modifier can lead to the inconsistent result errors. E.g. once the modifier set new value to traffic_filter and subsequent read returns another value from backend, e.g. a rule that is added by traffic filter assoc resource or UI.

This shouldn't be the case, and I've tested the flow where a rule is added with a ec_deployment_traffic_filter at the same time as all rules are removed from the ec_deployment resource (i.e the plan modifier actually modifies the plan). I'll double check the UI case, since that's theoretically different.

The test fails due to the order of operations - it looks like ec_deployment resource is handled before ec_deployment_traffic_filter in the test.

No, the test is setting the ec_deployment.traffic_filter attribute, not introducing a ec_deployment_traffic_filter resource.

Can we just fix the test by explicitly setting traffic_filter = [] instead of relying on the plan modifier?

Nope, I initially wanted to do this but basically it doesn't work. Explicitly setting this to an empty set led to a bunch of inconsistent result errors, to the point where the private state approach was simpler in the end.

Setting this to an empty set required updating the read result to set state to an empty set when traffic filters are not included in the API response. There were edge cases around having an explicit empty set in the ec_deployment resource, and rules applied via ec_deployment_traffic_filter leading to inconsistent results as well.

The private state approach theoretically allows users to manage traffic filters in the ec_deployment and ec_deployment_traffic_filter resources concurrently (I've tested this, it's works as expected). Whilst I wouldn't suggest that approach it does remove some hard edges from the provider.

@tobio tobio force-pushed the computed-traffic-filters branch from e5801bb to 06363dc Compare May 9, 2023 02:31
@tobio tobio requested a review from alaudazzi as a code owner May 9, 2023 05:31
Copy link
Contributor

@dimuon dimuon left a comment

Choose a reason for hiding this comment

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

👍

@dimuon
Copy link
Contributor

dimuon commented May 10, 2023

It looks like 2 acc tests related to traffic filters fail.

@tobio tobio force-pushed the computed-traffic-filters branch from aed66e7 to 90a71bb Compare May 11, 2023 04:34
@tobio tobio merged commit 1c62a49 into elastic:master May 15, 2023
@tobio tobio deleted the computed-traffic-filters branch May 15, 2023 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants