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.

Sign up for GitHub

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

Jump to bottom

Mirror from OCM around 56 #80

Merged
merged 7 commits into from
Apr 21, 2023
Merged

Mirror from OCM around 56 #80

merged 7 commits into from
Apr 21, 2023

Conversation

JustinKuli
Copy link

Closes #79

@JustinKuli
Refactor SendEvent
55d433f 
This should make it slightly more re-usable in other controllers. In
particular this allows the `instance` to be `nil`, which might be the
case if the template was not created.

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
(cherry picked from commit c6dadad)
@JustinKuli
Add clientset to template-sync reconciler
d0e4651 
This might be slightly more performant, and other things can use this
clientset.

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
(cherry picked from commit c01cf0a)
@JustinKuli
Synchronously emit template events
8f815dd 
Previously, the controller-runtime event recorder was used for these
events. Other policy controllers have moved away from that, for various
reasons. In this case, if a policy went from pending to noncompliant and
back to pending, the "old" pending event would be re-used by the event
recorder, and only the `lastTimestamp` would be updated. In this case,
if a policy controller emitted a compliance event within the same second
as the Pending event, the status-sync would see it as a tie, and use the
hex-encoded nanoseconds in the event name. But the event name was not
updated from the original instance when the policy was pending, so the
events would be ordered incorrectly.

Most error cases from this synchronous sending can be ignored because
they are already error cases that would be requeued.

Refs:
 - https://issues.redhat.com/browse/ACM-4699

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
(cherry picked from commit f0e2c60)
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 18, 2023
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

SonarCloud found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@dhaiducek
Copy link

dhaiducek commented Apr 18, 2023 •
edited
Loading

That's a lot of "vulnerabilities". If the error assignments are set to an empty assignment, maybe that'll make gosec (and the linter happy)? Alternatively, does the function need to return an error?

@JustinKuli
Copy link
Author

There are a small number of cases where the error is useful, so I'd rather not fully remove it. Slightly disappointing that the linter comments aren't read by gosec...

@JustinKuli
Copy link
Author

JustinKuli commented Apr 18, 2023 •
edited
Loading

Now it's failed three times with

• [FAILED] [185.881 seconds]
Test Gatekeeper ConstraintTemplate and constraint sync [skip-minimum]
/home/runner/work/governance-policy-framework-addon/governance-policy-framework-addon/governance-policy-framework-addon/test/e2e/case17_gatekeeper_sync_test.go:26
  [It] should return Gatekeeper audit results
  /home/runner/work/governance-policy-framework-addon/governance-policy-framework-addon/governance-policy-framework-addon/test/e2e/case17_gatekeeper_sync_test.go:230

  Begin Captured GinkgoWriter Output >>
    STEP: Checking if policy status is compliant for the constraint 04/18/23 19:25:44.25
    STEP: Adding ConfigMaps that violate the constraint 04/18/23 19:25:44.257
    STEP: Checking if policy status is noncompliant for the constraint 04/18/23 19:25:44.274
    STEP: Deleting the namespace case17-gk-test 04/18/23 19:28:44.275
    STEP: Deleting policy case17-gk-policy on the hub in ns:managed 04/18/23 19:28:44.279
    STEP: Cleaning up the events for the policy case17-gk-policy 04/18/23 19:28:44.291
    STEP: Deleting policy case17-gk-policy-2 on the hub in ns:managed 04/18/23 19:28:44.916
    STEP: Cleaning up the events for the policy case17-gk-policy-2 04/18/23 19:28:44.923
    STEP: Fixing the Gatekeeper webhook if required 04/18/23 19:28:45.101
    STEP: Waiting for the namespace case17-gk-test to be deleted 04/18/23 19:28:45.112
  << End Captured GinkgoWriter Output

  Timed out after 180.001s.
  Expected success, but got an error:
      <*errors.errorString | 0xc0003fbf90>: {
          s: "Assertion in callback at /home/runner/work/governance-policy-framework-addon/governance-policy-framework-addon/governance-policy-framework-addon/test/e2e/case17_gatekeeper_sync_test.go:306 failed:\nExpected\n    <string>: NonCompliant; NonCompliant; template-error; Mapping not found, check if the required ConstraintTemplate has been deployed: the resource version was not found: constraints.gatekeeper.sh/v1beta1, Kind=Case17ConstraintTemplate\nnot to equal\n    <string>: NonCompliant; NonCompliant; template-error; Mapping not found, check if the required ConstraintTemplate has been deployed: the resource version was not found: constraints.gatekeeper.sh/v1beta1, Kind=Case17ConstraintTemplate",
      }
      Assertion in callback at /home/runner/work/governance-policy-framework-addon/governance-policy-framework-addon/governance-policy-framework-addon/test/e2e/case17_gatekeeper_sync_test.go:306 failed:
      Expected
          <string>: NonCompliant; NonCompliant; template-error; Mapping not found, check if the required ConstraintTemplate has been deployed: the resource version was not found: constraints.gatekeeper.sh/v1beta1, Kind=Case17ConstraintTemplate
      not to equal
          <string>: NonCompliant; NonCompliant; template-error; Mapping not found, check if the required ConstraintTemplate has been deployed: the resource version was not found: constraints.gatekeeper.sh/v1beta1, Kind=Case17ConstraintTemplate
  In [It] at: /home/runner/work/governance-policy-framework-addon/governance-policy-framework-addon/governance-policy-framework-addon/test/e2e/case17_gatekeeper_sync_test.go:308

@JustinKuli
Copy link
Author

Quantum test: when I'm debugging it (including locally), it passes.

@JustinKuli
Copy link
Author

This PR will probably be replaced by something new incorporating open-cluster-management-io#59

@JustinKuli
Fix gosec:G104
2666d6d 
Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
(cherry picked from commit d6cb733)
@JustinKuli
Check gosec results in CI
b31170f 
The KinD tests action will now run the gosec-scan, and that target will
fail if any vulnerabilities are found. The target was also configured to
ignore the test code.

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
(cherry picked from commit 8c251da)
@JustinKuli
Add more information to the e2e-debug
e88d76f 
Information about the gatekeeper pods might help if those tests fail.

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
(cherry picked from commit 0034b03)
@JustinKuli
Fix test assertion for duplicate events
8a87b21 
The test is meant to ensure that the gatekeeper-sync is not emitting the
same event multiple times in a row. But the assertion was failing
sometimes because of duplicate events from template-errors. Those will
sometimes occur during normal (correct) operation of the template-sync.

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
(cherry picked from commit 906dcff)
@JustinKuli JustinKuli changed the title Mirror OCM 56 Mirror from OCM around 56 Apr 21, 2023
dhaiducek
dhaiducek approved these changes Apr 21, 2023
View reviewed changes
Copy link

@dhaiducek dhaiducek left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Having make gosec-scan in the workflow twice gives me pause, but it won't hurt and we can clean it up in a separate PR.

@openshift-ci openshift-ci bot added the lgtm label Apr 21, 2023
@openshift-ci
Copy link

openshift-ci bot commented Apr 21, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, JustinKuli

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 [JustinKuli,dhaiducek]

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

@sonarcloud
Copy link

sonarcloud bot commented Apr 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

79.8% 79.8% Coverage
7.0% 7.0% Duplication

@openshift-ci openshift-ci bot merged commit 0c6e481 into stolostron:main Apr 21, 2023
@JustinKuli JustinKuli deleted the mirror-ocm-56 branch May 2, 2023 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhaiducek dhaiducek dhaiducek approved these changes

Assignees

@dhaiducek dhaiducek

Labels
approved dco-signoff: yes lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

😿 Failed to sync the upstream PRs: #56, #57
2 participants
@JustinKuli @dhaiducek