-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: secret copy #741 #948
Open
docandrew
wants to merge
4
commits into
main
Choose a base branch
from
secretcopy
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
# Lint Codespell configurations | ||
[codespell] | ||
skip = .codespellrc,.git,node_modules,build,dist,*.zst,CHANGELOG.md,.playwright,.terraform | ||
ignore-words-list = NotIn,AKS,LICENS,aks | ||
ignore-words-list = NotIn,AKS,LICENS,aks,afterAll | ||
enable-colors = | ||
check-hidden = |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,175 @@ | ||
/** | ||
* Copyright 2024 Defense Unicorns | ||
* SPDX-License-Identifier: AGPL-3.0-or-later OR LicenseRef-Defense-Unicorns-Commercial | ||
*/ | ||
|
||
import { afterAll, beforeAll, describe, expect, it } from "@jest/globals"; | ||
import { K8s, kind } from "pepr"; | ||
|
||
const failIfReached = () => expect(true).toBe(false); | ||
|
||
describe("test secret copy", () => { | ||
const sourceSecret = { | ||
metadata: { | ||
name: "source-secret", | ||
namespace: "source-namespace", | ||
}, | ||
data: { key: "TESTCASE" }, | ||
}; | ||
|
||
beforeAll(async () => { | ||
// Setup test namespaces | ||
await K8s(kind.Namespace).Apply({ | ||
metadata: { name: "source-namespace" }, | ||
}); | ||
await K8s(kind.Namespace).Apply({ | ||
metadata: { name: "destination-namespace" }, | ||
}); | ||
await K8s(kind.Namespace).Apply({ | ||
metadata: { name: "destination-namespace2" }, | ||
}); | ||
await K8s(kind.Namespace).Apply({ | ||
metadata: { name: "destination-namespace3" }, | ||
}); | ||
|
||
// Create source secret | ||
await K8s(kind.Secret).Apply(sourceSecret); | ||
}); | ||
|
||
afterAll(async () => { | ||
// Cleanup test namespaces | ||
await K8s(kind.Namespace).Delete("source-namespace"); | ||
await K8s(kind.Namespace).Delete("destination-namespace"); | ||
await K8s(kind.Namespace).Delete("destination-namespace2"); | ||
// await K8s(kind.Namespace).Delete("destination-namespace3"); | ||
}); | ||
|
||
it("should copy a secret with the secrets.uds.dev/copy label", async () => { | ||
// Apply destination secret | ||
const destinationSecret = { | ||
metadata: { | ||
name: "destination-secret", | ||
namespace: "destination-namespace", | ||
labels: { "secrets.uds.dev/copy": "true" }, | ||
annotations: { | ||
"secrets.uds.dev/fromNamespace": "source-namespace", | ||
"secrets.uds.dev/fromName": "source-secret", | ||
}, | ||
}, | ||
}; | ||
|
||
// Check if destination secret has the same data as the source secret | ||
const destSecret = await K8s(kind.Secret).Apply(destinationSecret); | ||
expect(destSecret.data).toEqual({ key: "VEVTVENBU0U=" }); // base64 encoded "TESTCASE" | ||
|
||
// Confirm that label has changed from copy to copied | ||
expect(destSecret.metadata?.labels).toEqual({ | ||
"secrets.uds.dev/copied": "true", | ||
}); | ||
}); | ||
|
||
it("should not copy a secret without the secrets.uds.dev/copy=true label", async () => { | ||
// Apply destination secret | ||
const destinationSecret1 = { | ||
metadata: { | ||
name: "destination-secret-tc2a", | ||
namespace: "destination-namespace2", | ||
labels: { "secrets.uds.dev/copy": "false" }, | ||
annotations: { | ||
"secrets.uds.dev/fromNamespace": "source-namespace", | ||
"secrets.uds.dev/fromName": "source-secret", | ||
}, | ||
}, | ||
}; | ||
|
||
const destinationSecret2 = { | ||
metadata: { | ||
name: "destination-secret-tc2b", | ||
namespace: "destination-namespace2", | ||
labels: { asdf: "true" }, | ||
annotations: { | ||
"secrets.uds.dev/fromNamespace": "source-namespace", | ||
"secrets.uds.dev/fromName": "source-secret", | ||
}, | ||
}, | ||
}; | ||
|
||
const destSecret1 = await K8s(kind.Secret).Apply(destinationSecret1); | ||
const destSecret2 = await K8s(kind.Secret).Apply(destinationSecret2); | ||
|
||
// Confirm destination secrets are created "as is" | ||
expect(destSecret1.data).toEqual(undefined); | ||
expect(destSecret1.metadata?.labels).toEqual({ | ||
"secrets.uds.dev/copy": "false", | ||
}); | ||
|
||
expect(destSecret2.data).toEqual(undefined); | ||
expect(destSecret2.metadata?.labels).toEqual({ asdf: "true" }); | ||
}); | ||
|
||
it("should error when copy label is present but missing annotations", async () => { | ||
const destinationSecret = { | ||
metadata: { | ||
name: "destination-secret-tc3", | ||
namespace: "destination-namespace3", | ||
labels: { "secrets.uds.dev/copy": "true" }, | ||
}, | ||
}; | ||
|
||
const expected = (e: Error) => { | ||
expect(e).toMatchObject({ | ||
ok: false, | ||
data: { | ||
message: expect.stringContaining("denied the request"), | ||
}, | ||
}); | ||
}; | ||
|
||
return K8s(kind.Secret).Apply(destinationSecret).then(failIfReached).catch(expected); | ||
}); | ||
|
||
it("should error when missing source secret and onMissingSource=Deny", async () => { | ||
const destinationSecret = { | ||
metadata: { | ||
name: "destination-secret", | ||
namespace: "destination-namespace", | ||
labels: { "secrets.uds.dev/copy": "true" }, | ||
annotations: { | ||
"secrets.uds.dev/fromNamespace": "missing-namespace", | ||
"secrets.uds.dev/fromName": "missing-secret", | ||
"secrets.uds.dev/onMissingSource": "Deny", | ||
}, | ||
}, | ||
}; | ||
|
||
const expected = (e: Error) => { | ||
expect(e).toMatchObject({ | ||
ok: false, | ||
data: { | ||
message: expect.stringContaining("denied the request"), | ||
}, | ||
}); | ||
}; | ||
|
||
return K8s(kind.Secret).Apply(destinationSecret).then(failIfReached).catch(expected); | ||
}); | ||
|
||
it("should create empty secret when missing source secret and onMissingSource=LeaveEmpty", async () => { | ||
const destinationSecret = { | ||
metadata: { | ||
name: "destination-secret-tc4a", | ||
namespace: "destination-namespace", | ||
labels: { "secrets.uds.dev/copy": "true" }, | ||
annotations: { | ||
"secrets.uds.dev/fromNamespace": "source-namespace", | ||
"secrets.uds.dev/fromName": "missing-secret", | ||
"secrets.uds.dev/onMissingSource": "LeaveEmpty", | ||
}, | ||
}, | ||
}; | ||
|
||
const destSecret = await K8s(kind.Secret).Apply(destinationSecret); | ||
|
||
expect(destSecret.data).toEqual(undefined); | ||
}); | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Would be curious to get your thoughts on this design approach of mutating the "destination" secret. Initially I thought this was a mistake because I didn't read far enough in the PR 😅.
My inclination would be that it makes more sense to
Watch
(orReconcile
) secrets that have a label likeuds.dev/copyMe
(i.e. the "source" secret). That way you can guarantee you know when the secrets update at the source and ensure they are kept up to date in the destination. This current pattern would only watch changes/creations on the destination side, meaning you might miss changes if the source updates I think? There may be a few more design implementations to think through here, but overall I think it would make a bit more sense? (a few of those design decisions: do you need anoverwrite: true/false
annotation, do you support mutliple destinations and how, etc)I think this might also resolve most of @bburky's security concerns since the only secret being read would be one that has been explicitly labelled to be copied. The only caveat would be on that
overwrite
piece - we would want to be careful implementing this to not overprivilege that label to write to any secret without some safeguards (maybe we only allow overwriting if pepr is already controlling the secret, use ownership metadata, etc).The one obvious downside I could see to this is that you would need to have access to label/annotate the source secret which might be an issue with some upstream charts/operators? If that is a major sticking point we could move to a custom resource as @bburky recommended. If we want to go that route I think I'd lean slightly towards a new custom resource rather than using the
Package
CR.