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

feat: add flagd-proxy HA configuration #712

Merged

Conversation

xvzf
Copy link
Contributor

@xvzf xvzf commented Oct 14, 2024

This PR

  • Add support for multi-replica flagd-proxy deployments

Related Issues

Fixes #609

Notes

Follow-up Tasks

How to test

Deploy with e.g. FLAGD_PROXY_REPLICA_COUNT=3

@xvzf xvzf requested a review from a team as a code owner October 14, 2024 12:58
go.mod Show resolved Hide resolved
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 83.58209% with 22 lines in your changes missing coverage. Please review.

Project coverage is 86.25%. Comparing base (499661e) to head (2f6f42c).
Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
common/flagdproxy/flagdproxy.go 84.07% 12 Missing and 6 partials ⚠️
controllers/core/featureflagsource/controller.go 63.63% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #712      +/-   ##
==========================================
- Coverage   86.51%   86.25%   -0.26%     
==========================================
  Files          19       19              
  Lines        1587     1681      +94     
==========================================
+ Hits         1373     1450      +77     
- Misses        173      184      +11     
- Partials       41       47       +6     
Files with missing lines Coverage Δ
common/utils/utils.go 100.00% <100.00%> (ø)
controllers/core/featureflagsource/controller.go 54.21% <63.63%> (+2.90%) ⬆️
common/flagdproxy/flagdproxy.go 87.90% <84.07%> (-0.80%) ⬇️

... and 1 file with indirect coverage changes

Flag Coverage Δ
unit-tests 86.25% <83.58%> (-0.26%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@beeme1mr
Copy link
Member

Thanks for the PR! I'll look into the image scanning CI issue, which is unrelated to this change.

Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

Thank you @xvzf for the PR with some extra refactoring 🙌
I left only some minor comments. In particular, I am not 100% sure if we should retry continuously to reconcile on an unmanaged OFO resource or we should skip it. What's your opinion here?

common/flagdproxy/flagdproxy.go Outdated Show resolved Hide resolved
common/flagdproxy/flagdproxy.go Outdated Show resolved Hide resolved
common/flagdproxy/flagdproxy.go Show resolved Hide resolved
@beeme1mr
Copy link
Member

Hey @xvzf, could you please resolve any open threads that have been resolved? I'll rereview once you're ready. The change looks great so far. Thanks!

@xvzf xvzf force-pushed the feat/add-ha-configuration-flagd-proxy branch from b1eb7e0 to b2d5090 Compare October 21, 2024 09:24
@xvzf
Copy link
Contributor Author

xvzf commented Oct 21, 2024

Here we go @beeme1mr @thisthat - been off for a couple of days, sorry for the delay

Signed-off-by: Matthias Riegler <[email protected]>
@xvzf xvzf requested a review from thisthat October 21, 2024 12:38
Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution @xvzf 🙌

@beeme1mr beeme1mr merged commit e115159 into open-feature:main Oct 22, 2024
17 of 18 checks passed
@github-actions github-actions bot mentioned this pull request Oct 22, 2024
@beeme1mr
Copy link
Member

The change is now live in v0.8.0. Thanks @xvzf!

@xvzf
Copy link
Contributor Author

xvzf commented Oct 25, 2024

Thanks for the swift release @beeme1mr!

@xvzf xvzf deleted the feat/add-ha-configuration-flagd-proxy branch October 29, 2024 16:15
@beeme1mr
Copy link
Member

Hey @xvzf, it looks like this change introduced new permission requirements. Would you be able to update the RBAC configuration?

#717

Comment on lines +169 to +171
Labels: map[string]string{
common.ManagedByAnnotationKey: common.ManagedByAnnotationValue,
},
Copy link
Contributor

@wondertroy wondertroy Oct 30, 2024

Choose a reason for hiding this comment

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

I just noticed when upgrading to open-feature-operator 0.8.0 that previous older flagdProxy resources that already existed did not have this label - and therefore ran in to the 'not managed by OFO' in the ensureFlagdProxyResource method.

It was an easy workaround to manually add this label to the older resources but worth mentioning

Copy link
Contributor

Choose a reason for hiding this comment

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

The previous resources did seem to have the common.ManagedByAnnotationKey: common.ManagedByAnnotationValue, in the Selector though

@xvzf
Copy link
Contributor Author

xvzf commented Oct 30, 2024

Yes, gonna have a look this morning!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flagd-proxy HA setup
4 participants