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

[JENKINS-46547] Allow loading pipeline {} blocks from shared libraries #193

Merged
merged 17 commits into from Sep 15, 2017

Conversation

Projects
None yet
6 participants
@abayer
Copy link
Member

commented Sep 12, 2017

  • JENKINS issue(s):
  • Description:
    • This adds support for loading pipeline { ... } blocks from shared libraries. A call() method in a shared library "step" will now be able to include one or more pipeline { ... } blocks, though only one should be executed.
    • Only can be used in vars/*.groovy - not in src/... or via load(...).
    • Validation works - in fact, you can do if (something) { pipeline { ... } } else { pipeline { ... } } and both blocks will be validated.
    • Only the actually executing pipeline { ... } block will be available via the ExecutionModelAction.getStages() method, which is used by Blue Ocean to render the execution plan.
  • Tests to verify we won't stomp on a shared library "step" named pipeline. (EDIT: Reconsidering this - that won't work anyway currently, since we're already inserting pipeline as a "step"/global variable. The only scenario I can think of where this would be relevant would be a Jenkinsfile or shared library that has a closure it trampolines a la the old parser for Declarative, evaluating everything before ever getting to CPS evaluation. And frankly, I'm not sure it's worth worrying about that weird use case, especially since we're now only transforming pipeline blocks if they contain agent and stages anyway.)
  • Decide whether to fold JENKINS-42224 into this change or tackle that separately later. (EDIT: Deferring - we'll do that later)
  • Try to find more ways to guarantee we're only parsing/validating/transforming Jenkinsfile or vars/*.groovy and nothing else. (EDIT: Now only parsing etc for WorkflowScript and classes without packages whose name matches a GlobalVariable, since vars/*.groovy get added as GlobalVariables before they get compiled - nifty!)
  • Support multiple pipeline blocks in a shared library "step" - i.e., if statements etc.
  • Determine how ExecutionModelAction will know which stages instance is the one that's actually running if there are more than one. (EDIT: done - using ModelASTStages.hashCode() stored in the actual Root instance we execute to figure out what's run at runtime, and then having ExecutionModelAction.getStages() use that hashcode to figure out which specific instance to return)
  • Ensure we only execute one pipeline { ... } block - probably by failing the build if we attempt to execute a second?
  • Documentation changes:
    • Docs will be needed.
  • Users/aliases to notify:
[JENKINS-46547] Allow loading pipeline {} blocks from shared libraries
This is a work-in-progress - more is needed to be sure that we don't
parse/validate/transform things we shouldn't, etc

@abayer abayer requested a review from rsandell Sep 12, 2017

abayer added some commits Sep 12, 2017

Allow WorkflowScript with package to be parsed etc
Also removed pointless "package postStage" from postStage tests,
because that just confuses things.
Preliminary support for multiple pipeline blocks
Still needs better logic in ExecutionModelAction.

@abayer abayer added this to the 1.3 milestone Sep 12, 2017

@reviewbybees

This comment has been minimized.

Copy link

commented Sep 13, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@abayer abayer requested a review from jglick Sep 14, 2017

@abayer

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

@jglick - would appreciate your thoughts on this as well, if you have a chance.

@abayer abayer requested a review from svanoort Sep 14, 2017

@abayer abayer modified the milestones: 1.3, 1.2 Sep 14, 2017

@abayer

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

Moving this to 1.2 - while pipeline { ... } has never been officially supported in shared libraries, it seems kind of ridiculous to do a release that breaks that usage and then later another release that fixes it and makes it supported, when the fix is already up for review.

@rsandell

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

But why should this be fixed?
I just fear we start supporting something that we shouldn't. Just one more thing that can break with imho close to no value?

throw new IllegalStateException("Only one pipeline { ... } block can be executed in a single run.")
}
action.setStagesHashCode(astHashCode)
r.save()

This comment has been minimized.

Copy link
@rsandell

rsandell Sep 14, 2017

Member

Do you have to save here? Pipelines are hitting the master's storage too much as it is.

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

Yeah, we do - the action already exists, so just modifying it won't result in it being saved. Also, this literally happens only once per build (since if this method gets called a second time, we throw that exception above!)

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

Also, this is writing the run's config.xml, not the XML storage for an individual FlowNode, so it's no worse than, say, setting the build result, etc.

This comment has been minimized.

Copy link
@rsandell

rsandell Sep 14, 2017

Member

Well my other concern was around badly coded SaveableListeners out there that might get confused with more than one save per build.
I would guess the build is saved at the end as for freestyles but there might be more to it here if it should survive restarts.

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

Oh, no, run.save() gets called a bunch already - like I said, any time the build result gets set, a new action is added, you name it.

@@ -57,9 +57,11 @@ public class Root implements Serializable {

Libraries libraries

int astHashCode

This comment has been minimized.

Copy link
@rsandell

rsandell Sep 14, 2017

Member

final?

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

Will do.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

@rsandell So that was my thought for a long time too, but a couple things changed my mind.

  • I know people are using pipeline { ... } in shared libraries currently due to bug reports about environment and when expressions not working from there (due to the implementation of the old parsing logic).
  • In 1.2, all usage of pipeline { ... } in shared libraries will fail, due to the new parser, so this goes from just being an unsupported/undocumented behavior to what will appear to be a regression to people already using it.
  • On the positive side, the new parser made implementing the guts of supporting pipeline { ... } in shared libraries extremely easy - no need to worry about the Closure trampoline/translation stuff applying there, etc, just do the same transformation magic we do in Jenkinsfile. The harder parts were making sure we don't try to parse/transform code we shouldn't, limiting where the parsing/transformation can be done to where it'd actually work at runtime (i.e., to vars, not src), and not breaking the contract with Blue Ocean for the stages execution plan that'll actually be executed being available via ExecutionModelAction.getStages(). That's all done, so...

I don't think this adds much maintenance or exposure for bugs that wasn't there before - the parsing is exactly the same as in Jenkinsfiles, once you get past a tiny bit of additional logic for handling the potential of multiple pipeline { ... } blocks to be parsed (but not executed - i.e., if (whatever) { pipeline { ... } } else { pipeline { ... } }), and the logic for determining whether to transform a Groovy file is something I'm really confident in.

// Even if there are multiple pipeline blocks, just return the first one - this return value is only
// used in a few places: tests, where there will only ever be one, and linting/converting, which also
// are guaranteed to only have one, since we don't intend to support linting of pipelines defined in
// shared libraries.

This comment has been minimized.

Copy link
@rsandell

rsandell Sep 14, 2017

Member

So why waste time parsing them all?

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

Because I can't tell which one is going to be executed. Basically, the return value of parse is irrelevant except for those very few places, none of which are applicable to shared libraries in the first place, so we can keep just returning the first one we parse. But we do need to parse all of them in case of the if (whatever) { pipeline { ... } } else { pipeline { ... } } case.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

Hmm, couldn't reproduce the test failure locally. Will investigate why it's being flaky.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

Check that - reproduced when run via Maven, but not when run from the IDE. Weeeeeird, investigating.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

Nope, even weirder - when I run just the one test via Maven, it passes. But when I run all of BasicModelDefTest, the test fails. There's some lurking state somewhere...

@abayer

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

Oh geez I'm dumb. Hashcodes can be negative. facepalm

@jglick
Copy link
Member

left a comment

Some comments from a super quick view.

@@ -606,7 +610,8 @@ class RuntimeASTTransformer {
transformOptions(original.options),
transformTriggers(original.triggers),
transformParameters(original.parameters),
transformLibraries(original.libraries)))
transformLibraries(original.libraries),
constX(original.stages.hashCode())))

This comment has been minimized.

Copy link
@jglick

jglick Sep 14, 2017

Member

Humor me and use SHA-256. String.hashCode is weak enough that even accidental collisions are not out of the question. CRC-32 and MD-5 are better but spoofable. Probably this is not a security consideration but are you willing to stake your reputation on that?

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

So the only reason I'm actually using hashCode is that it's trivial to get off an object. If there's a better method I should be calling, point me to it and I will happily use it.

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

And also, yes, I actually am willing to stake my reputation that this is not a security consideration - the hash code is just used to try to determine which of the possibly multiple ModelASTStages instances in ExecutionModelAction is the one we actually invoked, so that we can make ExecutionModelAction.getStages() return that instance. And that method is only used by Blue Ocean to pre-render the stages that will run before they've actually run. So worst case is that the wrong group of stages renders initially.

That said, I'd be fine with literally anything as a unique identifier for a ModelASTStages instance - if there's a good way to randomly generate a string or whatever at instantiation time that I can add as a field to ModelASTStages, that'd be perfect. I just went with hashCode() because I knew it'd work...and I knew you'd eventually tell me the right way I should do it. =)

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

Switched around to store a UUID on ModelASTStages that we generate at initialization, and a String representation of it on Root (to make the sandboxed transformation stuff easier than trying to instantiate a UUID from the existing ModelASTStages.getUuid() String).

try {
FlowExecutionOwner owner = execution.getOwner();
if (owner != null && owner.getExecutable() instanceof WorkflowRun) {
WorkflowRun run = (WorkflowRun)owner.getExecutable();

This comment has been minimized.

Copy link
@jglick

jglick Sep 14, 2017

Member

Run suffices here

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

Gotcha. At some point I was doing something that actually cared about WorkflowRun.

FlowExecutionOwner owner = execution.getOwner();
if (owner != null && owner.getExecutable() instanceof WorkflowRun) {
WorkflowRun run = (WorkflowRun)owner.getExecutable();
for (GlobalVariable v : GlobalVariable.forRun(run)) {

This comment has been minimized.

Copy link
@jglick

jglick Sep 14, 2017

Member

Hmm, should probably make an overload taking FlowExecution[Owner].

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

Would be handy!

// First, we'll parse if this is WorkflowScript, i.e., a Jenkinsfile, or it's an autogenerated script
// with no package, i.e., Script1
if (classNode.getNameWithoutPackage().equals(Converter.PIPELINE_SCRIPT_NAME) ||
(classNode.getPackageName() == null && classNode.getNameWithoutPackage().matches("Script\\d+"))) {

This comment has been minimized.

Copy link
@jglick

jglick Sep 14, 2017

Member

Going to blow up when someone fixes JENKINS-31838. Find a better way. CodeSource?

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

This would bust the line above too, right? Which we've had in place for ages...hrm. Lemme think on it for a bit.

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

May have got it - see latest change.

if (pst != null) {
return parsePipelineStep(pst, secondaryRun)
} else {
// Look for the pipeline step inside methods named call.

This comment has been minimized.

Copy link
@jglick

This comment has been minimized.

Copy link
@abayer

abayer Sep 14, 2017

Author Member

Actually a conscious decision on my part - we're only going to support pipeline {...} blocks in the call() method of vars/*.groovy. Limits the scope of transformation and the ways things could go weird.

@svanoort
Copy link
Member

left a comment

Somewhat uncomprehending 🐝 -- at least looks like it has solid tests.

@@ -263,6 +264,18 @@ public class Utils {
return nodes
}

static void markExecutedStagesOnAction(CpsScript script, String astUUID) throws Exception {

This comment has been minimized.

Copy link
@svanoort

svanoort Sep 14, 2017

Member

Seems kind of hacky.

This comment has been minimized.

Copy link
@abayer

abayer Sep 15, 2017

Author Member

It is a bit hacky, yes, but given that I don't actually know which pipeline block (if there are multiple being read from the Jenkinsfile and shared libraries) is going to be executed until it actually is executed...and hey, it works. If a better idea comes along in the future, I'll take advantage of it. =)

@abayer abayer merged commit 6dba039 into jenkinsci:master Sep 15, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@abayer

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2017

Merged, cutting a beta.

@casz

This comment has been minimized.

Copy link
Member

commented Sep 28, 2017

@abayer THANK YOU 💓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.