feat(appliance): Merge unstructured objects from helm serialization and appliance cfg defaults#64021
Conversation
| } | ||
|
|
||
| // Merge the objects using strategic merge patch | ||
| mergedUnstructured, err := strategicpatch.StrategicMergeMapPatch(existingUnstructured, jsonUnstructured.Object, existingObj) |
There was a problem hiding this comment.
Oh nice, I'd forgotten about strategicpatch! This is probably useful to us. I think this is on the right track, but there might be an even simpler way. To overcommunicate and re-state the problem: we want to selectively overwrite fields on the frontend service+ingress. Specifically, we want to overwrite fields that the user has overriden in the configmap, whether first install or upgrade (e.g. ingress annotations). But we don't want to overwrite fields that originate from helm (whether overridden with helm values on first install or not), just because they are in the default object that the reconciler cooks up.
This code does something we want in the sense that preserves helm values, a few lines up on line 360 by using them as the basis of the json patch. But it never overwrites anything with admin-specified configmap values.
Idea: what if we make the FE service and ingress reconcile functions exceptional, in the sense that they don't construct k8s objects from scratch if there is an existing object in the cluster. We fetch that object and use that as the base, then cook up a json patch based solely on admin configmap overrides. So, under default config, it'd be {} (or null, whatever the library wants), and we overwrite nothing. We take the result of that and post it back to the cluster as an update. Specifically, we can map the configmap element to a skeletal k8s object subset, containing only the fields that need to be overwritten - that'd be the json patch.
Much of that idea relates to stuff outside of this MergeK8sObjects function, but there are parts in there relating to patching in the configmap overrides. Interestingly - this means we never need the helm values, surprisingly, and can close my draft in the helm chart. Hope it makes some sense! Alternatives welcome as always.
There was a problem hiding this comment.
Sweet yeah! I've gone ahead and implemented it that way, as we discussed this morning :)
|
Ughh, looks like it's failing the other test from adopting helm provisioned resources. I think the test case is actually wrong now? Since we do expect it to clobber the existing resource with cfg objects that are not default. |
craigfurman
left a comment
There was a problem hiding this comment.
I agree, the adoption golden fixture was now outdated. We do indeed want to preserve most of the changes from the original object during adoption.
We do still want the prometheus port on this service though. I took the liberty of pushing a commit that does this, and the golden fixture diffs now look sensible to me 🙏
Feel free to edit, squash, or even chop off my commit and dismiss this review if we want to make further changes, but I think we're good to go 🚀
P.S. the git remote contains 2 branches with this name, varying only by the case of "rel". It's playing havoc with mac's case-insensitive filesystem, since branches are represented as files 😅. I couldn't figure out how to square my local repo to use git normally, so I went on detached head and pushed like this: git push origin HEAD:al/rel-218/appliance-frontend-ingress
Once we merge this and github deletes the uppercase version PR'ed here, let's delete the other one so that git fetch doesn't keep trying to pull the duplicate branch
|
Deleted lowercase branch as well |
…nd appliance cfg defaults (#64021) <!-- PR description tips: https://www.notion.so/sourcegraph/Write-a-good-pull-request-description-610a7fd3e613496eb76f450db5a49b6e --> This PR implements [this comment](https://linear.app/sourcegraph/issue/REL-218/ingressservice-consistency-for-helm-deployment#comment-6e1b88b5) from [REL-218](https://linear.app/sourcegraph/issue/REL-218/ingressservice-consistency-for-helm-deployment). ~~Currently, not included in `reconcileFrontendService` or `reconcileFronteendIngress`. It's still a **work in progress**, requesting review to make sure I'm on the right path, the test doesn't currently pass.~~ Got the tests to pass locally, and added them to `reconcileFronteendService` and `reconcileFronteendIngress`, however, I'd like to deploy this locally with helm to ease my concern in it breaking. ~~This PR alone isn't enough to close REL-218 either, we'll need something like @craigfurman's [PR in deploy-sourcegraph-helm](sourcegraph/deploy-sourcegraph-helm#509) as well~~ This PR no longer requires that helm values be serialized, since we adopt the object that exists in k8s rather than reading serialized helm values. ## Test plan <!-- REQUIRED; info at https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles --> Unit tests included ## Changelog <!-- OPTIONAL; info at https://www.notion.so/sourcegraph/Writing-a-changelog-entry-dd997f411d524caabf0d8d38a24a878c --> - feat/appliance: Include existing objects when constructing Frontend's `Service` & `Ingress` --------- Co-authored-by: Craig Furman <craig.furman@sourcegraph.com>
This PR implements this comment from REL-218.
Currently, not included inreconcileFrontendServiceorreconcileFronteendIngress. It's still a work in progress, requesting review to make sure I'm on the right path, the test doesn't currently pass.Got the tests to pass locally, and added them to
reconcileFronteendServiceandreconcileFronteendIngress, however, I'd like to deploy this locally with helm to ease my concern in it breaking.This PR alone isn't enough to close REL-218 either, we'll need something like @craigfurman's PR in deploy-sourcegraph-helm as wellThis PR no longer requires that helm values be serialized, since we adopt the object that exists in k8s rather than reading serialized helm values.
Test plan
Unit tests included
Changelog
Service&Ingress