-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
H4HIP: Server Side Apply support #312
base: main
Are you sure you want to change the base?
Conversation
95e4e59
to
3c11723
Compare
hips/hip-00XX.md
Outdated
|
||
#### Special | ||
|
||
- `--dry-run=client` doesn't work with SSA enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattfarina points out that this will make it difficult for user's to preview changes
hips/hip-00XX.md
Outdated
|
||
#### Conflicts and forcing | ||
|
||
It is possible when Helm upgrades a chart, that there will be a field conflict with another field manager. In this case, Helm should report the error to the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: verify we can abort atomically across multiple objects (Ian Z points this out)
@gjenkins8 I think this HIP as written would apply to the release secret itself, too. Am I correct with this understanding? We're running into the issue that the release secret is > 1MB in https://github.com/community-tooling/charts because of the |
e1e4d73
to
9048d85
Compare
Signed-off-by: George Jenkins <[email protected]>
028f45f
to
82b28b6
Compare
Signed-off-by: George Jenkins <[email protected]>
Signed-off-by: George Jenkins <[email protected]>
Signed-off-by: George Jenkins <[email protected]>
Allowing Helm to update objects which have been modified by other processes. | ||
Kubernetes now offers a similar server-side process that has several advantages over the client-side apply (CSA) methods that Helm and `kubectl` (for example) have traditionally utilized. | ||
|
||
## Motivation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps also include that client-side validation is now largely unsupported (need to read up on the details):
hips/hip-00XX.md
Outdated
|
||
- `--dry-run=client` won't work with SSA enabled | ||
- If the user attempts to use SSA with a cluster which does not support it (unlikely: SSA [went GA](https://kubernetes.io/blog/2021/08/06/server-side-apply-ga/) in Kubernetes v1.22), Helm should error | ||
- Client-side API object validation isn't applicable when SSA enabled (`helm install|upgrade --disable-openapi-validation`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we still do this using the SSA with a dry-run?
### Implementation | ||
|
||
Helm will send object's manifests to the Kubernetes API server via SSA patches. Similar to `kubectl apply`. | ||
Utilizing the field manager name `"helm"`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be configurable? I'm thinking of a case where "Sam" and "Alex" might be managing the same namespace and want to be sure that they're not stepping on each other's toes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This HIP is only going as far as Helm supporting a single field manager. For the goal of switching from client-side to server-side apply.
I do think in the future, Helm supporting multiple field managers in the same chart with similar control on conflict resolution, should be done. But that ends up being a can of worm (stuff to design) I think we should defer for the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should mention, see Rejected Ideas for some discussion
- Arrays will be merged | ||
- Removal of unset fields | ||
- Default values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which side of the "vs" has which behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handling of removing previously set values in the 3-way merge by setting a null was recently fixed. Do we know how/if SSA will affect that behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which side of the "vs" has which behavior?
oops, right. these are benefits of SSA. Fixed language.
The handling of removing previously set values in the 3-way merge by setting a null was recently fixed. Do we know how/if SSA will affect that behavior?
This was the input values merge, and dropping fields set to null from input values. This (I guess is kinda the same) but for object manifests applied to the Kubernetes API
hips/hip-00XX.md
Outdated
|
||
For 1., chart-operators may opt-out out of SSA. | ||
For 2., the user can control by ensuring SSA is consistently used within their environment. | ||
For 3., the it is generally considered a limitation of Helm not updating custom resources, for which SSA is expected to be an improvement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For 3., the it is generally considered a limitation of Helm not updating custom resources, for which SSA is expected to be an improvement. | |
For 3., the it is generally considered a limitation of Helm not updating custom resource definitions, for which SSA is expected to be an improvement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
language here is correct. CRDs are the objects which create new Kubernetes API resources. Custom resources are the objects created from those resource.
I'll amend to "custom resource objects" to be clear
hips/hip-00XX.md
Outdated
For 3., the it is generally considered a limitation of Helm not updating custom resources, for which SSA is expected to be an improvement. | ||
For 4., the `--force-conflicts` flag exists. | ||
|
||
Additionally as noted, SSA isn't compatible with very old Kubernetes versions (Kubernetes v1.17 and prior) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said 1.22, earlier. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GA in 1.22
Iirc, SSA will work via beta release from 1.17. At this point, 1.17 vs 1.22 is probably water under the bridge now. Will improve this slightly.
hips/hip-00XX.md
Outdated
Helm won't support ownership transfer of fields. | ||
Similar to the `kubectl` example: <https://kubernetes.io/docs/reference/using-api/server-side-apply/#transferring-ownership>. | ||
|
||
Dissimilar to `kubectl`, is Helm's distinguishes between chart developers and chart operators. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dissimilar to `kubectl`, is Helm's distinguishes between chart developers and chart operators. | |
Dissimilar to `kubectl`, is Helm distinguishes between chart developers and chart operators. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh, terrible grammar here
hips/hip-00XX.md
Outdated
### Chart field manager support | ||
|
||
This HIP defers including field manager support in charts. | ||
Helm could enabling apply objects with a distinct field manager names. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helm could enabling apply objects with a distinct field manager names. | |
Helm could enable applying objects with a distinct field manager names. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh, thanks. fixed
Signed-off-by: George Jenkins <[email protected]>
Signed-off-by: George Jenkins <[email protected]>
HIP for Helm to support SSA (chart object/resources)