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

[research] Consider moving CRDs to beta, and making beta "hub" versions #352

Closed
toddbaert opened this issue Feb 15, 2023 · 22 comments
Closed
Assignees

Comments

@toddbaert
Copy link
Member

toddbaert commented Feb 15, 2023

In an effort move to 1.0, we should consider moving our CRDs to beta to imply stability. Things we should consider:

  • releasing beta CRD versions (all CRDs)
  • making the beta version the conversion "hub" so those objects are referenced in most functional code
  • making sure documentation is clear
  • deprecating all other CRDs
  • removing deprecated APIs
@toddbaert
Copy link
Member Author

FYI @odubajDT, this sounds like something you might be interested in.

@odubajDT
Copy link
Contributor

odubajDT commented Mar 8, 2023

I would love to do this, but I guess the team needs to decide on the specs of the resources for beta

@toddbaert
Copy link
Member Author

I would love to do this, but I guess the team needs to decide on the specs of the resources for beta

Yes, it's not a "right now" issue. I just wanted to you to aware of it. I think once we complete (or get closer to completing) the flagd-kube-proxy, we will have a better candidate for the beta CRD.

@odubajDT
Copy link
Contributor

Great, @toddbaert would it be possible to organize a team meeting to align on the specification of the CRDs ? Therefore I can start with this ticket and create a PoC

@Kavindu-Dodan
Copy link
Contributor

My take on this is to wait till we finalize kube-proxy related implementations. We will see more changes with proxy deployment and in my view, it's best to push those changes to alpha version.

Once we have the proxy changes and stability on the solution, we can draft the beta version of the API.

Timeline wise, this is 1 ~ 2 sprints of waiting

@Kavindu-Dodan
Copy link
Contributor

Consider addressing #433 along with this change

@odubajDT
Copy link
Contributor

odubajDT commented Oct 23, 2023

From the last conversation with @toddbaert here some items that need to be fullfilled about this issue and also results of the research:

  • introducing a new v1beta1 version leads to breaking backwards compatibility to v1alpha* versions
  • v1beta1 will be used as a default (and only) version in the controllers/webhooks logic
  • v1beta1.FeatureFlagConfiguration CRD will have the basis of v1alpha2.FeatureFlagConfiguration without deprecated fields
  • v1beta1.FlagSourceConfiguration CRD will have the basis of v1alpha3.FlagSourceConfiguration without deprecated fields
  • every supported deprecated feature/parameter/annotation will be cut in v1beta1
  • v1alpha* API version will still be part of the repository according to kubernetes standards -> not removed
  • v1beta1 (and therefore OFO 1.x) will contain the changes described in tickets Consider restricting FlagSourceConfiguration CRD #517 Discrepancy of OFO & flagd on file source naming  #433
  • code of OFO is cleaned up not to contain any logic supporting deprecated features/parameters/annotations
  • documentation is updated

To cover the defined items I would propose the following scenario for implementation:

  1. introduce v1beta1 API version to the repository
  2. adapt code to cover changes described in Discrepancy of OFO & flagd on file source naming  #433
  3. use of v1beta1 in controllers/webhook logic of OFO -> this is not a simple task, since the current version of OFO currently uses v1alpha1 as HUB version, which leads to the usage of deprecated fields in the whole operator logic
  4. adapt code to cover changes described in Consider restricting FlagSourceConfiguration CRD #517
  5. clean up leftover logic in v1alpha* API, but keep the CRD definitions
  6. clean up code and improve code quality (if necessary)
  7. adapt flagD to use v1beta1
  8. adapt documentation

Each of these steps should be a separate ticket if it is not yet. Should we proceed, create a tracking issue with all the tickets linked with exact descriptions what should be done? I would consider it as transparent for the open-source community, but I will leave the decision on the team.

If we decide not to create separate tickets, I would highly suggest at least have each of the steps implemented in separate PR.

After this is implemented and merged, firstly then we should consider releasing a stable version.

I would appreciate feedback on this @Kavindu-Dodan @toddbaert @thisthat @mowies @RealAnna @bacherfl . Any suggestions, objections or opinions are very welcome also from the community!

@thisthat
Copy link
Member

Thanks all for the effort 🙌 I have some questions about the points listed by @odubajDT

v1beta1 (and therefore OFO 1.x)

why a stable release with a beta API version? I would vote for having stable APIs for OFO 1.x

clean up leftover logic in v1alpha* API, but keep the CRD definitions

what type of clean up do you envision? 🤔 I would expect to drop any code line that deals with these CRDs

@odubajDT
Copy link
Contributor

Thanks all for the effort 🙌 I have some questions about the points listed by @odubajDT

v1beta1 (and therefore OFO 1.x)

why a stable release with a beta API version? I would vote for having stable APIs for OFO 1.x

True, but we actually cannot skip beta API version, maybe worth waiting for some time before releasing OFO 1.x with a stable API? @toddbaert

clean up leftover logic in v1alpha* API, but keep the CRD definitions

what type of clean up do you envision? 🤔 I would expect to drop any code line that deals with these CRDs

With this I meant removing the struct functions, for example here https://github.com/open-feature/open-feature-operator/blob/main/apis/core/v1alpha1/featureflagconfiguration_types.go#L111. There is no reason to keep these functions, as they won't be used anywhere. Also we can remove the conversion webhooks between alpha versions. According to k8s standards, the structs defining the CRDs should stay in the repo, but everything around it can be removed

@toddbaert
Copy link
Member Author

toddbaert commented Oct 23, 2023

True, but we actually cannot skip beta API version, maybe worth waiting for some time before releasing OFO 1.x with a stable API? @toddbaert

Yes. 1.0 is a more distant goal. We should have our eyes on it, but it's not an immediate concern. Completing all this work is necessary but not sufficient for a 1.0.

With this I meant removing the struct functions, for example here https://github.com/open-feature/open-feature-operator/blob/main/apis/core/v1alpha1/featureflagconfiguration_types.go#L111. There is no reason to keep these functions, as they won't be used anywhere. Also we can remove the conversion webhooks between alpha versions. According to k8s standards, the structs defining the CRDs should stay in the repo, but everything around it can be removed

I think we should do whatever the convention is, as you say. Maybe we can very explicitly mark them as only maintained for historical purposes.

@Kavindu-Dodan
Copy link
Contributor

@odubajDT thank you for the summary

I also like to discuss our CRD naming. Right now, our naming of two CRDs is a little confusing FlagSourceConfiguration vs FeatureFlagConfiguration. At least I time to time I mix the naming as they are too similar. Given we are going for a breaking change, shall we consider our options on renaming the CRDs ?

  • Flagd feature flag configuration CRD : FeatureFlags
  • Feature flag source configuration CRD : SourceConfigurations

@odubajDT
Copy link
Contributor

odubajDT commented Oct 24, 2023

@odubajDT thank you for the summary

I also like to discuss our CRD naming. Right now, our naming of two CRDs is a little confusing FlagSourceConfiguration vs FeatureFlagConfiguration. At least I time to time I mix the naming as they are too similar. Given we are going for a breaking change, shall we consider our options on renaming the CRDs ?

* Flagd feature flag configuration CRD : `FeatureFlags`

* Feature flag source configuration CRD : `SourceConfigurations`

Fine by me 👍 But we should keep the names singular, as according to k8s standards, CRD should be named as singular and k8s explicitly adds a plural value [1]. So I would suggest FeatureFlag and SourceConfiguration. Or also SidecarConfiguration might work on my side.

[1] https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#create-a-customresourcedefinition

@open-feature open-feature deleted a comment from thisthat Oct 24, 2023
@odubajDT
Copy link
Contributor

a PoC of a working solution is available here:

#527

Please note that some of the E2E tests are failing due to the usage of flagD, which is not yet adapted to use v1beta1 (kubernetes/flagd-proxy providers)

@RealAnna
Copy link
Contributor

@odubajDT thank you for the summary
I also like to discuss our CRD naming. Right now, our naming of two CRDs is a little confusing FlagSourceConfiguration vs FeatureFlagConfiguration. At least I time to time I mix the naming as they are too similar. Given we are going for a breaking change, shall we consider our options on renaming the CRDs ?

* Flagd feature flag configuration CRD : `FeatureFlags`

* Feature flag source configuration CRD : `SourceConfigurations`

Fine by me 👍 But we should keep the names singular, as according to k8s standards, CRD should be named as singular and k8s explicitly adds a plural value [1]. So I would suggest FeatureFlag and SourceConfiguration. Or also SidecarConfiguration might work on my side.

[1] https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#create-a-customresourcedefinition

I still find SourceConfiguration name a bit redundant 🤔 in the end the CR itself is a configuration, what I want to define as a user is a FeatureFlag and a Source so I would call them like that 😄

@odubajDT
Copy link
Contributor

@odubajDT thank you for the summary
I also like to discuss our CRD naming. Right now, our naming of two CRDs is a little confusing FlagSourceConfiguration vs FeatureFlagConfiguration. At least I time to time I mix the naming as they are too similar. Given we are going for a breaking change, shall we consider our options on renaming the CRDs ?

* Flagd feature flag configuration CRD : `FeatureFlags`

* Feature flag source configuration CRD : `SourceConfigurations`

Fine by me 👍 But we should keep the names singular, as according to k8s standards, CRD should be named as singular and k8s explicitly adds a plural value [1]. So I would suggest FeatureFlag and SourceConfiguration. Or also SidecarConfiguration might work on my side.
[1] https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#create-a-customresourcedefinition

I still find SourceConfiguration name a bit redundant 🤔 in the end the CR itself is a configuration, what I want to define as a user is a FeatureFlag and a Source so I would call them like that 😄

Source sounds too general for me, I would be more for SourceConfiguration or SidecarConfiguration

@mowies
Copy link
Member

mowies commented Oct 25, 2023

I agree more with Anna on the naming. Maybe something like FeatureFlag and FeatureFlagSource could also work?

@Kavindu-Dodan
Copy link
Contributor

I agree more with Anna on the naming. Maybe something like FeatureFlag and FeatureFlagSource could also work?

The main issue is sharing the same prefix FeatureFlag. I think this is one reason the existing naming is misleading 😞

@odubajDT I like the SourceConfiguration as this name clearly highlight the purpose of the crd - configurations of the side car

So the current candidates,

  • Feature flag string - FeatureFlag,
  • Sidecar/sources configurations - SourceConfiguration, SidecarConfiguration, FeatureFlagSource , Source

@odubajDT
Copy link
Contributor

odubajDT commented Oct 30, 2023

Let's create a pool, the name for the FeatureFlag is quite clear, let's vote for the configurations CRD:

  • ❤️ - SourceConfiguration
  • 👍 - SidecarConfiguration
  • 👎 - FeatureFlagSource
  • 👀 - Source

Please vote by marking this comment with the emoji, which represents your favorite name

@thisthat
Copy link
Member

thisthat commented Oct 31, 2023

I'd like to bring a bit more context on how it would feel to use each version to support the voting:

SourceConfiguration

kubectl get sourceconfiguration -A

apiVersion: core.openfeature.dev/v1beta1
kind: SourceConfiguration
metadata:
  name: end-to-end-test
spec:
 ...

SidecarConfiguration

kubectl get sidecarconfiguration -A

apiVersion: core.openfeature.dev/v1beta1
kind: SidecarConfiguration
metadata:
  name: end-to-end-test
spec:
 ...

FeatureFlagSource

kubectl get featureflagsource -A

apiVersion: core.openfeature.dev/v1beta1
kind: FeatureFlagSource
metadata:
  name: end-to-end-test
spec:
 ...

Source

kubectl get source -A

apiVersion: core.openfeature.dev/v1beta1
kind: Source
metadata:
  name: end-to-end-test
spec:
 ...

@odubajDT
Copy link
Contributor

I guess we have a winner, it will be FeatureFlagSource. I will adapt the PR #535

@toddbaert
Copy link
Member Author

Fine by me 👍 But we should keep the names singular, as according to k8s standards, CRD should be named as singular and k8s explicitly adds a plural value [1]. So I would suggest FeatureFlag and SourceConfiguration. Or also SidecarConfiguration might work on my side.

I agree with this after learning the background.

@odubajDT
Copy link
Contributor

The research as well as the implementation is done, closing this issue

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

Successfully merging a pull request may close this issue.

6 participants