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: DataplaneKonnectExtension CRD #453

Merged
merged 3 commits into from
Sep 5, 2024
Merged

Conversation

mlavacca
Copy link
Member

@mlavacca mlavacca commented Jul 31, 2024

What this PR does / why we need it:

Which issue this PR fixes

Part of #203.

The original issue is about adding a new konnect-related set of fields directly in the DataPlane API. I think we should be very cautious about this approach because KGO is an open-source project that aims to be used by Kong and Konnect customers AND community users. Including SaaS-related fields directly in the API would negatively affect the API itself form the community point of view. Hence this new extension CRD that with an approach very similar to the ControlPlaneMetricsExtension is intended to be used by the DataPlane controller to customize the DataPlane deployment with konnect-related configuration.

Special notes for your reviewer:

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

@mlavacca mlavacca force-pushed the mlavacca/dataplane-konnect branch 5 times, most recently from 89e8ac6 to b09db39 Compare August 1, 2024 14:53
@mlavacca mlavacca marked this pull request as ready for review August 1, 2024 14:54
@mlavacca mlavacca requested a review from a team as a code owner August 1, 2024 14:54
@mlavacca mlavacca changed the title feat: dataplane_konnect_extension CRD feat: DataplaneKonnectExtension CRD Aug 1, 2024
@mlavacca mlavacca force-pushed the mlavacca/dataplane-konnect branch 3 times, most recently from fa3c962 to 56279cb Compare August 1, 2024 15:38
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

Can we add a sample manifest showing how this can be used?

I believe also that further comment in the PR description about this CRD would be beneficial to clue in potential reviewers.

api/v1alpha1/dataplane_konnect_extension_types.go Outdated Show resolved Hide resolved
api/v1alpha1/dataplane_konnect_extension_types.go Outdated Show resolved Hide resolved
api/v1alpha1/dataplane_konnect_extension_types.go Outdated Show resolved Hide resolved
api/v1beta1/gatewayconfiguration_types.go Outdated Show resolved Hide resolved
api/v1beta1/dataplane_types.go Outdated Show resolved Hide resolved
@mlavacca mlavacca force-pushed the mlavacca/dataplane-konnect branch 2 times, most recently from 8baa7e9 to 0cc163f Compare August 2, 2024 10:28
@mlavacca
Copy link
Member Author

mlavacca commented Aug 2, 2024

Can we add a sample manifest showing how this can be used?

I believe also that further comment in the PR description about this CRD would be beneficial to clue in potential reviewers.

Yep, I added an example under config/samples and updated the PR description to make it clearer

@mlavacca mlavacca requested a review from pmalek August 2, 2024 10:35
@mlavacca mlavacca force-pushed the mlavacca/dataplane-konnect branch 2 times, most recently from ede4d5c to 6e5a01f Compare September 3, 2024 11:05
@mlavacca mlavacca force-pushed the mlavacca/dataplane-konnect branch from 6e5a01f to 078d6d3 Compare September 3, 2024 11:07
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.

Generally LGTM, only some minor comments.

api/v1alpha1/dataplane_konnect_extension_types.go Outdated Show resolved Hide resolved
api/v1beta1/dataplane_types.go Show resolved Hide resolved
api/v1beta1/gatewayconfiguration_types.go Show resolved Hide resolved
Signed-off-by: Mattia Lavacca <[email protected]>
@mlavacca mlavacca force-pushed the mlavacca/dataplane-konnect branch from 69cde85 to cc2851d Compare September 5, 2024 09:42
@mlavacca mlavacca requested a review from czeslavo September 5, 2024 09:44
@mlavacca mlavacca force-pushed the mlavacca/dataplane-konnect branch from cc2851d to 78a8135 Compare September 5, 2024 09:45
Signed-off-by: Mattia Lavacca <[email protected]>
@mlavacca mlavacca force-pushed the mlavacca/dataplane-konnect branch from 78a8135 to 6f9c392 Compare September 5, 2024 09:50
@mlavacca mlavacca merged commit a3d0a24 into main Sep 5, 2024
20 checks passed
@mlavacca mlavacca deleted the mlavacca/dataplane-konnect branch September 5, 2024 11:57
type DataPlaneKonnectExtensionSpec struct {
// ControlPlaneRef is a reference to a ControlPlane this DataPlaneKonnectExtension is associated with.
// +kubebuilder:validation:Required
ControlPlaneRef configurationv1alpha1.ControlPlaneRef `json:"controlPlaneRef"`
Copy link
Member

Choose a reason for hiding this comment

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

Just a note based on a sync with @mlavacca: when we get to implement the konnectNamespacedRef we should make the CP ref immutable to prevent going from 1 type of CP ref to another (unless that gets implemented)

@mlavacca mlavacca restored the mlavacca/dataplane-konnect branch October 9, 2024 15:15
@lahabana lahabana deleted the mlavacca/dataplane-konnect branch January 3, 2025 17:12
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.

4 participants