Validate config returned by plugin as part of 'migrate-config' call (g…

…ocd#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.
GaneshSPatil · Apr 9, 2019 · bf97a05 · bf97a05
1 parent 485e678
commit bf97a05
Show file tree

Hide file tree

Showing 6 changed files with 115 additions and 58 deletions.
diff --git a/...ugin-infra/src/main/java/com/thoughtworks/go/plugin/infra/FelixGoPluginOSGiFramework.java b/...ugin-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/...-infra/src/test/java/com/thoughtworks/go/plugin/infra/FelixGoPluginOSGiFrameworkTest.java b/...-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/...rc/main/java/com/thoughtworks/go/config/update/ReplaceElasticAgentInformationCommand.java b/...rc/main/java/com/thoughtworks/go/config/update/ReplaceElasticAgentInformationCommand.java
@@ -17,48 +17,43 @@
 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;
 import com.thoughtworks.go.plugin.access.elastic.models.ElasticAgentInformation;
 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<ElasticAgentInformation> {
-
+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<String, String> pluginSettings;
-    private ElasticAgentInformation migratedElasticAgentInformation;
 
-    public ReplaceElasticAgentInformationCommand(ClusterProfilesService clusterProfilesService, ElasticProfileService elasticProfileService, ElasticAgentExtension elasticAgentExtension, GoConfigService goConfigService, GoPluginDescriptor pluginDescriptor, HashMap<String, String> pluginSettings) {
+    public ReplaceElasticAgentInformationCommand(ClusterProfilesService clusterProfilesService, ElasticProfileService elasticProfileService, ElasticAgentExtension elasticAgentExtension, GoPluginDescriptor pluginDescriptor, HashMap<String, String> 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<ClusterProfile> clusterProfiles = clusterProfilesService.getPluginProfiles().findByPluginId(pluginId);
         List<ElasticProfile> elasticAgentProfiles = elasticProfileService.getPluginProfiles().findByPluginId(pluginId);
 
         ElasticAgentInformation elasticAgentInformation = new ElasticAgentInformation(pluginSettings, clusterProfiles, elasticAgentProfiles);
-        migratedElasticAgentInformation = elasticAgentExtension.migrateConfig(pluginId, elasticAgentInformation);
+
+        ElasticAgentInformation migratedElasticAgentInformation = elasticAgentExtension.migrateConfig(pluginId, elasticAgentInformation);
 
         List<ClusterProfile> migratedClusterProfiles = migratedElasticAgentInformation.getClusterProfiles();
         List<ElasticProfile> 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/...src/main/java/com/thoughtworks/go/server/service/ElasticAgentInformationMigratorImpl.java b/...src/main/java/com/thoughtworks/go/server/service/ElasticAgentInformationMigratorImpl.java
@@ -16,19 +16,19 @@
 
 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;
 import com.thoughtworks.go.plugin.infra.ElasticAgentInformationMigrator;
 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<String, String> 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/...ast/java/com/thoughtworks/go/config/update/ReplaceElasticAgentInformationCommandTest.java b/...ast/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/...fast/java/com/thoughtworks/go/server/service/ElasticAgentInformationMigratorImplTest.java b/...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);
     }
 }