OCPBUGS-1750: Trim ACL names according to RFC1123 #1282

tssurya · 2022-09-27T17:13:02Z

Cherry picks #1281 and c204519
First cherry-pick had to be modified to use libovsdbops.BuildACL instead of ovn.BuildACL since we are missing 44ad754 in 4.11
Second cherry-pick had a very minor conflict outlined in the commit message.

openshift-ci · 2022-09-27T17:13:06Z

@tssurya: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

OCPBUGS 1750: Trim ACL names according to RFC1123

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.

openshift-ci-robot · 2022-09-27T17:13:53Z

@tssurya: This pull request references Jira Issue OCPBUGS-1750, which is invalid:

expected the bug to target the "4.11.z" version, but no target version was set
expected Jira Issue OCPBUGS-1750 to depend on a bug targeting a version in 4.12.0 and in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), but no dependents were found

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Cherry picks #1281 and c204519
Second cherry-pick had a very minor conflict outlined in the commit message.

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.

openshift-ci · 2022-09-27T17:14:08Z

@tssurya: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

OCPBUGS-1750: Trim ACL names according to RFC1123

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.

tssurya · 2022-09-27T17:14:50Z

/jira refresh

openshift-ci-robot · 2022-09-27T17:14:52Z

@tssurya: This pull request references Jira Issue OCPBUGS-1750, which is invalid:

expected Jira Issue OCPBUGS-1750 to depend on a bug targeting a version in 4.12.0 and in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), but no dependents were found

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

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.

tssurya · 2022-09-27T17:17:06Z

/jira refresh

openshift-ci-robot · 2022-09-27T17:17:08Z

@tssurya: This pull request references Jira Issue OCPBUGS-1750, which is invalid:

expected dependent Jira Issue OCPBUGS-1705 to be in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), but it is POST instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

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.

tssurya · 2022-09-27T17:22:56Z

/hold

openshift-ci-robot · 2022-09-27T17:46:43Z

@tssurya: This pull request references Jira Issue OCPBUGS-1750, which is invalid:

expected dependent Jira Issue OCPBUGS-1705 to be in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), but it is POST instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Cherry picks #1281 and c204519
First cherry-pick had to be modified to use libovsdbops.BuildACL instead of ovn.BuildACL since we are missing 44ad754 in 4.11
Second cherry-pick had a very minor conflict outlined in the commit message.

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.

openshift-ci · 2022-09-27T17:46:44Z

@tssurya: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

OCPBUGS-1750: Trim ACL names according to RFC1123

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.

tssurya · 2022-09-28T06:02:05Z

/retest

tssurya · 2022-09-28T21:12:55Z

/jira refresh

openshift-ci-robot · 2022-09-28T21:12:58Z

@tssurya: This pull request references Jira Issue OCPBUGS-1750, which is invalid:

expected dependent Jira Issue OCPBUGS-1705 to be in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), but it is ON_QA instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

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.

tssurya · 2022-09-28T21:17:49Z

/hold cancel

tssurya · 2022-09-28T21:18:22Z

/assign @trozet
/assign @jcaamano

jcaamano · 2022-09-29T10:13:29Z

@tssurya Why do we need https://github.com/openshift/ovn-kubernetes/commit/c20451998bcc62ef5281d1316110ed44219386b9?
It doesn't really do much other than accommodating for the limitation of the libovsdb test server that does not handle referential integrity. So it should only be relevant for unit tests.

tssurya · 2022-09-29T11:05:28Z

@tssurya Why do we need https://github.com/openshift/ovn-kubernetes/commit/c20451998bcc62ef5281d1316110ed44219386b9? It doesn't really do much other than accommodating for the limitation of the libovsdb test server that does not handle referential integrity. So it should only be relevant for unit tests.

There is a place in the code: https://github.com/openshift/ovn-kubernetes/pull/1282/files#diff-cc83e19af1c257d5a09b711d5977d8f8c20beb34b7b5d3eb37b2f2c53ded1bf7L143 where we were calling DeleteACLs directly which would complain against the referential integrity violation. Nadia's PR will help with that and keep things consistent. SInce 4.11 will be around for some time, I don't want people stepping on the DeleteACLs function and forgetting to individually remove things from PGs and that PR adds that part: https://github.com/openshift/ovn-kubernetes/pull/1282/files#diff-ee54e95b45c5a079454546fed94fcef68b13d9dc6cd14e192585fe2465bcaefcR170 whereby calling DeleteACLs will auto-delete the the acls from PGs and switches.

tssurya · 2022-09-29T11:06:48Z

So it should only be relevant for unit tests.

Not sure if I follow this part, any place in code like efw or policies that call DeleteACLs - this will be relevant for all that right?

jcaamano · 2022-09-29T11:51:48Z

@tssurya Why do we need https://github.com/openshift/ovn-kubernetes/commit/c20451998bcc62ef5281d1316110ed44219386b9? It doesn't really do much other than accommodating for the limitation of the libovsdb test server that does not handle referential integrity. So it should only be relevant for unit tests.

There is a place in the code: https://github.com/openshift/ovn-kubernetes/pull/1282/files#diff-cc83e19af1c257d5a09b711d5977d8f8c20beb34b7b5d3eb37b2f2c53ded1bf7L143 where we were calling DeleteACLs directly which would complain against the referential integrity violation. Nadia's PR will help with that and keep things consistent. SInce 4.11 will be around for some time, I don't want people stepping on the DeleteACLs function and forgetting to individually remove things from PGs and that PR adds that part: https://github.com/openshift/ovn-kubernetes/pull/1282/files#diff-ee54e95b45c5a079454546fed94fcef68b13d9dc6cd14e192585fe2465bcaefcR170 whereby calling DeleteACLs will auto-delete the the acls from PGs and switches.

Ok. So comments about this:

That was because using DeleteACLs there is not the best thing to do. The call to use should be DeleteACLsFromPortGroup directly. The server will garbage-collect the ACLs once they are removed form the port groups. On some places we would call DeleteACLs after DeleteACLsFromPortGroup to counter the fact that the test server does not do the garbage collection; on other places we would account for the leftover ACLs in the tests. Arguably the other best alternative would have been to remove the ACLs inside DeleteACLsFromPortGroup instead of the other way around; and not having a DeleteACLs at all to avoid the confusion. Anyway we will live with it for now.
This seems like separate fix. Even if we use this PR, should we track it with a different bug so that people that encounters the issue is aware that this is now fixed?

jcaamano · 2022-09-29T11:59:27Z

How difficult would be to cherry pick ovn.BuildACL as well or how probable it is that is cherry-picked in the future?
The reason I ask is because if ovn.BuildACL is back-ported in the future, most likely the call to libovsdbops.BuildACL that is done here will be missed and not changed to ovn.BuildACL

… deleted. To make sure we don't forget about that, add port group and switch refs to DeleteACLs as args. UUIDs don't need to be cleaned up only if port group or switch is completely deleted. Update all DeleteACLs calls to cleanup refs. Signed-off-by: Nadia Pinaeva <npinaeva@redhat.com> (cherry picked from commit c204519) Conflicts in 4.11 because openshift@e1f7da0#diff-ee54e95b45c5a079454546fed94fcef68b13d9dc6cd14e192585fe2465bcaefcL141 is missing: go-controller/pkg/libovsdbops/acl.go

Initially when the commit to cleanup ACLs was done in ovn-org/ovn-kubernetes#3038 I had assumed that ACL names weren't truncated to fit the 63 char length and thought they were like other fields. Based on that assumption there were two places in code where I used a predicate match based on acl names which was plain stupidity. This PR fixes that: 1) When the arp && arp || nd bug was being cleaned up, I was trying to be extra careful in matching on old exp: "arp" and the acl name. I deleted the second part of that logic since there can be cases where the entire suffix is missing like when the namespace is 63 chars. So let us stick to matching on acl.Match alone 2) When the deny default ACL duplicates per namespace were being updated - again I was matching on ACL names to exclude the arp policies. I replaced this with using acl.Match which is more reliable and accurate. In ovn-org/ovn-kubernetes#3181 we realised we had to trim the acls and we fixed that, but didn't fix the above two issues. So let's fix that up and also I added a unit test case with a long named namespace. Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com> (cherry picked from commit 6f60497) Conflicts in 4.11: go-controller/pkg/ovn/policy_test.go because we are missing https://github.com/ovn-org/ovn-kubernetes/pull/3161/files#diff-2a5ea421d13f2f639ad217cada1f5c741e9ba6d68abcf431a700c0f68b71e04dR1729

anuragthehatter · 2022-10-07T20:03:39Z

QE testing passed on this PR covering all corner cases

/label qe-approved

anuragthehatter · 2022-10-07T20:13:14Z

/label cherry-pick-approved

jcaamano · 2022-10-10T08:25:48Z

/retest

tssurya · 2022-10-10T15:03:34Z

/test e2e-hypershift

openshift-ci · 2022-10-10T15:48:48Z

@tssurya: 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
ci/prow/e2e-openstack-ovn	`4d5d7d6`	link	false	`/test e2e-openstack-ovn`
ci/prow/e2e-hypershift	`4d5d7d6`	link	false	`/test e2e-hypershift`

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.

flavio-fernandes

/lgtm

openshift-ci · 2022-10-10T19:21:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flavio-fernandes, jcaamano, tssurya

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:

~~OWNERS~~ [jcaamano]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

flavio-fernandes · 2022-10-10T19:22:07Z

/jira refresh

openshift-ci-robot · 2022-10-10T19:22:09Z

@flavio-fernandes: This pull request references Jira Issue OCPBUGS-1750, which is invalid:

expected dependent Jira Issue OCPBUGS-1705 to be in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), but it is POST instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

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.

tssurya · 2022-10-10T21:10:31Z

/jira refresh

openshift-ci-robot · 2022-10-10T21:10:34Z

@tssurya: This pull request references Jira Issue OCPBUGS-1750, which is invalid:

expected dependent Jira Issue OCPBUGS-1705 to be in one of the following states: VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), but it is MODIFIED instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

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.

tssurya · 2022-10-10T21:15:29Z

/jira refresh

openshift-ci-robot · 2022-10-10T21:15:34Z

@tssurya: This pull request references Jira Issue OCPBUGS-1750, which is valid. The bug has been moved to the POST state.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.11.z) matches configured target version for branch (4.11.z)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)
dependent bug Jira Issue OCPBUGS-1705 is in the state Verified, which is one of the valid states (VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE))
dependent Jira Issue OCPBUGS-1705 targets the "4.12.0" version, which is one of the valid target versions: 4.12.0
bug has dependents

Requesting review from QA contact:
/cc @anuragthehatter

In response to this:

/jira refresh

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.

tssurya · 2022-10-10T21:18:24Z

/jira refresh

openshift-ci-robot · 2022-10-10T21:18:27Z

@tssurya: This pull request references Jira Issue OCPBUGS-1750, which is valid.

6 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.11.z) matches configured target version for branch (4.11.z)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)
dependent bug Jira Issue OCPBUGS-1705 is in the state Verified, which is one of the valid states (VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE))
dependent Jira Issue OCPBUGS-1705 targets the "4.12.0" version, which is one of the valid target versions: 4.12.0
bug has dependents

Requesting review from QA contact:
/cc @anuragthehatter

In response to this:

/jira refresh

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.

openshift-ci-robot · 2022-10-10T21:20:42Z

@tssurya: All pull requests linked via external trackers have merged:

openshift/ovn-kubernetes#1282

Jira Issue OCPBUGS-1750 has been moved to the MODIFIED state.

In response to this:

Cherry picks #1281 and c204519
First cherry-pick had to be modified to use libovsdbops.BuildACL instead of ovn.BuildACL since we are missing 44ad754 in 4.11
Second cherry-pick had a very minor conflict outlined in the commit message.

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.

tssurya changed the title ~~OCPBUGS 1750: Trim ACL names according to RFC1123~~ OCPBUGS-1750: Trim ACL names according to RFC1123 Sep 27, 2022

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Sep 27, 2022

openshift-ci bot requested review from abhat and jcaamano September 27, 2022 17:14

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2022

tssurya force-pushed the OCPBUGS-1750 branch from 7207d0c to b4bbe36 Compare September 27, 2022 17:44

tssurya force-pushed the OCPBUGS-1750 branch from b4bbe36 to ac0341e Compare September 27, 2022 18:22

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2022

openshift-ci bot assigned jcaamano and trozet Sep 28, 2022

tssurya force-pushed the OCPBUGS-1750 branch from ac0341e to a984fb3 Compare October 7, 2022 17:44

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2022

tssurya force-pushed the OCPBUGS-1750 branch from a984fb3 to 4d5d7d6 Compare October 7, 2022 18:19

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Oct 7, 2022

openshift-ci bot added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Oct 7, 2022

flavio-fernandes reviewed Oct 10, 2022

View reviewed changes

openshift-ci bot assigned flavio-fernandes Oct 10, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2022

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Oct 10, 2022

openshift-ci-robot removed the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Oct 10, 2022

openshift-ci bot requested a review from anuragthehatter October 10, 2022 21:15

openshift-ci bot merged commit 336fb2b into openshift:release-4.11 Oct 10, 2022

OCPBUGS-1750: Trim ACL names according to RFC1123 #1282

OCPBUGS-1750: Trim ACL names according to RFC1123 #1282

Conversation

tssurya commented Sep 27, 2022 • edited

openshift-ci bot commented Sep 27, 2022

openshift-ci-robot commented Sep 27, 2022

openshift-ci bot commented Sep 27, 2022

tssurya commented Sep 27, 2022

openshift-ci-robot commented Sep 27, 2022

tssurya commented Sep 27, 2022

openshift-ci-robot commented Sep 27, 2022

tssurya commented Sep 27, 2022

openshift-ci-robot commented Sep 27, 2022

openshift-ci bot commented Sep 27, 2022

tssurya commented Sep 28, 2022

tssurya commented Sep 28, 2022

openshift-ci-robot commented Sep 28, 2022

tssurya commented Sep 28, 2022

tssurya commented Sep 28, 2022

jcaamano commented Sep 29, 2022

tssurya commented Sep 29, 2022

tssurya commented Sep 29, 2022

jcaamano commented Sep 29, 2022

jcaamano commented Sep 29, 2022

anuragthehatter commented Oct 7, 2022

anuragthehatter commented Oct 7, 2022

jcaamano commented Oct 10, 2022

tssurya commented Oct 10, 2022

openshift-ci bot commented Oct 10, 2022

flavio-fernandes left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 10, 2022

flavio-fernandes commented Oct 10, 2022

openshift-ci-robot commented Oct 10, 2022

tssurya commented Oct 10, 2022

openshift-ci-robot commented Oct 10, 2022

tssurya commented Oct 10, 2022

openshift-ci-robot commented Oct 10, 2022

tssurya commented Oct 10, 2022

openshift-ci-robot commented Oct 10, 2022

openshift-ci-robot commented Oct 10, 2022

tssurya commented Sep 27, 2022 •

edited