Skip to content

Commit

Permalink
Differentiate naming between flag and annotation
Browse files Browse the repository at this point in the history
  • Loading branch information
@DamianSawicki
DamianSawicki committed May 18, 2023
1 parent 0d3a993 commit a4bf4ca
Show file tree
Hide file tree
Showing 6 changed files with 63 additions and 60 deletions.
4 changes: 2 additions & 2 deletions pkg/annotations/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
29 changes: 16 additions & 13 deletions pkg/controller/translator/translator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,36 +206,39 @@ 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) (flagWarning bool) {
THCEnabled, err := annotations.FromService(svc).ShouldEnableTHC()
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 THCEnabled && !t.enableTHC {
// Feature flag for Transparent Health Checks not set.
if thcOptIn && !t.enableTHC {
sp.THCConfiguration.THCEvents.THCAnnotationWithoutFlag = true
flagWarning = true
THCEnabled = false
thcOptIn = false
}

if THCEnabled && sp.BackendConfig != nil && sp.BackendConfig.Spec.HealthCheck != nil {
// There is a BackendConfig detailing the health check configuration for the service.
if thcOptIn && sp.BackendConfig != nil && sp.BackendConfig.Spec.HealthCheck != nil {
sp.THCConfiguration.THCEvents.BackendConfigOverridesTHC = true
THCEnabled = false
thcOptIn = false
}

if THCEnabled && !sp.NEGEnabled {
// THC works only with NEGs (not Instance Groups) and this is not a Service with NEG enabled.
if thcOptIn && !sp.NEGEnabled {
sp.THCConfiguration.THCEvents.THCAnnotationWithoutNEG = true
THCEnabled = false
thcOptIn = false
}

if THCEnabled {
if thcOptIn {
sp.THCConfiguration.THCEvents.THCConfigured = true
}

klog.Infof("Is THC enabled for the sevice %v with service port (%v, %v)? %v", svc.Name, sp.Port, sp.PortName, THCEnabled)
sp.THCConfiguration.THCEnabled = THCEnabled
klog.Infof("Is THC enabled for the sevice %v with service port (%v, %v)? %v", svc.Name, sp.Port, sp.PortName, thcOptIn)
sp.THCConfiguration.THCOptInOnSvc = thcOptIn

return
}
Expand Down Expand Up @@ -285,7 +288,7 @@ func (t *Translator) getServicePort(id utils.ServicePortID, params *getServicePo
return svcPort, err, false
}

flagWarning := t.setEnableTHC(svcPort, svc)
flagWarning := t.setThcOptInOnSvc(svcPort, svc)

return svcPort, nil, flagWarning
}
Expand Down
50 changes: 25 additions & 25 deletions pkg/controller/translator/translator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1185,7 +1185,7 @@ func TestSetTrafficScaling(t *testing.T) {
}
}

func TestSetEnableTHC(t *testing.T) {
func TestSetThcOptInOnSvc(t *testing.T) {
// No t.Parallel()

newService := func(ann map[string]string) *apiv1.Service {
Expand All @@ -1212,68 +1212,68 @@ func TestSetEnableTHC(t *testing.T) {
testCases := []*tc{
{
name: "no settings flag disabled",
sp: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCEnabled: true}},
sp: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true}},
svc: newService(map[string]string{}),
enableTHC: false,
want: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCEnabled: false}},
want: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: false}},
},
{
name: "no settings",
sp: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCEnabled: true}},
sp: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true}},
svc: newService(map[string]string{}),
enableTHC: true,
want: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{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, THCConfiguration: utils.THCConfiguration{
THCEnabled: true,
THCEvents: utils.THCEvents{THCConfigured: true},
THCOptInOnSvc: true,
THCEvents: utils.THCEvents{THCConfigured: true},
}},
},
{
name: "annotation flag disabled",
sp: &utils.ServicePort{NEGEnabled: true, THCConfiguration: utils.THCConfiguration{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, THCConfiguration: utils.THCConfiguration{
THCEnabled: false,
THCEvents: utils.THCEvents{THCAnnotationWithoutFlag: true},
THCOptInOnSvc: false,
THCEvents: utils.THCEvents{THCAnnotationWithoutFlag: true},
}},
},
{
name: "annotation NEG disabled",
sp: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCEnabled: true}},
sp: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true}},
svc: newService(map[string]string{thcLabel: thcValue}),
enableTHC: true,
want: &utils.ServicePort{THCConfiguration: utils.THCConfiguration{
THCEnabled: false,
THCEvents: utils.THCEvents{THCAnnotationWithoutNEG: true},
THCOptInOnSvc: false,
THCEvents: utils.THCEvents{THCAnnotationWithoutNEG: true},
}},
},
{
name: "invalid annotation flag disabled",
sp: &utils.ServicePort{NEGEnabled: true, THCConfiguration: utils.THCConfiguration{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, THCConfiguration: utils.THCConfiguration{THCEnabled: false}},
want: &utils.ServicePort{NEGEnabled: true, THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: false}},
wantEvent: true,
eventPrefix: "Warning THCAnnotationParsingFailed Parsing THC annotation failed",
},
{
name: "invalid annotation",
sp: &utils.ServicePort{
NEGEnabled: true,
THCConfiguration: utils.THCConfiguration{THCEnabled: true},
THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true},
},
svc: newService(map[string]string{thcLabel: "random text"}),
enableTHC: true,
want: &utils.ServicePort{
NEGEnabled: true,
THCConfiguration: utils.THCConfiguration{THCEnabled: false},
THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: false},
},
wantEvent: true,
eventPrefix: "Warning THCAnnotationParsingFailed Parsing THC annotation failed",
Expand All @@ -1283,16 +1283,16 @@ func TestSetEnableTHC(t *testing.T) {
sp: &utils.ServicePort{
BackendConfig: &backendconfig.BackendConfig{},
NEGEnabled: true,
THCConfiguration: utils.THCConfiguration{THCEnabled: false},
THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: false},
},
svc: newService(map[string]string{thcLabel: thcValue}),
enableTHC: true,
want: &utils.ServicePort{
BackendConfig: &backendconfig.BackendConfig{},
NEGEnabled: true,
THCConfiguration: utils.THCConfiguration{
THCEnabled: true,
THCEvents: utils.THCEvents{THCConfigured: true},
THCOptInOnSvc: true,
THCEvents: utils.THCEvents{THCConfigured: true},
},
},
},
Expand All @@ -1304,16 +1304,16 @@ func TestSetEnableTHC(t *testing.T) {
sp: &utils.ServicePort{
BackendConfig: BC,
NEGEnabled: true,
THCConfiguration: utils.THCConfiguration{THCEnabled: true},
THCConfiguration: utils.THCConfiguration{THCOptInOnSvc: true},
},
svc: newService(map[string]string{thcLabel: thcValue}),
enableTHC: true,
want: &utils.ServicePort{
BackendConfig: BC,
NEGEnabled: true,
THCConfiguration: utils.THCConfiguration{
THCEnabled: false,
THCEvents: utils.THCEvents{BackendConfigOverridesTHC: true},
THCOptInOnSvc: false,
THCEvents: utils.THCEvents{BackendConfigOverridesTHC: true},
},
},
})
Expand All @@ -1325,9 +1325,9 @@ func TestSetEnableTHC(t *testing.T) {
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))
t.Errorf("setThcOptInOnSvc, %s\ngot %s,\nwant %s", tc.name, pretty.Sprint(&sp), pretty.Sprint(tc.want))
}
})
}
Expand Down
28 changes: 14 additions & 14 deletions pkg/healthchecks/healthchecks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.THCConfiguration.THCEnabled {
if sp.THCConfiguration.THCOptInOnSvc {
translator.OverwriteWithTHC(hc)
}
hc.Name = sp.BackendName()
Expand Down Expand Up @@ -125,17 +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.THCConfiguration.THCEnabled=%v, h.thcEnabled=%v.", sp.ID, sp.NodePort, sp.Port, sp.PortName, sp.THCConfiguration.THCEnabled, h.thcEnabled)
if !h.thcEnabled && sp.THCConfiguration.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.THCConfiguration.THCEnabled = false
sp.THCConfiguration.THCOptInOnSvc = false
}
defer func() { sp.THCConfiguration.THCEvents = utils.THCEvents{} }()

hc := h.new(*sp)
if sp.THCConfiguration.THCEnabled {
if sp.THCConfiguration.THCOptInOnSvc {
klog.V(2).Infof("ServicePort %v has Transparent Health Checks enabled", sp.ID)
return h.sync(hc, nil, sp.THCConfiguration.THCEnabled, sp.THCConfiguration.THCEvents)
return h.sync(hc, nil, sp.THCConfiguration.THCOptInOnSvc, sp.THCConfiguration.THCEvents)
}
if probe != nil {
klog.V(2).Infof("Applying httpGet settings of readinessProbe to health check on port %+v", sp)
Expand All @@ -149,7 +149,7 @@ 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.THCConfiguration.THCEnabled, sp.THCConfiguration.THCEvents)
return h.sync(hc, bchcc, sp.THCConfiguration.THCOptInOnSvc, sp.THCConfiguration.THCEvents)
}

func (h *HealthChecks) notifyAboutTHC(hc *translator.HealthCheck, thcEvents utils.THCEvents) {
Expand Down Expand Up @@ -182,11 +182,11 @@ func (h *HealthChecks) notifyAboutTHC(hc *translator.HealthCheck, thcEvents util

// 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, thcEvents utils.THCEvents) (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, thcOptIn bool, thcEvents utils.THCEvents) (string, error) {
if bchcc != nil && thcOptIn {
klog.Warningf("BackendConfig exists and thcOptIn true simultaneously for %v. Ignoring transparent health check.", hc.Name)
thcOptIn = false
}
createOrUpdate := false

Expand Down Expand Up @@ -226,7 +226,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 !thcOptIn {
premergeHC := hc
hc = mergeUserSettings(existingHC, hc)
klog.V(3).Infof("Existing HC = %+v", existingHC)
Expand All @@ -248,7 +248,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, thcOptIn)
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") {
Expand Down
8 changes: 4 additions & 4 deletions pkg/healthchecks/healthchecks_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func init() {
}
}
if thc {
sp.THCConfiguration.THCEnabled = true
sp.THCConfiguration.THCOptInOnSvc = true
if mode == "reg" { // No THC without NEG.
continue
}
Expand Down Expand Up @@ -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, THCConfiguration: utils.THCConfiguration{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)
Expand Down Expand Up @@ -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, THCConfiguration: utils.THCConfiguration{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)
Expand Down Expand Up @@ -1647,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.THCConfiguration.THCEnabled == true {
if tc.sp.BackendConfig != nil || tc.sp.THCConfiguration.THCOptInOnSvc == true {
config := healthcheck.TransparentHC
if tc.sp.BackendConfig != nil {
config = healthcheck.BackendConfigHC
Expand Down
4 changes: 2 additions & 2 deletions pkg/utils/serviceport.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ type THCEvents struct {
}

type THCConfiguration struct {
THCEnabled bool
THCEvents THCEvents
THCOptInOnSvc bool
THCEvents THCEvents
}

// ServicePort maintains configuration for a single backend.
Expand Down

0 comments on commit a4bf4ca

Please sign in to comment.