You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
@@ -7,13 +7,92 @@ This approach could be used to fix the issues with `kubectl apply` simply by hav
7
7
8
8
### APIs
9
9
10
-
Add a go struct tag/openAPI value telling clients to replace the entire struct when doing a *PATCH*. For the *inline* fields, we have 2 options:
10
+
Add a go struct tag/openAPI value telling clients to replace the entire struct when doing a *PATCH*. For the *inlined* fields, we have 2 options:
11
11
12
-
1) Move the tag to the parent struct's. E.g. add a tag to `Volume` instead of `VolumeSource`. This will limit the flexibility to add more fields in the future.
12
+
1) Move the tag to the parent struct's. e.g. add a tag to `Volume` instead of `VolumeSource`.
13
+
This will limit the flexibility to add more fields in the parent struct that already has an inlined union.
14
+
And we need to examine all the impacted APIs to check if it is safe to replace the entire parent struct. e.g. if some controller is updating some fields.
15
+
16
+
Examples of how to add go struct tag for option 1:
17
+
```go
18
+
typePodSpecstruct {
19
+
...
20
+
// patchStrategyForListEntry:"replace" is an additional struct tag indicating we replace the entire entry in the list, if they have the same merge key.
*Backwards / Forward Compatibility Behavior using option 1*
37
+
38
+
We reuse `patchStrategy:"replace"` if possible. Otherwise, we introduce a new struct tag, `patchStrategyForListEntry:"replace"`.
39
+
40
+
If we are reusing `patchStrategy:"replace"`, the new patch has no new fields. So an old API server still understands the new patch and is able to apply the patch.
41
+
42
+
The new patch for non-inlined union will looks like:
43
+
```yaml
44
+
union-key:
45
+
$patch: replace
46
+
key1: v1
47
+
key2: v2
48
+
```
49
+
50
+
The new patch for inlined union will looks like:
51
+
```yaml
52
+
parent-key:
53
+
$patch: replace
54
+
inlined-union-key1: v1
55
+
inlined-union-key2: v2
56
+
non-union-key1: v3
57
+
non-union-key2: v4
58
+
```
59
+
60
+
We cannot reuse `patchStrategy:"replace"` for a list of unions, because `patchStrategy:"replace"` means replacing the entire list.
61
+
62
+
E.g. `Volume` is the parent of `VolumeSource`. And `Volumes`, which is a list of `Volume`, has the metadata `patchStrategy:"merge"`.
63
+
We cannot use `patchStrategy:"replace"` in this case.
In this case, we introduce struct tag`patchStrategyForListEntry:"replace"`.
74
+
75
+
The new patch for list of inlined union will looks like:
76
+
```yaml
77
+
podSpec:
78
+
volumes:
79
+
- name: vol1
80
+
$listEntryStrategy: replace
81
+
emptyDir:
82
+
...
83
+
- name: vol2
84
+
$listEntryStrategy: replace
85
+
hostPath:
86
+
...
87
+
```
88
+
89
+
Impacted APIs are listed in the [Appendix](#appendix).
13
90
14
91
2) Add some logic when lookup the metadata in go struct to find the tags for inline fields.
92
+
The limitation of this approach is that the metadata associated with the inlined APIs will not be reflected in the OpenAPI schema,
93
+
because OpenAPI schemas flatten inlined structs.
15
94
16
-
Examples of how to add go struct tag respect option 2:
95
+
Examples of how to add go struct tag for option 2:
17
96
```go
18
97
type Volume struct {
19
98
Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
@@ -31,22 +110,41 @@ type DeploymentSpec struct {
31
110
32
111
No required change.
33
112
34
-
##Strategic Merge Patch
113
+
### Open API
35
114
36
-
Add logic to support replacing the entire union fields when there is a `replace` directive. The logic will be similar to the logic of merge strategy for lists.
37
-
Add lookup logic to support lookup metadata of inline fields.
115
+
We would need to add extensions to the openapi spec we publish. This is something we already need to do for the `patchStrategy` and `mergeKey` struct tags.
38
116
39
-
The change in Strategic Merge Patch package will affect the behavior of how kubectl creating a patch and how API server apply a patch.
117
+
### Strategic Merge Patch
40
118
41
-
### Open API
119
+
The logic of `$patch: replace` already exists.
42
120
43
-
We would need to add extensions to the openapi spec we publish. This is something we already need to do for the `patchStrategy` and `mergeKey` struct tags.
121
+
We need to add additional logic to support:
122
+
- Generate the correct patch for a list of maps according to the struct tag.
123
+
- Replace the entire union when `$listEntryStrategy: replace` appears in the patch.
124
+
125
+
### Docs
126
+
127
+
Update `API-conventions-md` to include:
128
+
```
129
+
we should avoid adding new inlined unions in the future.
130
+
```
44
131
45
132
# Alternatives Considered
46
133
134
+
The proposals below are not mutually exclusive with the proposal above, and could be added at some point in the future.
135
+
47
136
# 1. Add Union Support to the API Server
48
137
49
-
The limitation is that it will be impossible or very hard to make it work for inline union types.
138
+
The limitation is that it will be very hard to make it work for inline union types. E.g.
139
+
```yaml
140
+
parent-key:
141
+
$patch: replace
142
+
inlined-union-key1: v1
143
+
inlined-union-key2: v2
144
+
non-union-key1: v3
145
+
non-union-key2: v4
146
+
```
147
+
If we want to use an approach similar to option 1 in section [APIs](#apis) of the proposal above, using `union:"oneof"` is not sufficient. Because the validator doesn't have enough metadata to tell which fields belongs to the inlined union.
50
148
51
149
## Proposed Changes
52
150
@@ -264,17 +362,54 @@ Apply the new config by `kubectl apply -f`. The operation should succeed now, be
- [`PodSignature`](https://github.com/kubernetes/kubernetes/blob/master/pkg/api/v1/types.go#L2973): It has only one field, but the comment says "Exactly one field should be set". Maybe we will add more in the future?
It is inlined. Besides `PersistentVolumeSource`, its parent [PersistentVolumeSpec](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/api/v1/types.go#L442) has the following fields:
It is inlined. Besides `Handler`, its parent struct [`Probe`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/api/v1/types.go#L1297) also has the following fields:
It has only one field, but the comment says "Exactly one field should be set". Maybe we will add more in the future? It is NOT inlined.
273
396
In `pkg/authorization/types.go`:
274
-
- [`SubjectAccessReviewSpec`](https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/authorization/types.go#L108)Exactly one of ResourceAttributes and NonResourceAttributes must be set. But other non-union fields also exist in this API.
- [`IngressRuleValue`](https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/extensions/v1beta1/types.go#L733): It says "exactly one of the following must be set". But it has only one field.
It says "exactly one of the following must be set". But it has only one field.
411
+
It is inlined. Its parent [`IngressRule`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/apis/extensions/v1beta1/types.go#L848) also has the following fields:
0 commit comments