-
Notifications
You must be signed in to change notification settings - Fork 619
fix(crd): correct EnvoyPatchPolicy printer columns #7776
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: main
Are you sure you want to change the base?
fix(crd): correct EnvoyPatchPolicy printer columns #7776
Conversation
chaima-belhedi
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.
Thanx for taking care of this @Aditya7880900936
|
Thanks a lot for the review and approval! Appreciate your time and guidance @chaima-belhedi |
|
how did you fix this without updating any markers |
|
Thanks for the pointer! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7776 +/- ##
==========================================
- Coverage 72.64% 72.61% -0.04%
==========================================
Files 235 235
Lines 34873 34873
==========================================
- Hits 25334 25323 -11
- Misses 7736 7744 +8
- Partials 1803 1806 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
please run |
|
I’ve run make -k gen-check locally and committed the updated helm test outputs. |
ae710b9 to
ed5ef5f
Compare
| // +kubebuilder:resource:categories=envoy-gateway,shortName=epp | ||
| // +kubebuilder:subresource:status | ||
| // +kubebuilder:printcolumn:name="Status",type=string,JSONPath=`.status.conditions[?(@.type=="Programmed")].reason` | ||
| // +kubebuilder:printcolumn:name="Accepted",type=string,JSONPath=`.status.ancestors[0].conditions[?(@.type=="Accepted")].status` |
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 we only check the first ancestors?
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.
is it possible to check any or all?
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.
status.ancestors can contain multiple entries, but kubectl printer columns need to resolve to a single scalar value and don’t support aggregation across arrays. Because of that, checking “any” or “all” ancestors isn’t supported in printer columns today.
Using ancestors[0] follows the existing Gateway API convention to surface the primary attachment point, and is consistent with how other policy CRDs expose status via printer columns.
Happy to adjust if there’s a preferred convention here.
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.
what would happen if we use ancestors[*] here?
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 we use ancestors[*], the JSONPath evaluation would return a list of values rather than a single scalar (e.g. ["True", "False"] when multiple ancestors exist).
Kubectl printer columns don’t support aggregation or reduction over arrays, so returning a list can lead to unstable or non-deterministic output (empty values, joined lists, or differing behavior across kubectl versions).
That’s why most CRDs constrain printer columns to a single, deterministic value. Using ancestors[0] follows the existing Gateway API convention to surface the primary attachment point and keeps the printer column output stable and predictable.
For full multi-ancestor status, users can still inspect .status.ancestors directly via kubectl get -o yaml.
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.
it's fine to me use ancestors[0], it's should be accepted for all or none.
@envoyproxy/gateway-maintainers PTAL
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 confirming! Happy to keep it consistent with ancestors[0] for both.
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.
yeah this is fine imo, order is unlikely to change
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 this work properly even when no status is set ? (when extensionApis.enableEnvoyPatchPolicy: false)
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.
Good question — yes, this should behave fine.
When extensionApis.enableEnvoyPatchPolicy is false, the controller doesn’t populate .status, so the JSONPath won’t resolve. In that case, kubectl printer columns simply render an empty value rather than erroring.
This is consistent with other Gateway API resources where status may be absent or delayed, and matches existing printer column behavior across CRDs.
Users can still inspect the full object via kubectl get -o yaml if needed.
|
The remaining CI failures are from coverage-test. Please let me know if you’d prefer: skipping/overriding coverage for this PR, or any specific test adjustments you’d like me to make. |
|
Friendly ping — happy to help unblock if anything else is needed. |
|
Thanks for the reviews and approvals! |
|
Hi @zirain , The PR has approvals now. The only remaining CI failures are coverage tests, which appear expected for this CRD-only change. Please let me know if you’re okay to merge as-is, or if you’d like me to make any changes before merge. |
Signed-off-by: Aditya7880900936 <[email protected]>
Signed-off-by: Aditya7880900936 <[email protected]>
Signed-off-by: Aditya7880900936 <[email protected]>
80535db to
9ba0c24
Compare
|
Thanks everyone for the reviews and help! |
|
/retest |
|
Friendly ping @zirain — whenever you’re back online, please take a look. |
|
Hi @zirain , They fail in ZoneAwareRouting / BackendTrafficPolicy tests, which don’t appear related to this PR (CRD printer column metadata only). I’ve updated the branch to latest main. |
What type of PR is this?
fix(crd): correct EnvoyPatchPolicy printer columns
What this PR does / why we need it:
This PR fixes the
additionalPrinterColumnsconfiguration for theEnvoyPatchPolicy CRD.
Previously, the STATUS column was empty because the printer column
referenced a non-existent field under
.status.conditions.EnvoyPatchPolicy follows Gateway API conventions and stores conditions
under
.status.ancestors[].conditions.This change updates the printer columns to reference condition
statusunder
.status.ancestors[].conditions, sokubectl get envoypatchpoliciescorrectly shows Accepted and Programmedstates.
Which issue(s) this PR fixes:
Fixes #7745
Release Notes: No