Operator Parameter and Template Verification #1106

kensipe · 2019-11-26T14:41:32Z

What this PR does / why we need it:
Provides additional package verification for operators.

This PR walks through the template nodes and provides validation associated with that which include:

errors for template params which are not defined
warnings for defined params which are not used
errors for templates which are not parsable

This PR also provides verification on templates and their use in the operator file:

errors for resource names which do not have associated templates
warnings for templates which are not used as a resource

Using: Replace go run cmd/kubectl-kudo/main.go package with kubectl kudo for binary use.

go run cmd/kubectl-kudo/main.go package verify ../operators/repository/kafka/operator/
Warnings                                                             
parameter "SSL_ENDPOINT_IDENTIFICATION_ENABLED" defined but not used.
parameter "CUSTOM_SERVER_PROPERTIES" defined but not used.           
package is valid

go run cmd/kubectl-kudo/main.go package verify ../operators/repository/flink/docs/demo/financial-fraud/demo-operator/
Errors                                                           
parameter "download_url" in template uploader.yaml is not defined
Error: package verification errors: 1
exit status 255

Fixes #1077 #1080

pkg/kudoctl/cmd/verify/verify.go

pkg/kudoctl/packages/types.go

kensipe · 2019-11-26T19:32:35Z

It is worth noting that the verifiers are by design built to follow a command pattern. It is somewhat expected that someone will request a flag to skip a specific verification and this will enable that.

alenkacz

Left couple of minor comments - I am not approving nor rejecting though. To get my approval, my main concern is what's in packages/types.go. We've refactored the packages package recently to give it some sense of encapsulation and I feel like we're breaking it again.

First types.go is a place for high-level exposed types for parsing packages (like the tarballs). Having a low-level code for parsing templates there feels like it's a bit abusing that place. Also the implicits are totally a renderer implementation detail in my head and now we expose them in the packages/types. I would like to see the whole template parsing being encapsulated somewhere nicely, we can pair on that when you're back if you want.

pkg/kudoctl/packages/types.go

pkg/kudoctl/cmd/verify/verify.go

zen-dog

Let's keep types.go only about types. All the methods belong in separate packages. I strongly suggest creating a new /packages/sub-package (can't come up with a good, single-word name yet) to avoid circular dependency hell.

pkg/kudoctl/packages/types.go

kensipe · 2019-11-27T18:08:02Z

Great feedback! Restructuring the packaging totally makes sense. I'll hit it on Monday after the US holiday weekend!

pkg/kudoctl/packages/types.go

Co-Authored-By: Andreas Neumann <aneumann@mesosphere.com>

…out breaking current verify or packaging

…s verifiers

additionally renamed `verify::Operator()` to `verify.PackageFiles` - term `Operator` is heavily overloaded both externally and in the code base.

alenkacz

I think no blockers for me, thanks 👍 I would like some parts still get more docs/a little bit of cleanup but it overall looks good already

pkg/kudoctl/packages/template/template_verify.go

pkg/kudoctl/packages/types.go

alenkacz · 2019-12-06T12:50:31Z

pkg/kudoctl/cmd/verify/verify.go

 // DuplicateVerifier provides verification that there are no duplicates disallowing casing (Kudo and kudo are duplicates)
 type DuplicateVerifier struct {
 }

-func (DuplicateVerifier) Verify(params packages.Parameter) (warnings ParamWarnings, errors ParamErrors) {
+func (DuplicateVerifier) Verify(pf *packages.Files) (warnings verifier.ParamWarnings, errors verifier.ParamErrors) {


should we move these to verify package as well? would be nice to have all the verifiers in one package or do you see any advantage of keeping these separate?

to get to the level of encapsulation @zen-dog requested which current makes sense to me as well... it is necessary for verifiers to be in different packages. The template verifiers for instance are under templates package. To put all verifiers in a verifiers package would request exposing template details... same for other verifiers.

alenkacz · 2019-12-06T12:54:48Z

pkg/kudoctl/packages/template/template_parsing.go

+}
+
+//walkNodes walks the nodes of a template providing an array of parameters
+func walkNodes(node parse.Node, fname string, nodeMap map[string]map[string]bool) {


this is very go-template internals thing, I don't really understand all the implications of how are certain types parsed. Maybe this deserves more comments/docs or some references to go template docs?

which now looks like: ``` ./pkg/kudoctl/packages/verifier/ ├── template │ ├── parser.go │ ├── parser_test.go │ ├── verify_parameters.go │ ├── verify_parameters_test.go.go │ ├── verify_references.go │ └── verify_references_test.go └── types.go ``` - moved `templates` pkg into `verify/templates` - this is the subpkg that handles verifying templates, leaving it open to verify other things (like operator.yaml etc.) - split existing verifiers into separate files - renamed `template_parsing` to `parser` since _template_ is already in the package name

zen-dog · 2019-12-06T12:01:51Z

pkg/engine/renderer/engine.go

-// returning errors when keys are missing.
-func (e *Engine) Render(tpl string, vals map[string]interface{}) (string, error) {
+// Template provides access to the engines template engine.
+func (e Engine) Template(name string) *template.Template {


I'm a little bit concerned about exposing template.Template to the outer world. So far, go-templates as implementation details have been nicely encapsulated within the renderer package. Exposing it here will make it harder to move to a different frontend e.g. Starlak. But that's a "future-us-problem" I guess ¯_(ツ)_/¯

pkg/kudoctl/packages/template/template_verify.go

zen-dog

This goes a long way from the initial POC 👏 Great work. A few things we still could improve:

parsing go-templates - seems that you have a lot of context there that I'd love to see in comments.
verify error types. would love to see verifier.Error encapsulating handling and printing of errors/warnings

None of that is a blocker so ✅

pkg/kudoctl/packages/verifier/template/parser.go

pkg/kudoctl/packages/verifier/template/parser_test.go

pkg/kudoctl/cmd/package_verify.go

pkg/kudoctl/packages/verifier/types.go

kensipe requested review from alenkacz, gerred and zen-dog as code owners November 26, 2019 14:41

ANeumann82 reviewed Nov 26, 2019

View reviewed changes

kensipe added this to the 0.9.0 milestone Nov 26, 2019

ANeumann82 approved these changes Nov 27, 2019

View reviewed changes

alenkacz reviewed Nov 27, 2019

View reviewed changes

pkg/kudoctl/packages/types.go Outdated Show resolved Hide resolved

pkg/kudoctl/cmd/verify/verify.go Outdated Show resolved Hide resolved

pkg/kudoctl/cmd/verify/verify.go Outdated Show resolved Hide resolved

zen-dog requested changes Nov 27, 2019

View reviewed changes

zen-dog reviewed Nov 27, 2019

View reviewed changes

pkg/kudoctl/packages/types.go Outdated Show resolved Hide resolved

zen-dog reviewed Nov 29, 2019

View reviewed changes

pkg/kudoctl/packages/types.go Outdated Show resolved Hide resolved

kensipe requested a review from nfnt as a code owner December 5, 2019 17:05

kensipe and others added 17 commits December 6, 2019 13:29

capturing params used in templates with tests

3fa25e9

walking deeper into nodes and adding tests

6792635

returning more structure from validation of templates and parameters

2b10a4a

changing verifier to be for package files and not just params

cd59804

param verifier complete, moving to templates

53f1885

template verifier added with implicits

029dc07

proper dereferencing of error msg

adca790

params now includes fields inside of command pipes

cd0e967

adding engine template parser with func map to verification code

676c9c4

Apply suggestions from code review

4bab98c

Co-Authored-By: Andreas Neumann <aneumann@mesosphere.com>

updates based on PR feedback

ff3fbc1

adding tests

67d673f

clean up

04e6d73

new type of param to test for

797f3eb

updated node param collection to allow for updates to templating with…

78b8430

…out breaking current verify or packaging

refactored templates to private package as identified in PR feedback

d8650e2

updates based on pr feedback

5496c82

heavy refactor to encapsulate template walking. All that is exposed i…

fc583d6

…s verifiers

zen-dog force-pushed the ken/param-valid branch from 50e5181 to fc583d6 Compare December 6, 2019 12:30

zen-dog added 2 commits December 6, 2019 13:31

Made verify work with newly added pipe-tasks

a40a0a0

additionally renamed `verify::Operator()` to `verify.PackageFiles` - term `Operator` is heavily overloaded both externally and in the code base.

Making template_verify_test green again

386b9a6

alenkacz approved these changes Dec 6, 2019

View reviewed changes

zen-dog added 2 commits December 6, 2019 14:04

Removed a few empty lines and minor style improvements

f00297b

zen-dog reviewed Dec 6, 2019

View reviewed changes

Go.go is no go! :D

11a9150

zen-dog approved these changes Dec 6, 2019

View reviewed changes

kensipe added 3 commits December 6, 2019 13:16

pr feedback update with additional dev docs

6ff5acb

adding more dev docs

23080ff

happy linter, happy life

c860d72

kensipe merged commit ef23d5a into master Dec 6, 2019

kensipe added this to Done in KUDO Global via automation Dec 6, 2019

kensipe deleted the ken/param-valid branch December 6, 2019 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Operator Parameter and Template Verification #1106

Operator Parameter and Template Verification #1106

kensipe commented Nov 26, 2019 •

edited

Loading

kensipe commented Nov 26, 2019

alenkacz left a comment

zen-dog left a comment

kensipe commented Nov 27, 2019

alenkacz left a comment

alenkacz Dec 6, 2019

kensipe Dec 6, 2019

alenkacz Dec 6, 2019

zen-dog Dec 6, 2019

zen-dog left a comment

Operator Parameter and Template Verification #1106

Operator Parameter and Template Verification #1106

Conversation

kensipe commented Nov 26, 2019 • edited Loading

kensipe commented Nov 26, 2019

alenkacz left a comment

Choose a reason for hiding this comment

zen-dog left a comment

Choose a reason for hiding this comment

kensipe commented Nov 27, 2019

alenkacz left a comment

Choose a reason for hiding this comment

alenkacz Dec 6, 2019

Choose a reason for hiding this comment

kensipe Dec 6, 2019

Choose a reason for hiding this comment

alenkacz Dec 6, 2019

Choose a reason for hiding this comment

zen-dog Dec 6, 2019

Choose a reason for hiding this comment

zen-dog left a comment

Choose a reason for hiding this comment

kensipe commented Nov 26, 2019 •

edited

Loading