Skip to content

CI variable expression conjunction/disjunction

What does this MR do?

The primary purpose of this merge request is to add support for && and || operators in the CI config. It is a subset of, and should be considered a replacement of MR 27554.

A necessary but breaking change is also made to the matching of Lexeme::Pattern. The scanner is intentionally designed to be greedy (spec/lib/gitlab/ci/pipeline/expression/lexeme/pattern_spec.rb:33) and match the outermost / characters in a statement, but this breaks down if we allow for multiple regular expressions in a statement, e.g.:

$VAR1 =~ /this/ && $VAR2 =~ /that/

I've written new Lexer specs to reproduce the issue and verify that statements are being correctly tokenized. The original failures looked like this:

       expected: ["$PRESENT_VARIABLE", "=~", "/my var/", "&&", "$EMPTY_VARIABLE", "=~", "/nope/"]
            got: ["$PRESENT_VARIABLE", "=~", "/my var/ && $EMPTY_VARIABLE =~ /nope/"]

It's not a problem with the conjuction or disjunction operators themselves, but the possibility of having two distinct regex pattern tokens in a single statement that raises this issue.

I'm hesitating to change this abruptly since there is explicit specification for it to work the way it does, and would like to get some feedback and/or context about how our Pattern matching works before we decide what to do. I'm not sure there's a way to implement this without changing the matching for Pattern, in which case we should discuss how to detect instances of this breaking logic and warn users.

The pipeline is currently failing around a single spec regarding the escaping of slashes inside regex patterns. I'm punting on that until we decide what to do about this generally.

@grzesiek: You seem to have written the spec about greedy pattern matching, could you shed some light on the design and what it would mean to change the matching?

@brendan: This will be a user-facing breaking change, so I'm not sure how we'd want to introduce it.

I'm hoping that there's something I haven't thought of, but so far I think this is unavoidable.

Closes #62867 (closed)

TODO

Does this MR meet the acceptance criteria?

Conformity

Performance and testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Brendan O'Leary 🐢

Merge request reports