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 proposal for ofo flagd service #59

Merged
merged 7 commits into from
Mar 23, 2023

Conversation

skyerus
Copy link
Contributor

@skyerus skyerus commented Mar 20, 2023

This PR

OFEP for ofo flagd service

Related Issues

open-feature/open-feature-operator#371

Notes

Follow-up Tasks

How to test

Copy link
Member

@beeme1mr beeme1mr left a comment

Choose a reason for hiding this comment

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

Simple and to the point. This sounds like a nice, flexible deployment option.

Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Approved with small suggestion.

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.

As a Platform Eng, I would have some reservation deploying an operator that can control K8s Services in any namespace. In the implementation details, I would suggest to enable/disable this behavior via (Open)Feauture Flag 😉 In the docs, we could explain that if this asking is too much permissions, this behavior can also be done manually by applying a manifest with the svc

OFEP-ofo-flagd-service.md Outdated Show resolved Hide resolved
skyerus added 5 commits March 21, 2023 12:58
Signed-off-by: Skye Gill <[email protected]>
Signed-off-by: Skye Gill <[email protected]>
Signed-off-by: Skye Gill <[email protected]>
@skyerus skyerus force-pushed the ofo-flagd-service branch from 1252505 to 8118d37 Compare March 21, 2023 13:00
@toddbaert
Copy link
Member

As a Platform Eng, I would have some reservation deploying an operator that can control K8s Services in any namespace. In the implementation details, I would suggest to enable/disable this behavior via (Open)Feauture Flag wink In the docs, we could explain that if this asking is too much permissions, this behavior can also be done manually by applying a manifest with the svc

I think it might make sense to restrict OFO to only create services in it's own namespace (which is customizable via helm). I think that would work for most use-cases and would help keep the permissions surface area smaller. I don't see a need for OFO to have the ability to create services in any namespace. Not sure that needs to be mentioned in this OFEP though

@toddbaert toddbaert requested a review from AlexsJones March 21, 2023 13:48
Signed-off-by: Skye Gill <[email protected]>
@skyerus
Copy link
Contributor Author

skyerus commented Mar 21, 2023

As a Platform Eng, I would have some reservation deploying an operator that can control K8s Services in any namespace. In the implementation details, I would suggest to enable/disable this behavior via (Open)Feauture Flag wink In the docs, we could explain that if this asking is too much permissions, this behavior can also be done manually by applying a manifest with the svc

I think it might make sense to restrict OFO to only create services in it's own namespace (which is customizable via helm). I think that would work for most use-cases and would help keep the permissions surface area smaller. I don't see a need for OFO to have the ability to create services in any namespace. Not sure that needs to be mentioned in this OFEP though

I agree with both points. I have updated the OFEP to restrict OFO's Services permissions to its own namespace.

Signed-off-by: Skye Gill <[email protected]>
@skyerus
Copy link
Contributor Author

skyerus commented Mar 23, 2023

Further POC work has led me to discover that services cannot expose deployments across namespaces.

The simplest circumvention to this is to enforce both the Deployment and Service to be created in OFO's namespace.
If this isn't extensive enough, an alternative could be to provide a list of allowed namespaces to OFO in which Service CRUD is permitted.
I prefer the former due to its simplicity. If OFO's namespace ends up being too limiting it can be built upon in the future.

Thoughts @toddbaert @beeme1mr @thisthat @AlexsJones ?

@beeme1mr beeme1mr merged commit c4553a1 into open-feature:main Mar 23, 2023
@AlexsJones
Copy link
Member

I am really confused, I left a comment on this and it's been merged but I can't find the comment

@AlexsJones
Copy link
Member

Okay I'll add what I think I wrote the other day:

The driving force behind this is to simplify the deployment of flag providers for use by client side applications

I think this is somewhat misleading, this doesn't make deploying clientside application deployment simpler, it makes it prescriptive, it's providing a way we think is best to do it, rather than letting people do it themself.

This is a common deployment pattern permitting access by any component that routes to the created Service (e.g. Ingress/Load Balancer).

This is going to make all the service and endpoint types resident inside the OFO namespace, meaning you can route between them, it breaks tenancy isolation boundaries.

In contrast, the described FlagService pattern permits an externally routable flag provider.

Why not simply route from the container that has the client SDK inside itself?

@beeme1mr
Copy link
Member

I am really confused, I left a comment on this and it's been merged but I can't find the comment

Here's a link to your comment: #59 (comment)

@skyerus
Copy link
Contributor Author

skyerus commented Mar 24, 2023

I am really confused, I left a comment on this and it's been merged but I can't find the comment

Your comment is still visible to me: #59 (comment)

Okay I'll add what I think I wrote the other day:

The driving force behind this is to simplify the deployment of flag providers for use by client side applications

I think this is somewhat misleading, this doesn't make deploying clientside application deployment simpler, it makes it prescriptive, it's providing a way we think is best to do it, rather than letting people do it themself.

People are still able to do it themselves if they don't fit to this prescription but imo the Service behind Deployment is common enough to warrant this enhancement.

In contrast, the described FlagService pattern permits an externally routable flag provider.

Why not simply route from the container that has the client SDK inside itself?

Can you elaborate further on this please? Not sure I fully understand.

@toddbaert
Copy link
Member

toddbaert commented Mar 24, 2023

@AlexsJones I think you may be misunderstanding what we mean when we say "client flags".

Why not simply route from the container that has the client SDK inside itself?

In client side flagging, there is no container with an SDK. The SDK and the provider exist in client code (an iOS app, web browser, etc).

@toddbaert
Copy link
Member

See my previous comment here for more background on the paradigm.

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

Successfully merging this pull request may close these issues.

5 participants