Skip to content
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

Toggle features based on what the remotely configured sensor version supports #189

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Toggle features based on what the remotely configured sensor version supports #189

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
82 changes: 65 additions & 17 deletions cbcontainers/remote_configuration/change_applier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,75 @@ import (
)

// ApplyConfigChangeToCR will modify CR according to the values in the configuration change provided
func ApplyConfigChangeToCR(change models.ConfigurationChange, cr *cbcontainersv1.CBContainersAgent) {
// If sensorMetadata is provided, specific supported features will be enabled or disabled based on their compatibility with the requested agent version
func ApplyConfigChangeToCR(change models.ConfigurationChange, cr *cbcontainersv1.CBContainersAgent, sensorMetadata []models.SensorMetadata) {
if change.AgentVersion != nil {
cr.Spec.Version = *change.AgentVersion

// We do not set the tag to the version as that would make it harder to upgrade manually
// Instead, we reset any "custom" tags, which will fall back to the default (spec.Version)
images := []*cbcontainersv1.CBContainersImageSpec{
&cr.Spec.Components.Basic.Monitor.Image,
&cr.Spec.Components.Basic.Enforcer.Image,
&cr.Spec.Components.Basic.StateReporter.Image,
&cr.Spec.Components.ClusterScanning.ImageScanningReporter.Image,
&cr.Spec.Components.ClusterScanning.ClusterScannerAgent.Image,
&cr.Spec.Components.RuntimeProtection.Sensor.Image,
&cr.Spec.Components.RuntimeProtection.Resolver.Image,
}
if cr.Spec.Components.Cndr != nil {
images = append(images, &cr.Spec.Components.Cndr.Sensor.Image)
}
resetImageTagsInCR(cr)
toggleFeaturesBasedOnCompatibility(cr, *change.AgentVersion, sensorMetadata)
}
}

for _, i := range images {
i.Tag = ""
func resetImageTagsInCR(cr *cbcontainersv1.CBContainersAgent) {
// We do not set the tag to the version as that would make it harder to upgrade manually
// Instead, we reset any "custom" tags, which will fall back to the default (spec.Version)
images := []*cbcontainersv1.CBContainersImageSpec{
&cr.Spec.Components.Basic.Monitor.Image,
&cr.Spec.Components.Basic.Enforcer.Image,
&cr.Spec.Components.Basic.StateReporter.Image,
&cr.Spec.Components.ClusterScanning.ImageScanningReporter.Image,
&cr.Spec.Components.ClusterScanning.ClusterScannerAgent.Image,
&cr.Spec.Components.RuntimeProtection.Sensor.Image,
&cr.Spec.Components.RuntimeProtection.Resolver.Image,
}
if cr.Spec.Components.Cndr != nil {
images = append(images, &cr.Spec.Components.Cndr.Sensor.Image)
}

for _, i := range images {
i.Tag = ""
}
}

func toggleFeaturesBasedOnCompatibility(cr *cbcontainersv1.CBContainersAgent, version string, sensorMetadata []models.SensorMetadata) {
var sensorMetadataForVersion *models.SensorMetadata
for _, sensor := range sensorMetadata {
if sensor.Version == version {
sensorMetadataForVersion = &sensor
break
}
}
if sensorMetadataForVersion == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is expected to happen in such case? I guess this means no sensor version was matched right? Do we want to return an error for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I thought we should return error as well and abandon the change, but the following convinced me otherwise:

  • For customers, the common case is to use the valid versions. We already do a check on the backend when creating the Change - and if the version is semantic, we require that we know about it (so this check here should never be nil). If the version is not semantic, we assume that it is a special version for the customer so we allow the flow to go through - but that should be a rare case for hotfixes or private betas. We don't expect customers to use versions not in the list otherwise.
  • For development flows, this check doesn't make sense as you often have sensor versions that are not in the "main" list and that is OK

So for these cases we assume nothing about the feature support and leave them as-is. Of course, the customer might make an invalid change but we can't verify that.

return
}

trueRef, falseRef := true, false

if sensorMetadataForVersion.SupportsClusterScanning {
cr.Spec.Components.ClusterScanning.Enabled = &trueRef
} else {
cr.Spec.Components.ClusterScanning.Enabled = &falseRef
}

if sensorMetadataForVersion.SupportsClusterScanningSecrets {
cr.Spec.Components.ClusterScanning.ClusterScannerAgent.CLIFlags.EnableSecretDetection = true
} else {
cr.Spec.Components.ClusterScanning.ClusterScannerAgent.CLIFlags.EnableSecretDetection = false
}

if sensorMetadataForVersion.SupportsRuntime {
cr.Spec.Components.RuntimeProtection.Enabled = &trueRef
} else {
cr.Spec.Components.RuntimeProtection.Enabled = &falseRef
}

if cr.Spec.Components.Cndr == nil {
cr.Spec.Components.Cndr = &cbcontainersv1.CBContainersCndrSpec{}
}
if sensorMetadataForVersion.SupportsCndr {
cr.Spec.Components.Cndr.Enabled = &trueRef
} else {
cr.Spec.Components.Cndr.Enabled = &falseRef
}
}
109 changes: 106 additions & 3 deletions cbcontainers/remote_configuration/change_applier_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package remote_configuration_test

import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
cbcontainersv1 "github.com/vmware/cbcontainers-operator/api/v1"
"github.com/vmware/cbcontainers-operator/cbcontainers/models"
"github.com/vmware/cbcontainers-operator/cbcontainers/remote_configuration"
Expand All @@ -16,7 +17,7 @@ func TestVersionIsAppliedCorrectly(t *testing.T) {
cr := cbcontainersv1.CBContainersAgent{Spec: cbcontainersv1.CBContainersAgentSpec{Version: originalVersion}}
change := models.ConfigurationChange{AgentVersion: &newVersion}

remote_configuration.ApplyConfigChangeToCR(change, &cr)
remote_configuration.ApplyConfigChangeToCR(change, &cr, nil)
assert.Equal(t, newVersion, cr.Spec.Version)
}

Expand All @@ -25,7 +26,7 @@ func TestMissingVersionDoesNotModifyCR(t *testing.T) {
cr := cbcontainersv1.CBContainersAgent{Spec: cbcontainersv1.CBContainersAgentSpec{Version: originalVersion}}
change := models.ConfigurationChange{AgentVersion: nil}

remote_configuration.ApplyConfigChangeToCR(change, &cr)
remote_configuration.ApplyConfigChangeToCR(change, &cr, nil)
assert.Equal(t, originalVersion, cr.Spec.Version)

}
Expand Down Expand Up @@ -90,7 +91,7 @@ func TestVersionOverwritesCustomTagsByRemovingThem(t *testing.T) {
newVersion := "new-version"
change := models.ConfigurationChange{AgentVersion: &newVersion}

remote_configuration.ApplyConfigChangeToCR(change, &cr)
remote_configuration.ApplyConfigChangeToCR(change, &cr, nil)
assert.Equal(t, newVersion, cr.Spec.Version)
// To avoid keeping "custom" tags forever, the apply change should instead reset all such fields
// => the operator will use the common version instead
Expand All @@ -104,6 +105,108 @@ func TestVersionOverwritesCustomTagsByRemovingThem(t *testing.T) {
assert.Empty(t, cr.Spec.Components.Cndr.Sensor.Image.Tag)
}

func TestFeatureToggles(t *testing.T) {
testCases := []struct {
name string
sensorCompatibility models.SensorMetadata
assert func(agent *cbcontainersv1.CBContainersAgent)
}{
{
name: "cluster scanner supported, should enable",
sensorCompatibility: models.SensorMetadata{
SupportsClusterScanning: true,
},
assert: func(agent *cbcontainersv1.CBContainersAgent) {
require.NotNil(t, agent.Spec.Components.ClusterScanning.Enabled)
assert.True(t, *agent.Spec.Components.ClusterScanning.Enabled)
},
},
{
name: "cluster scanner not supported, should disable",
sensorCompatibility: models.SensorMetadata{
SupportsClusterScanning: false,
},
assert: func(agent *cbcontainersv1.CBContainersAgent) {
require.NotNil(t, agent.Spec.Components.ClusterScanning.Enabled)
assert.False(t, *agent.Spec.Components.ClusterScanning.Enabled)
},
},
{
name: "secret scanning supported, should enable",
sensorCompatibility: models.SensorMetadata{
SupportsClusterScanningSecrets: true,
},
assert: func(agent *cbcontainersv1.CBContainersAgent) {
assert.True(t, agent.Spec.Components.ClusterScanning.ClusterScannerAgent.CLIFlags.EnableSecretDetection)
},
},
{
name: "secret scanning not supported, should disable",
sensorCompatibility: models.SensorMetadata{
SupportsClusterScanningSecrets: false,
},
assert: func(agent *cbcontainersv1.CBContainersAgent) {
assert.False(t, agent.Spec.Components.ClusterScanning.ClusterScannerAgent.CLIFlags.EnableSecretDetection)
},
},
{
name: "runtime protection supported, should enable",
sensorCompatibility: models.SensorMetadata{
SupportsRuntime: true,
},
assert: func(agent *cbcontainersv1.CBContainersAgent) {
require.NotNil(t, agent.Spec.Components.RuntimeProtection.Enabled)
assert.True(t, *agent.Spec.Components.RuntimeProtection.Enabled)
},
},
{
name: "runtime protection not supported, should disable",
sensorCompatibility: models.SensorMetadata{
SupportsRuntime: false,
},
assert: func(agent *cbcontainersv1.CBContainersAgent) {
require.NotNil(t, agent.Spec.Components.RuntimeProtection.Enabled)
assert.False(t, *agent.Spec.Components.RuntimeProtection.Enabled)
},
},
{
name: "CNDR supported, should enable",
sensorCompatibility: models.SensorMetadata{
SupportsCndr: true,
},
assert: func(agent *cbcontainersv1.CBContainersAgent) {
require.NotNil(t, agent.Spec.Components.Cndr)
require.NotNil(t, agent.Spec.Components.Cndr.Enabled)
assert.True(t, *agent.Spec.Components.Cndr.Enabled)
},
},
{
name: "CNDR not supported, should disable",
sensorCompatibility: models.SensorMetadata{
SupportsCndr: false,
},
assert: func(agent *cbcontainersv1.CBContainersAgent) {
require.NotNil(t, agent.Spec.Components.Cndr)
require.NotNil(t, agent.Spec.Components.Cndr.Enabled)
assert.False(t, *agent.Spec.Components.Cndr.Enabled)
},
},
}

for _, tC := range testCases {
t.Run(tC.name, func(t *testing.T) {
version := "2.3.4"
cr := &cbcontainersv1.CBContainersAgent{}
change := models.ConfigurationChange{AgentVersion: &version}
tC.sensorCompatibility.Version = version

remote_configuration.ApplyConfigChangeToCR(change, cr, []models.SensorMetadata{tC.sensorCompatibility})

tC.assert(cr)
})
}
}

// randomPendingConfigChange creates a non-empty configuration change with randomly populated fields in pending state
// the change is not guaranteed to be 100% valid
func randomPendingConfigChange() models.ConfigurationChange {
Expand Down
8 changes: 7 additions & 1 deletion cbcontainers/remote_configuration/configurator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,13 @@ func (configurator *Configurator) applyChangeToCR(ctx context.Context, apiGatewa
if err := validator.ValidateChange(change, cr); err != nil {
return invalidChangeError{msg: err.Error()}
}
ApplyConfigChangeToCR(change, cr)

sensorMeta, err := apiGateway.GetSensorMetadata()
if err != nil {
return fmt.Errorf("failed to load sensor metadata from backend; %w", err)
}

ApplyConfigChangeToCR(change, cr, sensorMeta)
return configurator.k8sClient.Update(ctx, cr)
}

Expand Down
49 changes: 49 additions & 0 deletions cbcontainers/remote_configuration/configurator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ func TestConfigChangeIsAppliedAndAcknowledgedCorrectly(t *testing.T) {

setupCRInK8S(mocks.k8sClient, cr)
mocks.validator.EXPECT().ValidateChange(configChange, cr).Return(nil)
mocks.apiGateway.EXPECT().GetSensorMetadata().Return([]models.SensorMetadata{{Version: expectedAgentVersion}}, nil)
mocks.apiGateway.EXPECT().GetConfigurationChanges(gomock.Any(), mocks.stubClusterID).Return([]models.ConfigurationChange{configChange}, nil)

// Setup mock assertions
Expand All @@ -113,6 +114,46 @@ func TestConfigChangeIsAppliedAndAcknowledgedCorrectly(t *testing.T) {
assert.NoError(t, err)
}

func TestWhenSensorMetadataIsAvailableItIsUsed(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

configurator, mocks := setupConfigurator(ctrl)

expectedAgentVersion := "3.0.0"
cr := &cbcontainersv1.CBContainersAgent{}
configChange := randomPendingConfigChange()
configChange.AgentVersion = &expectedAgentVersion

setupCRInK8S(mocks.k8sClient, cr)
setupValidatorAcceptAll(mocks.validator)
mocks.apiGateway.EXPECT().GetSensorMetadata().Return([]models.SensorMetadata{{
Version: expectedAgentVersion,
SupportsRuntime: true,
SupportsClusterScanning: true,
SupportsClusterScanningSecrets: true,
SupportsCndr: true,
}}, nil)
mocks.apiGateway.EXPECT().GetConfigurationChanges(gomock.Any(), mocks.stubClusterID).Return([]models.ConfigurationChange{configChange}, nil)

// Setup mock assertions
mocks.apiGateway.EXPECT().UpdateConfigurationChangeStatus(gomock.Any(), gomock.Any()).Return(nil)

setupUpdateCRMock(t, mocks.k8sClient, func(agent *cbcontainersv1.CBContainersAgent) {
assert.True(t, *agent.Spec.Components.ClusterScanning.Enabled)
assert.True(t, agent.Spec.Components.ClusterScanning.ClusterScannerAgent.CLIFlags.EnableSecretDetection)
assert.True(t, *agent.Spec.Components.RuntimeProtection.Enabled)
assert.True(t, *agent.Spec.Components.Cndr.Enabled)
})

err := configurator.RunIteration(context.Background())
assert.NoError(t, err)
}

func TestWhenSensorMetadataFailsShouldPropagateErr(t *testing.T) {

}

func TestWhenChangeIsNotApplicableShouldReturnError(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down Expand Up @@ -202,6 +243,7 @@ func TestWhenThereAreMultiplePendingChangesTheOldestIsSelected(t *testing.T) {

setupCRInK8S(mocks.k8sClient, nil)
setupValidatorAcceptAll(mocks.validator)
setupEmptySensorMetadata(mocks.apiGateway)
mocks.apiGateway.EXPECT().GetConfigurationChanges(gomock.Any(), mocks.stubClusterID).Return([]models.ConfigurationChange{newerChange, olderChange}, nil)

setupUpdateCRMock(t, mocks.k8sClient, func(agent *cbcontainersv1.CBContainersAgent) {
Expand Down Expand Up @@ -259,6 +301,7 @@ func TestWhenUpdatingCRFailsChangeIsUpdatedAsFailed(t *testing.T) {

setupCRInK8S(mocks.k8sClient, nil)
setupValidatorAcceptAll(mocks.validator)
setupEmptySensorMetadata(mocks.apiGateway)
mocks.apiGateway.EXPECT().GetConfigurationChanges(gomock.Any(), mocks.stubClusterID).Return([]models.ConfigurationChange{configChange}, nil)

errFromService := errors.New("some error")
Expand Down Expand Up @@ -292,6 +335,7 @@ func TestWhenUpdatingStatusToBackendFailsShouldReturnError(t *testing.T) {

setupCRInK8S(mocks.k8sClient, nil)
setupValidatorAcceptAll(mocks.validator)
setupEmptySensorMetadata(mocks.apiGateway)
mocks.apiGateway.EXPECT().GetConfigurationChanges(gomock.Any(), mocks.stubClusterID).Return([]models.ConfigurationChange{configChange}, nil)
mocks.k8sClient.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)

Expand Down Expand Up @@ -373,3 +417,8 @@ func setupUpdateCRMock(t *testing.T, mock *k8sMocks.MockClient, assert func(*cbc
func setupValidatorAcceptAll(validator *mocks.MockChangeValidator) {
validator.EXPECT().ValidateChange(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
}

// setupEmptySensorMetadata simulates the case where no sensor metadata is available; hence no feature toggles are enabled
func setupEmptySensorMetadata(api *mocks.MockApiGateway) {
api.EXPECT().GetSensorMetadata().Return([]models.SensorMetadata{}, nil)
}
Loading