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

Make credentials mandatory when launching x-pack/migrate #33972

Closed
wants to merge 1 commit into from

Conversation

atript8
Copy link

@atript8 atript8 commented Sep 23, 2018

Made credentials mandatory for x-pack. Closes #29847.
The x-pack user and roles APIs aren't available unless security is enabled, so the tool should always be called with the -u and -p options specified.

@tvernum tvernum self-requested a review September 24, 2018 00:34
@tvernum tvernum added review :Security/Security Security issues without another label labels Sep 24, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@tvernum
Copy link
Contributor

tvernum commented Sep 24, 2018

Thank @anuptripathi4u

The change looks good. We do like these sorts of fixes to have an accompanying test case.

An appropriate test would to be add something like this to ESNativeRealmMigrateToolTests:

public void testMissingPasswordParameter() throws Exception {
    ESNativeRealmMigrateTool.MigrateUserOrRoles muor = new ESNativeRealmMigrateTool.MigrateUserOrRoles();

    final OptionException ex = expectThrows(OptionException.class,
        () -> muor.getParser().parse("-u", "elastic", "-U", "http://localhost:9200"));

    assertThat(ex.getMessage(), containsString("password"));
}

Would you be able to add that?

@tvernum
Copy link
Contributor

tvernum commented Sep 24, 2018

@elasticmachine test this please

@atript8
Copy link
Author

atript8 commented Sep 24, 2018

Hi @tvernum , I stole your code but can't get the tests to work.
gradlew test -Dtests.class=org.elasticsearch.xpack.security.authc.esnative.ESNativeMigrateToolTests
test.log

Any suggestions? Do we really need hadoop setup to run the tests?

@tvernum
Copy link
Contributor

tvernum commented Sep 26, 2018

@anuptripathi4u
The error isn't actually Hadoop related - that message is indicating that the hadoop tests were skipped.

The issue is that if you specify a particular test case to run, then you can only run the project that includes that test class.
The :test:framework:test task is failing because there are no tests to run, and we treat that as an error.

You probably want to run:

gradlew :x-pack:plugin:security:test -Dtests.class=org.elasticsearch.xpack.security.authc.esnative.ESNativeMigrateToolTests

atript8 added a commit to atript8/elasticsearch that referenced this pull request Sep 26, 2018
@atript8
Copy link
Author

atript8 commented Sep 26, 2018

Thanks @tvernum . Tests work. Added commit to the pull request.
I suppose I need to squash the commits into one and rebase. I am looking into it.

Made credentials mandatory for xpack. Closes elastic#29847.
The x-pack user and roles APIs aren't available unless security is enabled, so the tool should always be called with the -u and -p options specified.
@atript8 atript8 force-pushed the master branch 2 times, most recently from 1eb1d7e to 7b54356 Compare September 28, 2018 07:20
@atript8
Copy link
Author

atript8 commented Sep 28, 2018

Hi @tvernum . I've squashed the commits.

@tvernum
Copy link
Contributor

tvernum commented Oct 4, 2018

Hi @anuptripathi4u

Thanks.

In general we prefer that you just do regular merges, and don't squash commits, as it breaks the github review history.
We always do squashed merges, so we don't need it to be squashed ahead of time.

@tvernum
Copy link
Contributor

tvernum commented Oct 4, 2018

@elasticmachine test this please.

@rjernst rjernst removed the review label Oct 10, 2018
@colings86 colings86 added v6.6.0 and removed v6.5.0 labels Oct 25, 2018
@tvernum
Copy link
Contributor

tvernum commented Dec 4, 2018

@anuptripathi4u I'm going to close this PR and open a new branch from your commit, so I can get CI working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants