OCPBUGS-2325: Add E2E test cases for ingress #173
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
msherif1234
reviewed
Sep 15, 2022
README.md
Outdated
| @@ -225,6 +225,8 @@ make test-race | |||
|
|
|||
| 1. Bring up KinD cluster and deploy ingress node firewall operator from the steps outlined previously. | |||
| 2. Run full E2E test | |||
|
|
|||
| Note: SCTP tests are disabled by default. Enable by setting environment variable `ENABLE_SCTP_TESTS=true` | |||
There was a problem hiding this comment.
since u already editing this we should callout all args to our e2e so user of the repo can know what are the different knobs
also u need to default the same for our BM ci run here
openshift-ci/run_e2e.sh ?
There was a problem hiding this comment.
also pls explain why the default for SCTP is disabled, and include lkm dependencies
There was a problem hiding this comment.
also u need to default the same for our BM ci run here
openshift-ci/run_e2e.sh ?
You talking about wanting to enable SCTP tests on BM?
There was a problem hiding this comment.
Unable to do it easily - SCTP module unloaded on CoreOS by default. Its loaded by default in the KinD image however and its tested there and works ok. Is this sufficient?
martinkennelly
force-pushed
the
some_e2es
branch
from
83e7d00
to
0966802
Compare
martinkennelly
changed the title
|
@msherif1234 When youre back, PTAL. |
msherif1234
reviewed
Oct 13, 2022
There was a problem hiding this comment.
some initial comments I will need to spend more time when get back from PTO, pls open JIRA bug and added to the title we need to bring this to 4.12.
The fact that ur CI run passes for me is very concerning given we have an issue with current RHEL with virtio #139
| # control over the test cases that we would not enjoy if we attached to any other interface. | ||
| # Pod IPs are visibly by the XDP program after they have been de-encapsulated and for OVN-Kubernetes as the CNI - this is a GENEVE interface. | ||
| # Since OVN-Kubernetes is the default CNI for OCP, we set the interface to test against as the GENEVE interface. | ||
| export NODE_INTERFACE=genev_sys_6081 |
There was a problem hiding this comment.
hmm why we need this ? what was wrong with the prev logic ?
There was a problem hiding this comment.
because we know in advance what the interface name is now so theres no need to look it up
There was a problem hiding this comment.
While this might be ok for ci but it’s not how will users use it can we at least comment out the old way till he have rhel9 then try it again on +ve side having ci passing is good
msherif1234
reviewed
Oct 13, 2022
|
why we no longer check event logging for the deny rules ? |
I didn't think it was the right way to test success or failure. I wanted to rely on a connection being allowed or denied and not what the XDP program was reporting via events. |
We need both making sure events is emitted is very important part of this feature |
martinkennelly
changed the title
|
@martinkennelly: An error was encountered querying GitHub for users with public email (asood@redhat.com) for bug OCPBUGS-2325 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details. Full error message.
Post "http://ghproxy/graphql": dial tcp 172.30.229.2:80: i/o timeout
Please contact an administrator to resolve this issue, then request a bug refresh with In response to this:
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. |
|
/Jira refresh |
|
@msherif1234: An error was encountered querying GitHub for users with public email (asood@redhat.com) for bug OCPBUGS-2325 on the Jira server at https://issues.redhat.com/. No known errors were detected, please see the full error message for details. Full error message.
Post "http://ghproxy/graphql": dial tcp 172.30.229.2:80: connect: connection refused
Please contact an administrator to resolve this issue, then request a bug refresh with In response to this:
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. |
msherif1234
reviewed
Oct 14, 2022
| inf.SetName(name) | ||
| inf.SetNamespace(operatorNamespace) | ||
| return inf | ||
| } |
There was a problem hiding this comment.
we should check/wait also nodeState object to be deleted, similar to how we do currently
There was a problem hiding this comment.
I am not familiar with the code surrounding this. Isnt it deleted when no ingress node firewall config no longer targets a node? If so, why would we wait for it to be deleted when we are just deleting INFs?
|
/jira refresh |
openshift-ci-robot
added
jira/severity-critical
|
@msherif1234: This pull request references Jira Issue OCPBUGS-2325, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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. |
|
@msherif1234: once the present PR merges, I will cherry-pick it on top of 4.12 in a new PR and assign it to you. In response to this:
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. |
martinkennelly
force-pushed
the
some_e2es
branch
2 times, most recently
from
0df1f8b
to
97a22cb
Compare
martinkennelly
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
msherif1234
reviewed
Oct 17, 2022
| // For OCP, pod IPs are visible on the GENEVE interface (with OVN-Kubernetes as the CNI) or vlan interface (with Openshift-SDN as the CNI). | ||
| // This is where we want to attach our XDP program and perform filtering. | ||
| // For OCP with OVN-Kubernetes as the CNI, the interface name is always 'genev_sys_6081'. | ||
| table := []struct { |
There was a problem hiding this comment.
BTW when I originally mentioned tables I was referring to https://onsi.github.io/ginkgo/#table-specs
since it was my mistake for lack of clarity I can convert your tables to ginko once ur changes are merged
martinkennelly
force-pushed
the
some_e2es
branch
from
97a22cb
to
566c4f3
Compare
|
Test output for run on OCP: |
msherif1234
reviewed
Oct 18, 2022
msherif1234
reviewed
Oct 18, 2022
| err := wait.PollImmediate(retryInterval, timeout, func() (done bool, err error) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), timeout) | ||
| defer cancel() | ||
| err = client.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, ds) |
There was a problem hiding this comment.
Nit can this be replaced with
if err := status.IsIngressNodeFirewallConfigAvailable(ctx, client, namespace); err != nil {
return false, err
}
return true, nil
msherif1234
reviewed
Oct 18, 2022
|
/retest |
martinkennelly
force-pushed
the
some_e2es
branch
2 times, most recently
from
a6aaa48
to
3aa6f2c
Compare
msherif1234
reviewed
Oct 18, 2022
| node := nodesList.Items[0] | ||
|
|
||
| for _, address := range node.Status.Addresses { | ||
| ip := net.ParseIP(address.Address) |
There was a problem hiding this comment.
don't need to check address.Type == v1.NodeInternalIP 1st gere and below ?
msherif1234
reviewed
Oct 18, 2022
| func GetIPV6(ips []corev1.PodIP) string { | ||
| for _, ip := range ips { | ||
| parsedIP := net.ParseIP(ip.IP) | ||
| if parsedIP.To4() == nil { |
There was a problem hiding this comment.
NIT: why not use .To16 != nil similar to GetIPv4
|
@msherif1234: once the present PR merges, I will cherry-pick it on top of release-4.12 in a new PR and assign it to you. In response to this:
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. |
martinkennelly
force-pushed
the
some_e2es
branch
2 times, most recently
from
018ab20
to
22df6da
Compare
martinkennelly
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
msherif1234
reviewed
Oct 19, 2022
Signed-off-by: Martin Kennelly <mkennell@redhat.com>
martinkennelly
force-pushed
the
some_e2es
branch
from
22df6da
to
b5d50d4
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: martinkennelly, msherif1234 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@martinkennelly: all tests passed! Full PR test history. Your PR dashboard. 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. |
|
@martinkennelly: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-2325 has been moved to the MODIFIED state. In response to this:
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Martin Kennelly mkennell@redhat.com
/cc @msherif1234
More to come but this is a start.