Skip to content
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: Add ability to hide certain annotations on secret resources #577

Merged
merged 7 commits into from
Oct 29, 2024

Conversation

svghadi
Copy link
Contributor

@svghadi svghadi commented May 14, 2024

Related to argoproj/argo-cd#15693.

This PR implements core logic from argoproj/argo-cd#hide-annotations.md proposal to hide annotations on secret resources.

This change will be integrated with Argo CD via argoproj/argo-cd#18216.

Integration results:

mask-annotations.mov

Copy link

sonarcloud bot commented May 14, 2024

Quality Gate Passed Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
3.4% Duplication on New Code

See analysis details on SonarCloud

@pasha-codefresh
Copy link
Member

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5] 3
🧪 Relevant tests Yes
🔒 Security concerns No
⚡ Key issues to review Possible Bug:
The function HideSecretData now accepts an optional hideAnnots parameter which is used to specify which annotations should be hidden. However, there is no check to ensure that the annotations specified actually exist in the object, which could lead to unnecessary processing or errors if non-existent annotations are specified.
Performance Concern:
The method of hiding annotations involves iterating over each annotation for each object (target, live, orig) which could be inefficient, especially with a large number of annotations or secrets. Consider optimizing this process.

pkg/diff/diff.go Outdated Show resolved Hide resolved
@svghadi
Copy link
Contributor Author

svghadi commented Jul 23, 2024

Hi @pasha-codefresh, sorry for the nudge 👉 😅. Did you get a chance to review it again?

@pasha-codefresh
Copy link
Member

@svghadi sorry, had urgent work to do, i will review it tomorrow

@isaccavalcante
Copy link

Hey @pasha-codefresh, were you able to review it again by any chance? thanks in advance.

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
pkg/diff/diff.go Outdated Show resolved Hide resolved
pkg/diff/diff.go Outdated Show resolved Hide resolved
pkg/diff/diff.go Outdated
@@ -969,10 +969,10 @@ func CreateTwoWayMergePatch(orig, new, dataStruct interface{}) ([]byte, bool, er
return patch, string(patch) != "{}", nil
}

// HideSecretData replaces secret data values in specified target, live secrets and in last applied configuration of live secret with stars. Also preserves differences between
// HideSecretData replaces secret data & optional annotations values in specified target, live secrets and in last applied configuration of live secret with stars. Also preserves differences between
Copy link
Member

Choose a reason for hiding this comment

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

I kinda object to Hide as a verb, this appears to be closer to Mask than Hide.

That appears to be a preexisting condition, but if I inherited this code, I'd want to change it.

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
pkg/diff/diff_test.go Show resolved Hide resolved
pkg/diff/diff.go Outdated Show resolved Hide resolved
Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
pkg/diff/diff.go Outdated Show resolved Hide resolved
pkg/diff/diff.go Show resolved Hide resolved
pkg/diff/diff.go Show resolved Hide resolved
Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
Copy link

sonarcloud bot commented Oct 23, 2024

Copy link
Member

@agaudreault agaudreault left a comment

Choose a reason for hiding this comment

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

LGTM

@pasha-codefresh
Copy link
Member

Taking one more day for manual testing, but overall LGTM to me

Copy link
Member

@pasha-codefresh pasha-codefresh left a comment

Choose a reason for hiding this comment

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

LGTM, thank you a lot

@pasha-codefresh pasha-codefresh merged commit 9ab0b2e into argoproj:master Oct 29, 2024
3 checks passed
@svghadi svghadi deleted the hide-annotations branch October 29, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants