diff --git a/pkg/annotations/service.go b/pkg/annotations/service.go index 2a319aa016..2911c97aa9 100644 --- a/pkg/annotations/service.go +++ b/pkg/annotations/service.go @@ -334,8 +334,8 @@ func (svc *Service) NEGAnnotation() (*NegAnnotation, bool, error) { return &res, true, nil } -// ShouldEnableTHC returns true if a THC annotation is found and its value is true. -func (svc *Service) ShouldEnableTHC() (bool, error) { +// IsThcAnnotated returns true if a THC annotation is found and its value is true. +func (svc *Service) IsThcAnnotated() (bool, error) { var res THCAnnotation annotation, ok := svc.v[THCAnnotationKey] if !ok { diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index df83d99b17..d4ee71db63 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -628,7 +628,7 @@ func (lbc *LoadBalancerController) sync(key string) error { } // Bootstrap state for GCP sync. - urlMap, errs := lbc.Translator.TranslateIngress(ing, lbc.ctx.DefaultBackendSvcPort.ID, lbc.ctx.ClusterNamer) + urlMap, errs, warnings := lbc.Translator.TranslateIngress(ing, lbc.ctx.DefaultBackendSvcPort.ID, lbc.ctx.ClusterNamer) if errs != nil { msg := fmt.Errorf("invalid ingress spec: %v", utils.JoinErrs(errs)) @@ -636,6 +636,11 @@ func (lbc *LoadBalancerController) sync(key string) error { return msg } + if warnings { + msg := "THC annotation is present for at least one Service, but the Transparent Health Checks feature is not enabled." + lbc.ctx.Recorder(ing.Namespace).Event(ing, apiv1.EventTypeWarning, "THCAnnotationWithoutFlag", msg) + } + // Sync GCP resources. syncState := &syncState{urlMap, ing, nil} syncErr := lbc.ingSyncer.Sync(syncState) @@ -776,7 +781,7 @@ func updateAnnotations(client kubernetes.Interface, ing *v1.Ingress, newAnnotati func (lbc *LoadBalancerController) ToSvcPorts(ings []*v1.Ingress) []utils.ServicePort { var knownPorts []utils.ServicePort for _, ing := range ings { - urlMap, _ := lbc.Translator.TranslateIngress(ing, lbc.ctx.DefaultBackendSvcPort.ID, lbc.ctx.ClusterNamer) + urlMap, _, _ := lbc.Translator.TranslateIngress(ing, lbc.ctx.DefaultBackendSvcPort.ID, lbc.ctx.ClusterNamer) knownPorts = append(knownPorts, urlMap.AllServicePorts()...) } return knownPorts diff --git a/pkg/controller/translator/translator.go b/pkg/controller/translator/translator.go index 7aa52aa2a9..4a5062a8f0 100644 --- a/pkg/controller/translator/translator.go +++ b/pkg/controller/translator/translator.go @@ -206,50 +206,67 @@ func (t *Translator) maybeEnableBackendConfig(sp *utils.ServicePort, svc *api_v1 return nil } -// setEnableTHC sets the THCEnabled for the service port as true or false depending on whether +// setThcOptInOnSvc sets the THCOptInOnSvc for the service port as true or false depending on whether // Transparent Health Checks should be enabled. -func (t *Translator) setEnableTHC(sp *utils.ServicePort, svc *api_v1.Service) { - THCEnabled := false - defer func() { - klog.Infof("Is THC enabled for the sevice %v with service port (%v, %v)? %v", svc.Name, sp.Port, sp.PortName, THCEnabled) - sp.THCEnabled = THCEnabled - }() +func (t *Translator) setThcOptInOnSvc(sp *utils.ServicePort, svc *api_v1.Service) (flagWarning bool) { + thcOptIn, err := annotations.FromService(svc).IsThcAnnotated() + if err != nil { + klog.Warningf("Parsing THC annotation failed: %+v.", err) + } + if !thcOptIn { + sp.THCConfiguration.THCOptInOnSvc = false + klog.Infof("THCOptInOnSvc=%v for the sevice %v with service port (%v, %v).", sp.THCConfiguration.THCOptInOnSvc, svc.Name, sp.Port, sp.PortName) + return + } + // Feature flag for Transparent Health Checks not set. if !t.enableTHC { + sp.THCConfiguration.THCEvents.THCAnnotationWithoutFlag = true + flagWarning = true + + sp.THCConfiguration.THCOptInOnSvc = false + klog.Infof("THCOptInOnSvc=%v for the sevice %v with service port (%v, %v).", sp.THCConfiguration.THCOptInOnSvc, svc.Name, sp.Port, sp.PortName) return } + // There is a BackendConfig detailing the health check configuration for the service. if sp.BackendConfig != nil && sp.BackendConfig.Spec.HealthCheck != nil { + sp.THCConfiguration.THCEvents.BackendConfigOverridesTHC = true + + sp.THCConfiguration.THCOptInOnSvc = false + klog.Infof("THCOptInOnSvc=%v for the sevice %v with service port (%v, %v).", sp.THCConfiguration.THCOptInOnSvc, svc.Name, sp.Port, sp.PortName) return } - THCEnabled, err := annotations.FromService(svc).ShouldEnableTHC() - if err != nil { - message := fmt.Sprintf("Parsing THC annotation failed: %+v.", err) - t.recorderGetter.Recorder(sp.ID.Service.Namespace).Event(svc, api_v1.EventTypeWarning, "THCAnnotationParsingFailed", message) - klog.Warning(message) - } + // THC works only with NEGs (not Instance Groups) and this is not a Service with NEG enabled. + if !sp.NEGEnabled { + sp.THCConfiguration.THCEvents.THCAnnotationWithoutNEG = true - if THCEnabled && !sp.NEGEnabled { - message := "THC annotation present, but NEG is disabled. Will not enable Transparent Health Checks." - t.recorderGetter.Recorder(sp.ID.Service.Namespace).Event(svc, api_v1.EventTypeWarning, "THCAnnotationWithoutNEG", message) - klog.Warning(message) - THCEnabled = false + sp.THCConfiguration.THCOptInOnSvc = false + klog.Infof("THCOptInOnSvc=%v for the sevice %v with service port (%v, %v).", sp.THCConfiguration.THCOptInOnSvc, svc.Name, sp.Port, sp.PortName) + return } + + sp.THCConfiguration.THCEvents.THCConfigured = true + + sp.THCConfiguration.THCOptInOnSvc = true + klog.Infof("THCOptInOnSvc=%v for the sevice %v with service port (%v, %v).", sp.THCConfiguration.THCOptInOnSvc, svc.Name, sp.Port, sp.PortName) + return } // getServicePort looks in the svc store for a matching service:port, // and returns the nodeport. -func (t *Translator) getServicePort(id utils.ServicePortID, params *getServicePortParams, namer namer_util.BackendNamer) (*utils.ServicePort, error) { +// The returned bool indicates whether there is a warning being returned or not. +func (t *Translator) getServicePort(id utils.ServicePortID, params *getServicePortParams, namer namer_util.BackendNamer) (*utils.ServicePort, error, bool) { svc, err := t.getCachedService(id) if err != nil { - return nil, err + return nil, err, false } port := ServicePort(*svc, id.Port) if port == nil { // This is a fatal error. - return nil, errors.ErrSvcPortNotFound{ServicePortID: id} + return nil, errors.ErrSvcPortNotFound{ServicePortID: id}, false } // We periodically add information to the ServicePort to ensure that we @@ -265,31 +282,33 @@ func (t *Translator) getServicePort(id utils.ServicePortID, params *getServicePo } if err := maybeEnableNEG(svcPort, svc); err != nil { - return nil, err + return nil, err, false } if err := setAppProtocol(svcPort, svc, port); err != nil { - return svcPort, err + return svcPort, err, false } if flags.F.EnableTrafficScaling { if err := setTrafficScaling(svcPort, svc); err != nil { - return nil, err + return nil, err, false } } if err := t.maybeEnableBackendConfig(svcPort, svc, port); err != nil { - return svcPort, err + return svcPort, err, false } - t.setEnableTHC(svcPort, svc) + flagWarning := t.setThcOptInOnSvc(svcPort, svc) - return svcPort, nil + return svcPort, nil, flagWarning } // TranslateIngress converts an Ingress into our internal UrlMap representation. -func (t *Translator) TranslateIngress(ing *v1.Ingress, systemDefaultBackend utils.ServicePortID, namer namer_util.BackendNamer) (*utils.GCEURLMap, []error) { +// The returned bool is for warnings (there is one type of warnings currently possible). +func (t *Translator) TranslateIngress(ing *v1.Ingress, systemDefaultBackend utils.ServicePortID, namer namer_util.BackendNamer) (*utils.GCEURLMap, []error, bool) { var errs []error + var warnings bool urlMap := utils.NewGCEURLMap() params := &getServicePortParams{} @@ -308,7 +327,8 @@ func (t *Translator) TranslateIngress(ing *v1.Ingress, systemDefaultBackend util errs = append(errs, err) continue } - svcPort, err := t.getServicePort(svcPortID, params, namer) + svcPort, err, warning := t.getServicePort(svcPortID, params, namer) + warnings = warnings || warning if err != nil { errs = append(errs, err) } @@ -342,26 +362,28 @@ func (t *Translator) TranslateIngress(ing *v1.Ingress, systemDefaultBackend util svcPortID, err := utils.BackendToServicePortID(*ing.Spec.DefaultBackend, ing.Namespace) if err != nil { errs = append(errs, err) - return urlMap, errs + return urlMap, errs, warnings } - svcPort, err := t.getServicePort(svcPortID, params, namer) + svcPort, err, warning := t.getServicePort(svcPortID, params, namer) + warnings = warnings || warning if err == nil { urlMap.DefaultBackend = svcPort - return urlMap, errs + return urlMap, errs, warnings } errs = append(errs, err) - return urlMap, errs + return urlMap, errs, warnings } - svcPort, err := t.getServicePort(systemDefaultBackend, params, namer) + svcPort, err, warning := t.getServicePort(systemDefaultBackend, params, namer) + warnings = warnings || warning if err == nil { urlMap.DefaultBackend = svcPort - return urlMap, errs + return urlMap, errs, warnings } errs = append(errs, fmt.Errorf("failed to retrieve the system default backend service %q with port %q: %v", systemDefaultBackend.Service.String(), systemDefaultBackend.Port.String(), err)) - return urlMap, errs + return urlMap, errs, warnings } // validateAndGetPaths will validate the path based on the specified path type and will return the diff --git a/pkg/controller/translator/translator_test.go b/pkg/controller/translator/translator_test.go index 68cdb280fc..6772f15597 100644 --- a/pkg/controller/translator/translator_test.go +++ b/pkg/controller/translator/translator_test.go @@ -21,7 +21,6 @@ import ( "fmt" "io/ioutil" "reflect" - "strings" "testing" "time" @@ -209,7 +208,7 @@ func TestTranslateIngress(t *testing.T) { for _, tc := range cases { t.Run(tc.desc, func(t *testing.T) { - gotGCEURLMap, gotErrs := translator.TranslateIngress(tc.ing, defaultBackend.ID, defaultNamer) + gotGCEURLMap, gotErrs, _ := translator.TranslateIngress(tc.ing, defaultBackend.ID, defaultNamer) if len(gotErrs) != tc.wantErrCount { t.Errorf("%s: TranslateIngress() = _, %+v, want %v errs", tc.desc, gotErrs, tc.wantErrCount) } @@ -232,6 +231,7 @@ func TestGetServicePort(t *testing.T) { wantPort bool params getServicePortParams wantedPort apiv1.ServicePort + wantWarning bool }{ { desc: "clusterIP service", @@ -342,6 +342,21 @@ func TestGetServicePort(t *testing.T) { wantPort: true, wantedPort: apiv1.ServicePort{Name: "https", Port: 443, TargetPort: intstr.FromString("pod-https")}, }, + { + annotations: map[string]string{annotations.THCAnnotationKey: `{"enabled":true}`}, + desc: "correct port spec THC annotation", + spec: apiv1.ServiceSpec{ + Type: apiv1.ServiceTypeNodePort, + Ports: []apiv1.ServicePort{ + {Name: "http", Port: 80, NodePort: 123, TargetPort: intstr.FromString("podport")}, + }, + }, + id: utils.ServicePortID{Port: v1.ServiceBackendPort{Number: 80}}, + wantErr: false, + wantPort: true, + wantedPort: apiv1.ServicePort{Name: "http", Port: 80, NodePort: 123, TargetPort: intstr.FromString("podport")}, + wantWarning: true, + }, } for _, tc := range cases { t.Run(tc.desc, func(t *testing.T) { @@ -354,7 +369,7 @@ func TestGetServicePort(t *testing.T) { svcLister.Add(svc) tc.id.Service = svcName - port, gotErr := translator.getServicePort(tc.id, &tc.params, defaultNamer) + port, gotErr, warning := translator.getServicePort(tc.id, &tc.params, defaultNamer) if (gotErr != nil) != tc.wantErr { t.Errorf("translator.getServicePort(%+v) = _, %v, want err? %v", tc.id, gotErr, tc.wantErr) } @@ -375,6 +390,9 @@ func TestGetServicePort(t *testing.T) { t.Errorf("Expected port.TargetPort %v, got %v", port.TargetPort, tc.wantedPort.TargetPort) } } + if tc.wantWarning && !warning { + t.Errorf("Expected warning %v, got warning %v.", tc.wantWarning, warning) + } }) } } @@ -445,7 +463,7 @@ func TestGetServicePortWithBackendConfigEnabled(t *testing.T) { svcLister.Add(svc) backendConfigLister.Add(backendConfig) - port, gotErr := translator.getServicePort(tc.id, &tc.params, defaultNamer) + port, gotErr, _ := translator.getServicePort(tc.id, &tc.params, defaultNamer) if (gotErr != nil) != tc.wantErr { t.Errorf("%s: translator.getServicePort(%+v) = _, %v, want err? %v", tc.desc, tc.id, gotErr, tc.wantErr) } @@ -740,7 +758,7 @@ func TestPathValidation(t *testing.T) { } expectedGCEURLMap.HostRules = []utils.HostRule{{Hostname: hostname, Paths: expectedPathRules}} - gotGCEURLMap, gotErrs := translator.TranslateIngress(ing, defaultBackend.ID, defaultNamer) + gotGCEURLMap, gotErrs, _ := translator.TranslateIngress(ing, defaultBackend.ID, defaultNamer) if tc.expectValid && len(gotErrs) > 0 { t.Fatalf("%s: TranslateIngress() = _, %+v, want no errs", tc.desc, gotErrs) } else if !tc.expectValid && len(gotErrs) == 0 { @@ -1167,8 +1185,8 @@ func TestSetTrafficScaling(t *testing.T) { } } -func TestSetEnableTHC(t *testing.T) { - // No t.Parallel() +func TestSetThcOptInOnSvc(t *testing.T) { + t.Parallel() newService := func(ann map[string]string) *apiv1.Service { return &apiv1.Service{ @@ -1177,13 +1195,11 @@ func TestSetEnableTHC(t *testing.T) { } type tc struct { - name string - sp *utils.ServicePort - svc *apiv1.Service - enableTHC bool - want *utils.ServicePort - wantEvent bool - eventPrefix string + name string + sp *utils.ServicePort + svc *apiv1.Service + enableTHC bool + want *utils.ServicePort } const ( @@ -1194,101 +1210,118 @@ func TestSetEnableTHC(t *testing.T) { testCases := []*tc{ { name: "no settings flag disabled", - sp: &utils.ServicePort{THCEnabled: true}, + sp: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true}}, svc: newService(map[string]string{}), enableTHC: false, - want: &utils.ServicePort{THCEnabled: false}, + want: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: false}}, }, { name: "no settings", - sp: &utils.ServicePort{THCEnabled: true}, + sp: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true}}, svc: newService(map[string]string{}), enableTHC: true, - want: &utils.ServicePort{THCEnabled: false}, + want: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: false}}, }, { name: "annotation", sp: &utils.ServicePort{NEGEnabled: true}, svc: newService(map[string]string{thcLabel: thcValue}), enableTHC: true, - want: &utils.ServicePort{NEGEnabled: true, THCEnabled: true}, + want: &utils.ServicePort{NEGEnabled: true, THCConfiguration: utils.THCConfiguration{ + THCOptInOnSvc: true, + THCEvents: utils.THCEvents{THCConfigured: true}, + }}, }, { name: "annotation flag disabled", - sp: &utils.ServicePort{NEGEnabled: true, THCEnabled: true}, + sp: &utils.ServicePort{NEGEnabled: true, THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true}}, svc: newService(map[string]string{thcLabel: thcValue}), enableTHC: false, - want: &utils.ServicePort{NEGEnabled: true, THCEnabled: false}, + want: &utils.ServicePort{NEGEnabled: true, THCConfiguration: utils.THCConfiguration{ + THCOptInOnSvc: false, + THCEvents: utils.THCEvents{THCAnnotationWithoutFlag: true}, + }}, }, { - name: "annotation NEG disabled", - sp: &utils.ServicePort{THCEnabled: true}, - svc: newService(map[string]string{thcLabel: thcValue}), - enableTHC: true, - want: &utils.ServicePort{THCEnabled: false}, - wantEvent: true, - eventPrefix: "Warning THCAnnotationWithoutNEG THC annotation present, but NEG is disabled. Will not enable Transparent Health Checks.", + name: "annotation NEG disabled", + sp: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true}}, + svc: newService(map[string]string{thcLabel: thcValue}), + enableTHC: true, + want: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{ + THCOptInOnSvc: false, + THCEvents: utils.THCEvents{THCAnnotationWithoutNEG: true}, + }}, }, { name: "invalid annotation flag disabled", - sp: &utils.ServicePort{NEGEnabled: true, THCEnabled: true}, + sp: &utils.ServicePort{NEGEnabled: true, THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true}}, svc: newService(map[string]string{thcLabel: "random text"}), enableTHC: false, - want: &utils.ServicePort{NEGEnabled: true, THCEnabled: false}, + want: &utils.ServicePort{NEGEnabled: true, THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: false}}, }, { - name: "invalid annotation", - sp: &utils.ServicePort{NEGEnabled: true, THCEnabled: true}, - svc: newService(map[string]string{thcLabel: "random text"}), - enableTHC: true, - want: &utils.ServicePort{NEGEnabled: true, THCEnabled: false}, - wantEvent: true, - eventPrefix: "Warning THCAnnotationParsingFailed Parsing THC annotation failed", + name: "invalid annotation", + sp: &utils.ServicePort{ + NEGEnabled: true, + THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true}, + }, + svc: newService(map[string]string{thcLabel: "random text"}), + enableTHC: true, + want: &utils.ServicePort{ + NEGEnabled: true, + THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: false}, + }, }, { - name: "annotation empty backendconfig", - sp: &utils.ServicePort{BackendConfig: &backendconfig.BackendConfig{}, NEGEnabled: true, THCEnabled: false}, + name: "annotation empty backendconfig", + sp: &utils.ServicePort{ + BackendConfig: &backendconfig.BackendConfig{}, + NEGEnabled: true, + THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: false}, + }, svc: newService(map[string]string{thcLabel: thcValue}), enableTHC: true, - want: &utils.ServicePort{BackendConfig: &backendconfig.BackendConfig{}, NEGEnabled: true, THCEnabled: true}, + want: &utils.ServicePort{ + BackendConfig: &backendconfig.BackendConfig{}, + NEGEnabled: true, + THCConfiguration: utils.THCConfiguration{ + THCOptInOnSvc: true, + THCEvents: utils.THCEvents{THCConfigured: true}, + }, + }, }, } BC := &backendconfig.BackendConfig{} BC.Spec.HealthCheck = &backendconfig.HealthCheckConfig{} testCases = append(testCases, &tc{ - name: "annotation backendconfig", - sp: &utils.ServicePort{BackendConfig: BC, NEGEnabled: true, THCEnabled: true}, + name: "annotation backendconfig", + sp: &utils.ServicePort{ + BackendConfig: BC, + NEGEnabled: true, + THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true}, + }, svc: newService(map[string]string{thcLabel: thcValue}), enableTHC: true, - want: &utils.ServicePort{BackendConfig: BC, NEGEnabled: true, THCEnabled: false}, + want: &utils.ServicePort{ + BackendConfig: BC, + NEGEnabled: true, + THCConfiguration: utils.THCConfiguration{ + THCOptInOnSvc: false, + THCEvents: utils.THCEvents{BackendConfigOverridesTHC: true}, + }, + }, }) for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { translator := fakeTranslator() - bufferSize := 0 - if tc.wantEvent { - bufferSize = 1 - } - fakeSingletonRecorderGetter := healthchecks.NewFakeSingletonRecorderGetter(bufferSize) - translator.recorderGetter = fakeSingletonRecorderGetter + translator.recorderGetter = healthchecks.NewFakeRecorderGetter(0) translator.enableTHC = tc.enableTHC sp := *tc.sp - translator.setEnableTHC(&sp, tc.svc) + translator.setThcOptInOnSvc(&sp, tc.svc) if !reflect.DeepEqual(&sp, tc.want) { - t.Errorf("setEnableTHC, %s\ngot %s,\nwant %s", tc.name, pretty.Sprint(&sp), pretty.Sprint(tc.want)) - } - if tc.wantEvent { - fakeRecorder := fakeSingletonRecorderGetter.FakeRecorder() - select { - case output := <-fakeRecorder.Events: - if !strings.HasPrefix(output, tc.eventPrefix) { - t.Fatalf("Incorrect event emitted: %s.", output) - } - case <-time.After(10 * time.Second): - t.Fatalf("Timeout when expecting Event.") - } + t.Errorf("setThcOptInOnSvc, %s\ngot %s,\nwant %s", tc.name, pretty.Sprint(&sp), pretty.Sprint(tc.want)) } }) } diff --git a/pkg/firewalls/controller.go b/pkg/firewalls/controller.go index 231f3d6842..73d28f7df1 100644 --- a/pkg/firewalls/controller.go +++ b/pkg/firewalls/controller.go @@ -191,7 +191,7 @@ func NewFirewallController( func (fwc *FirewallController) ToSvcPorts(ings []*v1.Ingress) []utils.ServicePort { var knownPorts []utils.ServicePort for _, ing := range ings { - urlMap, _ := fwc.translator.TranslateIngress(ing, fwc.ctx.DefaultBackendSvcPort.ID, fwc.ctx.ClusterNamer) + urlMap, _, _ := fwc.translator.TranslateIngress(ing, fwc.ctx.DefaultBackendSvcPort.ID, fwc.ctx.ClusterNamer) knownPorts = append(knownPorts, urlMap.AllServicePorts()...) } return knownPorts diff --git a/pkg/healthchecks/healthchecks.go b/pkg/healthchecks/healthchecks.go index 7ce21ba929..fc007582a9 100644 --- a/pkg/healthchecks/healthchecks.go +++ b/pkg/healthchecks/healthchecks.go @@ -84,7 +84,7 @@ func (h *HealthChecks) new(sp utils.ServicePort) *translator.HealthCheck { // TODO: rename backend-service and health-check to not use port as key hc.Port = sp.NodePort hc.RequestPath = h.pathFromSvcPort(sp) - if sp.THCEnabled { + if sp.THCConfiguration.THCOptInOnSvc { translator.OverwriteWithTHC(hc) } hc.Name = sp.BackendName() @@ -125,16 +125,17 @@ func (h *HealthChecks) generateServiceInfo(sp utils.ServicePort, iLB bool) healt // SyncServicePort implements HealthChecker. func (h *HealthChecks) SyncServicePort(sp *utils.ServicePort, probe *v1.Probe) (string, error) { - klog.Infof("SyncServicePort: sp.ID=%v, sp.NodePort=%v, sp.Port=%v, sp.PortName=%v, sp.THCEnabled=%v, h.thcEnabled=%v.", sp.ID, sp.NodePort, sp.Port, sp.PortName, sp.THCEnabled, h.thcEnabled) - if !h.thcEnabled && sp.THCEnabled { + klog.Infof("SyncServicePort: sp.ID=%v, sp.NodePort=%v, sp.Port=%v, sp.PortName=%v, sp.THCConfiguration.THCOptInOnSvc=%v, h.thcEnabled=%v.", sp.ID, sp.NodePort, sp.Port, sp.PortName, sp.THCConfiguration.THCOptInOnSvc, h.thcEnabled) + if !h.thcEnabled && sp.THCConfiguration.THCOptInOnSvc { klog.Warningf("THC flag disabled for HealthChecks, but ServicePort %v has Transparent Health Checks enabled. Disabling.", sp.ID) - sp.THCEnabled = false + sp.THCConfiguration.THCOptInOnSvc = false } + defer func() { sp.THCConfiguration.THCEvents = utils.THCEvents{} }() hc := h.new(*sp) - if sp.THCEnabled { + if sp.THCConfiguration.THCOptInOnSvc { klog.V(2).Infof("ServicePort %v has Transparent Health Checks enabled", sp.ID) - return h.sync(hc, nil, sp.THCEnabled) + return h.sync(hc, nil, sp.THCConfiguration) } if probe != nil { klog.V(2).Infof("Applying httpGet settings of readinessProbe to health check on port %+v", sp) @@ -148,16 +149,46 @@ func (h *HealthChecks) SyncServicePort(sp *utils.ServicePort, probe *v1.Probe) ( if bchcc != nil { klog.V(2).Infof("ServicePort %v has BackendConfig healthcheck override", sp.ID) } - return h.sync(hc, bchcc, sp.THCEnabled) + return h.sync(hc, bchcc, sp.THCConfiguration) +} + +// notifyAboutTHC emits Events about successful or attempted THC configuration. +// Currently called on creation or update of a health check. +func (h *HealthChecks) notifyAboutTHC(hc *translator.HealthCheck, thcEvents utils.THCEvents) { + if thcEvents.THCConfigured { + message := "Transparent Health Check successfully configured." + h.recorderGetter.Recorder(hc.Service.Namespace).Event( + hc.Service, v1.EventTypeNormal, "THCConfigured", message) + klog.Infof("%s Health check name: %s.", message, hc.Name) + } + if thcEvents.BackendConfigOverridesTHC { + message := "Both THC and BackendConfig annotations present and the BackendConfig has spec.healthCheck. The THC annotation will be ignored." + h.recorderGetter.Recorder(hc.Service.Namespace).Event( + hc.Service, v1.EventTypeWarning, "BackendConfigOverridesTHC", message) + klog.Warningf("%s Health check name: %s.", message, hc.Name) + } + if thcEvents.THCAnnotationWithoutFlag { + message := "THC annotation present, but the Transparent Health Checks feature is not enabled." + h.recorderGetter.Recorder(hc.Service.Namespace).Event( + hc.Service, v1.EventTypeWarning, "THCAnnotationWithoutFlag", message) + klog.Warningf("%s Health check name: %s.", message, hc.Name) + } + if thcEvents.THCAnnotationWithoutNEG { + message := "THC annotation present, but NEG is disabled. Will not enable Transparent Health Checks." + h.recorderGetter.Recorder(hc.Service.Namespace).Event( + hc.Service, v1.EventTypeWarning, "THCAnnotationWithoutNEG", message) + klog.Warningf("%s Health check name: %s.", message, hc.Name) + } + } // sync retrieves a health check based on port, checks type and settings and updates/creates if necessary. // sync is only called by the backends.Add func - it's not a pool like other resources. -// We assume that bchcc cannot be non-nil and thcEnabled be true simultaneously. -func (h *HealthChecks) sync(hc *translator.HealthCheck, bchcc *backendconfigv1.HealthCheckConfig, thcEnabled bool) (string, error) { - if bchcc != nil && thcEnabled { - klog.Warningf("BackendConfig exists and thcEnabled simultaneously for %v. Ignoring transparent health check.", hc.Name) - thcEnabled = false +// We assume that bchcc cannot be non-nil and thcOptIn be true simultaneously. +func (h *HealthChecks) sync(hc *translator.HealthCheck, bchcc *backendconfigv1.HealthCheckConfig, thcConf utils.THCConfiguration) (string, error) { + if bchcc != nil && thcConf.THCOptInOnSvc { + klog.Warningf("BackendConfig exists and thcOptIn true simultaneously for %v. Ignoring transparent health check.", hc.Name) + thcConf.THCOptInOnSvc = false } var scope meta.KeyType @@ -175,6 +206,7 @@ func (h *HealthChecks) sync(hc *translator.HealthCheck, bchcc *backendconfigv1.H klog.Errorf("Health check %q creation error: %v", hc.Name, err) return "", err } + h.notifyAboutTHC(hc, thcConf.THCEvents) // TODO(bowei) -- we don't need to fetch the self-link here as it is // returned as part of the GCE call. selfLink, err := h.getHealthCheckLink(hc.Name, hc.Version(), scope) @@ -188,7 +220,7 @@ func (h *HealthChecks) sync(hc *translator.HealthCheck, bchcc *backendconfigv1.H // First, merge in the configuration from the existing healthcheck to cover // the case where the user has changed healthcheck settings outside of // GKE. - if !thcEnabled { + if !thcConf.THCOptInOnSvc { premergeHC := hc hc = mergeUserSettings(existingHC, hc) klog.V(3).Infof("Existing HC = %+v", existingHC) @@ -210,7 +242,7 @@ func (h *HealthChecks) sync(hc *translator.HealthCheck, bchcc *backendconfigv1.H return &ans } - changes := calculateDiff(filter(existingHC), filter(hc), bchcc, thcEnabled) + changes := calculateDiff(filter(existingHC), filter(hc), bchcc, thcConf.THCOptInOnSvc) if changes.hasDiff() { klog.V(2).Infof("Health check %q needs update (%s)", existingHC.Name, changes) if flags.F.EnableUpdateCustomHealthCheckDescription && changes.size() == 1 && changes.has("Description") { @@ -226,6 +258,7 @@ func (h *HealthChecks) sync(hc *translator.HealthCheck, bchcc *backendconfigv1.H if err != nil { klog.Errorf("Health check %q update error: %v", existingHC.Name, err) } + h.notifyAboutTHC(hc, thcConf.THCEvents) return existingHC.SelfLink, err } diff --git a/pkg/healthchecks/healthchecks_test.go b/pkg/healthchecks/healthchecks_test.go index 05644f260b..d3ca3b0ff1 100644 --- a/pkg/healthchecks/healthchecks_test.go +++ b/pkg/healthchecks/healthchecks_test.go @@ -108,7 +108,7 @@ func init() { } } if thc { - sp.THCEnabled = true + sp.THCConfiguration.THCOptInOnSvc = true if mode == "reg" { // No THC without NEG. continue } @@ -158,7 +158,7 @@ func TestHealthCheckAdd(t *testing.T) { t.Fatalf("expected the health check to exist, err: %v", err) } - sp = &utils.ServicePort{NodePort: 8080, Protocol: annotations.ProtocolHTTP, NEGEnabled: false, BackendNamer: testNamer, THCEnabled: true} + sp = &utils.ServicePort{NodePort: 8080, Protocol: annotations.ProtocolHTTP, NEGEnabled: false, BackendNamer: testNamer, THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true}} _, err = healthChecks.SyncServicePort(sp, nil) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -198,7 +198,7 @@ func TestHealthCheckAddExisting(t *testing.T) { } // Enable Transparent Health Checks - sp = &utils.ServicePort{NodePort: 3000, Protocol: annotations.ProtocolHTTP, NEGEnabled: false, BackendNamer: testNamer, THCEnabled: true} + sp = &utils.ServicePort{NodePort: 3000, Protocol: annotations.ProtocolHTTP, NEGEnabled: false, BackendNamer: testNamer, THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true}} _, err = healthChecks.SyncServicePort(sp, nil) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -386,7 +386,7 @@ func TestHealthCheckUpdate(t *testing.T) { // Change to HTTPS hc.Type = string(annotations.ProtocolHTTPS) - _, err = healthChecks.sync(hc, nil, false) + _, err = healthChecks.sync(hc, nil, utils.THCConfiguration{}) if err != nil { t.Fatalf("unexpected err while syncing healthcheck, err %v", err) } @@ -404,7 +404,7 @@ func TestHealthCheckUpdate(t *testing.T) { // Change to HTTP2 hc.Type = string(annotations.ProtocolHTTP2) - _, err = healthChecks.sync(hc, nil, false) + _, err = healthChecks.sync(hc, nil, utils.THCConfiguration{}) if err != nil { t.Fatalf("unexpected err while syncing healthcheck, err %v", err) } @@ -423,7 +423,7 @@ func TestHealthCheckUpdate(t *testing.T) { // Change to NEG Health Check hc.ForNEG = true hc.PortSpecification = "USE_SERVING_PORT" - _, err = healthChecks.sync(hc, nil, false) + _, err = healthChecks.sync(hc, nil, utils.THCConfiguration{}) if err != nil { t.Fatalf("unexpected err while syncing healthcheck, err %v", err) @@ -444,7 +444,7 @@ func TestHealthCheckUpdate(t *testing.T) { hc.Port = 3000 hc.PortSpecification = "" - _, err = healthChecks.sync(hc, nil, false) + _, err = healthChecks.sync(hc, nil, utils.THCConfiguration{}) if err != nil { t.Fatalf("unexpected err while syncing healthcheck, err %v", err) } @@ -507,7 +507,7 @@ func TestEnableTHC(t *testing.T) { translator.OverwriteWithTHC(hc) hc.Name = oldName // Enable Transparent Health Checks - _, err = healthChecks.sync(hc, nil, true) + _, err = healthChecks.sync(hc, nil, utils.THCConfiguration{THCOptInOnSvc: true}) if err != nil { t.Fatalf("unexpected err while syncing healthcheck, err %v", err) } @@ -535,6 +535,67 @@ func getSingletonHealthcheck(t *testing.T, c *gce.Cloud) *compute.HealthCheck { return utils.DeepCopyComputeHealthCheck(computeHCs[0]) // Make a copy to avoid reading an overwritten version later. } +func TestNotifyAboutTHC(t *testing.T) { + t.Parallel() + + testClusterValues := gce.DefaultTestClusterValues() + fakeGCE := gce.NewFakeGCECloud(testClusterValues) + + fakeSingletonRecorderGetter := NewFakeSingletonRecorderGetter(10) + healthChecks := NewHealthChecker(fakeGCE, "/", defaultBackendSvc, fakeSingletonRecorderGetter, NewFakeServiceGetter(), false) + + hc := translator.DefaultHealthCheck(3000, annotations.ProtocolHTTP) + hc.Service = &v1.Service{} + + type tc = struct { + wantTexts []string + events utils.THCEvents + } + + testCases := []tc{ + { + wantTexts: []string{"Normal THCConfigured Transparent Health Check successfully configured."}, + events: utils.THCEvents{THCConfigured: true}, + }, + { + wantTexts: []string{"Warning BackendConfigOverridesTHC Both THC and BackendConfig annotations present and the BackendConfig has spec.healthCheck. The THC annotation will be ignored."}, + events: utils.THCEvents{BackendConfigOverridesTHC: true}, + }, + { + wantTexts: []string{"Warning THCAnnotationWithoutFlag THC annotation present, but the Transparent Health Checks feature is not enabled."}, + events: utils.THCEvents{THCAnnotationWithoutFlag: true}, + }, + { + wantTexts: []string{"Warning THCAnnotationWithoutFlag THC annotation present, but the Transparent Health Checks feature is not enabled."}, + events: utils.THCEvents{THCAnnotationWithoutFlag: true}, + }, + { + wantTexts: []string{"Warning THCAnnotationWithoutNEG THC annotation present, but NEG is disabled. Will not enable Transparent Health Checks."}, + events: utils.THCEvents{THCAnnotationWithoutNEG: true}, + }, + } + + fakeRecorder := fakeSingletonRecorderGetter.FakeRecorder() + for _, tc := range testCases { + healthChecks.notifyAboutTHC(hc, tc.events) + for _, wantText := range tc.wantTexts { + select { + case output := <-fakeRecorder.Events: + if output != wantText { + t.Errorf("Incorrect event emitted on healthcheck update: %s.", output) + } + case <-time.After(10 * time.Second): + t.Fatalf("Timeout when expecting Event.") + } + } + select { + case output := <-fakeRecorder.Events: + t.Fatalf("Unexpected event: %s", output) + case <-time.After(100 * time.Millisecond): + } + } +} + // Test changing the value of the flag EnableUpdateCustomHealthCheckDescription from false to true. func TestRolloutUpdateCustomHCDescription(t *testing.T) { // No parallel() because we modify the value of the flags: @@ -1586,7 +1647,7 @@ func TestSyncServicePort(t *testing.T) { tc.updateHCDescription = true tc.desc = tc.desc + " with updateHCDescription" copyOfWant := *tc.wantComputeHC - if tc.sp.BackendConfig != nil || tc.sp.THCEnabled == true { + if tc.sp.BackendConfig != nil || tc.sp.THCConfiguration.THCOptInOnSvc == true { config := healthcheck.TransparentHC if tc.sp.BackendConfig != nil { config = healthcheck.BackendConfigHC diff --git a/pkg/translator/healthchecks.go b/pkg/translator/healthchecks.go index ade33c78f9..9d14b7fb1c 100644 --- a/pkg/translator/healthchecks.go +++ b/pkg/translator/healthchecks.go @@ -235,7 +235,7 @@ func OverwriteWithTHC(hc *HealthCheck) { hc.UnhealthyThreshold = defaultUnhealthyThreshold hc.HealthyThreshold = thcHealthyThreshold hc.Type = thcType - hc.Description = DescriptionForTransparentHealthChecks //TODO(DamianSawicki): JSONify. + hc.Description = DescriptionForTransparentHealthChecks hc.PortSpecification = thcPortSpecification hc.Port = THCPort hc.RequestPath = thcRequestPath diff --git a/pkg/utils/serviceport.go b/pkg/utils/serviceport.go index cba1405a66..a74648d56b 100644 --- a/pkg/utils/serviceport.go +++ b/pkg/utils/serviceport.go @@ -37,6 +37,19 @@ func (id ServicePortID) String() string { return fmt.Sprintf("%v/%v", id.Service.String(), id.Port.String()) } +// THCEvents stores information relevant for emitting Events when THC is configured or misconfigured. +type THCEvents struct { + THCConfigured bool + BackendConfigOverridesTHC bool + THCAnnotationWithoutFlag bool + THCAnnotationWithoutNEG bool +} + +type THCConfiguration struct { + THCOptInOnSvc bool + THCEvents THCEvents +} + // ServicePort maintains configuration for a single backend. type ServicePort struct { // Ingress backend-specified service name and port @@ -46,16 +59,16 @@ type ServicePort struct { // Numerical port of the Service, retrieved from the Service Port int32 // Name of the port of the Service, retrieved from the Service - PortName string - Protocol annotations.AppProtocol - TargetPort intstr.IntOrString - NEGEnabled bool - VMIPNEGEnabled bool - L4RBSEnabled bool - L7ILBEnabled bool - THCEnabled bool - BackendConfig *backendconfigv1.BackendConfig - BackendNamer namer.BackendNamer + PortName string + Protocol annotations.AppProtocol + TargetPort intstr.IntOrString + NEGEnabled bool + VMIPNEGEnabled bool + L4RBSEnabled bool + L7ILBEnabled bool + THCConfiguration THCConfiguration + BackendConfig *backendconfigv1.BackendConfig + BackendNamer namer.BackendNamer // Traffic policy fields that apply if non-nil. MaxRatePerEndpoint *float64 CapacityScaler *float64