Config Repo Preflight API endpoint by kierarad · Pull Request #5579 · gocd/gocd

kierarad · 2018-12-15T00:00:40Z

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

tomzo · 2018-12-19T09:21:40Z

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 · 2018-12-19T12:34:35Z

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

marques-work · 2018-12-22T00:50:02Z

@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 · 2018-12-23T09:52:10Z

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

marques-work · 2018-12-25T08:23:07Z

@tomzo will rebase/squash frivolous commits :)

arvindsv

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

arvindsv · 2018-12-26T12:07:49Z

api/api-base/src/main/java/com/thoughtworks/go/api/ApiController.java


 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";


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.

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?

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().

arvindsv · 2018-12-26T12:07:56Z

api/api-base/src/main/java/com/thoughtworks/go/api/ApiController.java

        }
    }

+    protected void setMultpipartUpload(Request req, Response res) {


Typo in name.

arvindsv · 2018-12-26T12:08:22Z

api/api-base/src/main/java/com/thoughtworks/go/api/ApiController.java

    }

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


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

@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?

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

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.

arvindsv · 2018-12-26T12:13:26Z

config/config-server/src/main/java/com/thoughtworks/go/config/MagicalGoConfigXmlLoader.java

 import static org.apache.commons.io.IOUtils.toInputStream;

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


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

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.");

arvindsv · 2018-12-26T12:38:02Z

server/src/main/java/com/thoughtworks/go/config/update/PartialConfigUpdateCommand.java

+    @Override
+    public CruiseConfig update(CruiseConfig cruiseConfig) {
+        if (partial != null && fingerprint != null) {
+            cruiseConfig.getPartials().remove(resolver.findPartialByFingerprint(cruiseConfig, fingerprint));


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

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.

arvindsv · 2018-12-26T12:40:16Z

server/src/main/java/com/thoughtworks/go/server/service/GoConfigService.java

        return cruiseConfig().getArtifactStores();
    }

+    public CruiseConfig validateCruiseConfig(CruiseConfig cruiseConfig) throws Exception {


Except for this method, just moves in this file?

Exactly correct :)

`parse-content` will transform arbitrary string content into the CRPipeline serialized JSON equivalent.

… 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.

…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.

…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

…ept custom messages

…ltiple file uploads

- Fix type in method name - Null-check partial config removal - Refactor multipart configuration to use Jetty constant

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

kierarad force-pushed the preflight-api branch from b673854 to d00907d Compare December 17, 2018 22:31

marques-work force-pushed the preflight-api branch 3 times, most recently from cebf61e to 022613d Compare December 18, 2018 22:57

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

marques-work force-pushed the preflight-api branch from 022613d to dd1822c Compare December 18, 2018 23:33

kierarad mentioned this pull request Dec 19, 2018

Add support for parsing the content of a file tomzo/gocd-yaml-config-plugin#94

Merged

marques-work force-pushed the preflight-api branch from 111d874 to ee7ff88 Compare December 22, 2018 00:19

tomzo approved these changes Dec 23, 2018

View reviewed changes

arvindsv reviewed Dec 26, 2018

View reviewed changes

marques-work added 10 commits December 31, 2018 20:30

Add parse-content message to ConfigRepo extension/contract

38c99e4

`parse-content` will transform arbitrary string content into the CRPipeline serialized JSON equivalent.

IDEA reformatting and import optimization.

062e7ae

Allow String.format() syntax in HaltApiResponses for methods that acc…

22f27bf

…ept custom messages

Optimize imports and cleanup

f811620

Refactor preflight endpoint to accept multipart/form-data to allow mu…

93a8afc

…ltiple file uploads

Reformat

f1ba8c1

Verify that preflight uploads are posted as files[]=...

474e6fc

marques-work force-pushed the preflight-api branch from 7f14a48 to 2cff944 Compare January 1, 2019 07:08

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

Enhancements per comments on gocd#5579

2cff944

- Fix type in method name - Null-check partial config removal - Refactor multipart configuration to use Jetty constant

Enhancements per comments on gocd#5579

ef4bd09

- Fix type in method name - Null-check partial config removal - Refactor multipart configuration to use Jetty constant

marques-work force-pushed the preflight-api branch from 795ab2e to ef4bd09 Compare January 1, 2019 07:11

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

arvindsv mentioned this pull request Jan 18, 2019

Epic: Thoughts about improving pipelines as code (config-repo) #5175

Closed

8 tasks

GaneshSPatil mentioned this pull request May 7, 2019

Pipeline-as-Code dry-run mode for PSV use #3911

Closed

Uh oh!

Conversation

kierarad commented Dec 15, 2018 • edited by marques-work Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notable changes

Uh oh!

tomzo commented Dec 19, 2018

Uh oh!

arvindsv commented Dec 19, 2018

Uh oh!

marques-work commented Dec 22, 2018

Uh oh!

tomzo commented Dec 23, 2018

Uh oh!

marques-work commented Dec 25, 2018

Uh oh!

arvindsv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kierarad commented Dec 15, 2018 •

edited by marques-work

Loading