roll our own strict config loading #912

BenTheElder · 2019-10-03T00:52:43Z

/hold

this is an interesting possible alternative to #911

it's less code, simpler, doesn't use unsafe, doesn't fail lints ....

BenTheElder · 2019-10-03T01:23:45Z

/hold cancel

reasonably happy with this now, might experiment with other strict parsers later... (namely: https://godoc.org/gopkg.in/yaml.v3)

aojea · 2019-10-03T05:26:58Z

What are the pros and cons of moving away from apimachinery?

BenTheElder · 2019-10-03T05:52:12Z

What are the pros and cons of moving away from apimachinery?

pros:

less code
less dependency on generated code (easier to develop, and the generators need work... especially to work well with go modules)
apimachinery has unsafe code (conversions trying to be simple to generate and fast), we can eliminate that
loading config files without the kubernetes api machinery is a perfectly normal task to do in any language without large dependencies and generate code ...
we stop leaking internal types via apimachinery's runtime type system
we're already writing most of eg the defaulting, conversions by hand anyhow. any non-trivial changes to the types requires this already.
developers do not need to learn the apimachinery libraries / generators to work on kind / contribute to the config
we're able to do things like stricter parsing with tight control over it and without pulling in super new kubernetes dependencies. go modules users are somewhat stuck on client-go v11 / kubernetes 1.14...

cons:

experienced kubernetes developers know the api machinery
you need to be thorough with eg writing conversions. this is mitigated by tests.
this diff could be smaller

The api machinery makes sense for large sets of kubernetes APIs. It's overkill for loading a config file and doing some defaulting / converting between sets of types.

DeepCopy is always purely mechanical though, so it's kinda nice to keep for those, and has a very simple API.

aojea · 2019-10-03T06:25:21Z

then, it's clear the path here 😄

neolit123 · 2019-10-05T19:41:02Z

the picture around component-config vs usage of api-machinery is not very clear, yet component-config is still leaning towards continuing to use api-machinery for config management.

the alternatives introduces by this approach are fine and would work.

my personal vote would be to continue using api-machinery, because without it kind will deviate from the plan for the ecosystem. yet in the end of the day the maintainers know best what is best for the project.

BenTheElder · 2019-10-15T00:44:39Z

this gives considerably better errors.

$ kind create cluster --config=$HOME/kind.yaml --name=aa
Creating cluster "aa" ...
ERROR: failed to create cluster: unable to decode config: yaml: unmarshal errors:
  line 12: field aa not found in type v1alpha3.Node
  line 13: field cc not found in type v1alpha3.Node

BenTheElder · 2019-10-15T00:51:38Z

my personal vote would be to continue using api-machinery, because without it kind will deviate from the plan for the ecosystem. yet in the end of the day the maintainers know best what is best for the project.

Totally understand and sympathize with that, but the ecosystem has it's own tech-debt (EG switching to yaml.v3 has been "fun" I gather and not looking super feasible soon...) that kind does not need to be held back by, and I think many of the components have different needs. kind is not a component, it's a user facing CLI tool.

kind is a good place to try things and report back, it's OK to be a little different. We can always switch back at a later date if the trade-off changes.

With a little more cleanup this:

is less code (yay!)
less dependency on code generation (code-gen = bad contributor experience)
gives better errors (main reason I'm interested)

BenTheElder · 2019-10-15T01:07:10Z

/hold
forgot to finish converting everything to have yaml tags.

BenTheElder · 2019-10-15T17:29:57Z

/test all

BenTheElder · 2019-10-15T17:34:45Z

(confirming https://status.docker.com/pages/533c6539221ae15e3f000031 impact, see #951 for temporary work around)

aojea · 2019-10-15T17:36:13Z

how is the workflow of adding a new api field or adding a new api version?

BenTheElder · 2019-10-15T17:42:13Z

how is the workflow of adding a new api field or adding a new api version?

~unchanged.
EDIT: slightly changed, you need less changes to hack/update/generated.sh and the conversion / load methods are now just normal code. otherwise you need to do almost exactly the same thing. all of the same methods need modifying etc.

amwat · 2019-10-15T18:21:08Z

/retest

k8s-ci-robot · 2019-10-15T18:31:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amwat, BenTheElder

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [BenTheElder]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

BenTheElder · 2019-10-15T20:09:41Z

/hold cancel

neolit123 · 2019-10-15T23:21:17Z

pkg/apis/config/v1alpha3/types.go


 	// Nodes contains the list of nodes defined in the `kind` Cluster
 	// If unset this will default to a single control-plane node
 	// Note that if more than one control plane is specified, an external
 	// control plane load balancer will be provisioned implicitly
-	Nodes []Node `json:"nodes,omitempty"`
+	Nodes []Node `yaml:"nodes,omitempty" json:"nodes,omitempty"`


sorry for not having a look at this PR again.

i think this dropped support for user provided JSON kind config files?
which is not very critical but diverges from the "component config" / api-machinery trend.

this also has json tags for now? (reread the diff?)

we never supported JSON config files. I don't intend to. yaml is a superset of json.

Kubernetes needs a seperate JSON parser for other reasons (because it gives better errors?). yaml.v3 also gives better errors kubernetes/kubernetes#78946

missed the extra tags in the diff.

neolit123 · 2019-10-15T23:24:02Z

pkg/internal/apis/config/encoding/load.go

+
+	// get kind & apiVersion
+	tm := typeMeta{}
+	if err := yaml.Unmarshal(raw, &tm); err != nil {


hm, i've heard that v3 enables strict by default for the Unmarshal function.
kubernetes-sigs/yaml#26 (comment)

it doesn't. we parse a config file in CI.

https://github.com/go-yaml/yaml/blob/fc94e3f7165204dd261b8d39d39a1e1773b931c9/yaml.go#L89

(I took a pass through the actual yaml.v3 code before switching)

i see, perhaps the linked commend was misleading.

also the new Node object's Decode() does not preserve strict being enabled 😞

AFAICT implementing the secret "obsolete" (v2 api) custom YamlUnmarshal method works fine though.

k8s-ci-robot requested review from amwat and neolit123 October 3, 2019 00:52

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 3, 2019

BenTheElder mentioned this pull request Oct 3, 2019

Update apimachinery and enable strict decoding #911

Closed

BenTheElder force-pushed the bespoke-config branch 2 times, most recently from 994b539 to 346ad70 Compare October 3, 2019 01:04

roll our own config loading

d0f4a5a

BenTheElder force-pushed the bespoke-config branch from 346ad70 to d0f4a5a Compare October 3, 2019 01:04

BenTheElder changed the title ~~[WIP] roll our own config loading~~ roll our own strict config loading Oct 3, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 3, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 3, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 15, 2019

BenTheElder added 2 commits October 14, 2019 18:25

switch to yaml.v3

0447dd2

deepcopy before converting

9f8ce52

BenTheElder force-pushed the bespoke-config branch from d112cef to d60c95f Compare October 15, 2019 01:26

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 15, 2019

BenTheElder force-pushed the bespoke-config branch from d60c95f to 140c6a3 Compare October 15, 2019 01:28

handle custom decode

53bd2eb

BenTheElder force-pushed the bespoke-config branch from 140c6a3 to 53bd2eb Compare October 15, 2019 01:32

amwat approved these changes Oct 15, 2019

View reviewed changes

k8s-ci-robot assigned amwat Oct 15, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 15, 2019

k8s-ci-robot merged commit f97b720 into kubernetes-sigs:master Oct 15, 2019

BenTheElder deleted the bespoke-config branch October 15, 2019 20:11

BenTheElder mentioned this pull request Oct 15, 2019

use strict config parsing #907

Closed

neolit123 reviewed Oct 15, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roll our own strict config loading #912

roll our own strict config loading #912

BenTheElder commented Oct 3, 2019

BenTheElder commented Oct 3, 2019

aojea commented Oct 3, 2019

BenTheElder commented Oct 3, 2019

aojea commented Oct 3, 2019

neolit123 commented Oct 5, 2019

BenTheElder commented Oct 15, 2019

BenTheElder commented Oct 15, 2019

BenTheElder commented Oct 15, 2019

BenTheElder commented Oct 15, 2019

BenTheElder commented Oct 15, 2019

aojea commented Oct 15, 2019

BenTheElder commented Oct 15, 2019 •

edited

Loading

amwat commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

BenTheElder commented Oct 15, 2019

neolit123 Oct 15, 2019

BenTheElder Oct 15, 2019 •

edited

Loading

BenTheElder Oct 15, 2019

neolit123 Oct 15, 2019

neolit123 Oct 15, 2019

BenTheElder Oct 15, 2019

BenTheElder Oct 15, 2019

BenTheElder Oct 15, 2019

neolit123 Oct 15, 2019

BenTheElder Oct 15, 2019

roll our own strict config loading #912

roll our own strict config loading #912

Conversation

BenTheElder commented Oct 3, 2019

BenTheElder commented Oct 3, 2019

aojea commented Oct 3, 2019

BenTheElder commented Oct 3, 2019

aojea commented Oct 3, 2019

neolit123 commented Oct 5, 2019

BenTheElder commented Oct 15, 2019

BenTheElder commented Oct 15, 2019

BenTheElder commented Oct 15, 2019

BenTheElder commented Oct 15, 2019

BenTheElder commented Oct 15, 2019

aojea commented Oct 15, 2019

BenTheElder commented Oct 15, 2019 • edited Loading

amwat commented Oct 15, 2019

k8s-ci-robot commented Oct 15, 2019

BenTheElder commented Oct 15, 2019

Choose a reason for hiding this comment

BenTheElder Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Oct 15, 2019 •

edited

Loading

BenTheElder Oct 15, 2019 •

edited

Loading