layered future: add policy ancestor status #3022
Conversation
Signed-off-by: Ian Rudie <ian.rudie@solo.io>
istio-testing
added
the
size/L
Signed-off-by: Ian Rudie <ian.rudie@solo.io>
ilrudie
force-pushed
the
layered-future/PolicyAncestorStatus
branch
from
c093215
to
8009f77
Compare
|
|
||
| // Status from Ancestors | ||
| // +optional | ||
| repeated istio.type.v1beta1.PolicyAncestorStatus ancestors = 4; |
There was a problem hiding this comment.
IMO this should remain hidden for now, certainly until its implemented. However its still experimental in GW-API afaik, so we don't want to be ahead of the curve there (in the past, we have done similar and they changed their API. oops).
There was a problem hiding this comment.
I'm not sure what exactly hidden means. Do you mean just skip this implementation entirely for now and don't expose a status or do you mean leverage the allow unknown fields in status to write a status without actually adding the experimental fields to the API?
There was a problem hiding this comment.
Maybe he meant something like // $hide_from_docs?
| repeated Condition conditions = 3; | ||
| } | ||
|
|
||
| message Condition { |
There was a problem hiding this comment.
IstioCondition is basically identical. @therealmitchconnors do you remember why we have LastProbeTime but metav1.Condition has ObservedGeneration?
There was a problem hiding this comment.
Maybe we could add the field as optional, possibly with hide_from_docs, into IstioCondition instead?
Signed-off-by: Ian Rudie <ian.rudie@solo.io>
|
@ilrudie: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| message PolicyAncestorStatus { | ||
| // AncestorRef corresponds with a ParentRef in the spec that this | ||
| // PolicyAncestorStatus struct describes the status of. | ||
| ParentReference ancestor_ref = 1; |
There was a problem hiding this comment.
Not sure what does this mean, can you give an concrete example
| string controller_name = 2; | ||
|
|
||
| // repeated k8s.io.apimachinery.pkg.apis.meta.v1.Condition conditions = 3; | ||
| repeated Condition conditions = 3; |
There was a problem hiding this comment.
If the ancestor conditions copied here, does that mean the status will be stored in both ancestor and child resource status?
There was a problem hiding this comment.
After looking at https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1alpha2.PolicyAncestorStatus.
I donot think that's a good design, declaritive APIs are originally standalone APIs, if coupled with others, maintaining statuses of other dependencies. It would be very difficult for the controllers.
cc @louiscryan to weigh in as he has been leading the design. FYI @howardjohn @keithmattix |
istio-testing
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
We can re-open if/when needed |
adds minimal policy ancestor status
closes #3014