Skip to content

Commit 0561b82

Browse files
committed
Clean up for submission as provisional
1 parent e93a214 commit 0561b82

File tree

2 files changed

+107
-28
lines changed

2 files changed

+107
-28
lines changed

keps/sig-api-machinery/2144-clientgo-apply/README.md

Lines changed: 105 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,3 @@
1-
<!--
2-
- [ ] **Fill out this file as best you can.**
3-
At minimum, you should fill in the "Summary" and "Motivation" sections.
4-
These should be easy if you've preflighted the idea of the KEP with the
5-
appropriate SIG(s).
6-
- [ ] **Create a PR for this KEP.**
7-
Assign it to people in the SIG who are sponsoring this process.
8-
- [ ] **Merge early and iterate.**
9-
Avoid getting hung up on specific details and instead aim to get the goals of
10-
the KEP clarified and merged quickly. The best way to do this is to just
11-
start with the high-level sections and fill out details incrementally in
12-
subsequent PRs.
13-
-->
141
# KEP-2155: Apply for client-go's typed client
152

163
<!-- toc -->
@@ -29,7 +16,9 @@
2916
- [Alternative 2: Generated &quot;builders&quot;](#alternative-2-generated-builders)
3017
- [Comparison of alternatives](#comparison-of-alternatives)
3118
- [DeepCopy support](#deepcopy-support)
32-
- [client-gen changes](#client-gen-changes)
19+
- [Code Generator Changes](#code-generator-changes)
20+
- [Addition of applyconfiguration-gen](#addition-of-applyconfiguration-gen)
21+
- [client-gen changes](#client-gen-changes)
3322
- [Interoperability with structured and unstructured types](#interoperability-with-structured-and-unstructured-types)
3423
- [Test Plan](#test-plan)
3524
- [Fuzz-based round-trip testing](#fuzz-based-round-trip-testing)
@@ -48,7 +37,9 @@
4837
- [Implementation History](#implementation-history)
4938
- [Drawbacks](#drawbacks)
5039
- [Alternatives](#alternatives)
51-
- [Infrastructure Needed (Optional)](#infrastructure-needed-optional)
40+
- [Alternative: Use YAML directly](#alternative-use-yaml-directly)
41+
- [Alternative: Combine go structs with fieldset mask](#alternative-combine-go-structs-with-fieldset-mask)
42+
- [Alternative: Use varadic function based builders](#alternative-use-varadic-function-based-builders)
5243
<!-- /toc -->
5344

5445
## Release Signoff Checklist
@@ -91,10 +82,8 @@ Items marked with (R) are required *prior to targeting to a milestone / release*
9182

9283
## Summary
9384

94-
client-go's typed clients need a typesafe, programmatic way to construct apply
95-
configurations. `Apply` functions will be added to the typed clients in client-go
96-
and accept the declarative apply configuration via a strongly typed representation
97-
that is generated for this purpose.
85+
client-go's typed clients need a typesafe, programmatic way to make apply
86+
requests.
9887

9988
## Motivation
10089

@@ -109,8 +98,8 @@ deficiencies:
10998
Use the existing go structs to construct an apply configuration, serialize
11099
it to JSON, and pass it to `Patch`. This can cause zero valued required
111100
fields being accientally included in the apply configuration resulting
112-
in fields being accidently set to incorrect values and/or fields accidently
113-
being accidently being clamed as owned.
101+
in fields being accidentally set to incorrect values and/or fields accidentally
102+
being accidentally being clamed as owned.
114103

115104
Both sig-api-machinery and wg-api-expression agree that this enhancement is
116105
required before server side apply to be promoted to GA.
@@ -144,6 +133,10 @@ this enhancement.
144133

145134
## Proposal
146135

136+
`Apply` functions should be included in the typed clients generated for
137+
client-go and should accept the apply configuration using a strongly typed
138+
representation, which will need to be generated for this purpose.
139+
147140
### Risks and Mitigations
148141

149142
#### Poor adoption
@@ -191,7 +184,7 @@ clientset. There are a couple reasons for this:
191184
- If a client has multiple code paths where it makes apply requests to the same
192185
object, but with different field sets, they must use different field manager
193186
names. If they use the same field manager name they will cause fields to be
194-
accidently removed or disowned. This is a potential foot gun we would like to
187+
accidentally removed or disowned. This is a potential foot gun we would like to
195188
avoid.
196189

197190
- Apply requests always conflict with update requests, even if they were made by
@@ -303,7 +296,20 @@ If "structs with pointers" approach is used, the existing deepcopy-gen
303296
can be used to generate deep copy impelemntations for the generated apply
304297
configuration types.
305298

306-
### client-gen changes
299+
#### Code Generator Changes
300+
301+
hack/update-codegen.sh and hack/verify-codegen.sh will be updated to generate
302+
the apply functions and apply configuration types.
303+
304+
##### Addition of applyconfiguration-gen
305+
306+
- Add staging/src/k8s.io/code-generator/cmd/applyconfigurations-gen
307+
- Generates into staging/vendor/k8s.io/client-go/applyconfigurations/
308+
- Only generate builders for struct types reachable from the types that have the +clientgen annotation
309+
- Don't generate builders for MarshalJSON types (Quantity, IntOrString)
310+
- Don't generate builders for RawExtension or Unknown
311+
312+
##### client-gen changes
307313

308314
Since client-gen is available for use with 3rd party project, we must ensure all
309315
changes to it are backward compatible. The Apply functions will only be generated
@@ -346,7 +352,7 @@ it is correct using the existing e2e tests, expanding coverage as needed.
346352
### Graduation Criteria
347353

348354
This enhancement will graduate to GA as part of Server Side Apply. It does
349-
not make sense to graduate it independantly.
355+
not make sense to graduate it independently.
350356

351357
### Upgrade / Downgrade Strategy
352358

@@ -456,4 +462,78 @@ Major milestones might include:
456462

457463
## Alternatives
458464

459-
TODO(jpbetz): Fill this out
465+
### Alternative: Use YAML directly
466+
467+
For fields that need to be set programmatically, use templating.
468+
469+
Limitations:
470+
471+
- Not typesafe, so arguably should be part of a dynamic client only (which can already do apply)
472+
- Templating doesn't work well for some cases. E.g. a variable number of containers
473+
474+
475+
### Alternative: Combine go structs with fieldset mask
476+
477+
User directly provides the go structs as they exist today and also provides a fieldset "mask" that enumerates all the fields included in the apply configuration. A custom serializer would be required to combine the object and the mask together.
478+
479+
```
480+
obj := &appsv1.Deployment{ …}
481+
mask := TODO
482+
tombstoned := TODO: is another fieldset required for tombstones?
483+
Apply(..., obj, mask, tombstoned, …)
484+
```
485+
486+
Limitations:
487+
488+
- Error prone. No way to ensure that the mask and the object have the same set of fields directly set by the caller (e.g. if the user directly sets a field to its zero value, there is no way to warn them that they forgot to add it to the mask)
489+
- Even if there was some typesafe way to define masks and tombstones, constructing them is going to add to the work required by client-go apply users.
490+
491+
### Alternative: Use varadic function based builders
492+
493+
```
494+
appsv1apply.Deployment(
495+
metav1apply.ObjectMeta(
496+
appsv1apply.Name("nginx-deployment"),
497+
),
498+
appsv1apply.DeploymentSpec(
499+
appsv1apply.Replicas(0),
500+
appsv1apply.PodTemplate(
501+
appsv1apply.PodSpec(
502+
appsv1apply.TombStoned("hostname"),
503+
appsv1apply.PodContainer(
504+
appsv1apply.Name("nginx"),
505+
appsv1apply.Image("nginx:1.14.2"),
506+
),
507+
appsv1apply.TombStoned(
508+
appsv1apply.PodContainer(
509+
appsv1apply.Name("sidecar"),
510+
),
511+
),
512+
),
513+
),
514+
),
515+
)
516+
517+
```
518+
519+
This could be implemented by generating varadic functions, e.g.:
520+
521+
```
522+
func Deployment(fields ...DeploymentField{}) {
523+
var object map[string]interface{} // This is the underlying data structure
524+
for field := range fields {
525+
switch f := fields.(type) {
526+
case NameField:
527+
object["name"] = f.value
528+
// other types
529+
}
530+
}
531+
}
532+
533+
func Name(value string) DeploymentField { … }
534+
```
535+
536+
Limitations:
537+
538+
- Lots of identifier collision issues to deal with. For example, we can't have multiple "Name" functions in the same package. This can probably be mitigated by either generating more unique names or by allowing a common field like Name, which is typically a string, to be shared across multiple structs that have name fields.
539+

keps/sig-api-machinery/2144-clientgo-apply/kep.yaml

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ kep-number: 2144
33
authors:
44
- "@jpbetz"
55
owning-sig: sig-api-machinery
6-
status: implementable
6+
status: provisional
77
creation-date: 2020-11-16
88
reviewers:
99
- "@apelisse"
@@ -12,8 +12,7 @@ approvers:
1212
- "@deads2k"
1313
- "@lavalamp"
1414
prr-approvers:
15-
- TBD
16-
- "@bob.doe"
15+
- "@deads2k"
1716
see-also:
1817
- "/keps/sig-api-machinery/0006-apply.md"
1918

0 commit comments

Comments
 (0)