-
Notifications
You must be signed in to change notification settings - Fork 295
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
Fix: PSS label reset issue #821
base: master
Are you sure you want to change the base?
Fix: PSS label reset issue #821
Conversation
Signed-off-by: Varsha B <[email protected]>
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Thanks @varshab1210. I have left a suggestion in watch logic. PTAL. It would also be great to have some unit tests for this. Maybe a test to verify that labels are added to the namespace and correctly reconciled when modified. You can use below tests for reference.
func TestReconcile_GitOpsNamespace(t *testing.T) { - Fix: Ensure ConfigMap and StatefulSet updates are applied during operator upgrades argoproj-labs/argocd-operator#1619
Watches(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{ | ||
Name: "openshift-gitops", | ||
}}, | ||
handler.EnqueueRequestsFromMapFunc(namespaceMapper), | ||
). |
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.
The current watch is not functioning as expected. It triggers reconciliation for changes to all namespaces. For example, I made a change to the default
namespace, and the watch still processed it:
2025-01-03T13:14:56+05:30 INFO controller_gitopsservice Reconciling GitopsService {"Request.Namespace": "", "Request.Name": "default"}
2025-01-03T13:14:56+05:30 INFO controller_gitopsservice No ResourceQuota set for namespace {"Request.Namespace": "", "Request.Name": "default", "Name": "openshift-gitops"}
2025-01-03T13:14:56+05:30 INFO controller_gitopsservice Reconciling ArgoCD {"Request.Namespace": "", "Request.Name": "default", "Namespace": "openshift-gitops", "Name": "openshift-gitops"}
2025-01-03T13:14:56+05:30 INFO controller_gitopsservice Reconciling plugin deployment {"Request.Namespace": "", "Request.Name": "default", "Namespace": "openshift-gitops", "Name": "gitops-plugin"}
We need to use a predicate to filter events for only the openshift-gitops
namespace. Can you try the following patch? It sets up a watch with the necessary filtering predicate. Since the watch provides the namespace name, we can use EnqueueRequestForObject
instead of a custom EnqueueRequestsFromMapFunc
to simplify the logic.
Watches(&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{ | |
Name: "openshift-gitops", | |
}}, | |
handler.EnqueueRequestsFromMapFunc(namespaceMapper), | |
). | |
Watches( | |
&corev1.Namespace{}, | |
&handler.EnqueueRequestForObject{}, | |
builder.WithPredicates(predicate.NewPredicateFuncs(func(obj client.Object) bool{ | |
return obj.GetName() == "openshift-gitops" | |
})), | |
). |
What type of PR is this?
/kind bug
What does this PR do / why we need it:
Previously, PSS labels in openshift-gitops ns did not get reconciled and reset upon modification to the values. This PR fixes the issue
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
https://issues.redhat.com/browse/GITOPS-5945
Fixes #?
Test acceptance criteria:
How to test changes / Special notes to the reviewer: