Skip to content

Guardrails VAP replacement#4731

Draft
yjst2012 wants to merge 8 commits intomasterfrom
yjst2012/ARO-23216
Draft

Guardrails VAP replacement#4731
yjst2012 wants to merge 8 commits intomasterfrom
yjst2012/ARO-23216

Conversation

@yjst2012
Copy link
Copy Markdown
Collaborator

@yjst2012 yjst2012 commented Mar 31, 2026

Which issue this PR addresses:

ARO-23216 Convert All Policies from Rego to CEL
ARO-23217 Implement VAP Deployment and Version Detection
ARO-23218 Implement Migration Logic

What this PR does / why we need it:

Initial code for Guardrails VAP Replacement

Test plan for issue:

Tested on my cluster

Is there any documentation that needs to be updated for this PR?

Internal eng.ms doc to be updated in a separate pr

How do you know this will function as expected in production?

It should work the same way as in my test env

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Mar 31, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@yjst2012 yjst2012 force-pushed the yjst2012/ARO-23216 branch from ea078df to 726fe57 Compare March 31, 2026 04:37
@yjst2012
Copy link
Copy Markdown
Collaborator Author

/azp run ci

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@yjst2012 yjst2012 force-pushed the yjst2012/ARO-23216 branch from 726fe57 to 34bcf1f Compare March 31, 2026 05:19
@yjst2012 yjst2012 force-pushed the yjst2012/ARO-23216 branch from 35320f4 to fdafeaa Compare April 1, 2026 00:23
@yjst2012 yjst2012 force-pushed the yjst2012/ARO-23216 branch from fdafeaa to 1757058 Compare April 1, 2026 00:36
@yjst2012 yjst2012 force-pushed the yjst2012/ARO-23216 branch from 123d36c to 4fd63ab Compare April 1, 2026 03:08
@yjst2012 yjst2012 force-pushed the yjst2012/ARO-23216 branch from 4fd63ab to 0e87efd Compare April 1, 2026 03:18

func (r *Reconciler) stopVAPTicker() {
if r.vapTickerDone != nil {
r.vapTickerDone <- true
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

stopVAPTicker does a blocking send on vapTickerDone before closing the channel. While the ticker goroutine is handling ticker.C, it can be inside deployVAP and not selecting on vapTickerDone, so a reconcile that calls stopVAPTicker can block indefinitely. Could we switch this to a non-blocking stop (or cancellation-driven shutdown) so ticker shutdown cannot deadlock? Context:

func (r *Reconciler) stopVAPTicker() {
if r.vapTickerDone != nil {
r.vapTickerDone <- true
close(r.vapTickerDone)
}

for {
select {
case done := <-r.vapTickerDone:
if done {
Copy link
Copy Markdown
Collaborator

@tuxerrante tuxerrante Apr 3, 2026

Choose a reason for hiding this comment

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

maybe this could be passed as an argument for a more clear flow, eg:

func (r *Reconciler) vapTicker(ctx context.Context, instance *arov1alpha1.Cluster, done <-chan struct{}) {
...
	case <-done:
			return

...
}


func (r *Reconciler) stopVAPTicker() {
	r.vapTickerMu.Lock()
	done := r.vapTickerDone
	r.vapTickerDone = nil
	r.vapTickerMu.Unlock()
	if done != nil {
		close(done) // non-blocking broadcast stop
	}
}

see comment below

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants