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

Config Repo Preflight API endpoint #5579

Merged
merged 11 commits into from Jan 2, 2019

Conversation

Projects
4 participants
@kierarad
Copy link
Contributor

commented Dec 15, 2018

This adds an endpoint for parsing arbitrary pipeline/environment definition content with a specified config-repo plugin and validating structure, collisions, and dependencies -- basically the full set of validations on a CruiseConfig.

This preflight endpoint lives under the config-repo operations API, which has been repurposed as a public API.

For reviewers, there is a lot of refactoring and IDEA reformatting/optimization. As such, it might be easier to understand if one review the individual commits to see the intended change and progression of logic, rather than looking over the Files changed tab.

Notable changes

  • Introduces a new parse-content message to the config repo contract. Will need to update the documentation.
  • As such, this will not work on the bundled versions of config-repo plugins; will cut a PR on @tomzo's repositories that include the changes.

Part of #5489

@arvindsv arvindsv added this to the Release 19.1.0 milestone Dec 17, 2018

@kierarad kierarad force-pushed the kierarad:preflight-api branch from b673854 to d00907d Dec 17, 2018

@marques-work marques-work force-pushed the kierarad:preflight-api branch 3 times, most recently from cebf61e to 022613d Dec 18, 2018

@marques-work marques-work changed the title WIP - Config Repo Preflight Config Repo Preflight API endpoint Dec 18, 2018

@marques-work marques-work force-pushed the kierarad:preflight-api branch from 022613d to dd1822c Dec 18, 2018

@tomzo

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

Hi @kierarad @marques-work ,

I've read the PR, I have only one suggestion. Don't you think API should accept multipart/form-data so that multiple files with their names could be proposed?
There are 2 reasons for this:

  • (this is minor) working in JSON plugin, the file extension has a meaning, pipeline or environment
  • in yaml, if multiple files are changed within the same repository, and then we have validation errors, then how location of the error will be shown? It might be impossible to guess in which file the error is really, because this location context was lost in the request to begin with.

Also it would be nice to add a test which sort of showcases the error location in the returned message.

@arvindsv

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

I agree with all of that. I think we should definitely send the whole directory, since there can be dependencies between files.

@marques-work marques-work force-pushed the kierarad:preflight-api branch from 111d874 to ee7ff88 Dec 22, 2018

@marques-work

This comment has been minimized.

Copy link
Member

commented Dec 22, 2018

@tomzo @arvindsv Multipart upload should work now; just need to update the plugin PRs with the contract change.

Here's what the request looks like:

curl -u user:pass -H 'Accept: application/vnd.go.cd.v1+json' -F 'files[]=@path/to/file1.gocd.yaml' -F 'files[]=@path/to/file2.gocd.yaml' "http://localhost:8153/go/api/admin/config_repo_ops/preflight?pluginId=yaml.config.plugin"

# existing repo also passes repoId=xxx
curl -u user:pass -H 'Accept: application/vnd.go.cd.v1+json' -F 'files[]=@path/to/file1.gocd.yaml' -F 'files[]=@path/to/file2.gocd.yaml' "http://localhost:8153/go/api/admin/config_repo_ops/preflight?pluginId=yaml.config.plugin&repoId=myExistingRepo"
@tomzo

tomzo approved these changes Dec 23, 2018

@tomzo

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

I think this is OK now, but a few commits could be squashed and rebased before merge.

@marques-work

This comment has been minimized.

Copy link
Member

commented Dec 25, 2018

@tomzo will rebase/squash frivolous commits :)

@arvindsv
Copy link
Member

left a comment

I just took a quick look and had some comments. Don't consider this a review. :)

@@ -34,6 +35,7 @@

public abstract class ApiController implements ControllerMethods, SparkController {
private static final Set<String> UPDATE_HTTP_METHODS = new HashSet<>(Arrays.asList("PUT", "POST", "PATCH"));
private static final String MULTIPART_REQ_ATTR = "org.eclipse.jetty.multipartConfig";

This comment has been minimized.

Copy link
@arvindsv

arvindsv Dec 26, 2018

Member

Can we push this to somewhere closer to Jetty? My worry is, if a Jetty upgrade changes this property, we won't know to change it here.

This comment has been minimized.

Copy link
@marques-work

marques-work Jan 1, 2019

Member

I will move it into GoServer, as I don't want to add any more module dependencies (mostly because I'm unsure of consequences). This might be close enough to the jetty9 module?

This comment has been minimized.

Copy link
@marques-work

marques-work Jan 1, 2019

Member

Better yet, I found the constant in org.eclipse.jetty.server.Request.__MULTIPART_CONFIG_ELEMENT and extracted this into the server module as RequestUtils.configureMultipart().

@@ -76,6 +78,10 @@ protected void verifyContentType(Request request, Response response) throws IOEx
}
}

protected void setMultpipartUpload(Request req, Response res) {

This comment has been minimized.

Copy link
@arvindsv

arvindsv Dec 26, 2018

Member

Typo in name.

This comment has been minimized.

Copy link
@marques-work
@@ -76,6 +78,10 @@ protected void verifyContentType(Request request, Response response) throws IOEx
}
}

protected void setMultpipartUpload(Request req, Response res) {
req.raw().setAttribute(MULTIPART_REQ_ATTR, new MultipartConfigElement(System.getProperty("java.io.tmpdir", "tmp")));

This comment has been minimized.

Copy link
@arvindsv

arvindsv Dec 26, 2018

Member

Does this mean that, once the request is served, this directory will not be cleaned up?

This comment has been minimized.

Copy link
@marques-work

marques-work Jan 1, 2019

Member

@arvindsv I had the same question in my head, but I empirically found (at least in my basic trials) that nothing was being written to java.io.tmpdir after a multipart request.

FWIW, there's a method on MultiPartInputStreamParser that can explicitly delete the parts from disk, but we don't have a handle on that object. There's also a flag for "delete on exit" but same lack of handle problem. So...shrug?

This comment has been minimized.

Copy link
@arvindsv

arvindsv Jan 2, 2019

Member

Maybe it uses it only if the payload is bigger than its buffer?

This comment has been minimized.

Copy link
@marques-work

marques-work Jan 2, 2019

Member

It’s certainly possible— I’ll try with a larger payload (100mb?). If I see a file not being cleaned, I’ll have an explicit cleanup at the end.

@@ -47,35 +47,52 @@
import static org.apache.commons.io.IOUtils.toInputStream;

public class MagicalGoConfigXmlLoader {
private static final Logger LOGGER = LoggerFactory.getLogger(MagicalGoConfigXmlLoader.class);

This comment has been minimized.

Copy link
@arvindsv

arvindsv Dec 26, 2018

Member

Most (all?) of the changes in this file are moving stuff around, right?

This comment has been minimized.

Copy link
@marques-work

marques-work Jan 1, 2019

Member

Most. Just these minor changes largely due to changing the constructor signature of certain exceptions:

diff --git a/config/config-server/src/main/java/com/thoughtworks/go/config/MagicalGoConfigXmlLoader.java b/config/config-server/src/main/java/com/thoughtworks/go/config/MagicalGoConfigXmlLoader.java
index cbdb1dc2b..d28cec426 100644
--- a/config/config-server/src/main/java/com/thoughtworks/go/config/MagicalGoConfigXmlLoader.java
+++ b/config/config-server/src/main/java/com/thoughtworks/go/config/MagicalGoConfigXmlLoader.java
@@ -111,11 +111,11 @@ public class MagicalGoConfigXmlLoader {
     }
 
     public CruiseConfig preprocessAndValidate(CruiseConfig config) throws Exception {
-        LOGGER.debug("[Config Save] In preprocessAndValidate: Cloning.");
+        LOGGER.debug("[Config Validation] In preprocessAndValidate: Cloning.");
         CruiseConfig cloned = CLONER.deepClone(config);
-        LOGGER.debug("[Config Save] In preprocessAndValidate: Validating.");
+        LOGGER.debug("[Config Validation] In preprocessAndValidate: Validating.");
         validateCruiseConfig(cloned);
-        LOGGER.debug("[Config Save] In preprocessAndValidate: Done.");
+        LOGGER.debug("[Config Validation] In preprocessAndValidate: Done.");
         config.encryptSecureProperties(cloned);
         return cloned;
     }
@@ -133,14 +133,14 @@ public class MagicalGoConfigXmlLoader {
         }
     }
 
-    private CruiseConfig validateCruiseConfig(CruiseConfig config) throws Exception {
+    public CruiseConfig validateCruiseConfig(CruiseConfig config) throws Exception {
         LOGGER.debug("[Config Save] In validateCruiseConfig: Starting.");
         List<ConfigErrors> allErrors = validate(config);
         if (!allErrors.isEmpty()) {
             if (config.isLocal())
-                throw new GoConfigInvalidException(config, allErrors.get(0).asString());
+                throw new GoConfigInvalidException(config, allErrors);
             else
-                throw new GoConfigInvalidMergeException("Merged validation failed", config, config.getMergedPartials(), allErrors);
+                throw new GoConfigInvalidMergeException(config, config.getMergedPartials(), allErrors);
         }
 
         LOGGER.debug("[Config Save] In validateCruiseConfig: Running validate.");
@Override
public CruiseConfig update(CruiseConfig cruiseConfig) {
if (partial != null && fingerprint != null) {
cruiseConfig.getPartials().remove(resolver.findPartialByFingerprint(cruiseConfig, fingerprint));

This comment has been minimized.

Copy link
@arvindsv

arvindsv Dec 26, 2018

Member

findPartialByFingerprint can be null. Is that going to be a problem? In some race condition maybe?

This comment has been minimized.

Copy link
@marques-work

marques-work Jan 1, 2019

Member

Not sure - this was a refactor (move method, rename) so this was likely a problem before. I can add a null check just in case anyway.

@@ -1043,10 +1042,68 @@ public ArtifactStores artifactStores() {
return cruiseConfig().getArtifactStores();
}

public CruiseConfig validateCruiseConfig(CruiseConfig cruiseConfig) throws Exception {

This comment has been minimized.

Copy link
@arvindsv

arvindsv Dec 26, 2018

Member

Except for this method, just moves in this file?

This comment has been minimized.

Copy link
@marques-work

marques-work Jan 1, 2019

Member

Exactly correct :)

marques-work added some commits Dec 18, 2018

Add `parse-content` message to ConfigRepo extension/contract
`parse-content` will transform arbitrary string content into the CRPipeline serialized JSON equivalent.
Add GoPartialConfig.merge() as a convenience to merge a PartialConfig…
… into a CruiseConfig

To achieve this, we also extracted the inner instance class GoPartialUpdateCommand into a top-level
class, PartialConfigUpdateCommand. This required abstracting some behavior behind a new interface,
PartialConfigResolver and the implementation was moved to CachedGoPartials as it seemed more appropriate
given the fact that all of the logic only concerned the partial caches.
Add `GoConfigService.validateCruiseConfig()` as a convenience method …
…to perform validations

This required some significant refactoring to GoConfigInvalidException and GoConfigInvalidMergeException
so that we can access the errors as a list from the exceptions directly. This will help build the preflight
result that will be returned by the API, as it is generally more useful to retain a list of errors rather
than a concatenated error message. The `summary` string was also dropped from the exception constructors
as the value wasn't being used(!) in code anyway.

MagicalGoConfigXmlLoader now exposes `validateCruiseConfig()`, which is delegated to by GoConfigService.
Introduce preflight API endpoint, repurposing ConfigRepoOperationsCon…
…trollerV1 as a public API

  - POST endpoint accepts arbitrary file content that will parse and validate syntax, structure,
    and dependencies against a config-repo plugin
  - Requires user to provide a config-repo plugin ID
  - Optionally accepts a config-repo ID; preflighting changes to existing config-repos should
    always provide this, or else validations will likely fail on duplicate pipeline names

@marques-work marques-work force-pushed the kierarad:preflight-api branch from 7f14a48 to 2cff944 Jan 1, 2019

marques-work added a commit to kierarad/gocd that referenced this pull request Jan 1, 2019

Enhancements per comments on gocd#5579
  - Fix type in method name
  - Null-check partial config removal
  - Refactor multipart configuration to use Jetty constant
Enhancements per comments on #5579
  - Fix type in method name
  - Null-check partial config removal
  - Refactor multipart configuration to use Jetty constant

@marques-work marques-work force-pushed the kierarad:preflight-api branch from 795ab2e to ef4bd09 Jan 1, 2019

@marques-work marques-work merged commit fde8612 into gocd:master Jan 2, 2019

8 checks passed

build-linux-PR/build-non-server
Details
build-linux-PR/build-server
Details
build-windows-PR/build-non-server
Details
build-windows-PR/build-server
Details
installers-PR/dist
Details
license/cla Contributor License Agreement is signed.
Details
plugins-PR/build
Details
trigger/do-nothing
Details

@rajiesh rajiesh added this to In progress in 19.1.0 via automation Jan 3, 2019

@maheshp maheshp moved this from In progress to Done in 19.1.0 Jan 4, 2019

@rajiesh rajiesh moved this from Done to QA Done in 19.1.0 Jan 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.