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.

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

Ignore synced policies for hosted clusters #135

Conversation

JustinKuli
Copy link
Member

@JustinKuli JustinKuli commented Oct 11, 2023

Fixes issues noticed by CI run in stolostron, but not here. In particular, an issue identified by SonarCloud, and an issue with hosted mode clusters.

It calls them "Implicit memory aliasing in for loop"

Signed-off-by: Justin Kulikauskas <[email protected]>
(cherry picked from commit daa75992ebf0c8d7fd674081edfe8abcd723f069)
Signed-off-by: Justin Kulikauskas <[email protected]>
(cherry picked from commit bbe07497d20faf3ac2223fe1606afa5ca345965b)
@@ -97,13 +97,14 @@ func (r *ReplicatedPolicyReconciler) Reconcile(ctx context.Context, request ctrl
return reconcile.Result{}, nil
}

// do not handle a replicated policy which does not belong to the current cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is handle replicated policies which belong to the current cluster?

Copy link
Member Author

Choose a reason for hiding this comment

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

The special case being considered here, where it needs to skip dealing with a policy, is caused by the current cluster being a managed cluster for some other hub. In that hub-of-hubs situation, there can be a synced policy on this current cluster, which this reconciler would think is invalid because it can't find that root policy.

if pb.SubFilter == policiesv1.Restricted {
continue
}

found, err := r.isSingleClusterInDecisions(ctx, &pb, rootPlc.GetName(), clusterName)
found, err := r.isSingleClusterInDecisions(ctx, &pbList.Items[i], rootPlc.GetName(), clusterName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for saving memory?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the vulnerability/smell that SonarCloud noticed.

When you take the address of a loop variable like this was doing, you don't get what you'd necessarily expect. Golang kind of re-uses the loop variable in different iterations, so you end up with the same address each time, I think. I believe they want to change it in Go more generally, although I might be confusing some details, and this might not be exactly the same situation: https://go.dev/blog/loopvar-preview

Copy link
Contributor

@yiraeChristineKim yiraeChristineKim Oct 11, 2023

Choose a reason for hiding this comment

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

oh that... got it!! Good catch I need to be careful too

@openshift-ci
Copy link

openshift-ci bot commented Oct 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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,yiraeChristineKim]

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants