Skip to content

Add centralized field ownership tracker for shared resources #4523

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

Draft
caseydavenport wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add centralized field ownership tracker for shared resources #4523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
58 changes: 14 additions & 44 deletions pkg/controller/installation/bpf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,41 +15,34 @@
package installation

import (
"errors"
"reflect"
"strconv"

v3 "github.com/tigera/api/pkg/apis/projectcalico/v3"
"github.com/tigera/operator/pkg/controller/utils"

"github.com/tigera/operator/pkg/common"
"github.com/tigera/operator/pkg/controller/utils"
"github.com/tigera/operator/pkg/controller/utils/fieldowner"
"github.com/tigera/operator/pkg/render"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
)

// bpfValidateAnnotations validate Felix Configuration annotations match BPF Enabled spec for all scenarios.
func bpfValidateAnnotations(fc *v3.FelixConfiguration) error {
var annotationValue *bool
if fc.Annotations[render.BPFOperatorAnnotation] != "" {
v, err := strconv.ParseBool(fc.Annotations[render.BPFOperatorAnnotation])
annotationValue = &v
if err != nil {
return err
}
}
const installationControllerName = "installation"

// The values are considered matching if one of the following is true:
// - Both values are nil
// - Neither are nil and they have the same value.
// Otherwise, the we consider the annotation to not match the spec field.
match := annotationValue == nil && fc.Spec.BPFEnabled == nil
match = match || annotationValue != nil && fc.Spec.BPFEnabled != nil && *annotationValue == *fc.Spec.BPFEnabled
func setBPFEnabledOnFelixConfiguration(fc *v3.FelixConfiguration, bpfEnabled bool) error {
t := fieldowner.ForObject(installationControllerName, fc)
t.MigrateAnnotation(fc, "BPFEnabled", render.BPFOperatorAnnotation)

if !match {
return errors.New(`unable to set bpfEnabled: FelixConfiguration "default" has been modified by someone else, refusing to override potential user configuration`)
desired := strconv.FormatBool(bpfEnabled)
shouldSet, err := t.Manage("BPFEnabled", fieldowner.FormatValue(fc.Spec.BPFEnabled), desired, fieldowner.ConflictError)
if err != nil {
return err
}

if shouldSet {
fc.Spec.BPFEnabled = &bpfEnabled
}
t.Flush(fc)
return nil
}

Expand All @@ -71,7 +64,6 @@ func isRolloutCompleteWithBPFVolumes(ds *appsv1.DaemonSet) bool {

for _, volume := range ds.Spec.Template.Spec.Volumes {
if volume.Name == render.BPFVolumeName {
// return ds.Status.CurrentNumberScheduled == ds.Status.UpdatedNumberScheduled && ds.Status.CurrentNumberScheduled == ds.Status.NumberAvailable
if ds.Status.CurrentNumberScheduled == ds.Status.UpdatedNumberScheduled && ds.Status.CurrentNumberScheduled == ds.Status.NumberAvailable {
return true
} else {
Expand All @@ -82,28 +74,6 @@ func isRolloutCompleteWithBPFVolumes(ds *appsv1.DaemonSet) bool {
return false
}

func setBPFEnabledOnFelixConfiguration(fc *v3.FelixConfiguration, bpfEnabled bool) error {
err := bpfValidateAnnotations(fc)
if err != nil {
return err
}

text := strconv.FormatBool(bpfEnabled)

// Add an annotation matching the field value. This allows the operator to compare the annotation to the field
// when performing an update to determine if another entity has modified the value since the last write.
var fcAnnotations map[string]string
if fc.Annotations == nil {
fcAnnotations = make(map[string]string)
} else {
fcAnnotations = fc.Annotations
}
fcAnnotations[render.BPFOperatorAnnotation] = text
fc.SetAnnotations(fcAnnotations)
fc.Spec.BPFEnabled = &bpfEnabled
return nil
}

func bpfEnabledOnDaemonsetWithEnvVar(ds *appsv1.DaemonSet) (bool, error) {
bpfEnabledStatus := false
var err error
Expand Down
148 changes: 89 additions & 59 deletions pkg/controller/installation/bpf_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,30 +15,38 @@
package installation

import (
"strconv"

v3 "github.com/tigera/api/pkg/apis/projectcalico/v3"
"github.com/tigera/operator/pkg/common"

"github.com/tigera/operator/pkg/render"
"encoding/json"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

v3 "github.com/tigera/api/pkg/apis/projectcalico/v3"

"github.com/tigera/operator/pkg/common"
"github.com/tigera/operator/pkg/render"
)

const managedFieldsAnnotation = "operator.tigera.io/managed-fields-installation"

// trackedFields parses the consolidated managed-fields annotation on the FC.
func trackedFields(fc *v3.FelixConfiguration) map[string]string {
raw, ok := fc.Annotations[managedFieldsAnnotation]
if !ok {
return nil
}
var fields map[string]string
ExpectWithOffset(1, json.Unmarshal([]byte(raw), &fields)).To(Succeed())
return fields
}

var _ = Describe("BPF functional tests", func() {
Context("Annotations validation tests", func() {
Context("setBPFEnabledOnFelixConfiguration conflict detection", func() {
var fc *v3.FelixConfiguration
var textTrue, textFalse string
var enabled, notEnabled bool

textTrue = strconv.FormatBool(true)
textFalse = strconv.FormatBool(false)

enabled = true
notEnabled = false

Expand All @@ -52,57 +60,86 @@ var _ = Describe("BPF functional tests", func() {
}
})

It("should return error if the value is not a boolean", func() {
fc.Annotations[render.BPFOperatorAnnotation] = "NotBoolean"
err := bpfValidateAnnotations(fc)
It("should error when spec was modified out-of-band (tracked true, spec false)", func() {
// Simulate: operator previously set BPFEnabled=true, user changed to false.
fc.Annotations[managedFieldsAnnotation] = `{"BPFEnabled":"true"}`
fc.Spec.BPFEnabled = &notEnabled
err := setBPFEnabledOnFelixConfiguration(fc, true)
Expect(err).Should(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("modified by another actor"))
})

It("should return error if the annotation is nil and the spec field is not", func() {
fc.Annotations = nil
It("should error when spec was modified out-of-band (tracked false, spec true)", func() {
fc.Annotations[managedFieldsAnnotation] = `{"BPFEnabled":"false"}`
fc.Spec.BPFEnabled = &enabled
err := bpfValidateAnnotations(fc)
err := setBPFEnabledOnFelixConfiguration(fc, false)
Expect(err).Should(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("modified by another actor"))
})

It("should return error if the annotation is not nil and the spec field is", func() {
fc.Annotations[render.BPFOperatorAnnotation] = textFalse
err := bpfValidateAnnotations(fc)
Expect(err).Should(HaveOccurred())
It("should succeed when both are nil (fresh install)", func() {
fc.Annotations = nil
err := setBPFEnabledOnFelixConfiguration(fc, true)
Expect(err).ShouldNot(HaveOccurred())
Expect(*fc.Spec.BPFEnabled).To(BeTrue())
Expect(trackedFields(fc)).To(HaveKeyWithValue("BPFEnabled", "true"))
})

It("should return error if the annotation is true and the spec field is false", func() {
fc.Annotations[render.BPFOperatorAnnotation] = textTrue
It("should succeed when tracked matches spec", func() {
fc.Annotations[managedFieldsAnnotation] = `{"BPFEnabled":"false"}`
fc.Spec.BPFEnabled = &notEnabled
err := bpfValidateAnnotations(fc)
Expect(err).Should(HaveOccurred())
})

It("should return error if the annotation is false and the spec field is true", func() {
fc.Annotations[render.BPFOperatorAnnotation] = textFalse
fc.Spec.BPFEnabled = &enabled
err := bpfValidateAnnotations(fc)
Expect(err).Should(HaveOccurred())
})

It("should return valid if both annotation and the spec field are nil", func() {
fc.Annotations = nil
err := bpfValidateAnnotations(fc)
err := setBPFEnabledOnFelixConfiguration(fc, false)
Expect(err).ShouldNot(HaveOccurred())
})

It("should return valid if the annotation is false and the spec field is false", func() {
fc.Annotations[render.BPFOperatorAnnotation] = textFalse
It("should update tracked value when operator changes desired value", func() {
fc.Annotations[managedFieldsAnnotation] = `{"BPFEnabled":"false"}`
fc.Spec.BPFEnabled = &notEnabled
err := bpfValidateAnnotations(fc)
err := setBPFEnabledOnFelixConfiguration(fc, true)
Expect(err).ShouldNot(HaveOccurred())
Expect(*fc.Spec.BPFEnabled).To(BeTrue())
Expect(trackedFields(fc)).To(HaveKeyWithValue("BPFEnabled", "true"))
})
})

It("should return valid if the annotation is true and the spec field is true", func() {
fc.Annotations[render.BPFOperatorAnnotation] = textTrue
fc.Spec.BPFEnabled = &enabled
err := bpfValidateAnnotations(fc)
Context("Legacy annotation migration", func() {
It("should migrate old per-field annotation to consolidated tracker", func() {
fc := &v3.FelixConfiguration{
ObjectMeta: metav1.ObjectMeta{
Name: "default",
Annotations: map[string]string{
render.BPFOperatorAnnotation: "true",
},
},
Spec: v3.FelixConfigurationSpec{
BPFEnabled: boolPtr(true),
},
}

err := setBPFEnabledOnFelixConfiguration(fc, true)
Expect(err).ShouldNot(HaveOccurred())
// Old annotation should be removed.
Expect(fc.Annotations).NotTo(HaveKey(render.BPFOperatorAnnotation))
// New consolidated annotation should exist.
Expect(trackedFields(fc)).To(HaveKeyWithValue("BPFEnabled", "true"))
})

It("should detect conflict after migrating legacy annotation", func() {
fc := &v3.FelixConfiguration{
ObjectMeta: metav1.ObjectMeta{
Name: "default",
Annotations: map[string]string{
render.BPFOperatorAnnotation: "true",
},
},
Spec: v3.FelixConfigurationSpec{
BPFEnabled: boolPtr(false), // User changed it.
},
}

err := setBPFEnabledOnFelixConfiguration(fc, true)
Expect(err).Should(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("modified by another actor"))
})
})

Expand Down Expand Up @@ -263,29 +300,22 @@ var _ = Describe("BPF functional tests", func() {
}
})

It("should return error if annotation validation failed", func() {
fc.Annotations = make(map[string]string)
fc.Annotations[render.BPFOperatorAnnotation] = "NotBoolean"
err := bpfValidateAnnotations(fc)
Expect(err).Should(HaveOccurred())
err = setBPFEnabledOnFelixConfiguration(fc, true)
Expect(err).Should(HaveOccurred())
})

It("should set correct annotation", func() {
It("should set correct annotation and spec value", func() {
err := setBPFEnabledOnFelixConfiguration(fc, true)
Expect(err).ShouldNot(HaveOccurred())

annotations := fc.Annotations[render.BPFOperatorAnnotation]
Expect(annotations).To(Equal("true"))
Expect(trackedFields(fc)).To(HaveKeyWithValue("BPFEnabled", "true"))
Expect(*fc.Spec.BPFEnabled).To(Equal(true))

err = setBPFEnabledOnFelixConfiguration(fc, false)
Expect(err).ShouldNot(HaveOccurred())

annotations = fc.Annotations[render.BPFOperatorAnnotation]
Expect(annotations).To(Equal("false"))
Expect(trackedFields(fc)).To(HaveKeyWithValue("BPFEnabled", "false"))
Expect(*fc.Spec.BPFEnabled).To(Equal(false))
})
})
})

func boolPtr(v bool) *bool {
return &v
}
Loading
Loading