openshift-pipelines / pipelines-as-code

Pipelines-as-Code for Tekton
https://pipelinesascode.com
Apache License 2.0
137 stars 82 forks source link

PAC requests /ok-to-test even if PLR shouldn't run #1774

Open MartinBasti opened 1 month ago

MartinBasti commented 1 month ago

For PLR with configured CEL expression that shouldn't run for given PR, PAC is requesting approval from maintainers.

Steps to reproduce:

  1. Have a user without permissions to run PLRs (PR must be approved by /ok-to-test)
  2. Have a PLR definition with CEL expressions, that should skip opened PR
  3. User opens PR with changes that doesn't match CEL expressions
  4. PAC reports that PR has to /ok-to-test labeled
  5. Owner adds /ok-to-test
  6. Nothing happens; PAC still shows the same - queued, we cannot get rid of that test status from github

With a user that has permissions, PAC don't report anything.

It seems that RBAC is evaluated before CEL, and asks for permissions even if they aren't needed.

MartinBasti commented 1 month ago

From https://github.com/openshift-pipelines/pipelines-as-code/blob/a10162c315c7bca601d8ef7775df6bc7ead74f3c/pkg/pipelineascode/match.go#L23 first runs verifyRepoAndUser that calls checkAccessOrErrror https://github.com/openshift-pipelines/pipelines-as-code/blob/a10162c315c7bca601d8ef7775df6bc7ead74f3c/pkg/pipelineascode/match.go#L138, responsible for checking access. Later getPipelineRunsFromRepo runs that calls MatchPipelineRunByAnnotation, responsible for CEL expressions https://github.com/openshift-pipelines/pipelines-as-code/blob/a10162c315c7bca601d8ef7775df6bc7ead74f3c/pkg/matcher/annotation_matcher.go#L205

tnevrlka commented 1 month ago

From a quick look, this might be relevant. Here, the MatchPipelinerunByAnnotation responsible for CEL expressions is ran before checkAccessOrErrror https://github.com/openshift-pipelines/pipelines-as-code/blob/a10162c315c7bca601d8ef7775df6bc7ead74f3c/pkg/pipelineascode/match.go#L229-L239

Could this be the reason why adding /ok-to-test doesn't do anything?