Skip to content
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

layered future: add policy ancestor status #3022

Closed

Conversation

ilrudie
Copy link
Contributor

@ilrudie ilrudie commented Dec 8, 2023

adds minimal policy ancestor status

closes #3014

Signed-off-by: Ian Rudie <ian.rudie@solo.io>
@ilrudie ilrudie requested a review from a team as a code owner December 8, 2023 20:25
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 8, 2023
Signed-off-by: Ian Rudie <ian.rudie@solo.io>
@ilrudie ilrudie force-pushed the layered-future/PolicyAncestorStatus branch from c093215 to 8009f77 Compare December 8, 2023 20:35

// Status from Ancestors
// +optional
repeated istio.type.v1beta1.PolicyAncestorStatus ancestors = 4;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe he meant something like // $hide_from_docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will take a look at this today

repeated Condition conditions = 3;
}

message Condition {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IstioCondition is basically identical. @therealmitchconnors do you remember why we have LastProbeTime but metav1.Condition has ObservedGeneration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@istio-testing
Copy link
Collaborator

@ilrudie: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
release-notes_api 83ac99a link false /test release-notes
gencheck_api 83ac99a link true /test gencheck

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.

@howardjohn howardjohn added the do-not-merge/hold Block automatic merging of a PR. label Jan 25, 2024
message PolicyAncestorStatus {
// AncestorRef corresponds with a ParentRef in the spec that this
// PolicyAncestorStatus struct describes the status of.
ParentReference ancestor_ref = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the ancestor conditions copied here, does that mean the status will be stored in both ancestor and child resource status?

Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@linsun
Copy link
Member

linsun commented Jan 26, 2024

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 istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 2, 2024
@istio-testing
Copy link
Collaborator

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.

@ilrudie
Copy link
Contributor Author

ilrudie commented Mar 4, 2024

We can re-open if/when needed

@ilrudie ilrudie closed this Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Block automatic merging of a PR. needs-rebase Indicates a PR needs to be rebased before being merged size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

layered future: indicate whether a policy is implemented by the waypoint
5 participants