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

Added ReadOnlyRootFileSystem to the Argo CD components #1659

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

anandf
Copy link
Collaborator

@anandf anandf commented Feb 8, 2025

What type of PR is this?

What does this PR do / why we need it:
Upstream Argo CD has all components running with readOnlyRootFileSystem set to true. For security reasons this needs to be enabled even for the components that are created by the operator.

Application Controller:
https://github.com/argoproj/argo-cd/blob/d183d9c614d05979f1327a554942a9e656f1c2ec/manifests/base/application-controller-deployment/argocd-application-controller-deployment.yaml#L263
Note: Stateful set based application controller does not have this flag set. So need to validate if enabling it will cause any breaking change
https://github.com/argoproj/argo-cd/blob/master/manifests/base/application-controller-deployment/argocd-application-controller-statefulset.yaml
AppSet Controller: https://github.com/argoproj/argo-cd/blob/d183d9c614d05979f1327a554942a9e656f1c2ec/manifests/base/applicationset-controller/argocd-applicationset-controller-deployment.yaml#L202
Notification Controller :
https://github.com/argoproj/argo-cd/blob/d183d9c614d05979f1327a554942a9e656f1c2ec/manifests/base/notification/argocd-notifications-controller-deployment.yaml#L92
Dex:
https://github.com/argoproj/argo-cd/blob/d183d9c614d05979f1327a554942a9e656f1c2ec/manifests/base/dex/argocd-dex-server-deployment.yaml#L34
Redis:
https://github.com/argoproj/argo-cd/blob/d183d9c614d05979f1327a554942a9e656f1c2ec/manifests/base/redis/argocd-redis-deployment.yaml#L31
https://github.com/argoproj/argo-cd/blob/d183d9c614d05979f1327a554942a9e656f1c2ec/manifests/base/redis/argocd-redis-deployment.yaml#L60
RepoServer:
https://github.com/argoproj/argo-cd/blob/d183d9c614d05979f1327a554942a9e656f1c2ec/manifests/base/repo-server/argocd-repo-server-deployment.yaml#L261
https://github.com/argoproj/argo-cd/blob/d183d9c614d05979f1327a554942a9e656f1c2ec/manifests/base/repo-server/argocd-repo-server-deployment.yaml#L295
Server:
https://github.com/argoproj/argo-cd/blob/d183d9c614d05979f1327a554942a9e656f1c2ec/manifests/base/server/argocd-server-deployment.yaml#L340

Have you updated the necessary documentation?

  • Documentation update is required by this PR.
  • Documentation has been updated.

Which issue(s) this PR fixes:

Fixes #?

How to test changes / Special notes to the reviewer:

@anandf anandf requested a review from svghadi February 8, 2025 17:46
@svghadi
Copy link
Collaborator

svghadi commented Feb 10, 2025

Note: Stateful set based application controller does not have this flag set. So need to validate if enabling it will cause any breaking change
https://github.com/argoproj/argo-cd/blob/master/manifests/base/application-controller-deployment/argocd-application-controller-statefulset.yaml

The readonlyfs flag is set for statefulset in install.yaml from github release page. I think it will work, we just need to add some volumes which operator doesn't setup for application-controller.

Also, for argocd-server we are missing some mounts

@anandf anandf force-pushed the fix_read_only_root_fs branch from 6ed2830 to 95f2e3c Compare February 18, 2025 11:49
@anandf anandf requested a review from jgwest February 18, 2025 13:42
Copy link
Member

@jgwest jgwest left a comment

Choose a reason for hiding this comment

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

I have reviewed and the changes LGTM! Nothing jumped out at me as incorrect, and I verified that the volumes/paths we are adding are consistent with what is upstream.

If you haven't already, you may want to run the upstream Argo CD E2E tests (in remote mode[1]) against the argocd-operator, to ensure that we've not regressed any upstream features.

Since this is potentially a risky change, I recommmend NOT backporting this to 1.15.x, and only supporting it in 1.16.x. (Or, at least, not backporting it YET, until it soaks)

[1] https://github.com/argoproj/argo-cd/blob/master/test/remote/README.md or https://gitlab.cee.redhat.com/gitops/gitops-components-automated-testing

@anandf
Copy link
Collaborator Author

anandf commented Feb 19, 2025

/retest

@anandf anandf force-pushed the fix_read_only_root_fs branch from ed13a71 to 853710e Compare February 19, 2025 06:43
@jgwest
Copy link
Member

jgwest commented Feb 19, 2025

The latest ApplicationSet updates on PR re: removing unused repo server TLS volume LGTM!

@anandf
Copy link
Collaborator Author

anandf commented Feb 21, 2025

I have reviewed and the changes LGTM! Nothing jumped out at me as incorrect, and I verified that the volumes/paths we are adding are consistent with what is upstream.

If you haven't already, you may want to run the upstream Argo CD E2E tests (in remote mode[1]) against the argocd-operator, to ensure that we've not regressed any upstream features.

Since this is potentially a risky change, I recommmend NOT backporting this to 1.15.x, and only supporting it in 1.16.x. (Or, at least, not backporting it YET, until it soaks)

[1] https://github.com/argoproj/argo-cd/blob/master/test/remote/README.md or https://gitlab.cee.redhat.com/gitops/gitops-components-automated-testing

There is no plan to backport this PR to 1.15.x. It will be only for 1.16.x and future versions.

Copy link
Collaborator

@svghadi svghadi left a comment

Choose a reason for hiding this comment

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

Found some issues during testing. I feel we should push this change for 1.17. There could be other issues that crop up after integrating the change with gitops-operator.

Copy link
Collaborator

@svghadi svghadi left a comment

Choose a reason for hiding this comment

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

Ran basic upgrade test. Looks good. Thanks @anandf.

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.

3 participants