-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
policy: Add e2e egress tests #13390
base: main
Are you sure you want to change the base?
policy: Add e2e egress tests #13390
Conversation
Signed-off-by: Zahari Dichev <[email protected]>
status: None, | ||
}, | ||
) | ||
.await; |
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.
Would it make sense to await for the egressnetwork to become accepted before attempting to send traffic?
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.
Yes this makes sense. We should do that.
egress.spec.traffic_policy = k8s::policy::TrafficPolicy::Deny; | ||
update(&client, egress).await; | ||
|
||
let not_allowed = curl |
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 there a potential race condition where we send this traffic before the policy control picks up the update to the egressnetwork? is there something we can await on to wait until we know the policy control has picked up the update? or should we just retry the curl so that if the update hasn't been picked up yet we can try again?
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 call, we can await on a condition here just to make sure the update is reflected in the kuberntes API. When it comes to making sure the policy controller has picked up the update, I am not sure what exactly we can do here.
Retrying the curl request feels like an overkill to me.
Signed-off-by: Zahari Dichev <[email protected]>
policy-test/src/lib.rs
Outdated
move |egress_net: Option<&EgressNetwork>| { | ||
if let Some(egress_net) = &egress_net { | ||
return egress_net.spec.traffic_policy == policy; | ||
} | ||
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.
Should this also include something like
egress_net
.status
.conditions
.iter()
.any(|c| c.type_ == "Accepted" && c.status == "True")
to make sure the status controller has consumed it?
I notice there are some other places in tests, though, where we don't check statuses fully so maybe that's not necessary? E.g. pub async fn await_grpc_route_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.
Good point, I will do one more pass over the tests and see whether there are opportunities to tighten things up in terms of status checking.
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.
I have added code to assert that egress network is accepted after creation or update in d99216a
We can consider tightening up the assertions around route statuses as well as this is done sporadically now. I think however that this is beyond the cope of this PR as it spans most of the outbound API tests, not only the egress ones. It is a good change however.
Signed-off-by: Zahari Dichev <[email protected]>
This change adds e2e
EgressNetwork
tests that exercise:Signed-off-by: Zahari Dichev [email protected]