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

Add support for reconcile policy #2060

Merged
merged 4 commits into from Feb 9, 2022

Conversation

matthchr
Copy link
Member

This closes #1633.

Adds support for a new reconcile-policy annotation that allows users to
prevent all modifications to Azure resources (PUT, DELETE), or just all
DELETE's.

If applicable:

  • this PR contains documentation
  • this PR contains tests

@matthchr matthchr added this to the codegen-beta-0 milestone Jan 28, 2022
@matthchr matthchr added this to In progress in CRD Code Generation via automation Jan 28, 2022
@matthchr matthchr force-pushed the feature/reconcile-policy branch 4 times, most recently from e72730b to 44e519f Compare January 28, 2022 23:32
Copy link
Contributor

@jonnylangefeld jonnylangefeld left a comment

Choose a reason for hiding this comment

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

Thanks for coming around so quickly with this PR!

docs/hugo/content/introduction/annotations.md Outdated Show resolved Hide resolved
docs/hugo/content/introduction/annotations.md Outdated Show resolved Hide resolved
v2/internal/controllers/reconcile_policy_test.go Outdated Show resolved Hide resolved
Comment on lines 43 to 46
default:
// Defaulting to skip. The user is attempting to configure policy but has done it wrong,
// if we default to Run we may inadvertently modify their object
return ReconcilePolicySkip, errors.Errorf("%q is not a known reconcile policy", policy)
Copy link
Contributor

Choose a reason for hiding this comment

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

However, this also makes so that as soon as the serviceoperator.azure.com/reconcile-policy annotation is present we skip, no matter the string value. That also makes the verb skip unnecessary (as if it doesn't matter if I put skip or sk1p or piks in there). I think this has great implications that as soon as the annotation is present, the operator skips reconcile.

That is fine, I would just be more explicit in the docs about that. Essentially there's three states:

  1. I don't set anything and the operator reconciles
  2. I set the serviceoperator.azure.com/reconcile-policy annotation with any value and the operator doesn't reconcile.
  3. I set serviceoperator.azure.com/reconcile-policy: skip-delete and the operator reconciles but skips delete.

Given the name of the annotation however, I probably wouldn't expect that the operator skips reconciling as soon as the annotation is set. I'd probably expect that if the annotation was called serviceoperator.azure.com/skip-reconcile. But since it's called serviceoperator.azure.com/reconcile-policy, I'd probably expect that I have to set the correct policy to achieve the behavior I want (either skip or skip-delete).

Copy link
Member Author

@matthchr matthchr Feb 4, 2022

Choose a reason for hiding this comment

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

Agree that an empty reconcile-policy doing skip probably doesn't make sense. I'm going to change this behavior and default to manage (aka behave normally). That seems to be the standard behavior if you get an annotation like this "wrong".

If we really want to enforce that people don't set an incorrect value here we could prevent it at the webhook level (which has the advantage of being able to actually tell them they did it wrong as opposed to just doing something they didn't expect), but that seems overkill for now. We could investigate doing that in the future if there's actual desire for it from users.

v2/internal/reconcilers/azure_generic_arm_reconciler.go Outdated Show resolved Hide resolved
docs/hugo/content/introduction/annotations.md Outdated Show resolved Hide resolved
docs/hugo/content/introduction/annotations.md Outdated Show resolved Hide resolved
docs/hugo/content/introduction/annotations.md Outdated Show resolved Hide resolved
docs/hugo/content/introduction/annotations.md Outdated Show resolved Hide resolved
docs/hugo/content/introduction/annotations.md Outdated Show resolved Hide resolved
docs/hugo/content/introduction/annotations.md Show resolved Hide resolved
@matthchr matthchr force-pushed the feature/reconcile-policy branch 3 times, most recently from a234163 to 0d2a1e4 Compare February 7, 2022 18:18
@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2022

Codecov Report

Merging #2060 (26f2ce0) into main (ed772b2) will increase coverage by 0.02%.
The diff coverage is 72.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2060      +/-   ##
==========================================
+ Coverage   56.67%   56.69%   +0.02%     
==========================================
  Files         481      484       +3     
  Lines      100338   100468     +130     
==========================================
+ Hits        56863    56959      +96     
- Misses      36456    36487      +31     
- Partials     7019     7022       +3     
Impacted Files Coverage Δ
v2/internal/testcommon/kube_matcher.go 78.57% <0.00%> (-21.43%) ⬇️
v2/internal/testcommon/kube_per_test_context.go 86.05% <0.00%> (-1.69%) ⬇️
...g/genruntime/conditions/ready_condition_builder.go 78.78% <0.00%> (-21.22%) ⬇️
...ternal/reconcilers/azure_generic_arm_reconciler.go 73.91% <75.34%> (+0.53%) ⬆️
...l/util/annotations/select_annotations_predicate.go 77.77% <77.77%> (ø)
v2/internal/reconcilers/reconcile_policy.go 91.30% <91.30%> (ø)
v2/internal/controllers/generic_controller.go 81.81% <100.00%> (+0.56%) ⬆️
v2/internal/genericarmclient/generic_client.go 75.00% <100.00%> (ø)
v2/internal/reconcilers/annotations.go 100.00% <100.00%> (ø)
v2/pkg/genruntime/base_types.go 93.10% <100.00%> (+0.51%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed772b2...26f2ce0. Read the comment docs.

Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

Looks good. A couple of minor suggestions.

gif

docs/hugo/content/introduction/annotations.md Show resolved Hide resolved
v2/internal/reconcilers/reconcile_policy.go Show resolved Hide resolved
v2/internal/reconcilers/reconcile_policy.go Outdated Show resolved Hide resolved
v2/internal/reconcilers/reconcile_policy.go Outdated Show resolved Hide resolved
@matthchr
Copy link
Member Author

matthchr commented Feb 8, 2022

/ok-to-test sha=779930e

1 similar comment
@matthchr
Copy link
Member Author

matthchr commented Feb 8, 2022

/ok-to-test sha=779930e

@matthchr
Copy link
Member Author

matthchr commented Feb 9, 2022

/ok-to-test sha=26f2ce0

@matthchr
Copy link
Member Author

matthchr commented Feb 9, 2022

/ok-to-test sha=ceb136c

This closes Azure#1633.

Adds support for a new reconcile-policy annotation that allows users to
prevent all modifications to Azure resources (PUT, DELETE), or just all
DELETE's.
@matthchr
Copy link
Member Author

matthchr commented Feb 9, 2022

/ok-to-test sha=823f0eb

@matthchr matthchr merged commit c7b716e into Azure:main Feb 9, 2022
CRD Code Generation automation moved this from In progress to Done Feb 9, 2022
@matthchr matthchr deleted the feature/reconcile-policy branch February 9, 2022 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Feature: Add a no-delete annotation
6 participants