fix: wrong gatewayclass status without paramsRef#1999
fix: wrong gatewayclass status without paramsRef#1999cnvergence wants to merge 1 commit intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Karol Szwaj <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1999 +/- ##
==========================================
+ Coverage 65.51% 65.56% +0.04%
==========================================
Files 92 92
Lines 13528 13528
==========================================
+ Hits 8863 8869 +6
+ Misses 4116 4110 -6
Partials 549 549
|
|
PTAL @arkodg |
| if !refsEnvoyProxy(gc) { | ||
| return fmt.Errorf("unsupported parametersRef for gatewayclass %s", gc.Name) | ||
| } | ||
| } |
There was a problem hiding this comment.
here's the issue with this new code that was added
- its a
processParamsReffunction for a specificgcbut it fetches allEnvoyProxiesand then tries to remove the finalizer for unrelatedEnvoyProxies
I see another bug on L1675 where we are removing a finalizer from the EnvoyProxy without actually checking if the EnvoyProxy is being referenced by any other GatewayClass or not.
We are very close to our v0.6 release and I prefer reverting #1534 and adding the feature later when all issues have been addressed
There was a problem hiding this comment.
This L1675 was actually added to support this case #1534 (comment)
There was a problem hiding this comment.
yes, but the way its written it, it can just rm another EnvoyProxy's finalizer which may actually be referenced by a GatewayClass
There was a problem hiding this comment.
I assumed it was supporting only one gateway class and therefore only one envoy proxy config or am I understanding it wrong?
Finalizer will be only there if it was referenced by a GatewayClass that was accepted.
There was a problem hiding this comment.
its a processParamsRef function for a specific gc but it fetches all EnvoyProxies and then tries to remove the finalizer for unrelated EnvoyProxies
I agree with that, it could definitely be refactored now and have a list taken out of this function
There was a problem hiding this comment.
the issue is - its a one level check for the EnvoyProxy without backtracking which GatewayClass it belongs to, it may belong to a accepted GatewayClass from another EG controller
There was a problem hiding this comment.
Hmm, that could be the truth, so we would need to add the check for ControllerName as well
There was a problem hiding this comment.
ive raised #2000 for the revert
@cnvergence we really appreciate the hard work on this feature, the provider codebase is already so complex, and there are many edge cases that needed to be handled before we introduce the Envoyproxy finalizer which is not a must have yet, so prefer reverting for now
There was a problem hiding this comment.
I agree, there are a lot of corner cases, I would be happy to help again, after the 0.6.0.
There was a problem hiding this comment.
thanks for understanding, I actually prefer pausing on this one, until we've cleaned up some of the GC and EP handling in the code before re adding it
2 participants
What this PR does / why we need it:
Fixing the bug:
Providing GatewayClass without paramsRef
results in error in the provider, not accepting the GatewayClass.
status: conditions: - message: 'Invalid parametersRef: failed to find envoyproxy referenced by gatewayclass: example-class' observedGeneration: 2 reason: InvalidParameters status: "False" type: Accepted NAME CONTROLLER ACCEPTED AGE example-class gateway.envoyproxy.io/gatewayclass-controller False 2m