feat: Application resource watching #43
Conversation
pkg/app_watcher/appwatcher.go
Outdated
| // TODO | ||
| // have any of the Source repoURLs changed? | ||
| log.Trace().Str("key", key).Msg("appwatcher: onApplicationUpdated") |
There was a problem hiding this comment.
at this point we'll have to remove the old app (in all the various maps), then add the new one back in
There was a problem hiding this comment.
yeah that's a smart way to handle it instead of doing an upsert
pkg/app_watcher/appwatcher.go
Outdated
| log.Warn().Err(err).Msg("appwatcher: could not get key for deleted application") | ||
| } | ||
|
|
||
| log.Trace().Str("key", key).Msg("appwatcher: onApplicationDeleted") |
There was a problem hiding this comment.
here we'll have to remove the app from the app directory
cmd/controller_cmd.go
Outdated
| @@ -69,6 +71,7 @@ func init() { | |||
| flags.Bool("show-debug-info", false, "Set to true to print debug info to the footer of MR comments (KUBECHECKS_SHOW_DEBUG_INFO).") | |||
| flags.Bool("enable-conftest", false, "Set to true to enable conftest policy checking of manifests (KUBECHECKS_ENABLE_CONFTEST).") | |||
| flags.String("label-filter", "", "(Optional) If set, The label that must be set on an MR (as \"kubechecks:<value>\") for kubechecks to process the merge request webhook (KUBECHECKS_LABEL_FILTER).") | |||
| flags.String("argocd-namespace", "argocd", "The namespace to watch for Application resources") | |||
There was a problem hiding this comment.
With the new Argocd releases, applications can be in different namespaces. I think it's worth using a cluster role to watch for all apps
|
Temporary image deleted. |
|
Picking this up |
pkg/server/server.go
Outdated
| } else { | ||
| s.cfg.VcsToArgoMap = argoMap | ||
| if s.appWatcher != nil { | ||
| go s.appWatcher.Run(context.Background(), 1) |
There was a problem hiding this comment.
I think we want this to be ctx, not a new context, so it gets canceled when the app gets canceled
| } | ||
| } | ||
|
|
||
| if !isGitRepo(app.Spec.Source.RepoURL) { |
There was a problem hiding this comment.
might want to check app.Spec.Source != nil. not sure if it would happen in real life, but why worry?
| key, err := cache.MetaNamespaceKeyFunc(obj) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("appwatcher: could not get key for added application") | ||
| } | ||
| log.Debug().Str("key", key).Msg("appwatcher: onApplicationAdded") |
There was a problem hiding this comment.
do we use key for anything other than logging?
There was a problem hiding this comment.
Not really. Just for logging
pkg/app_watcher/app_watcher.go
Outdated
| // onAdd is the function executed when the informer notifies the | ||
| // presence of a new Application in the namespace | ||
| func (ctrl *ApplicationWatcher) onApplicationAdded(obj interface{}) { | ||
| if !canProcessApp(obj) { |
There was a problem hiding this comment.
each time we use this, we end up casting obj to *appv1alpha1.Application, which this function does too. it'd probably clean things up if you changed canProcessApp to return (*appv1alpha1.Application, bool). This would also let you reimplement cache.MetaNamespaceKeyFunc to just be return fmt.Sprintf("%s/%s", app.Metadata.Namespace, app.Metadata.Name) and avoid the possible error altogether.
| newApp := new.(*appv1alpha1.Application) | ||
|
|
||
| // We want to update when any of Source or Sources parameters has changed | ||
| if !reflect.DeepEqual(oldApp.Spec.Source, newApp.Spec.Source) || !reflect.DeepEqual(oldApp.Spec.Sources, newApp.Spec.Sources) { |
There was a problem hiding this comment.
UpdateApp isn't a particularly expensive operation, to avoid any possible issues with DeepEqual we could always call UpdateApp whenever a change is made.
There was a problem hiding this comment.
The problem here is onApplicationUpdated gets called more often than I realized. It's called when any element of the app is modified, this could be .Spec, .metadata or .Status. .Status is modified on every Argocd refresh, hence why were restricting the change to just .Spec.Source. DeepEqual has a predictable behavior with Objects 😀😀, so hopefully, nothing goes south.
pkg/server/server.go
Outdated
| func NewServer(cfg *config.ServerConfig) *Server { | ||
| return &Server{cfg: cfg} | ||
| var appWatcher *app_watcher.ApplicationWatcher | ||
| if viper.GetBool("monitor-all-applications") { | ||
| argoMap, err := config.BuildAppsMap(context.TODO()) |
There was a problem hiding this comment.
we should probably pass ctx from the parent context here as well
MeNsaaH
changed the title
No description provided.