-
Notifications
You must be signed in to change notification settings - Fork 317
feat(L4): expose GCP resource state via Service Status Conditions #3018
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
base: master
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 08volt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
54d68bc to
f8f2767
Compare
|
/retest |
f8f2767 to
1b97b12
Compare
|
/retest |
|
/assign @TortillaZHawaii |
TortillaZHawaii
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.
First round
pkg/l4resources/l4netlbipv6.go
Outdated
|
|
||
| if ipv6fr.IPProtocol == string(corev1.ProtocolTCP) { | ||
| syncResult.Annotations[annotations.TCPForwardingRuleIPv6Key] = ipv6fr.Name | ||
| metaapi.SetStatusCondition(&syncResult.Conditions, utils.NewTCPIPv6ForwardingRuleCondition(ipv6fr.Name)) |
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.
does the api has to be iterative? Can we have declarative, similar to what is set above for the annotations? meaning that we would not care about deleting existing ones, but just always set whatever was there
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 would be the official/safe way to handle conditions
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.
Why can't we hide "official/safe" way in some kind of abstraction? I'm wondering if we could just get away with some struct that would read the annotations which are set in the result structs. Based on the annotations map it could create conditions and calculate the diff between the got set of conditions and the wanted set, perform "official/safe" condition updates, and in the end make a PATCH to k8s API.
In the current form we have a lot of messy code, especially with Forwarding Rules, where we need to delete 2 other variants. In my opinion, if another resource comes along, it will become dreadful to find every place that needs to be updated for both conditions and annotations to work, especially given that you need to think about all the possible previous states of the LB. I worry, that if someone does something that is supported, but is non obvious (for example going from IPv4 to IPv6, or ILB to NetLB), current approach would result with leftover or conflicting conditions.
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.
Thats right, I kept the slice type but change the initilization to empty slice
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.
Reverted it back, as the controller should also make sure that other conditions in the service are not deleted. The updates mentioned should be tested
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.
If they are tested, we can check that the conditions in the service are always only the one we expect to have
6e572b5 to
2624378
Compare
432ec08 to
6c9bd7a
Compare
…ctor related functions
6c9bd7a to
6548129
Compare
Objective
Improve observability of L4 ILB and NetLB services by propagating the names of underlying GCP resources directly into Service.Status.Conditions.
Design
Introduces a new feature gate, EnableL4LBConditions (default: false), to control the population of these conditions.
When enabled, the L4 controller sync loop tracks the provisioning success of individual GCP components. The L4ILBSyncResult and L4NetLBSyncResult structs has been extended to accumulate metav1.Condition updates throughout the sync lifecycle.
Key Changes:
Feature Flag: Added
--enable-l4lb-svc-conditionsto flags.go and propagated it through the ControllerContext.Condition Management:
Leverages k8s.io/apimachinery/pkg/api/meta to Set/Remove conditions.
State Transitions: The logic correctly handles protocol updates (e.g., switching a Service from TCP to UDP) by explicitly removing conditions associated with the previous protocol to prevent stale status reporting.
Implementation Details
Test Plan
Added coverage in l4_test.go and l4netlb_test.go to verify: