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

Add repository validation #7680

Closed

Conversation

@imotov
Copy link
Member

imotov commented Sep 11, 2014

Adds automatic and manual verification of repository settings and access rights.

Fixes #7096

@clintongormley clintongormley changed the title [SNAPSHOT] Add repository validation Snapshot/Restore: Add repository validation Sep 11, 2014
@s1monw
s1monw reviewed Sep 11, 2014
View changes
src/main/java/org/elasticsearch/action/admin/cluster/repositories/put/PutRepositoryRequest.java Outdated
if (in.getVersion().onOrAfter(Version.V_1_4_0)) {
verify = in.readBoolean();
} else {
verify = false;

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 11, 2014

Contributor

hmm this is different from the default? maybe you can put a comment here why if its' not a bug

@Override
public ActionRequestValidationException validate() {
ActionRequestValidationException validationException = null;
if (name == null) {

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 11, 2014

Contributor

should we also fail if the name is empty? maybe use String.isEmpty(name)

This comment has been minimized.

Copy link
@imotov

imotov Sep 11, 2014

Author Member

empty string (as any other incorrectly formatted repository names) will fail on the next step with "RepositoryMissingException[[] missing]" exception. So, I am not sure we need to add additional test for empty strings here.

}

@Override
public String toString() {

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 11, 2014

Contributor

at some point we should have a XContentActionResponse base class that has this tostring method ;) something for fixit friday?

This comment has been minimized.

Copy link
@imotov

imotov Sep 11, 2014

Author Member

This pattern is common among other classes inherited from TransportResponse not just from ActionResponse. So, maybe we can start with moving this into an utility method.

@s1monw
s1monw reviewed Sep 11, 2014
View changes
src/main/java/org/elasticsearch/index/snapshots/blobstore/BlobStoreIndexShardRepository.java Outdated
BlobContainer testBlobContainer = blobStore.blobContainer(basePath);;
DiscoveryNode localNode = clusterService.localNode();
if (testBlobContainer.blobExists(testBlobPrefix(seed) + "-master")) {
byte[] testBytes = Strings.toUTF8Bytes(seed);

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 11, 2014

Contributor

can we inline the testBytes into the write Call? just simpler?

@@ -191,6 +201,36 @@ public boolean mustAck(DiscoveryNode discoveryNode) {
});
}

public void verifyRepository(final String repositoryName, final ActionListener<VerifyResponse> listener) {
final Repository repository = repository(repositoryName);
try {

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 11, 2014

Contributor

we really need to make these listeners simpler in the case of exceptions :)

@s1monw
s1monw reviewed Sep 11, 2014
View changes
src/main/java/org/elasticsearch/repositories/RepositoriesService.java Outdated
public String failureDescription() {
StringBuilder builder = new StringBuilder();
builder.append("[");
boolean first = true;

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 11, 2014

Contributor

maybe just use Joiner.java ie.

StringBuilder builder = new StringBuilder('[');
Joiner.on(",").appendTo(builder, failure);
return builder.append(']').toString();
@s1monw
s1monw reviewed Sep 11, 2014
View changes
src/main/java/org/elasticsearch/repositories/Repository.java Outdated
* @return verification token that should be passed to all Index Shard Repositories for additional verification
*/
String startVerification();

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 11, 2014

Contributor

can we have a doc string for endVerification too?

}

public VerificationFailure(String nodeId, String cause) {
this.nodeId = nodeId;

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 11, 2014

Contributor

can we verify this is never null? otherwise maybe we should write optinalString()?

@s1monw
s1monw reviewed Sep 11, 2014
View changes
src/main/java/org/elasticsearch/repositories/VerifyNodeRepositoryAction.java Outdated
public class VerifyNodeRepositoryAction extends AbstractComponent {
public static final String ACTION_NAME = "internal:admin/repository/verify";

public static interface VerifyActionListener {

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 11, 2014

Contributor

can we please use ActionListener here instead and use a struct that holds the arrays? I really don't think we need these listeners

@s1monw
s1monw reviewed Sep 11, 2014
View changes
src/main/java/org/elasticsearch/repositories/uri/URLRepository.java Outdated

@Override
public String startVerification() {
//TODO: Add check that URL exists and accessible

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 11, 2014

Contributor

hmm can we open a followup for this and add the id to the comment

@s1monw
s1monw reviewed Sep 11, 2014
View changes
src/main/java/org/elasticsearch/repositories/uri/URLRepositoryModule.java Outdated
@@ -39,7 +39,7 @@ public URLRepositoryModule() {
@Override
protected void configure() {
bind(Repository.class).to(URLRepository.class).asEagerSingleton();
bind(IndexShardRepository.class).to(BlobStoreIndexShardRepository.class).asEagerSingleton();
bind(IndexShardRepository.class).to(URLIndexShardRepository.class).asEagerSingleton();

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 11, 2014

Contributor

oh! Did I mess this up?

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 11, 2014

Contributor

nevermind it's new

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 11, 2014

I did a review, one more thing, can we call this API in the BWC tests too just to make sure we didn't break anything?

@s1monw s1monw removed the review label Sep 11, 2014
@@ -20,6 +20,10 @@
"timeout": {
"type" : "time",
"description" : "Explicit operation timeout"
},
"verify": {

This comment has been minimized.

Copy link
@dadoonet

dadoonet Sep 11, 2014

Member

Wondering if we should also document this new option in create repository doc?

@s1monw s1monw added v1.5.0 and removed v1.4.0.Beta1 labels Sep 11, 2014
@imotov imotov force-pushed the imotov:issue-7096-repository-validation branch 2 times, most recently Sep 23, 2014
@imotov imotov added the review label Sep 23, 2014
@@ -92,6 +92,21 @@ Other repository backends are available in these official plugins:
* https://github.com/elasticsearch/elasticsearch-hadoop/tree/master/repository-hdfs[HDFS Plugin] for Hadoop environments
* https://github.com/elasticsearch/elasticsearch-cloud-azure#azure-repository[Azure Cloud Plugin] for Azure storage repositories


===== Repository Verification
added[1.4.0]

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 2, 2014

Contributor

seems like this goes to 1.5 only ?

@s1monw
s1monw reviewed Oct 2, 2014
View changes
src/test/java/org/elasticsearch/transport/ActionNamesBackwardsCompatibilityTest.java Outdated
@@ -135,5 +137,8 @@ private static boolean isActionNotFoundExpected(Version version, String action)
actionsVersions.put(DeleteIndexedScriptAction.NAME, Version.V_1_3_0);
actionsVersions.put(PutIndexedScriptAction.NAME, Version.V_1_3_0);

actionsVersions.put(VerifyRepositoryAction.NAME, Version.V_1_5_0);

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 2, 2014

Contributor

I think I am ok with this going into 1.4.0 so you need to move this to 1.4.0 too no?

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Oct 2, 2014

left only minor comments, LGTM and can go into 1.4?

@s1monw s1monw added v1.4.0 and removed v1.5.0 labels Oct 6, 2014
@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Oct 7, 2014

@imotov can you bring this up to date and push ?

@s1monw s1monw removed the review label Oct 7, 2014
Fixes #7096
@imotov imotov force-pushed the imotov:issue-7096-repository-validation branch to 286b603 Oct 7, 2014
@imotov

This comment has been minimized.

Copy link
Member Author

imotov commented Oct 8, 2014

Merged into 1.4, 1.x and master.

@imotov imotov closed this Oct 8, 2014
@clintongormley clintongormley changed the title Snapshot/Restore: Add repository validation Add repository validation Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.