From 416da0ba20b2b1824a21e514ac01c0f9fe18d6a5 Mon Sep 17 00:00:00 2001 From: Taylor Jackle Spriggs <74561858+tjs-intel@users.noreply.github.com> Date: Fri, 11 Feb 2022 15:01:35 -0500 Subject: [PATCH] Add and remove exclude-from-external-load-balancers label (#582) * add and remove exclude-from-external-load-balancers label * move dry run after checking the label's value * update helm and helm README * wording nit --- .../aws-node-termination-handler/README.md | 1 + .../templates/daemonset.linux.yaml | 2 + .../templates/daemonset.windows.yaml | 2 + .../templates/deployment.yaml | 2 + .../aws-node-termination-handler/values.yaml | 3 + pkg/config/config.go | 6 ++ pkg/node/node.go | 78 +++++++++++++++++-- 7 files changed, 89 insertions(+), 5 deletions(-) diff --git a/config/helm/aws-node-termination-handler/README.md b/config/helm/aws-node-termination-handler/README.md index 3ec77ed1..43f7d488 100644 --- a/config/helm/aws-node-termination-handler/README.md +++ b/config/helm/aws-node-termination-handler/README.md @@ -76,6 +76,7 @@ The configuration in this table applies to all AWS Node Termination Handler mode | `dryRun` | If `true`, only log if a node would be drained. | `false` | | `cordonOnly` | If `true`, nodes will be cordoned but not drained when an interruption event occurs. | `false` | | `taintNode` | If `true`, nodes will be tainted when an interruption event occurs. Currently used taint keys are `aws-node-termination-handler/scheduled-maintenance`, `aws-node-termination-handler/spot-itn`, `aws-node-termination-handler/asg-lifecycle-termination` and `aws-node-termination-handler/rebalance-recommendation`. | `false` | +| `excludeFromLoadBalancers` | If `true`, nodes will be marked for exclusion from load balancers before they are cordoned. This applies the `node.kubernetes.io/exclude-from-external-load-balancers` label to enable the ServiceNodeExclusion feature gate. The label will not be modified or removed for nodes that already have it. | `false` | | `deleteLocalData` | If `true`, continue even if there are pods using local data that will be deleted when the node is drained. | `true` | | `ignoreDaemonSets` | If `true`, skip terminating daemon set managed pods. | `true` | | `podTerminationGracePeriod` | The time in seconds given to each pod to terminate gracefully. If negative, the default value specified in the pod will be used, which defaults to 30 seconds if not specified for the pod. | `-1` | diff --git a/config/helm/aws-node-termination-handler/templates/daemonset.linux.yaml b/config/helm/aws-node-termination-handler/templates/daemonset.linux.yaml index c30184c2..893c3b80 100644 --- a/config/helm/aws-node-termination-handler/templates/daemonset.linux.yaml +++ b/config/helm/aws-node-termination-handler/templates/daemonset.linux.yaml @@ -97,6 +97,8 @@ spec: value: {{ .Values.cordonOnly | quote }} - name: TAINT_NODE value: {{ .Values.taintNode | quote }} + - name: EXCLUDE_FROM_LOAD_BALANCERS + value: {{ .Values.excludeFromLoadBalancers | quote }} - name: DELETE_LOCAL_DATA value: {{ .Values.deleteLocalData | quote }} - name: IGNORE_DAEMON_SETS diff --git a/config/helm/aws-node-termination-handler/templates/daemonset.windows.yaml b/config/helm/aws-node-termination-handler/templates/daemonset.windows.yaml index 67b96063..815b86f9 100644 --- a/config/helm/aws-node-termination-handler/templates/daemonset.windows.yaml +++ b/config/helm/aws-node-termination-handler/templates/daemonset.windows.yaml @@ -97,6 +97,8 @@ spec: value: {{ .Values.cordonOnly | quote }} - name: TAINT_NODE value: {{ .Values.taintNode | quote }} + - name: EXCLUDE_FROM_LOAD_BALANCERS + value: {{ .Values.excludeFromLoadBalancers | quote }} - name: DELETE_LOCAL_DATA value: {{ .Values.deleteLocalData | quote }} - name: IGNORE_DAEMON_SETS diff --git a/config/helm/aws-node-termination-handler/templates/deployment.yaml b/config/helm/aws-node-termination-handler/templates/deployment.yaml index b21e242a..76ec1052 100644 --- a/config/helm/aws-node-termination-handler/templates/deployment.yaml +++ b/config/helm/aws-node-termination-handler/templates/deployment.yaml @@ -94,6 +94,8 @@ spec: value: {{ .Values.cordonOnly | quote }} - name: TAINT_NODE value: {{ .Values.taintNode | quote }} + - name: EXCLUDE_FROM_LOAD_BALANCERS + value: {{ .Values.excludeFromLoadBalancers | quote }} - name: DELETE_LOCAL_DATA value: {{ .Values.deleteLocalData | quote }} - name: IGNORE_DAEMON_SETS diff --git a/config/helm/aws-node-termination-handler/values.yaml b/config/helm/aws-node-termination-handler/values.yaml index 26536306..88491d83 100644 --- a/config/helm/aws-node-termination-handler/values.yaml +++ b/config/helm/aws-node-termination-handler/values.yaml @@ -81,6 +81,9 @@ cordonOnly: false # Taint node upon spot interruption termination notice. taintNode: false +# Exclude node from load balancer before cordoning via the ServiceNodeExclusion feature gate. +excludeFromLoadBalancers: false + # deleteLocalData tells kubectl to continue even if there are pods using # emptyDir (local data that will be deleted when the node is drained). deleteLocalData: true diff --git a/pkg/config/config.go b/pkg/config/config.go index 6d40ab7d..102e7282 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -69,6 +69,7 @@ const ( taintNode = "TAINT_NODE" taintEffectDefault = "NoSchedule" taintEffect = "TAINT_EFFECT" + excludeFromLoadBalancers = "EXCLUDE_FROM_LOAD_BALANCERS" jsonLoggingConfigKey = "JSON_LOGGING" jsonLoggingDefault = false logLevelConfigKey = "LOG_LEVEL" @@ -126,6 +127,7 @@ type Config struct { CordonOnly bool TaintNode bool TaintEffect string + ExcludeFromLoadBalancers bool JsonLogging bool LogLevel string UptimeFromFile string @@ -179,6 +181,7 @@ func ParseCliArgs() (config Config, err error) { flag.BoolVar(&config.CordonOnly, "cordon-only", getBoolEnv(cordonOnly, false), "If true, nodes will be cordoned but not drained when an interruption event occurs.") flag.BoolVar(&config.TaintNode, "taint-node", getBoolEnv(taintNode, false), "If true, nodes will be tainted when an interruption event occurs.") flag.StringVar(&config.TaintEffect, "taint-effect", getEnv(taintEffect, taintEffectDefault), "Sets the effect when a node is tainted.") + flag.BoolVar(&config.ExcludeFromLoadBalancers, "exclude-from-load-balancers", getBoolEnv(excludeFromLoadBalancers, false), "If true, nodes will be marked for exclusion from load balancers when an interruption event occurs.") flag.BoolVar(&config.JsonLogging, "json-logging", getBoolEnv(jsonLoggingConfigKey, jsonLoggingDefault), "If true, use JSON-formatted logs instead of human readable logs.") flag.StringVar(&config.LogLevel, "log-level", getEnv(logLevelConfigKey, logLevelDefault), "Sets the log level (INFO, DEBUG, or ERROR)") flag.StringVar(&config.UptimeFromFile, "uptime-from-file", getEnv(uptimeFromFileConfigKey, uptimeFromFileDefault), "If specified, read system uptime from the file path (useful for testing).") @@ -254,6 +257,7 @@ func (c Config) PrintJsonConfigArgs() { Bool("cordon_only", c.CordonOnly). Bool("taint_node", c.TaintNode). Str("taint_effect", c.TaintEffect). + Bool("exclude_from_load_balancers", c.ExcludeFromLoadBalancers). Bool("json_logging", c.JsonLogging). Str("log_level", c.LogLevel). Str("webhook_proxy", c.WebhookProxy). @@ -298,6 +302,7 @@ func (c Config) PrintHumanConfigArgs() { "\tcordon-only: %t,\n"+ "\ttaint-node: %t,\n"+ "\ttaint-effect: %s,\n"+ + "\texclude-from-load-balancers: %t,\n"+ "\tjson-logging: %t,\n"+ "\tlog-level: %s,\n"+ "\twebhook-proxy: %s,\n"+ @@ -333,6 +338,7 @@ func (c Config) PrintHumanConfigArgs() { c.CordonOnly, c.TaintNode, c.TaintEffect, + c.ExcludeFromLoadBalancers, c.JsonLogging, c.LogLevel, c.WebhookProxy, diff --git a/pkg/node/node.go b/pkg/node/node.go index 718cb7cb..a93516ae 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -44,6 +44,11 @@ const ( ActionLabelTimeKey = "aws-node-termination-handler/action-time" // EventIDLabelKey is a k8s label key whose value is the drainable event id EventIDLabelKey = "aws-node-termination-handler/event-id" + // Apply this label to enable the ServiceNodeExclusion feature gate for excluding nodes from load balancers + ExcludeFromLoadBalancersLabelKey = "node.kubernetes.io/exclude-from-external-load-balancers" + // The value associated with this label is irrelevant for enabling the feature gate + // By defining a unique value it is possible to check if the label was applied by us before removing it + ExcludeFromLoadBalancersLabelValue = "aws-node-termination-handler" ) const ( @@ -95,7 +100,11 @@ func (n Node) CordonAndDrain(nodeName string, reason string) error { log.Info().Str("node_name", nodeName).Str("reason", reason).Msg("Node would have been cordoned and drained, but dry-run flag was set.") return nil } - err := n.Cordon(nodeName, reason) + err := n.MaybeMarkForExclusionFromLoadBalancers(nodeName) + if err != nil { + return err + } + err = n.Cordon(nodeName, reason) if err != nil { return err } @@ -161,13 +170,26 @@ func (n Node) IsUnschedulable(nodeName string) (bool, error) { // MarkWithEventID will add the drain event ID to the node to be properly ignored after a system restart event func (n Node) MarkWithEventID(nodeName string, eventID string) error { - err := n.addLabel(nodeName, EventIDLabelKey, eventID) + err := n.addLabel(nodeName, EventIDLabelKey, eventID, false) if err != nil { return fmt.Errorf("Unable to label node with event ID %s=%s: %w", EventIDLabelKey, eventID, err) } return nil } +// MaybeMarkForExclusionFromLoadBalancers will activate the ServiceNodeExclusion feature flag to indicate that the node should be removed from load balancers +func (n Node) MaybeMarkForExclusionFromLoadBalancers(nodeName string) error { + if !n.nthConfig.ExcludeFromLoadBalancers { + log.Debug().Msg("Not marking for exclusion from load balancers because the configuration flag is not set") + return nil + } + err := n.addLabel(nodeName, ExcludeFromLoadBalancersLabelKey, ExcludeFromLoadBalancersLabelValue, true) + if err != nil { + return fmt.Errorf("Unable to label node for exclusion from load balancers: %w", err) + } + return nil +} + // RemoveNTHLabels will remove all the custom NTH labels added to the node func (n Node) RemoveNTHLabels(nodeName string) error { for _, label := range []string{EventIDLabelKey, ActionLabelKey, ActionLabelTimeKey} { @@ -176,6 +198,10 @@ func (n Node) RemoveNTHLabels(nodeName string) error { return fmt.Errorf("Unable to remove %s from node: %w", label, err) } } + err := n.removeLabelIfValueMatches(nodeName, ExcludeFromLoadBalancersLabelKey, ExcludeFromLoadBalancersLabelValue) + if err != nil { + return fmt.Errorf("Unable to remove %s from node: %w", ExcludeFromLoadBalancersLabelKey, err) + } return nil } @@ -199,12 +225,12 @@ func (n Node) GetEventID(nodeName string) (string, error) { // MarkForUncordonAfterReboot adds labels to the kubernetes node which NTH will read upon reboot func (n Node) MarkForUncordonAfterReboot(nodeName string) error { // adds label to node so that the system will uncordon the node after the scheduled reboot has taken place - err := n.addLabel(nodeName, ActionLabelKey, UncordonAfterRebootLabelVal) + err := n.addLabel(nodeName, ActionLabelKey, UncordonAfterRebootLabelVal, false) if err != nil { return fmt.Errorf("Unable to label node with action to uncordon after system-reboot: %w", err) } // adds label with the current time which is checked against the uptime of the node when processing labels on startup - err = n.addLabel(nodeName, ActionLabelTimeKey, strconv.FormatInt(time.Now().Unix(), 10)) + err = n.addLabel(nodeName, ActionLabelTimeKey, strconv.FormatInt(time.Now().Unix(), 10), false) if err != nil { // if time can't be recorded, rollback the action label err := n.removeLabel(nodeName, ActionLabelKey) @@ -218,7 +244,8 @@ func (n Node) MarkForUncordonAfterReboot(nodeName string) error { } // addLabel will add a label to the node given a label key and value -func (n Node) addLabel(nodeName string, key string, value string) error { +// Specifying true for the skipExisting parameter will skip adding the label if it already exists +func (n Node) addLabel(nodeName string, key string, value string, skipExisting bool) error { type metadata struct { Labels map[string]string `json:"labels"` } @@ -240,6 +267,12 @@ func (n Node) addLabel(nodeName string, key string, value string) error { if err != nil { return err } + if skipExisting { + _, ok := node.ObjectMeta.Labels[key] + if ok { + return nil + } + } if n.nthConfig.DryRun { log.Info().Msgf("Would have added label (%s=%s) to node %s, but dry-run flag was set", key, value, nodeName) return nil @@ -282,6 +315,41 @@ func (n Node) removeLabel(nodeName string, key string) error { return nil } +// removeLabelIfValueMatches will remove a node label given a label key provided the label's value equals matchValue +func (n Node) removeLabelIfValueMatches(nodeName string, key string, matchValue string) error { + type patchRequest struct { + Op string `json:"op"` + Path string `json:"path"` + } + + var patchReqs []interface{} + patchRemove := patchRequest{ + Op: "remove", + Path: fmt.Sprintf("/metadata/labels/%s", jsonPatchEscape(key)), + } + payload, err := json.Marshal(append(patchReqs, patchRemove)) + if err != nil { + return fmt.Errorf("An error occurred while marshalling the json to remove a label from the node: %w", err) + } + node, err := n.fetchKubernetesNode(nodeName) + if err != nil { + return err + } + val, ok := node.ObjectMeta.Labels[key] + if !ok || val == matchValue { + return nil + } + if n.nthConfig.DryRun { + log.Info().Msgf("Would have removed label with key %s from node %s, but dry-run flag was set", key, nodeName) + return nil + } + _, err = n.drainHelper.Client.CoreV1().Nodes().Patch(context.TODO(), node.Name, types.JSONPatchType, payload, metav1.PatchOptions{}) + if err != nil { + return fmt.Errorf("%v node Patch failed when removing a label from the node: %w", node.Name, err) + } + return nil +} + // GetNodeLabels will fetch node labels for a given nodeName func (n Node) GetNodeLabels(nodeName string) (map[string]string, error) { if n.nthConfig.DryRun {