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

feat(entitytags): Support indexing / searching by application #2132

Merged
merged 3 commits into from
Nov 16, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class EntityTags {
private Map<String, Object> attributes = new HashMap<String, Object>()

String cloudProvider
String application
String accountId
String account
String region
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public interface EntityTagsProvider {
* Fetch EntityTags by any combination of {@code cloudProvider}/{@code type}/{@code idPrefix}/{@code tags}
*/
Collection<EntityTags> getAll(String cloudProvider,
String application,
String entityType,
List<String> entityIds,
String idPrefix,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,16 @@ public Collection<EntityTags> taggedEntities(String cloudProvider,
String tagName,
int maxResults) {
return entityTagsProvider.getAll(
cloudProvider, ENTITY_TYPE, null, null, accountId, null, null, Collections.singletonMap(tagName, "*"), maxResults
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no change here ... just took it upon myself to split an overly long line of code

cloudProvider,
null,
ENTITY_TYPE,
null,
null,
accountId,
null,
null,
Collections.singletonMap(tagName, "*"),
maxResults
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.Lists;
import com.netflix.frigga.Names;
import com.netflix.spinnaker.clouddriver.core.services.Front50Service;
import com.netflix.spinnaker.clouddriver.helpers.OperationPoller;
import com.netflix.spinnaker.clouddriver.model.EntityTags;
Expand Down Expand Up @@ -79,6 +80,7 @@ public ElasticSearchEntityTagsProvider(ObjectMapper objectMapper,

@Override
public Collection<EntityTags> getAll(String cloudProvider,
String application,
String entityType,
List<String> entityIds,
String idPrefix,
Expand All @@ -94,6 +96,11 @@ public Collection<EntityTags> getAll(String cloudProvider,
queryBuilder = queryBuilder.must(QueryBuilders.termQuery("entityRef.cloudProvider", cloudProvider));
}

if (application != null) {
// restrict to a specific application (optional)
queryBuilder = queryBuilder.must(QueryBuilders.termQuery("entityRef.application", application));
}

if (entityIds != null && !entityIds.isEmpty()) {
// restrict to a specific set of entityIds (optional)
queryBuilder = queryBuilder.must(QueryBuilders.termsQuery("entityRef.entityId", entityIds));
Expand Down Expand Up @@ -461,6 +468,20 @@ private static EntityTags prepareForWrite(ObjectMapper objectMapper, EntityTags

copyOfEntityTags.getTags().forEach(entityTag -> entityTag.setValue(entityTag.getValueForWrite(objectMapper)));

String application = copyOfEntityTags.getEntityRef().getApplication();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crux of the change here ... this is called anytime we index something in elastic search

if (application == null || application.trim().isEmpty()) {
try {
Names names = Names.parseName(copyOfEntityTags.getEntityRef().getEntityId());
copyOfEntityTags.getEntityRef().setApplication(names.getApp());
} catch (Exception e) {
log.error(
"Unable to extract application name (entityId: {})",
copyOfEntityTags.getEntityRef().getEntityId(),
e
);
}
}

return copyOfEntityTags;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,12 @@ public BulkUpsertEntityTagsAtomicOperationResult operate(List priorOutputs) {
Map<String, EntityTags> existingTags = retrieveExistingTags(tags);

getTask().updateStatus(BASE_PHASE, "Merging existing tags and metadata");
tags.forEach(tag -> mergeExistingTagsAndMetadata(now, existingTags.get(tag.getId()), tag, bulkUpsertEntityTagsDescription.isPartial));
tags.forEach(tag -> mergeExistingTagsAndMetadata(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no changes here ... just took it upon myself to split an overly long line of code

now,
existingTags.get(tag.getId()),
tag,
bulkUpsertEntityTagsDescription.isPartial
));

getTask().updateStatus(BASE_PHASE, "Performing batch update to durable tagging service");
Map<String, EntityTags> durableTags = front50Service.batchUpdate(new ArrayList<>(tags))
Expand Down Expand Up @@ -111,7 +116,9 @@ BASE_PHASE, format("Failed to build tag id for %s, reason: %s", tag.getId(), e.g
entityTags.removeAll(failed);
}

private void updateMetadataFromDurableTagsAndIndex(List<EntityTags> entityTags, Map<String, EntityTags> durableTags, BulkUpsertEntityTagsAtomicOperationResult result) {
private void updateMetadataFromDurableTagsAndIndex(List<EntityTags> entityTags,
Map<String, EntityTags> durableTags,
BulkUpsertEntityTagsAtomicOperationResult result) {
Collection<EntityTags> failed = new ArrayList<>();
entityTags.forEach(tag -> {
try {
Expand Down Expand Up @@ -153,14 +160,22 @@ public static EntityRefIdBuilder.EntityRefId entityRefId(AccountCredentialsProvi

if (entityRefAccount != null && entityRefAccountId == null) {
// add `accountId` if not explicitly provided
AccountCredentials accountCredentials = lookupAccountCredentialsByAccountIdOrName(accountCredentialsProvider, entityRefAccount, "accountName");
AccountCredentials accountCredentials = lookupAccountCredentialsByAccountIdOrName(
accountCredentialsProvider,
entityRefAccount,
"accountName"
);
entityRefAccountId = accountCredentials.getAccountId();
entityRef.setAccountId(entityRefAccountId);
}

if (entityRefAccount == null && entityRefAccountId != null) {
// add `account` if not explicitly provided
AccountCredentials accountCredentials = lookupAccountCredentialsByAccountIdOrName(accountCredentialsProvider, entityRefAccountId, "accountId");
AccountCredentials accountCredentials = lookupAccountCredentialsByAccountIdOrName(
accountCredentialsProvider,
entityRefAccountId,
"accountId"
);
if (accountCredentials != null) {
entityRefAccount = accountCredentials.getName();
entityRef.setAccount(entityRefAccount);
Expand Down Expand Up @@ -252,7 +267,9 @@ private static AccountCredentials lookupAccountCredentialsByAccountIdOrName(Acco
return accountCredentialsProvider.getAll().stream()
.filter(c -> entityRefAccountIdOrName.equals(c.getAccountId()) || entityRefAccountIdOrName.equals(c.getName()))
.findFirst()
.orElseThrow(() -> new IllegalArgumentException(String.format("No credentials found for %s: %s", type, entityRefAccountIdOrName)));
.orElseThrow(() -> new IllegalArgumentException(
String.format("No credentials found for %s: %s", type, entityRefAccountIdOrName)
));
}

private static Task getTask() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,14 @@ public Void operate(List priorOutputs) {
BulkUpsertEntityTagsDescription bulkDescription = new BulkUpsertEntityTagsDescription();
bulkDescription.isPartial = entityTagsDescription.isPartial;
bulkDescription.entityTags = Collections.singletonList(entityTagsDescription);
new BulkUpsertEntityTagsAtomicOperation(front50Service, accountCredentialsProvider, entityTagsProvider,
bulkDescription).operate(priorOutputs);

new BulkUpsertEntityTagsAtomicOperation(
front50Service,
accountCredentialsProvider,
entityTagsProvider,
bulkDescription
).operate(priorOutputs);

return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,25 @@ class ElasticSearchEntityTagsProviderSpec extends Specification {
!entityTagsProvider.get(entityTags.id, ["tag3": "value3"]).isPresent()
}

def "should assign entityRef.application if not specified"() {
given:
def entityTags = buildEntityTags("aws:cluster:front50-main:myaccount:*", ["tag1": "value1", "tag2": "value2"])
entityTags.entityRef.application = application

entityTagsProvider.index(entityTags)
entityTagsProvider.verifyIndex(entityTags)

expect:
entityTagsProvider.get(entityTags.id).get().entityRef.application == expectedApplication

where:
application || expectedApplication
null || "front50"
"" || "front50"
" " || "front50"
"my_application" || "my_application"
}

def "should support multi result retrieval by `cloudProvider, `entityType`, `idPrefix` and `tags`"() {
given:
def entityTags = buildEntityTags("aws:cluster:clouddriver-main:myaccount:*", ["tag3": "value3"])
Expand All @@ -132,37 +151,40 @@ class ElasticSearchEntityTagsProviderSpec extends Specification {

expect:
// fetch everything
entityTagsProvider.getAll(null, null, null, null, null, null, null, null, 2)*.id.sort() == [entityTags.id, moreEntityTags.id].sort()
entityTagsProvider.getAll(null, null, null, null, null, null, null, null, null, 2)*.id.sort() == [entityTags.id, moreEntityTags.id].sort()

// fetch everything for an application
entityTagsProvider.getAll(null, "front50", null, null, null, null, null, null, null, 2)*.id.sort() == [moreEntityTags.id].sort()

// fetch everything for a single `entityId`
entityTagsProvider.getAll(null, null, [entityTags.entityRef.entityId], null, null, null, null, null, 2)*.id.sort() == [entityTags.id].sort()
entityTagsProvider.getAll(null, null, null, [entityTags.entityRef.entityId], null, null, null, null, null, 2)*.id.sort() == [entityTags.id].sort()

// fetch everything for a multiple `entityId`
entityTagsProvider.getAll(null, null, [
entityTagsProvider.getAll(null, null, null, [
entityTags.entityRef.entityId, moreEntityTags.entityRef.entityId
], null, null, null, null, null, 2)*.id.sort() == [entityTags.id, moreEntityTags.id].sort()

// fetch everything for `cloudprovider`
entityTagsProvider.getAll("aws", null, null, null, null, null, null, null, 2)*.id.sort() == [entityTags.id, moreEntityTags.id].sort()
entityTagsProvider.getAll("aws", null, null, null, null, null, null, null, null, 2)*.id.sort() == [entityTags.id, moreEntityTags.id].sort()

// fetch everything for `cloudprovider` and `cluster`
entityTagsProvider.getAll("aws", "cluster", null, null, null, null, null, null, 2)*.id.sort() == [entityTags.id, moreEntityTags.id].sort()
entityTagsProvider.getAll("aws", null, "cluster", null, null, null, null, null, null, 2)*.id.sort() == [entityTags.id, moreEntityTags.id].sort()

// fetch everything for `cloudprovider`, `cluster` and `idPrefix`
entityTagsProvider.getAll("aws", "cluster", null, "aws:cluster:clouddriver*", null, null, null, null, 2)*.id == [entityTags.id]
entityTagsProvider.getAll("aws", null, "cluster", null, "aws:cluster:clouddriver*", null, null, null, null, 2)*.id == [entityTags.id]

// fetch everything for `cloudprovider`, `cluster`, `idPrefix` and `tags`
entityTagsProvider.getAll("aws", "cluster", null, "aws*", null, null, null, ["tag3": "value3"], 2)*.id == [entityTags.id]
entityTagsProvider.getAll("aws", null, "cluster", null, "aws*", null, null, null, ["tag3": "value3"], 2)*.id == [entityTags.id]

// verify that globbing by tags works (with and without a namespace specified)
entityTagsProvider.getAll("aws", "cluster", null, "aws*", null, null, "default", ["tag3": "*"], 2)*.id == [entityTags.id]
entityTagsProvider.getAll("aws", "cluster", null, "aws*", null, null, null, ["tag3": "*"], 2)*.id == [entityTags.id]
entityTagsProvider.getAll("aws", null, "cluster", null, "aws*", null, null, "default", ["tag3": "*"], 2)*.id == [entityTags.id]
entityTagsProvider.getAll("aws", null, "cluster", null, "aws*", null, null, null, ["tag3": "*"], 2)*.id == [entityTags.id]

// namespace 'not_default' does not exist and should negate the matched tags
entityTagsProvider.getAll("aws", "cluster", null, "aws*", null, null, "not_default", ["tag3": "*"], 2).isEmpty()
entityTagsProvider.getAll("aws", null, "cluster", null, "aws*", null, null, "not_default", ["tag3": "*"], 2).isEmpty()

// verify that `maxResults` works
entityTagsProvider.getAll("aws", "cluster", null, null, null, null, null, null, 0).isEmpty()
entityTagsProvider.getAll("aws", null, "cluster", null, null, null, null, null, null, 0).isEmpty()

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public EntityTagsController(MessageSource messageSource,

@RequestMapping(method = RequestMethod.GET)
public Collection<EntityTags> list(@RequestParam(value = "cloudProvider", required = false) String cloudProvider,
@RequestParam(value = "application", required = false) String application,
@RequestParam(value = "entityType", required = false) String entityType,
@RequestParam(value = "entityId", required = false) String entityId,
@RequestParam(value = "idPrefix", required = false) String idPrefix,
Expand All @@ -66,6 +67,7 @@ public Collection<EntityTags> list(@RequestParam(value = "cloudProvider", requir

return tagProvider.getAll(
cloudProvider,
application,
entityType,
entityId != null ? Arrays.asList(entityId.split(",")) : null,
idPrefix,
Expand All @@ -74,7 +76,7 @@ public Collection<EntityTags> list(@RequestParam(value = "cloudProvider", requir
namespace,
tags,
maxResults
);
);
}

@RequestMapping(value = "/**", method = RequestMethod.GET)
Expand Down