refactor: server side apply over create / update #3075

AustinAbro321 · 2024-10-07T15:30:49Z

Description

There are several situations where we create, check if the resource already existed, and if it does then we update. It is simpler in this situation to just use server side apply

Related Issue

Fixes #2626

Checklist before merging

Test, docs, adr added or updated as needed
Contributor Guide Steps followed

Signed-off-by: Austin Abro <[email protected]>

netlify · 2024-10-07T15:31:08Z

✅ Deploy Preview for zarf-docs ready!

Name	Link
🔨 Latest commit	`e6d25d3`
🔍 Latest deploy log	https://app.netlify.com/sites/zarf-docs/deploys/672513aae416a7000719b35c
😎 Deploy Preview	https://deploy-preview-3075--zarf-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: Austin Abro <[email protected]>

codecov · 2024-10-17T21:36:00Z

Codecov Report

Attention: Patch coverage is 77.89474% with 42 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/pkg/cluster/zarf.go	0.00%	19 Missing ⚠️
src/pkg/packager/remove.go	0.00%	9 Missing ⚠️
src/internal/packager/helm/post-render.go	0.00%	8 Missing ⚠️
src/pkg/packager/deploy.go	0.00%	4 Missing ⚠️
src/pkg/cluster/state.go	85.71%	2 Missing ⚠️

Files with missing lines	Coverage Δ
src/pkg/cluster/cluster.go	`0.00% <ø> (ø)`
src/pkg/cluster/injector.go	`65.49% <100.00%> (-3.83%)`	⬇️
src/pkg/cluster/namespace.go	`16.66% <100.00%> (-22.47%)`	⬇️
src/pkg/cluster/secrets.go	`63.06% <100.00%> (-3.61%)`	⬇️
src/pkg/cluster/state.go	`60.52% <85.71%> (-0.66%)`	⬇️
src/pkg/packager/deploy.go	`5.43% <0.00%> (+0.10%)`	⬆️
src/internal/packager/helm/post-render.go	`12.85% <0.00%> (+1.56%)`	⬆️
src/pkg/packager/remove.go	`0.00% <0.00%> (ø)`
src/pkg/cluster/zarf.go	`14.83% <0.00%> (+2.46%)`	⬆️

Signed-off-by: Austin Abro <[email protected]>

AustinAbro321 · 2024-10-18T20:31:10Z

src/pkg/cluster/testdata/expected-injection-pod.json

expanded this so it'd be easier to read / edit. The only differences is that creationTimestamp and status no longer show up in the json (because the apply objects use pointers instead of values)

AustinAbro321 · 2024-10-18T20:31:51Z

src/pkg/cluster/testdata/expected-injection-service.json

Expanded for clarity, but content is the same

src/internal/packager/helm/post-render.go

Signed-off-by: Austin Abro <[email protected]>

phillebaba

Small comment. Additionally have we looked into if it is possible to set a global field manager?

src/pkg/cluster/state.go

AustinAbro321 · 2024-10-25T14:22:04Z

@phillebaba As far as I can tell it's not possible to set a global field manager. I thought about making a function like cluster.DefaultApplyOpts(), lmk if you'd prefer that

Signed-off-by: Austin Abro <[email protected]>

phillebaba · 2024-11-01T16:24:24Z

For now just make a constant called something like FieldManagerName so that we can track where it is being used.

src/pkg/cluster/secrets_test.go

phillebaba · 2024-11-01T16:38:26Z

src/internal/packager/helm/post-render.go

-		// TODO: Refactor as error is not checked instead of checking for not found error.
-		currentRegistrySecret, _ := c.Clientset.CoreV1().Secrets(name).Get(ctx, config.ZarfImagePullSecretName, metav1.GetOptions{})
-		sameSecretData := maps.EqualFunc(currentRegistrySecret.Data, validRegistrySecret.Data, func(v1, v2 []byte) bool { return bytes.Equal(v1, v2) })
-		if currentRegistrySecret.Name != config.ZarfImagePullSecretName || !sameSecretData {


What is the effect of removing this logic?

The current code only updates the secret if the name or the data is different. Maybe it was there to preserve labels and annotations? I'm not sure, but that's no longer a worry with apply at least.

Additionally, the current code updates the git server secret if the registry secret has a different name or different data, this seems like it was a bug with indentation, but I'm not 100% sure. I don't see any reason for that logic.

Still, I should have made a unit test to verify the behavior, I will do that now.

Actually, I am realizing the existing logic is flawed. I don't think there's much value in testing it. I could separate the new logic and write a test for it, but since it's just two apply calls, I'd pretty much just be testing that the fake client works.

Additionally, after evaluating further, there is no reason to dog sled errors here. We always want both of these secrets created

Signed-off-by: Austin Abro <[email protected]>

AustinAbro321 · 2024-11-01T17:50:31Z

src/pkg/cluster/secrets.go

@@ -138,11 +117,8 @@ func (c *Cluster) UpdateZarfManagedImageSecrets(ctx context.Context, state *type
 		if err != nil {
 			return err
 		}
-		if maps.EqualFunc(currentRegistrySecret.Data, newRegistrySecret.Data, func(v1, v2 []byte) bool { return bytes.Equal(v1, v2) }) {


Given that apply operations do not cost much, even if the data is the same, IMO it's better to simplify the code and let the k8s server figure out what to change / keep the same.

changing create/updates to use ssa

466e27a

Signed-off-by: Austin Abro <[email protected]>

AustinAbro321 added 8 commits October 9, 2024 20:13

WIP, more to do but tests passing

3e2fd88

Signed-off-by: Austin Abro <[email protected]>

merge

b978890

Signed-off-by: Austin Abro <[email protected]>

tests passing

bdb756d

Signed-off-by: Austin Abro <[email protected]>

patch-> apply

9d1a35d

Signed-off-by: Austin Abro <[email protected]>

remove patches

9f757e9

Signed-off-by: Austin Abro <[email protected]>

tests passing

0219569

Signed-off-by: Austin Abro <[email protected]>

merge

0b17664

Signed-off-by: Austin Abro <[email protected]>

fix debug

21d8c04

Signed-off-by: Austin Abro <[email protected]>

AustinAbro321 added 12 commits October 17, 2024 22:24

health checks

4516b5f

Signed-off-by: Austin Abro <[email protected]>

pointer

905711d

Signed-off-by: Austin Abro <[email protected]>

using correct svc again

71eb153

Signed-off-by: Austin Abro <[email protected]>

ssa in zarf

7c38c13

Signed-off-by: Austin Abro <[email protected]>

add field manager

00868f4

Signed-off-by: Austin Abro <[email protected]>

adopt existing resources

b121ded

Signed-off-by: Austin Abro <[email protected]>

comment

e83f6e6

Signed-off-by: Austin Abro <[email protected]>

delete comment

570b244

Signed-off-by: Austin Abro <[email protected]>

add comment

5736f56

Signed-off-by: Austin Abro <[email protected]>

make injection pod and service easier to read

ca48559

Signed-off-by: Austin Abro <[email protected]>

injector

52643f7

Signed-off-by: Austin Abro <[email protected]>

remove redundant test

72f07c9

Signed-off-by: Austin Abro <[email protected]>

AustinAbro321 commented Oct 18, 2024

View reviewed changes

src/pkg/cluster/testdata/expected-injection-service.json Outdated

Copy link

Contributor Author

AustinAbro321 Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanded for clarity, but content is the same

Merge branch 'main' into try-out-ssa

9a898dc

AustinAbro321 commented Oct 21, 2024

View reviewed changes

src/internal/packager/helm/post-render.go Outdated Show resolved Hide resolved

AustinAbro321 added 3 commits October 21, 2024 14:13

fix post render logic

5cae3e5

Signed-off-by: Austin Abro <[email protected]>

fix post render logic

16ed156

Signed-off-by: Austin Abro <[email protected]>

better error

f1d1886

Signed-off-by: Austin Abro <[email protected]>

AustinAbro321 and others added 8 commits October 23, 2024 11:12

add namespace and api version

9f1d13d

Signed-off-by: Austin Abro <[email protected]>

move namespaces back to create / update

593c317

Signed-off-by: Austin Abro <[email protected]>

create->apply

7543833

Signed-off-by: Austin Abro <[email protected]>

newline

e825432

Signed-off-by: Austin Abro <[email protected]>

consistent creation

aa8c22b

Signed-off-by: Austin Abro <[email protected]>

verbiage

dc8bc71

Signed-off-by: Austin Abro <[email protected]>

Merge branch 'main' into try-out-ssa

dd5f752

Merge branch 'try-out-ssa' of github.com:zarf-dev/zarf into try-out-ssa

f5e30a8

AustinAbro321 marked this pull request as ready for review October 24, 2024 17:07

AustinAbro321 requested review from a team as code owners October 24, 2024 17:07

AustinAbro321 added the ready-for-review label Oct 24, 2024

AustinAbro321 changed the title ~~refactor: change create or update if not exists operations to use server side apply~~ refactor: server side apply over create / update Oct 24, 2024

phillebaba reviewed Oct 25, 2024

View reviewed changes

src/pkg/cluster/state.go Show resolved Hide resolved

AustinAbro321 and others added 2 commits October 29, 2024 14:13

Merge branch 'main' into try-out-ssa

94376a5

merge

ad93bcb

Signed-off-by: Austin Abro <[email protected]>

phillebaba reviewed Nov 1, 2024

View reviewed changes

src/pkg/cluster/secrets_test.go Outdated Show resolved Hide resolved

phillebaba reviewed Nov 1, 2024

View reviewed changes

AustinAbro321 and others added 5 commits November 1, 2024 16:40

field manager name

a00784d

Signed-off-by: Austin Abro <[email protected]>

no nil needed

c15887e

Signed-off-by: Austin Abro <[email protected]>

remove maps equal logic

e1ff23f

Signed-off-by: Austin Abro <[email protected]>

newline

ac86e82

Signed-off-by: Austin Abro <[email protected]>

Merge branch 'main' into try-out-ssa

e6d25d3

AustinAbro321 commented Nov 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: server side apply over create / update #3075

refactor: server side apply over create / update #3075

AustinAbro321 commented Oct 7, 2024

netlify bot commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

AustinAbro321 Oct 18, 2024

AustinAbro321 Oct 18, 2024

phillebaba left a comment

AustinAbro321 commented Oct 25, 2024

phillebaba commented Nov 1, 2024

phillebaba Nov 1, 2024

AustinAbro321 Nov 1, 2024

AustinAbro321 Nov 1, 2024

AustinAbro321 Nov 1, 2024

refactor: server side apply over create / update #3075

Are you sure you want to change the base?

refactor: server side apply over create / update #3075

Conversation

AustinAbro321 commented Oct 7, 2024

Description

Related Issue

Checklist before merging

netlify bot commented Oct 7, 2024 • edited Loading

✅ Deploy Preview for zarf-docs ready!

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

AustinAbro321 Oct 18, 2024

Choose a reason for hiding this comment

AustinAbro321 Oct 18, 2024

Choose a reason for hiding this comment

phillebaba left a comment

Choose a reason for hiding this comment

AustinAbro321 commented Oct 25, 2024

phillebaba commented Nov 1, 2024

phillebaba Nov 1, 2024

Choose a reason for hiding this comment

AustinAbro321 Nov 1, 2024

Choose a reason for hiding this comment

AustinAbro321 Nov 1, 2024

Choose a reason for hiding this comment

AustinAbro321 Nov 1, 2024

Choose a reason for hiding this comment

netlify bot commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading