-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(ImageUpdater): Provide option in Argo CD CR to enable/disable image updater #1897
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
Conversation
fb52a2a to
b745b20
Compare
b745b20 to
65d7cc8
Compare
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.
Thanks @dkarpele for working on this. Looks good overall. I have left some comments.
-
We should also bundle the ImageUpdater CRD into operator, similar to how we handle others here.
-
In the PR description, it mentions pulling from the crd branch in go.mod:
go get github.com/argoproj-labs/argocd-image-updater@crdIs there a specific reason for using the crd branch instead of main or a release branch/tag?
controllers/argocd/image_updater.go
Outdated
| { | ||
| NamePattern: "app-name-pattern", | ||
| Images: []imageupdaterv1alpha1.ImageConfig{ | ||
| { | ||
| Alias: "image-alias", | ||
| ImageName: "image-name", | ||
| }, |
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.
Are these placeholder values or actual ones? I haven’t looked into the image-updater project yet, so just confirming.
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.
Yes, these are placeholders. NamePattern, Alias, ImageName are all mandatory, so we should give them some names.
controllers/argocd/image_updater.go
Outdated
|
|
||
| func (r *ReconcileArgoCD) reconcileImageUpdaterConfigurationCR(cr *argoproj.ArgoCD) error { | ||
|
|
||
| defaultImageUpdaterConfigurationCR := &imageupdaterv1alpha1.ImageUpdater{ |
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.
Just trying to understand the use case for having a default ImageUpdater CR?
Is it meant to serve as a catch-all configuration for all image update operations, or is the expectation that users should modify this default ImageUpdater CR to suit their needs, rather than creating new ones?
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.
Users can modify this CR or create a new one.
My initial idea was to create a basic ImageUpdater CR that can help user to start working with image updater. @chengfang do you think we should have default ImageUpdater CR or not?
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.
It should be straightforward for users to create their ImageUpdater CR, and likely in their application git repo. I imagine most users will probably not actually use or modify this default CR for their purpose. I feel we can remove it to avoid the runtime overhead.
controllers/argocd/imageUpdater.go
Outdated
| podSpec.Containers = []corev1.Container{{ | ||
| Command: []string{"/manager"}, | ||
| Args: []string{"run"}, | ||
| Image: argoutil.CombineImageTag(DefaultImageUpdaterImage, DefaultImageUpdaterTag), |
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.
We should make this configurable via env var so that downstream images can be set through the CSV in gitops-operator.
For ref:
argocd-operator/common/keys.go
Line 176 in ef71979
| ArgoCDDexImageEnvName = "ARGOCD_DEX_IMAGE" |
| Command: []string{"/manager"}, | ||
| Args: []string{"run"}, | ||
| Image: argoutil.CombineImageTag(DefaultImageUpdaterImage, DefaultImageUpdaterTag), | ||
| ImagePullPolicy: corev1.PullAlways, |
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.
There are ongoing efforts to make the pull policy configurable, just wanted to highlight that.
The reason is that at the moment crd development of image updater goes in crd branch (not master). When we make the crd version "main" we will update the dependencies here to use |
ba72d19 to
082fc04
Compare
|
Thanks for the review @svghadi |
082fc04 to
788b201
Compare
| get-image-updater-crd: ## Download Image Updater CRD. | ||
| @echo "downloading image updater crd" | ||
| @curl -sSLo config/crd/bases/argocd-image-updater.argoproj.io_imageupdaters.yaml https://raw.githubusercontent.com/argoproj-labs/argocd-image-updater/crd/config/crd/bases/argocd-image-updater.argoproj.io_imageupdaters.yaml | ||
|
|
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.
do we still need this target now that the ImageUpdater crd is bundled in?
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.
I was thinking it could be useful in case someone wants to update argocd-image-updater.argoproj.io_imageupdaters.yaml. The original of this file is located in Image Updater repo and to reflect the future changes in this crd the target get-image-updater-crd: can be used.
26f9a2c to
d1c16bf
Compare
I don't plan any more changes in this PR. |
d1c16bf to
2c90cf0
Compare
controllers/argocd/util.go
Outdated
| // Add new predicate to delete ImageUpdater Resources. The predicate watches the Argo CD CR for changes to the `.spec.ImageUpdater.Enabled` | ||
| // field. When a change is detected that results in image updater being disabled, we trigger deletion of image updater resources |
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.
I was thinking we could let the main reconcile handle the deletion when the component is disabled.
Predicates are generally meant to be lightweight filters that decide whether an event should be queued or skipped, rather than performing side-effect operations like updates or deletions. We already have a few places in the operator doing this, but it might be something we can gradually refactor later for consistency. Thoughts?
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.
Yes, that makes sense. Initially I created the same predicate as deleteNotificationsPred.
The main problem with deletion is that the order of resource deletion (deployment first) is different from resource creation (deployment last). To handle this I
- removed predicate for ImageUpdater
- created 2 functions:
reconcileImageUpdaterControllerEnabledandreconcileImageUpdaterControllerDisabled.
Each reconcile function (reconcileImageUpdaterDeployment, reconcileImageUpdaterClusterRoleBinding etc.) handles deletion itself.
2c90cf0 to
d186084
Compare
…age updater Signed-off-by: Denis Karpelevich <[email protected]>
d186084 to
85735e3
Compare
svghadi
left a comment
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.
LGTM. Thanks
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
Image Updater: Provide option in Argo CD CR to enable/disable image updater
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #?
Image Updater CRD: https://github.com/argoproj-labs/argocd-image-updater/blob/crd/api/v1alpha1/imageupdater_types.go
The required resources are described in https://github.com/argoproj-labs/argocd-image-updater/tree/crd/config
How to test changes / Special notes to the reviewer:
Tests and documentation will be created ASAP.
To test it manually you can
kubectl apply -f examples/argocd-image-updater.yamlResources will be reconciled.