fix: add member conditions in status of KonnectGatewayControlPlane #138
Conversation
| @@ -88,6 +93,18 @@ func (c *KonnectGatewayControlPlane) GetKonnectAPIAuthConfigurationRef() Konnect | |||
| return c.Spec.KonnectConfiguration.APIAuthConfigurationRef | |||
| } | |||
|
|
|||
| // SetMemberStatus sets conditions of a member of the control plane group in status. | |||
| // REVIEW: is it better to replace it with `SetMemberCondition` to set a single condition of a member? | |||
There was a problem hiding this comment.
Do we see a use case for setting more than 1 condition for a member?
I believe at this stage we're OK with setting just 1 (e.g. Programmed). Changing this API in the future will be simple so accommodate for in consumers (KGO) so I'm OK with allowing a single condition in the declaration here.
There was a problem hiding this comment.
How about not adding another field and having it granular per member, but only populating a MembersReferencesResolved condition in the CP's conditions? That would be True if all are valid, and False if any are invalid (with a message indicating which ones).
When creating Kong/gateway-operator#797 I didn't take that into account, but I think that might be easier to implement and cleaner.
There was a problem hiding this comment.
👍 This would already be a nice improvement and as you've mentioned: will be easier, less error prone and more efficient to implement.
WDYT @randmonkey ?
There was a problem hiding this comment.
It sounds good to add a specific condition type in the CP representing the CP group. For further details, we can add fields map[string]MemeberStatus for status of each member. The detail part could be in the next release. How about this?
There was a problem hiding this comment.
Let's focus on the CP group condition like suggested by @czeslavo - MembersReferencesResolved. At this point, I'd treat adding status conditions for each members as a nice to have, until requested by a user.
There was a problem hiding this comment.
Can we add tests here for the added field?
There was a problem hiding this comment.
I tried but it requires "error happens in updating status". This requires more changes in the testing framework.
There was a problem hiding this comment.
Since this change introduces CEL validation, I'd halt merging this until we can figure out what is the cause of this error. We don't want to merge a change that would break updating the CP CRD for any supported use case.
There was a problem hiding this comment.
I'm not sure if we should validate status fields with CEL rules 🤔 It will only be populated by the controller - that's where we should ensure it's generated valid.
There was a problem hiding this comment.
Good point @czeslavo. This makes a lot of sense. 👍
| // MemberStatus is the status of each member in the control plane group. | ||
| // This field is only applicable for control planes with spec.cluster_type = CLUSTER_TYPE_CONTROL_PLANE_GROUP. | ||
| MemberStatus map[string][]metav1.Condition `json:"members,omitempty"` |
There was a problem hiding this comment.
Just to allow an extension of this field in the future, I suggest to put the conditions in a struct, which could then have other fields added in the future.
| // MemberStatus is the status of each member in the control plane group. | |
| // This field is only applicable for control planes with spec.cluster_type = CLUSTER_TYPE_CONTROL_PLANE_GROUP. | |
| MemberStatus map[string][]metav1.Condition `json:"members,omitempty"` | |
| // MembersStatus is the status of each member in the control plane group. | |
| // This field is only applicable for control planes with spec.cluster_type = CLUSTER_TYPE_CONTROL_PLANE_GROUP. | |
| MembersStatus map[string][]metav1.Condition `json:"members,omitempty"` |
What this PR does / why we need it:
Add
status.membersfor conditions of members in the control plane group.Which issue this PR fixes
Part of Kong/gateway-operator#797
Special notes for your reviewer:
PR Readiness Checklist:
Complete these before marking the PR as
ready to review:CHANGELOG.mdrelease notes have been updated to reflect significant changes