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(api): add Konnect API group #443

Closed
wants to merge 1 commit into from
Closed

Conversation

pmalek
Copy link
Member

@pmalek pmalek commented Jul 29, 2024

What this PR does / why we need it:

  • move api/v1beta1 and api/v1alpha1 under api/gateway-operator/
  • add api/konnect with v1alpha1 placeholder

Which issue this PR fixes

Part of #442

Special notes for your reviewer:

This is a breaking change in the Go API but not in YAML manifests as API group name does not change.

We do not treat the Go API currently as part of the user contract so we do not label this change as breaking.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect significant changes

@pmalek pmalek added this to the KGO v1.4.x milestone Jul 29, 2024
@pmalek pmalek self-assigned this Jul 29, 2024
@pmalek pmalek force-pushed the konnect-separate-api-group branch from c31903f to 1de3cda Compare July 29, 2024 20:49
@pmalek pmalek marked this pull request as ready for review July 29, 2024 20:51
@pmalek pmalek requested a review from a team as a code owner July 29, 2024 20:51
Copy link
Contributor

@randmonkey randmonkey left a comment

Choose a reason for hiding this comment

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

Looks like no go types for Konnect CRDs are defined in this PR but we have manifests for two CRDs. If we do not want include konnect CRDs in this PR, I think we should remove the manifests.

@pmalek pmalek force-pushed the konnect-separate-api-group branch from 1de3cda to 3c98f5c Compare July 30, 2024 08:28
@pmalek pmalek requested a review from randmonkey July 30, 2024 08:29
CHANGELOG.md Show resolved Hide resolved
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

I thought we were going to add the new konnect CRDs in kubernetes-configuration rather than here. We agreed on putting off the migration of the existing CRDs to the new repo, but for what concerns the new CRDs, why are we splitting them up between here and kubernetes-config?

@pmalek
Copy link
Member Author

pmalek commented Jul 30, 2024

I thought we were going to add the new konnect CRDs in kubernetes-configuration rather than here. We agreed on putting off the migration of the existing CRDs to the new repo, but for what concerns the new CRDs, why are we splitting them up between here and kubernetes-config?

There are 2 "types" of CRDs we're talking about here:

  • Kong configuration CRDs which are already in https://github.com/Kong/kubernetes-configuration/tree/main/api/configuration (KongConsumer, KongConsumerGroup, KongPlugin etc) and those that might still end up there or are being actively worked on (KongService or KongRoute)
  • Top level Konnect entity types like KonnectControlPlane. This is the type of CRDs that will land in the newly added API group.

The former make sense in both KGO and KIC context. The latter are only used by the operator and are only used in the Konnect context.


From what I understand @mlavacca, you're suggesting to introduce the latter also in https://github.com/Kong/kubernetes-configuration under konnect API group, leaving KGO API unchanged.

I'm wondering if there are any benefits to this approach. I'm curious about your thoughts on this. Given that we'd only use those CRDs in the operator I'm not sure there's much value in putting them outside of this repo. 🤔

@mlavacca
Copy link
Member

I thought we were going to add the new konnect CRDs in kubernetes-configuration rather than here. We agreed on putting off the migration of the existing CRDs to the new repo, but for what concerns the new CRDs, why are we splitting them up between here and kubernetes-config?

There are 2 "types" of CRDs we're talking about here:

  • Kong configuration CRDs which are already in https://github.com/Kong/kubernetes-configuration/tree/main/api/configuration (KongConsumer, KongConsumerGroup, KongPlugin etc) and those that might still end up there or are being actively worked on (KongService or KongRoute)
  • Top level Konnect entity types like KonnectControlPlane. This is the type of CRDs that will land in the newly added API group.

The former make sense in both KGO and KIC context. The latter are only used by the operator and are only used in the Konnect context.

From what I understand @mlavacca, you're suggesting to introduce the latter also in https://github.com/Kong/kubernetes-configuration under konnect API group, leaving KGO API unchanged.

I'm wondering if there are any benefits to this approach. I'm curious about your thoughts on this. Given that we'd only use those CRDs in the operator I'm not sure there's much value in putting them outside of this repo. 🤔

My point is that since we are making the effort to group the CRDs in a unique place, it could make sense to treat this new place as where "Kong CRDs live". No matters if they are shared among KIC or KGO, or just used by KGO, I think that having a centralized place where our APIs live is a good approach, rather than having them scattered across multiple repositories.

Does it make sense to you?

@mlavacca
Copy link
Member

One additional point to the discussion is there we will inevitably split the konnect CRDs between k8s-conf and kgo if we choose the approach of putting the KonnectControlPlanes here: the kubernetes configuration objects will need the konnectAPIAuthConfiguration API as it is used in their status, and to avoid circular dependencies we'll have to put such resources in the same repo. We'd end up having the Kong CRDs scattered across multiple repos, AND the same API group scattered across multiple repositories, which is something I think we really need to stay away from.

@pmalek
Copy link
Member Author

pmalek commented Jul 31, 2024

One additional point to the discussion is there we will inevitably split the konnect CRDs between k8s-conf and kgo if we choose the approach of putting the KonnectControlPlanes here: the kubernetes configuration objects will need the konnectAPIAuthConfiguration API as it is used in their status, and to avoid circular dependencies we'll have to put such resources in the same repo. We'd end up having the Kong CRDs scattered across multiple repos, AND the same API group scattered across multiple repositories, which is something I think we really need to stay away from.

I was almost going to nod on this but then I realized that with the approach proposed by @czeslavo in the design doc (https://docs.google.com/document/d/1TxW2ovqPRm-w1td1e_V0CyaET4lpiEBCJ7hiYMZJqng/edit?disco=AAABS5JK2YM) we could drop the idea of having the KonnectAuthConfiguration reference in resources living within the CP because the latter would contain the auth config anyway. This way we don't need the KonnectAuthConfiguration type information.

Would you agree @mlavacca?

I guess at this point, it's a matter of where we'd prefer this CRDs to live. Let's discuss this on today's epic review.

@pmalek
Copy link
Member Author

pmalek commented Jul 31, 2024

After a discussion with @mlavacca and @czeslavo we've decided that it will be better to place all new Konnect related types and CRDs into https://github.com/Kong/kubernetes-configuration.

With this, Kong/kubernetes-configuration#12 supersedes this PR.

@pmalek pmalek closed this Jul 31, 2024
@lahabana lahabana deleted the konnect-separate-api-group branch January 3, 2025 17:07
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.

4 participants