-
Notifications
You must be signed in to change notification settings - Fork 4
chore: add v2alpha1 group and add ControlPlane type #441
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
Conversation
a60ec58 to
179dde6
Compare
179dde6 to
72293bb
Compare
programmer04
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM, nits
Co-authored-by: Jakub Warczarek <[email protected]>
|
Letting @mlavacca chip in on the review and thus leaving this unmerged for now. |
Co-authored-by: Jintao Zhang <[email protected]>
mlavacca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the effort on this, @pmalek!
I've left a few comments, curious to hear your opinion :)
requesting changes to avoid accidental merge.
…at enabled is set for controllers and featureGates
45fa28a to
01289db
Compare
01289db to
bc8a3b1
Compare
|
@mlavacca This is ready for review once more. PTAL. |
| // ControlPlaneDataPlaneTargetManagedByType indicates that the DataPlane target | ||
| // is managed by the owner of the ControlPlane. | ||
| // This is the case when using a Gateway resource to manage the DataPlane | ||
| // and the ControlPlane is responsible for configuring it. | ||
| ControlPlaneDataPlaneTargetManagedByType ControlPlaneDataPlaneTargetType = "managedByOwner" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried constraining the created ControlPlane with this target type to ensure that it has an owner set but CEL doesn't allow it for some reason:
The CustomResourceDefinition "controlplanes.gateway-operator.konghq.com" is invalid: spec.versions[1].schema.openAPIV3Schema.x-kubernetes-validations[0].rule: Invalid value: apiextensions.ValidationRule{Rule:"self.spec.dataplane.type != 'managedByOwner' || self.ownerReferences[0].kind == 'Gateway'", Message:"X", MessageExpression:"", Reason:(*apiextensions.FieldValueErrorReason)(nil), FieldPath:"", OptionalOldSelf:(*bool)(nil)}: compilation failed: ERROR: <input>:1:53: undefined field 'ownerReferences'
| self.spec.dataplane.type != 'managedByOwner' || self.ownerReferences[0].kind == 'Gateway'
| ....................................................^
slack thread asking about the reason and the comprehensive list of allowed metadata fields to use in CEL: https://kubernetes.slack.com/archives/C0EG7JC6T/p1749212565802129
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is explained in https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/
The apiVersion, kind, metadata.name and metadata.generateName are always accessible from the root of the object and from any x-kubernetes-embedded-resource annotated objects. No other metadata properties are accessible.
So we can't do this validation (at this moment) via CEL.
mlavacca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly lgtm, just one nit about validation
mlavacca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are good to go 🚀 Thanks for putting this together, @pmalek!
What this PR does / why we need it:
This PR adds a
ControlPlaneversion:v2alpha1which contains the following breaking changes with respect tov1beta1:deploymentfielddataplanefield from*stringto a typed union field which can be set to either a url (of Admin API) or a name ofDataPlaneresource (the same use case as with olddataplanefield)featureGates,controllersandadminAPIfields. The first 2 have string based arrays instead of strongly types fields (most likely the direction we'll move forward with but can potentially be changed based on feedback).CRD validation tests have not been added (yet) to allow feedback which could influence the design.
Which issue this PR fixes
Part of Kong/kong-operator#1358
Special notes for your reviewer:
Further changes to the new
v2alpha1API (like new, individual fields) will be added when this PR gets reviewed so that throw away work is avoided.PR Readiness Checklist:
Complete these before marking the PR as
ready to review:CHANGELOG.mdrelease notes have been updated to reflect significant changes