From bf97a0571de90561815d75b48645590a93847505 Mon Sep 17 00:00:00 2001 From: GaneshSPatil Date: Tue, 9 Apr 2019 18:23:37 +0530 Subject: [PATCH] Validate config returned by plugin as part of 'migrate-config' call (#5538) * Perform a full config save validations to check: - Valid cluster profiles. - Valid elastic agent profiles. - Valid elastic agent profile reference from job config. * Mark plugin invalid in case if it fails to migrate the existing config. --- .../infra/FelixGoPluginOSGiFramework.java | 4 ++ .../infra/FelixGoPluginOSGiFrameworkTest.java | 69 +++++++++++++++++++ ...ReplaceElasticAgentInformationCommand.java | 37 ++-------- .../ElasticAgentInformationMigratorImpl.java | 20 +++--- ...aceElasticAgentInformationCommandTest.java | 21 ++---- ...asticAgentInformationMigratorImplTest.java | 22 +++++- 6 files changed, 115 insertions(+), 58 deletions(-) diff --git a/plugin-infra/go-plugin-infra/src/main/java/com/thoughtworks/go/plugin/infra/FelixGoPluginOSGiFramework.java b/plugin-infra/go-plugin-infra/src/main/java/com/thoughtworks/go/plugin/infra/FelixGoPluginOSGiFramework.java index 352ac339481..5c180ad52ad 100644 --- a/plugin-infra/go-plugin-infra/src/main/java/com/thoughtworks/go/plugin/infra/FelixGoPluginOSGiFramework.java +++ b/plugin-infra/go-plugin-infra/src/main/java/com/thoughtworks/go/plugin/infra/FelixGoPluginOSGiFramework.java @@ -131,6 +131,10 @@ private Bundle getBundle(GoPluginDescriptor pluginDescriptor, File bundleLocatio elasticAgentInformationMigrator.migrate(pluginDescriptor); } + if (pluginDescriptor.isInvalid()) { + return bundle; + } + IterableUtils.forEach(pluginChangeListeners, notifyPluginLoadedEvent(pluginDescriptor)); return bundle; } catch (Exception e) { diff --git a/plugin-infra/go-plugin-infra/src/test/java/com/thoughtworks/go/plugin/infra/FelixGoPluginOSGiFrameworkTest.java b/plugin-infra/go-plugin-infra/src/test/java/com/thoughtworks/go/plugin/infra/FelixGoPluginOSGiFrameworkTest.java index 4519b4c32fd..3b76f851c73 100644 --- a/plugin-infra/go-plugin-infra/src/test/java/com/thoughtworks/go/plugin/infra/FelixGoPluginOSGiFrameworkTest.java +++ b/plugin-infra/go-plugin-infra/src/test/java/com/thoughtworks/go/plugin/infra/FelixGoPluginOSGiFrameworkTest.java @@ -387,6 +387,75 @@ void shouldMarkPluginDescriptorInvalidAndNotNotifyPluginChangeListenersWhenExten verify(pluginDescriptor).markAsInvalid(anyList(), eq(null)); } + @Test + void shouldMigrateElasticAgentInformationAsPartOfGetBundleCall() throws BundleException { + GoPluginDescriptor pluginDescriptor = mock(GoPluginDescriptor.class); + final PluginExtensionsAndVersionValidator.ValidationResult result = mock(PluginExtensionsAndVersionValidator.ValidationResult.class); + when(pluginDescriptor.bundle()).thenReturn(bundle); + when(pluginDescriptor.bundleLocation()).thenReturn(new File("foo")); + when(pluginDescriptor.isInvalid()).thenReturn(false); + when(bundleContext.installBundle(any(String.class))).thenReturn(bundle); + when(pluginExtensionsAndVersionValidator.validate(pluginDescriptor)).thenReturn(result); + when(result.hasError()).thenReturn(false); + + ElasticAgentInformationMigrator migrator = mock(ElasticAgentInformationMigrator.class); + spy.setElasticAgentInformationMigrator(migrator); + + PluginChangeListener listener1 = mock(PluginChangeListener.class); + PluginChangeListener listener2 = mock(PluginChangeListener.class); + PluginChangeListener listener3 = mock(PluginChangeListener.class); + + spy.addPluginChangeListener(listener1); + spy.addPluginChangeListener(listener2); + spy.addPluginChangeListener(listener3); + spy.start(); + + spy.loadPlugin(pluginDescriptor); + + verify(listener1, times(1)).pluginLoaded(pluginDescriptor); + verify(listener2, times(1)).pluginLoaded(pluginDescriptor); + verify(listener3, times(1)).pluginLoaded(pluginDescriptor); + + verify(migrator, times(1)).migrate(pluginDescriptor); + + verify(bundle, times(1)).start(); + } + + @Test + void shouldNotCallListenerWhenMigrateElasticAgentInformationMarksPluginInvalid() throws BundleException { + GoPluginDescriptor pluginDescriptor = mock(GoPluginDescriptor.class); + final PluginExtensionsAndVersionValidator.ValidationResult result = mock(PluginExtensionsAndVersionValidator.ValidationResult.class); + when(pluginDescriptor.bundle()).thenReturn(bundle); + when(pluginDescriptor.isInvalid()).thenReturn(false).thenReturn(true); + when(pluginDescriptor.bundleLocation()).thenReturn(new File("foo")); + when(bundleContext.installBundle(any(String.class))).thenReturn(bundle); + when(pluginExtensionsAndVersionValidator.validate(pluginDescriptor)).thenReturn(result); + when(result.hasError()).thenReturn(false); + + ElasticAgentInformationMigrator migrator = mock(ElasticAgentInformationMigrator.class); + + spy.setElasticAgentInformationMigrator(migrator); + + PluginChangeListener listener1 = mock(PluginChangeListener.class); + PluginChangeListener listener2 = mock(PluginChangeListener.class); + PluginChangeListener listener3 = mock(PluginChangeListener.class); + + spy.addPluginChangeListener(listener1); + spy.addPluginChangeListener(listener2); + spy.addPluginChangeListener(listener3); + spy.start(); + + spy.loadPlugin(pluginDescriptor); + + verifyNoMoreInteractions(listener1); + verifyNoMoreInteractions(listener2); + verifyNoMoreInteractions(listener3); + + verify(migrator, times(1)).migrate(pluginDescriptor); + + verify(bundle, times(1)).start(); + } + @Test void shouldSkipUninstallIfPluginIsPreviouslyUninstalled() throws BundleException { GoPluginDescriptor pluginDescriptor = mock(GoPluginDescriptor.class); diff --git a/server/src/main/java/com/thoughtworks/go/config/update/ReplaceElasticAgentInformationCommand.java b/server/src/main/java/com/thoughtworks/go/config/update/ReplaceElasticAgentInformationCommand.java index 34187c703ed..a60aa3377ef 100644 --- a/server/src/main/java/com/thoughtworks/go/config/update/ReplaceElasticAgentInformationCommand.java +++ b/server/src/main/java/com/thoughtworks/go/config/update/ReplaceElasticAgentInformationCommand.java @@ -17,7 +17,7 @@ package com.thoughtworks.go.config.update; import com.thoughtworks.go.config.CruiseConfig; -import com.thoughtworks.go.config.commands.EntityConfigUpdateCommand; +import com.thoughtworks.go.config.UpdateConfigCommand; import com.thoughtworks.go.config.elastic.ClusterProfile; import com.thoughtworks.go.config.elastic.ElasticProfile; import com.thoughtworks.go.plugin.access.elastic.ElasticAgentExtension; @@ -25,40 +25,35 @@ import com.thoughtworks.go.plugin.infra.plugininfo.GoPluginDescriptor; import com.thoughtworks.go.server.service.ClusterProfilesService; import com.thoughtworks.go.server.service.ElasticProfileService; -import com.thoughtworks.go.server.service.GoConfigService; import java.util.HashMap; import java.util.List; -public class ReplaceElasticAgentInformationCommand implements EntityConfigUpdateCommand { - +public class ReplaceElasticAgentInformationCommand implements UpdateConfigCommand { private final ClusterProfilesService clusterProfilesService; private final ElasticProfileService elasticProfileService; private final ElasticAgentExtension elasticAgentExtension; - private final GoConfigService goConfigService; private final GoPluginDescriptor pluginDescriptor; private final HashMap pluginSettings; - private ElasticAgentInformation migratedElasticAgentInformation; - public ReplaceElasticAgentInformationCommand(ClusterProfilesService clusterProfilesService, ElasticProfileService elasticProfileService, ElasticAgentExtension elasticAgentExtension, GoConfigService goConfigService, GoPluginDescriptor pluginDescriptor, HashMap pluginSettings) { + public ReplaceElasticAgentInformationCommand(ClusterProfilesService clusterProfilesService, ElasticProfileService elasticProfileService, ElasticAgentExtension elasticAgentExtension, GoPluginDescriptor pluginDescriptor, HashMap pluginSettings) { this.clusterProfilesService = clusterProfilesService; this.elasticProfileService = elasticProfileService; this.elasticAgentExtension = elasticAgentExtension; - this.goConfigService = goConfigService; this.pluginDescriptor = pluginDescriptor; this.pluginSettings = pluginSettings; } - @Override - public void update(CruiseConfig preprocessedConfig) throws Exception { + public CruiseConfig update(CruiseConfig preprocessedConfig) throws Exception { String pluginId = pluginDescriptor.id(); List clusterProfiles = clusterProfilesService.getPluginProfiles().findByPluginId(pluginId); List elasticAgentProfiles = elasticProfileService.getPluginProfiles().findByPluginId(pluginId); ElasticAgentInformation elasticAgentInformation = new ElasticAgentInformation(pluginSettings, clusterProfiles, elasticAgentProfiles); - migratedElasticAgentInformation = elasticAgentExtension.migrateConfig(pluginId, elasticAgentInformation); + + ElasticAgentInformation migratedElasticAgentInformation = elasticAgentExtension.migrateConfig(pluginId, elasticAgentInformation); List migratedClusterProfiles = migratedElasticAgentInformation.getClusterProfiles(); List migratedElasticAgentProfiles = migratedElasticAgentInformation.getElasticAgentProfiles(); @@ -68,25 +63,7 @@ public void update(CruiseConfig preprocessedConfig) throws Exception { preprocessedConfig.getElasticConfig().getProfiles().removeAll(elasticAgentProfiles); preprocessedConfig.getElasticConfig().getProfiles().addAll(migratedElasticAgentProfiles); - } - - @Override - public boolean isValid(CruiseConfig preprocessedConfig) { - //todo: Add validation to check if plugin returns valid config - return true; - } - @Override - public void clearErrors() { - } - - @Override - public ElasticAgentInformation getPreprocessedEntityConfig() { - return migratedElasticAgentInformation; - } - - @Override - public boolean canContinue(CruiseConfig cruiseConfig) { - return true; + return preprocessedConfig; } } diff --git a/server/src/main/java/com/thoughtworks/go/server/service/ElasticAgentInformationMigratorImpl.java b/server/src/main/java/com/thoughtworks/go/server/service/ElasticAgentInformationMigratorImpl.java index 2c188cd7da0..0e573b8d890 100644 --- a/server/src/main/java/com/thoughtworks/go/server/service/ElasticAgentInformationMigratorImpl.java +++ b/server/src/main/java/com/thoughtworks/go/server/service/ElasticAgentInformationMigratorImpl.java @@ -16,7 +16,7 @@ package com.thoughtworks.go.server.service; -import com.thoughtworks.go.config.commands.EntityConfigUpdateCommand; +import com.thoughtworks.go.config.UpdateConfigCommand; import com.thoughtworks.go.config.update.ReplaceElasticAgentInformationCommand; import com.thoughtworks.go.domain.Plugin; import com.thoughtworks.go.plugin.access.elastic.ElasticAgentExtension; @@ -24,11 +24,11 @@ import com.thoughtworks.go.plugin.infra.PluginManager; import com.thoughtworks.go.plugin.infra.plugininfo.GoPluginDescriptor; import com.thoughtworks.go.server.dao.PluginSqlMapDao; -import com.thoughtworks.go.server.domain.Username; import com.thoughtworks.go.util.json.JsonHelper; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Component; +import java.util.Collections; import java.util.HashMap; import static com.thoughtworks.go.plugin.domain.common.PluginConstants.ELASTIC_AGENT_EXTENSION; @@ -64,17 +64,21 @@ public void migrate(GoPluginDescriptor pluginDescriptor) { Plugin plugin = pluginSqlMapDao.findPlugin(pluginId); String pluginConfiguration = plugin.getConfiguration(); HashMap pluginSettings = (pluginConfiguration == null) ? new HashMap<>() : JsonHelper.fromJson(pluginConfiguration, HashMap.class); - ReplaceElasticAgentInformationCommand command = new ReplaceElasticAgentInformationCommand(clusterProfilesService, elasticProfileService, elasticAgentExtension, goConfigService, pluginDescriptor, pluginSettings); + ReplaceElasticAgentInformationCommand command = new ReplaceElasticAgentInformationCommand(clusterProfilesService, elasticProfileService, elasticAgentExtension, pluginDescriptor, pluginSettings); - update(command); + update(command, pluginDescriptor); } - private void update(EntityConfigUpdateCommand command) { + private void update(UpdateConfigCommand command, GoPluginDescriptor pluginDescriptor) { try { - goConfigService.updateConfig(command, new Username("GoCD")); + goConfigService.updateConfig(command); } catch (Exception e) { - //todo: mark plugin invalid if migration fails -// e.getMessage() + String pluginId = pluginDescriptor.id(); + String pluginAPIRequest = "cd.go.elastic-agent.migrate-config"; + String reason = e.getMessage(); + String errorMessage = String.format("Plugin '%s' failed to perform '%s' call. Plugin sent an invalid config. Reason: %s.\n Please fix the errors and restart GoCD server.", pluginId, pluginAPIRequest, reason); + + pluginDescriptor.markAsInvalid(Collections.singletonList(errorMessage), e); } } } diff --git a/server/src/test-fast/java/com/thoughtworks/go/config/update/ReplaceElasticAgentInformationCommandTest.java b/server/src/test-fast/java/com/thoughtworks/go/config/update/ReplaceElasticAgentInformationCommandTest.java index ea89308ddce..6bbf816c08e 100644 --- a/server/src/test-fast/java/com/thoughtworks/go/config/update/ReplaceElasticAgentInformationCommandTest.java +++ b/server/src/test-fast/java/com/thoughtworks/go/config/update/ReplaceElasticAgentInformationCommandTest.java @@ -17,6 +17,7 @@ package com.thoughtworks.go.config.update; import com.thoughtworks.go.config.BasicCruiseConfig; +import com.thoughtworks.go.config.CruiseConfig; import com.thoughtworks.go.config.elastic.*; import com.thoughtworks.go.plugin.access.elastic.ElasticAgentExtension; import com.thoughtworks.go.plugin.access.elastic.models.ElasticAgentInformation; @@ -70,7 +71,7 @@ void setUp() { elasticProfiles = new ElasticProfiles(); elasticProfiles.add(new ElasticProfile("profile-id", pluginId)); - replaceElasticAgentInformationCommand = new ReplaceElasticAgentInformationCommand(clusterProfilesService, elasticProfileService, elasticAgentExtension, goConfigService, pluginDescriptor, pluginSettings); + replaceElasticAgentInformationCommand = new ReplaceElasticAgentInformationCommand(clusterProfilesService, elasticProfileService, elasticAgentExtension, pluginDescriptor, pluginSettings); when(pluginDescriptor.id()).thenReturn(pluginId); when(clusterProfilesService.getPluginProfiles()).thenReturn(clusterProfiles); @@ -88,7 +89,7 @@ void shouldMakeCallToElasticAgentExtensionToMigrateElasticAgentRelatedConfig() t @Test void shouldMakeCallToElasticAgentExtensionToMigrateElasticAgentRelatedConfig_WhenNoPluginSettingsAreConfigured() throws Exception { - replaceElasticAgentInformationCommand = new ReplaceElasticAgentInformationCommand(clusterProfilesService, elasticProfileService, elasticAgentExtension, goConfigService, pluginDescriptor, new HashMap<>()); + replaceElasticAgentInformationCommand = new ReplaceElasticAgentInformationCommand(clusterProfilesService, elasticProfileService, elasticAgentExtension, pluginDescriptor, new HashMap<>()); replaceElasticAgentInformationCommand.update(basicCruiseConfig); @@ -103,7 +104,7 @@ void shouldUpdateGoCDConfigWithPluginReturnedMigratedConfig() throws Exception { assertThat(elasticConfig.getProfiles()).hasSize(0); assertThat(elasticConfig.getClusterProfiles()).hasSize(0); - replaceElasticAgentInformationCommand.update(basicCruiseConfig); + CruiseConfig basicCruiseConfig = replaceElasticAgentInformationCommand.update(this.basicCruiseConfig); verify(elasticAgentExtension).migrateConfig(pluginId, new ElasticAgentInformation(Collections.emptyMap(), clusterProfiles, elasticProfiles)); @@ -112,18 +113,4 @@ void shouldUpdateGoCDConfigWithPluginReturnedMigratedConfig() throws Exception { assertThat(basicCruiseConfig.getElasticConfig().getClusterProfiles()).hasSize(1); assertThat(basicCruiseConfig.getElasticConfig().getClusterProfiles()).isEqualTo(clusterProfiles); } - - @Test - void shouldGetPreprocessedConfig() throws Exception { - replaceElasticAgentInformationCommand.update(basicCruiseConfig); - - ElasticAgentInformation preprocessedEntityConfig = replaceElasticAgentInformationCommand.getPreprocessedEntityConfig(); - - assertThat(preprocessedEntityConfig).isEqualTo(new ElasticAgentInformation(Collections.emptyMap(), clusterProfiles, elasticProfiles)); - } - - @Test - void shouldAlwaysAllowTheReplaceElasticAgentInformationCommandToProceed() { - assertThat(replaceElasticAgentInformationCommand.canContinue(basicCruiseConfig)).isTrue(); - } } diff --git a/server/src/test-fast/java/com/thoughtworks/go/server/service/ElasticAgentInformationMigratorImplTest.java b/server/src/test-fast/java/com/thoughtworks/go/server/service/ElasticAgentInformationMigratorImplTest.java index ace9e546d89..b5b72e96c69 100644 --- a/server/src/test-fast/java/com/thoughtworks/go/server/service/ElasticAgentInformationMigratorImplTest.java +++ b/server/src/test-fast/java/com/thoughtworks/go/server/service/ElasticAgentInformationMigratorImplTest.java @@ -22,7 +22,6 @@ import com.thoughtworks.go.plugin.infra.PluginManager; import com.thoughtworks.go.plugin.infra.plugininfo.GoPluginDescriptor; import com.thoughtworks.go.server.dao.PluginSqlMapDao; -import com.thoughtworks.go.server.domain.Username; import com.thoughtworks.go.util.json.JsonHelper; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -32,6 +31,7 @@ import java.util.Map; import static com.thoughtworks.go.plugin.domain.common.PluginConstants.ELASTIC_AGENT_EXTENSION; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.*; import static org.mockito.MockitoAnnotations.initMocks; @@ -80,7 +80,7 @@ void shouldPerformReplaceElasticAgentInformationCommandToMigrateElasticAgentInfo elasticAgentInformationMigrator.migrate(goPluginDescriptor); - verify(goConfigService, times(1)).updateConfig(any(ReplaceElasticAgentInformationCommand.class), any(Username.class)); + verify(goConfigService, times(1)).updateConfig(any(ReplaceElasticAgentInformationCommand.class)); } @Test @@ -90,6 +90,22 @@ void shouldPerformReplaceElasticAgentInformationCommandToMigrateElasticAgentInfo elasticAgentInformationMigrator.migrate(goPluginDescriptor); - verify(goConfigService, times(1)).updateConfig(any(ReplaceElasticAgentInformationCommand.class), any(Username.class)); + verify(goConfigService, times(1)).updateConfig(any(ReplaceElasticAgentInformationCommand.class)); + } + + @Test + void shouldMarkPluginDescriptorInvalidIncaseOfErrors() { + when(pluginManager.isPluginOfType(ELASTIC_AGENT_EXTENSION, goPluginDescriptor.id())).thenReturn(true); + when(pluginSqlMapDao.findPlugin(PLUGIN_ID)).thenReturn(new Plugin(PLUGIN_ID, null)); + when(goConfigService.updateConfig(any())).thenThrow(new RuntimeException("Boom!")); + + assertThat(goPluginDescriptor.isInvalid()).isFalse(); + + elasticAgentInformationMigrator.migrate(goPluginDescriptor); + + String expectedErrorMessage = "Plugin 'plugin-id' failed to perform 'cd.go.elastic-agent.migrate-config' call. Plugin sent an invalid config. Reason: Boom!.\n Please fix the errors and restart GoCD server."; + + assertThat(goPluginDescriptor.isInvalid()).isTrue(); + assertThat(goPluginDescriptor.getStatus().getMessages().get(0)).isEqualTo(expectedErrorMessage); } }