search

jenkinsci / google-compute-engine-plugin

https://plugins.jenkins.io/google-compute-engine/
Apache License 2.0
57 stars 88 forks source link

Use `ADMINISTER` not `RUN_SCRIPTS` #364

Closed jglick closed 1 year ago

jglick commented 2 years ago

Correcting a deprecation. I think this was responsible for

SEVERE  hudson.triggers.SafeTimerTask#run: Timer task hudson.slaves.NodeProvisioner$NodeProvisionerInvoker@… failed
java.lang.NullPointerException
    at hudson.model.Label$2.resolve(Label.java:191)
    at hudson.model.Label$2.resolve(Label.java:188)
    at hudson.model.labels.LabelAtom.matches(LabelAtom.java:163)
    at hudson.model.Label.matches(Label.java:188)
    at com.google.jenkins.plugins.computeengine.ComputeEngineCloud.matchesLabel(ComputeEngineCloud.java:408)
    at com.google.jenkins.plugins.computeengine.ComputeEngineCloud.lambda$getInstanceConfigurations$1(ComputeEngineCloud.java:393)
    at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:176)
    at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1655)
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
    at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
    at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
    at com.google.jenkins.plugins.computeengine.ComputeEngineCloud.getInstanceConfigurations(ComputeEngineCloud.java:394)
    at com.google.jenkins.plugins.computeengine.ComputeEngineCloud.canProvision(ComputeEngineCloud.java:375)
    at hudson.model.Label.getClouds(Label.java:259)
    at hudson.model.Label.isEmpty(Label.java:434)
    at jenkins.model.Jenkins.getLabels(Jenkins.java:2109)
    at hudson.slaves.NodeProvisioner$NodeProvisionerInvoker.doRun(NodeProvisioner.java:822)

since using /script to debug why InstanceConfiguration.labelSet was null, I tried running readResolve directly and it threw an exception noting that my admin user lacked RUN_SCRIPTS.

I am not really sure why you are checking permissions in readResolve to begin with, but if you are going to do so, check one which still exists.

duemir commented 1 year ago

Does this obsolete my PR #354 ?

jglick commented 1 year ago

Well, it looks like I hit the same bug you did. My change is the more conservative. Why this permission check was there to begin with, I have no idea. 6f37e39e45b188c2d21c8b1c91551b5e78db3913 by @evandbrown was not linked to a PR or Jira ticket and does not have any real explanation. I agree that there is no obvious reason why parsing a list of label names should merit a permission check. (A permission check inside readResolve is rare but not unheard of: if you wished to ensure that a POST of config.xml did not allow access to some feature limited to admins.)