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.

Sign up for GitHub

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

[NET-11297] Purge on disable #4378

Merged
merged 11 commits into from
Oct 21, 2024
Merged

[NET-11297] Purge on disable #4378

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2cc3fe3
purge services on disable of sync catalog
jm96441n Oct 1, 2024
ca3df7c
gofumpt
jm96441n Oct 1, 2024
0ecfe35
update tests for sync catalog
jm96441n Oct 1, 2024
b667860
Added changelog
jm96441n Oct 10, 2024
0fd8e01
create separate jobs to handle deregistering services when disabling …
jm96441n Oct 10, 2024
9f491d4
Check error
jm96441n Oct 17, 2024
b1f29a1
Merge branch 'main' into purge-on-disable
jm96441n Oct 21, 2024
bde9f05
fix tests
jm96441n Oct 21, 2024
681eee1
rename field in values file, remove references to pod security policy
jm96441n Oct 21, 2024
8dd18f4
remove psp
jm96441n Oct 21, 2024
6a2b63d
added bats testing
jm96441n Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion charts/consul/templates/sync-catalog-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 (or $syncCatalogEnabled .Values.syncCatalog.purgeServicesOnDisable) }}
jm96441n marked this conversation as resolved.
Show resolved Hide resolved
{{- template "consul.reservedNamesFailer" (list .Values.syncCatalog.consulNamespaces.consulDestinationNamespace "syncCatalog.consulNamespaces.consulDestinationNamespace") }}
{{ template "consul.validateRequiredCloudSecretsExist" . }}
{{ template "consul.validateCloudSecretKeys" . }}
Expand Down Expand Up @@ -213,6 +214,7 @@ spec:
-metrics-port={{ .Values.syncCatalog.metrics.port }} \
{{- end }}
-prometheus-retention-time={{ .Values.global.metrics.agentMetricsRetentionTime }} \
-purgeServicesOnDisable={{ and (not $syncCatalogEnabled) .Values.syncCatalog.purgeServicesOnDisable }} \
livenessProbe:
httpGet:
path: /health/ready
Expand Down
4 changes: 4 additions & 0 deletions charts/consul/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

this needs a better name

Copy link
Member

Choose a reason for hiding this comment

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

Also, as we discussed before, do we need to add more fields here to allow users to control the purging in more granular ways?

Copy link
Member Author

Choose a reason for hiding this comment

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

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"

Copy link
Member Author

Choose a reason for hiding this comment

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

this still needs a better name

Copy link
Member

Choose a reason for hiding this comment

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

Just brain dumping other possibilities:

  • cleanUpServicesOnDisable
  • cleanUpOnDisable (might be my vote thus far 🤷‍♂️ )
  • deregisterServicesOnDisable
  • deregisterOnDisable

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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"


# The name of the Docker image (including any tag) for consul-k8s-control-plane
# to run the sync program.
# @type: string
Expand Down
83 changes: 16 additions & 67 deletions control-plane/subcommand/sync-catalog/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,17 @@ import (
"os"
"os/signal"
"regexp"
"strings"
"sync"
"syscall"
"time"

"github.com/armon/go-metrics/prometheus"
"github.com/cenkalti/backoff"
mapset "github.com/deckarep/golang-set"
"github.com/hashicorp/consul-server-connection-manager/discovery"
"github.com/hashicorp/consul/api"
"github.com/hashicorp/go-hclog"
"github.com/mitchellh/cli"
"github.com/prometheus/client_golang/prometheus/promhttp"
"golang.org/x/sync/errgroup"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
Expand All @@ -37,7 +34,6 @@ import (
"github.com/hashicorp/consul-k8s/control-plane/helper/controller"
"github.com/hashicorp/consul-k8s/control-plane/subcommand"
"github.com/hashicorp/consul-k8s/control-plane/subcommand/common"
metricsutil "github.com/hashicorp/consul-k8s/control-plane/subcommand/common"
"github.com/hashicorp/consul-k8s/control-plane/subcommand/flags"
)

Expand Down Expand Up @@ -67,7 +63,7 @@ type Command struct {
flagAddK8SNamespaceSuffix bool
flagLogLevel string
flagLogJSON bool
flagPurgeK8SServicesFromNode string
flagPurgeK8SServicesFromNode bool
flagFilter string

// Flags to support namespaces
Expand Down Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

👏

"Specifies if services should be purged from the Consul node. If set, all K8S services will be removed from the Consul node.")
c.flags.StringVar(&c.flagFilter, "filter", "",
"Specifies the expression used to filter the services on the Consul node that will be deregistered. "+
"The syntax for this filter is the same as the syntax used in the List Services for Node API in the Consul catalog.")
Expand Down Expand Up @@ -279,7 +275,7 @@ func (c *Command) Run(args []string) int {
}
c.ready = true

if c.flagPurgeK8SServicesFromNode != "" {
if c.flagPurgeK8SServicesFromNode {
consulClient, err := consul.NewClientFromConnMgr(consulConfig, c.connMgr)
if err != nil {
c.UI.Error(fmt.Sprintf("unable to instantiate consul client: %s", err))
Expand Down Expand Up @@ -462,64 +458,15 @@ func (c *Command) Run(args []string) int {

// remove all k8s services from Consul.
func (c *Command) removeAllK8SServicesFromConsulNode(consulClient *api.Client) error {
node, _, err := consulClient.Catalog().NodeServiceList(c.flagPurgeK8SServicesFromNode, &api.QueryOptions{Filter: c.flagFilter})
_, err := consulClient.Catalog().Deregister(&api.CatalogDeregistration{
xwa153 marked this conversation as resolved.
Show resolved Hide resolved
Node: c.flagConsulNodeName,
Partition: c.consul.Partition,
}, nil)
if err != nil {
c.UI.Error(fmt.Sprintf("unable to deregister all K8S services from Consul: %s", err))
return err
}

var firstErr error
services := node.Services
batchSize := 300
maxRetries := 2
retryDelay := 200 * time.Millisecond

// Ask for user confirmation before purging services
for {
c.UI.Info(fmt.Sprintf("Are you sure you want to delete %v K8S services from %v? (y/n): ", len(services), c.flagPurgeK8SServicesFromNode))
var input string
fmt.Scanln(&input)
if input = strings.ToLower(input); input == "y" {
break
} else if input == "n" {
return nil
} else {
c.UI.Info("Invalid input. Please enter 'y' or 'n'.")
}
}

for i := 0; i < len(services); i += batchSize {
end := i + batchSize
if end > len(services) {
end = len(services)
}

var eg errgroup.Group
for _, service := range services[i:end] {
s := service
eg.Go(func() error {
var b backoff.BackOff = backoff.NewConstantBackOff(retryDelay)
b = backoff.WithMaxRetries(b, uint64(maxRetries))
return backoff.Retry(func() error {
_, err := consulClient.Catalog().Deregister(&api.CatalogDeregistration{
Node: c.flagPurgeK8SServicesFromNode,
ServiceID: s.ID,
}, nil)
return err
}, b)
})
}
if err := eg.Wait(); err != nil {
if firstErr == nil {
c.UI.Info("Some K8S services were not deregistered from Consul")
firstErr = err
}
}
c.UI.Info(fmt.Sprintf("Processed %v K8S services from %v", end-i, c.flagPurgeK8SServicesFromNode))
}

if firstErr != nil {
return firstErr
}
c.UI.Info("All K8S services were deregistered from Consul")
return nil
}
Expand Down Expand Up @@ -553,7 +500,7 @@ func (c *Command) validateFlags() error {
// For the Consul node name to be discoverable via DNS, it must contain only
// dashes and alphanumeric characters. Length is also constrained.
// These restrictions match those defined in Consul's agent definition.
var invalidDnsRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`)
invalidDnsRe := regexp.MustCompile(`[^A-Za-z0-9\\-]+`)
const maxDNSLabelLength = 63

if invalidDnsRe.MatchString(c.flagConsulNodeName) {
Expand All @@ -570,7 +517,7 @@ func (c *Command) validateFlags() error {
}

if c.flagMetricsPort != "" {
if _, valid := metricsutil.ParseScrapePort(c.flagMetricsPort); !valid {
if _, valid := common.ParseScrapePort(c.flagMetricsPort); !valid {
return errors.New("-metrics-port must be a valid unprivileged port number")
}
}
Expand All @@ -586,7 +533,7 @@ func (c *Command) recordMetrics() (*prometheus.PrometheusSink, error) {
return &prometheus.PrometheusSink{}, err
}

var counters = [][]prometheus.CounterDefinition{
counters := [][]prometheus.CounterDefinition{
catalogtoconsul.SyncToConsulCounters,
catalogtok8s.SyncToK8sCounters,
}
Expand Down Expand Up @@ -621,8 +568,9 @@ func (c *Command) authorizeMiddleware() func(http.Handler) http.Handler {
}
}

const synopsis = "Sync Kubernetes services and Consul services."
const help = `
const (
synopsis = "Sync Kubernetes services and Consul services."
help = `
Usage: consul-k8s-control-plane sync-catalog [options]

Sync K8S pods, services, and more with the Consul service catalog.
Expand All @@ -631,3 +579,4 @@ Usage: consul-k8s-control-plane sync-catalog [options]
K8S services.

`
)
Loading
Loading