From 2cff723024d282851ad47fd6a5f8b5279cae90a6 Mon Sep 17 00:00:00 2001 From: Koonwah Chen Date: Thu, 13 Feb 2020 13:44:39 -0800 Subject: [PATCH] Add asm-ready as the status signal of asm mode. --- cmd/e2e-test/asm_test.go | 39 +++++++++++++++++++++++++-------- pkg/cmconfig/config.go | 7 +++++- pkg/cmconfig/config_test.go | 6 +++++ pkg/cmconfig/controller.go | 36 +++++++++++++++++++++++++----- pkg/cmconfig/controller_test.go | 2 +- pkg/context/context.go | 14 +++++++----- pkg/e2e/helpers.go | 2 +- 7 files changed, 83 insertions(+), 23 deletions(-) diff --git a/cmd/e2e-test/asm_test.go b/cmd/e2e-test/asm_test.go index fedacd8091..401053bea3 100644 --- a/cmd/e2e-test/asm_test.go +++ b/cmd/e2e-test/asm_test.go @@ -30,27 +30,32 @@ func TestASMConfig(t *testing.T) { for _, tc := range []struct { desc string configMap map[string]string + wantASMReady bool wantConfigMapEvents []string }{ { desc: "Invalid ConfigMap value equals to disable", configMap: map[string]string{"enable-asm": "INVALID"}, + wantASMReady: false, wantConfigMapEvents: []string{"The map provided a unvalid value for field: enable-asm, value: INVALID"}, }, { desc: "Invalid ConfigMap filed equals to disable", configMap: map[string]string{"enable-unknow-feild": "INVALID1"}, + wantASMReady: false, wantConfigMapEvents: []string{"The map contains a unknown key-value pair: enable-unknow-feild:INVALID1"}, }, { - desc: "Set enable-asm to true should restart the controller", - configMap: map[string]string{"enable-asm": "true"}, - wantConfigMapEvents: []string{"ConfigMapConfigController: Get a update on the ConfigMapConfig, Restarting Ingress controller"}, + desc: "Set enable-asm to true should restart the controller", + configMap: map[string]string{"enable-asm": "true"}, + wantASMReady: true, + wantConfigMapEvents: []string{"ConfigMapConfigController: Get a update on the ConfigMapConfig, Restarting Ingress controller", + "NEG controller is running in ASM Mode with Istio API: v1alpha3"}, }, - // TODO(koonwah): The below case is not fully tested, should update the neg controller to include a enable-asm event to the target CM. { - desc: "Invalid ConfigMap value equals to disable", - configMap: map[string]string{"enable-asm": "INVALID2"}, + desc: "Invalid ConfigMap value equals to disable", + configMap: map[string]string{"enable-asm": "INVALID2"}, + wantASMReady: false, wantConfigMapEvents: []string{"The map provided a unvalid value for field: enable-asm, value: INVALID2", "ConfigMapConfigController: Get a update on the ConfigMapConfig, Restarting Ingress controller"}, }, @@ -59,10 +64,26 @@ func TestASMConfig(t *testing.T) { if err := e2e.EnsureConfigMap(s, asmConfigNamespace, asmConfigName, tc.configMap); err != nil { t.Errorf("Failed to ensure ConfigMap, error: %s", err) } + + if err := wait.Poll(5*time.Second, 3*time.Minute, func() (bool, error) { + cmData, err := e2e.GetConfigMap(s, asmConfigNamespace, asmConfigName) + if err != nil { + return false, err + } + if val, ok := cmData["asm-ready"]; ok { + return val == strconv.FormatBool(tc.wantASMReady), nil + } + return false, nil + + }); err != nil { + t.Fatalf("Failed to validate asm-ready = %s. Error: %s", strconv.FormatBool(tc.wantASMReady), err) + } + if err := e2e.WaitConfigMapEvents(s, asmConfigNamespace, asmConfigName, tc.wantConfigMapEvents, negControllerRestartTimeout); err != nil { t.Fatalf("Failed to get events: %v; Error %e", strings.Join(tc.wantConfigMapEvents, ";"), err) } } + e2e.DeleteConfigMap(s, asmConfigNamespace, asmConfigName) }) } @@ -229,13 +250,13 @@ func TestNoIstioASM(t *testing.T) { if err != nil { return false, err } - if val, ok := cmData["enable-asm"]; ok && val == "false" { + if val, ok := cmData["asm-ready"]; ok && val == "false" { return true, nil } - return false, fmt.Errorf("ConfigMap: %s/%s, nable-asm is not false. Value: %v", asmConfigNamespace, asmConfigName, cmData) + return false, fmt.Errorf("ConfigMap: %s/%s, asm-ready is not false. Value: %v", asmConfigNamespace, asmConfigName, cmData) }); err != nil { - t.Fatalf("Failed to validate enable-asm = false. Error: %s", err) + t.Fatalf("Failed to validate asm-ready = false. Error: %s", err) } }) diff --git a/pkg/cmconfig/config.go b/pkg/cmconfig/config.go index b0f2581157..7804a5481f 100644 --- a/pkg/cmconfig/config.go +++ b/pkg/cmconfig/config.go @@ -11,6 +11,7 @@ import ( // Config holds configmap based configurations. type Config struct { EnableASM bool + ASMReady bool ASMServiceNEGSkipNamespaces []string } @@ -19,6 +20,7 @@ const ( falseValue = "false" enableASM = "enable-asm" + asmReady = "asm-ready" asmSkipNamespaces = "asm-skip-namespaces" ) @@ -29,7 +31,7 @@ func NewConfig() Config { // Equals returns true if c equals to other. func (c *Config) Equals(other *Config) bool { - return reflect.DeepEqual(c, other) + return c.EnableASM == other.EnableASM && reflect.DeepEqual(c.ASMServiceNEGSkipNamespaces, other.ASMServiceNEGSkipNamespaces) } // LoadValue loads configs from a map, it will ignore any unknow/unvalid field. @@ -46,6 +48,9 @@ func (c *Config) LoadValue(m map[string]string) error { } } else if k == asmSkipNamespaces { c.ASMServiceNEGSkipNamespaces = strings.Split(v, ",") + } else if k == asmReady { + // Ignore this because it's a status. + continue } else { errList = append(errList, fmt.Errorf("The map contains a unknown key-value pair: %s:%s", k, v)) } diff --git a/pkg/cmconfig/config_test.go b/pkg/cmconfig/config_test.go index b2922bae2d..2c55d55746 100644 --- a/pkg/cmconfig/config_test.go +++ b/pkg/cmconfig/config_test.go @@ -31,6 +31,12 @@ func TestLoadValue(t *testing.T) { wantConfig: Config{EnableASM: false, ASMServiceNEGSkipNamespaces: []string{"kube-system", "istio-system"}}, wantLog: "The map provided a unvalid value for field: enable-asm, value: f, valid values are: true/false", }, + { + desc: "LoadValue and compare func should ignore the asm-ready field", + inputMap: map[string]string{"enable-asm": "f", "asm-ready": "true"}, + wantConfig: Config{EnableASM: false, ASMServiceNEGSkipNamespaces: []string{"kube-system", "istio-system"}}, + wantLog: "", + }, { desc: "LoadValue should be tolerant for unknow field.", inputMap: map[string]string{"A": "B"}, diff --git a/pkg/cmconfig/controller.go b/pkg/cmconfig/controller.go index 095e275b07..8643a1a898 100644 --- a/pkg/cmconfig/controller.go +++ b/pkg/cmconfig/controller.go @@ -63,21 +63,38 @@ func (c *ConfigMapConfigController) GetConfig() Config { return *c.currentConfig } -// DisableASMMode disables the ASM Mode by updating the ConfigMap and setting the internal flag. -func (c *ConfigMapConfigController) DisableASMMode() { - patchBytes, err := utils.StrategicMergePatchBytes(v1.ConfigMap{Data: map[string]string{enableASM: trueValue}}, - v1.ConfigMap{Data: map[string]string{enableASM: falseValue}}, v1.ConfigMap{}) +func (c *ConfigMapConfigController) updateASMReady(status string) { + patchBytes, err := utils.StrategicMergePatchBytes(v1.ConfigMap{Data: map[string]string{}}, + v1.ConfigMap{Data: map[string]string{asmReady: status}}, v1.ConfigMap{}) if err != nil { - c.RecordEvent("Warning", "FailedDisableASMMode", fmt.Sprintf("Failed to disable ASM Mode, failed to create patch for ASM ConfigMap, error: %s", err)) + c.RecordEvent("Warning", "FailedToUpdateASMStatus", fmt.Sprintf("Failed to update ASM Status, failed to create patch for ASM ConfigMap, error: %s", err)) return } cm, err := c.kubeClient.CoreV1().ConfigMaps(c.configMapNamespace).Patch(c.configMapName, apimachinerytypes.MergePatchType, patchBytes, "") if err != nil { - c.RecordEvent("Warning", "FailedDisableASMMode", fmt.Sprintf("Failed to patch ASM ConfigMap, error: %s", err)) + if errors.IsNotFound(err) { + return + } + c.RecordEvent("Warning", "FailedToUpdateASMStatus", fmt.Sprintf("Failed to patch ASM ConfigMap, error: %s", err)) return } c.currentConfigMapObject = cm +} + +// DisableASM sets the internal ASM mode to off and update the ASMReady to False. +func (c *ConfigMapConfigController) DisableASM() { c.currentConfig.EnableASM = false + c.updateASMReady(falseValue) +} + +// SetASMReadyTrue update the ASMReady to True. +func (c *ConfigMapConfigController) SetASMReadyTrue() { + c.updateASMReady(trueValue) +} + +// SetASMReadyFalse update the ASMReady to False. +func (c *ConfigMapConfigController) SetASMReadyFalse() { + c.updateASMReady(falseValue) } // RecordEvent records a event to the ASMConfigmap @@ -133,5 +150,12 @@ func (c *ConfigMapConfigController) processItem(obj interface{}, cancel func()) if !config.Equals(c.currentConfig) { c.RecordEvent("Normal", "ASMConfigMapTiggerRestart", "ConfigMapConfigController: Get a update on the ConfigMapConfig, Restarting Ingress controller") cancel() + } else { + // If the config has no change, make sure the ASMReady is updated. + if config.EnableASM { + c.SetASMReadyTrue() + } else { + c.SetASMReadyFalse() + } } } diff --git a/pkg/cmconfig/controller_test.go b/pkg/cmconfig/controller_test.go index 80a727ce8d..97166ece9f 100644 --- a/pkg/cmconfig/controller_test.go +++ b/pkg/cmconfig/controller_test.go @@ -9,7 +9,7 @@ import ( "bytes" "os" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" informerv1 "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes/fake" diff --git a/pkg/context/context.go b/pkg/context/context.go index e87d879065..eeaae7739b 100644 --- a/pkg/context/context.go +++ b/pkg/context/context.go @@ -147,6 +147,8 @@ func (ctx *ControllerContext) Init() { cmConfig := ctx.ASMConfigController.GetConfig() if cmConfig.EnableASM { ctx.initEnableASM() + } else { + ctx.ASMConfigController.SetASMReadyFalse() } } @@ -165,7 +167,7 @@ func (ctx *ControllerContext) initEnableASM() { if err != nil { msg := fmt.Sprintf("Failed to create ApiextensionClient for DestinationRule, disabling ASM Mode, error: %s", err) ctx.ASMConfigController.RecordEvent("Warning", "FailedValidateDestinationRuleCRD", msg) - ctx.ASMConfigController.DisableASMMode() + ctx.ASMConfigController.DisableASM() return } destinationRuleCRD, err := apiextensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Get(destinationRuleCRDName, metav1.GetOptions{}) @@ -175,22 +177,22 @@ func (ctx *ControllerContext) initEnableASM() { } else { ctx.ASMConfigController.RecordEvent("Warning", "FailedValidateDestinationRuleCRD", fmt.Sprintf("Failed to load DestinationRule CRD, disabling the ASM Mode, please check Istio setup. Error: %s", err)) } - ctx.ASMConfigController.DisableASMMode() + ctx.ASMConfigController.DisableASM() return } if destinationRuleCRD.Spec.Version != destinationRuleAPIVersion { ctx.ASMConfigController.RecordEvent("Warning", "FailedValidateDestinationRuleCRD", fmt.Sprintf("Only Support Istio API: %s, but found %s, disabling the ASM Mode, please check Istio setup.", destinationRuleAPIVersion, destinationRuleCRD.Spec.Version)) - ctx.ASMConfigController.DisableASMMode() + ctx.ASMConfigController.DisableASM() return } dynamicClient, err := dynamic.NewForConfig(ctx.KubeConfig) if err != nil { msg := fmt.Sprintf("Failed to create kubernetes dynamic client, disabling ASM Mode, please retry. Error: %v", err) - klog.Fatalf(msg) + klog.Error(msg) ctx.ASMConfigController.RecordEvent("Warning", "FailedCreateDynamicClient", msg) - ctx.ASMConfigController.DisableASMMode() + ctx.ASMConfigController.DisableASM() return } @@ -201,6 +203,8 @@ func (ctx *ControllerContext) initEnableASM() { nil) ctx.DestinationRuleInformer = drDynamicInformer.Informer() ctx.DestinationRuleClient = dynamicClient.Resource(destrinationGVR) + ctx.ASMConfigController.RecordEvent("Normal", "ASMModeOn", fmt.Sprintf("NEG controller is running in ASM Mode with Istio API: %s.", destinationRuleAPIVersion)) + ctx.ASMConfigController.SetASMReadyTrue() } // HasSynced returns true if all relevant informers has been synced. diff --git a/pkg/e2e/helpers.go b/pkg/e2e/helpers.go index b3c5c915a2..83d3b0d765 100644 --- a/pkg/e2e/helpers.go +++ b/pkg/e2e/helpers.go @@ -588,7 +588,7 @@ func WaitConfigMapEvents(s *Sandbox, namespace, name string, msgs []string, time for _, event := range events[len(events)-len(msgs):] { allMsg += event.Message } - klog.Infof("WaitDestinationRuleAnnotation, allMsg: %s, want: %v", allMsg, msgs) + klog.Infof("WaitConfigMapEvents, allMsg: %s, want: %v", allMsg, msgs) for _, msg := range msgs { if !strings.Contains(allMsg, msg) {