[NET-11297] Purge on disable #4378
Conversation
charts/consul/values.yaml
Outdated
| @@ -2090,6 +2090,10 @@ syncCatalog: | |||
| # global.enabled. | |||
| enabled: false | |||
|
|
|||
| # True if you want to deregister all services in this cluster from consul that have been registered by the catalog sync when the `enabled` flag is set to false. | |||
| # @type: boolean | |||
| purgeServicesOnDisable: false | |||
There was a problem hiding this comment.
this needs a better name
There was a problem hiding this comment.
Also, as we discussed before, do we need to add more fields here to allow users to control the purging in more granular ways?
There was a problem hiding this comment.
we don't, it's not really a requirement and since we're doing this on a disable of the catalog-sync we don't want to potentially leave behind services that might no longer exist. The main requirement is "remove services registered by catalog sync when catalog sync is disabled"
There was a problem hiding this comment.
this still needs a better name
There was a problem hiding this comment.
Just brain dumping other possibilities:
cleanUpServicesOnDisablecleanUpOnDisable(might be my vote thus far 🤷♂️ )deregisterServicesOnDisablederegisterOnDisable
It feels like the name needs to be some version of ..OnDisable because putting disable as a prefix makes it seem like you are disabling something additional.
There was a problem hiding this comment.
yeah I don't like the prefix disable I was thinking maybe cleanupNodeOnRemoval since the Disable can be a bit of misnomer because this runs on uninstall of consul and when you disable syncCatalog whereas the removal can mean "removal of sync catalog" or "removal of consul"
charts/consul/values.yaml
Outdated
| @@ -2090,6 +2090,10 @@ syncCatalog: | |||
| # global.enabled. | |||
| enabled: false | |||
|
|
|||
| # True if you want to deregister all services in this cluster from consul that have been registered by the catalog sync when the `enabled` flag is set to false. | |||
| # @type: boolean | |||
| purgeServicesOnDisable: false | |||
There was a problem hiding this comment.
Also, as we discussed before, do we need to add more fields here to allow users to control the purging in more granular ways?
| @@ -156,8 +152,8 @@ func (c *Command) init() { | |||
| "\"debug\", \"info\", \"warn\", and \"error\".") | |||
| c.flags.BoolVar(&c.flagLogJSON, "log-json", false, | |||
| "Enable or disable JSON output format for logging.") | |||
| c.flags.StringVar(&c.flagPurgeK8SServicesFromNode, "purge-k8s-services-from-node", "", | |||
| "Specifies the name of the Consul node for which to deregister synced Kubernetes services.") | |||
| c.flags.BoolVar(&c.flagPurgeK8SServicesFromNode, "purge-k8s-services-from-node", false, | |||
| @@ -1,4 +1,5 @@ | |||
| {{- if (or (and (ne (.Values.syncCatalog.enabled | toString) "-") .Values.syncCatalog.enabled) (and (eq (.Values.syncCatalog.enabled | toString) "-") .Values.global.enabled)) }} | |||
| {{- $syncCatalogEnabled := (or (and (ne (.Values.syncCatalog.enabled | toString) "-") .Values.syncCatalog.enabled) (and (eq (.Values.syncCatalog.enabled | toString) "-") .Values.global.enabled)) }} | |||
| {{- if $syncCatalogEnabled }} | |||
charts/consul/templates/sync-catalog-cleanup-podsecuritypolicy.yaml
Outdated
Show resolved
Hide resolved
charts/consul/templates/sync-catalog-cleanup-on-upgrade-job.yaml
Outdated
Show resolved
Hide resolved
charts/consul/templates/sync-catalog-cleanup-on-uninstall-job.yaml
Outdated
Show resolved
Hide resolved
|
bats tests added with commit 6a2b63d |
🙏 Thank you!
Changes proposed in this PR
How I've tested this PR
How I expect reviewers to test this PR
Checklist