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

Save after calling setSecurityRealm or setAuthorizationStrategy #2790

Merged
merged 12 commits into from Mar 17, 2017

Conversation

4 participants
@jglick
Member

jglick commented Mar 9, 2017

Very confusing to have a RestartableJenkinsRule test which sets up security, only to see no trace of that in the next session, because you forgot to rr.j.jenkins.save().

GlobalSecurityConfiguration.doConfigure of course already saves, in a BulkChange, so it should be unaffected; this is for isolated API calls.

@reviewbybees

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Mar 9, 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.

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.

@oleg-nenashev

Generally it is preferable, but the original behavior is a common approach in Jenkins.

I think we need to ensure that all BulkOperations are in place at least, just to prevent double save operations

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Mar 14, 2017

Member

I think we need to ensure that all BulkOperations are in place

BulkChange you mean? As noted in the PR description, GlobalSecurityConfiguration.doConfigure does this already.

Member

jglick commented Mar 14, 2017

I think we need to ensure that all BulkOperations are in place

BulkChange you mean? As noted in the PR description, GlobalSecurityConfiguration.doConfigure does this already.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Mar 14, 2017

Member

Test failures indicate tests which try to set a non-Serializable object, typically an inner class. Those need to be fixed.

Member

jglick commented Mar 14, 2017

Test failures indicate tests which try to set a non-Serializable object, typically an inner class. Those need to be fixed.

@oleg-nenashev

I would prefer explicit failure instead of saveQuietly(), but generally LGTM 🐝

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Mar 16, 2017

Member

I would prefer explicit failure instead of saveQuietly()

Too late—the setters do not throw IOException.

Member

jglick commented Mar 16, 2017

I would prefer explicit failure instead of saveQuietly()

Too late—the setters do not throw IOException.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Mar 16, 2017

Member

Hmm, CI builders failed with

java.lang.AssertionError: 

Expected: <true>
     but: was <false>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
	at hudson.cli.ConnectNodeCommandTest.connectNodeShouldSucceedWithForce(ConnectNodeCommandTest.java:114)

though I cannot reproduce that locally or find any explanation related to this PR.

(There was one case where CLICommandInvoker.authorizedTo caused problems, but only because it was overwriting $JENKINS_HOME/config.xml, which AFAICT should not matter here.)

Member

jglick commented Mar 16, 2017

Hmm, CI builders failed with

java.lang.AssertionError: 

Expected: <true>
     but: was <false>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
	at hudson.cli.ConnectNodeCommandTest.connectNodeShouldSucceedWithForce(ConnectNodeCommandTest.java:114)

though I cannot reproduce that locally or find any explanation related to this PR.

(There was one case where CLICommandInvoker.authorizedTo caused problems, but only because it was overwriting $JENKINS_HOME/config.xml, which AFAICT should not matter here.)

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Mar 16, 2017

Member

Wait, it fails if you run the whole test suite, but not if you run just this test case. Looking…

Member

jglick commented Mar 16, 2017

Wait, it fails if you run the whole test suite, but not if you run just this test case. Looking…

Depending on what seems to have been timing conditions, connectNodeSh…
…ouldSucceedWithForce could fail with a cryptic message.

Found an error in the launch log:
java.lang.IllegalStateException: Already connected
	at hudson.slaves.SlaveComputer.setChannel(SlaveComputer.java:567)
	at hudson.slaves.SlaveComputer.setChannel(SlaveComputer.java:390)
	at hudson.slaves.CommandLauncher.launch(CommandLauncher.java:132)
	at hudson.slaves.SlaveComputer$1.call(SlaveComputer.java:262)
I think the slave was being launched in the background, and then the test tried to disconnect and reconnect it.
Perhaps if these actions overlapped in time, the loose locking semantics of SlaveComputer.setChannel could cause the error.
@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
Member

oleg-nenashev commented Mar 17, 2017

🐝

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Mar 17, 2017

Member

Presumably

java.lang.AssertionError: deletion stopped
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.assertTrue(Assert.java:41)
	at org.junit.Assert.assertFalse(Assert.java:64)
	at hudson.UtilTest.testDeleteRecursive_onWindows(UtilTest.java:453)

is not related.

Member

jglick commented Mar 17, 2017

Presumably

java.lang.AssertionError: deletion stopped
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.assertTrue(Assert.java:41)
	at org.junit.Assert.assertFalse(Assert.java:64)
	at hudson.UtilTest.testDeleteRecursive_onWindows(UtilTest.java:453)

is not related.

@jglick jglick merged commit 4fd4bd3 into jenkinsci:master Mar 17, 2017

1 of 2 checks passed

continuous-integration/jenkins/pr-head This commit has test failures
Details
Jenkins This pull request looks good
Details

@jglick jglick deleted the jglick:security-settings-save branch Mar 17, 2017

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