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

Jump to bottom

Possibility of disabling audit log functionality per landscape #367

Merged
merged 10 commits into from
Sep 10, 2024
Merged

Possibility of disabling audit log functionality per landscape #367

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2b8e327
Checking if AL need to be enabled in strict mode
mvshao Sep 5, 2024
bea2e29
Refactor
mvshao Sep 5, 2024
1129976
Change the flow of Audit Log state
mvshao Sep 9, 2024
1e03eef
Merge branch 'main' into auditlog-strict
mvshao Sep 9, 2024
214ba67
Fix linter, fix tests
mvshao Sep 9, 2024
a6e909e
Fix imports
mvshao Sep 9, 2024
cf6ea01
gci file
mvshao Sep 9, 2024
0ea85ee
Apply review sugestions
mvshao Sep 9, 2024
eea82e3
Fix linter
mvshao Sep 9, 2024
fd9388d
Apply review sugestions
mvshao Sep 10, 2024
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
7 changes: 4 additions & 3 deletions api/v1/runtime_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,10 @@ const (
ConditionReasonSerializationError = RuntimeConditionReason("SerializationErr")
ConditionReasonDeleted = RuntimeConditionReason("Deleted")

ConditionReasonAdministratorsConfigured = RuntimeConditionReason("AdministratorsConfigured")
ConditionReasonAuditLogConfigured = RuntimeConditionReason("AuditLogConfigured")
ConditionReasonAuditLogError = RuntimeConditionReason("AuditLogErr")
ConditionReasonAdministratorsConfigured = RuntimeConditionReason("AdministratorsConfigured")
ConditionReasonAuditLogConfigured = RuntimeConditionReason("AuditLogConfigured")
ConditionReasonAuditLogError = RuntimeConditionReason("AuditLogErr")
ConditionReasonAuditLogMissingRegionMapping = RuntimeConditionReason("AuditLogMissingRegionMappingErr")
)

//+kubebuilder:object:root=true
Expand Down
62 changes: 59 additions & 3 deletions cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package main

import (
"encoding/json"
"flag"
"fmt"
"io"
Expand All @@ -27,6 +28,7 @@ import (
gardener_apis "github.com/gardener/gardener/pkg/client/core/clientset/versioned/typed/core/v1beta1"
"github.com/go-playground/validator/v10"
infrastructuremanagerv1 "github.com/kyma-project/infrastructure-manager/api/v1"
"github.com/kyma-project/infrastructure-manager/internal/auditlogging"
kubeconfig_controller "github.com/kyma-project/infrastructure-manager/internal/controller/kubeconfig"
"github.com/kyma-project/infrastructure-manager/internal/controller/metrics"
runtime_controller "github.com/kyma-project/infrastructure-manager/internal/controller/runtime"
Expand Down Expand Up @@ -76,6 +78,7 @@ func main() {
var enableRuntimeReconciler bool
var converterConfigFilepath string
var shootSpecDumpEnabled bool
var auditLogMandatory bool

flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
Expand All @@ -90,6 +93,7 @@ func main() {
flag.BoolVar(&enableRuntimeReconciler, "runtime-reconciler-enabled", defaultRuntimeReconcilerEnabled, "Feature flag for all runtime reconciler functionalities")
flag.StringVar(&converterConfigFilepath, "converter-config-filepath", "/converter-config/converter_config.json", "A file path to the gardener shoot converter configuration.")
flag.BoolVar(&shootSpecDumpEnabled, "shoot-spec-dump-enabled", false, "Feature flag to allow persisting specs of created shoots")
flag.BoolVar(&auditLogMandatory, "audit-log-mandatory", true, "Feature flag to enable strict mode for audit log configuration")
Copy link
Member

Choose a reason for hiding this comment

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

Please document this new paramenter (link)


opts := zap.Options{
Development: true,
Expand Down Expand Up @@ -171,10 +175,17 @@ func main() {
os.Exit(1)
}

err = validateAuditLogConfiguration(converterConfig.AuditLog.TenantConfigPath)
if err != nil {
setupLog.Error(err, "invalid Audit Log configuration")
os.Exit(1)
}

cfg := fsm.RCCfg{
Finalizer: infrastructuremanagerv1.Finalizer,
ShootNamesapace: gardenerNamespace,
ConverterConfig: converterConfig,
Finalizer: infrastructuremanagerv1.Finalizer,
ShootNamesapace: gardenerNamespace,
ConverterConfig: converterConfig,
AuditLogMandatory: auditLogMandatory,
}
if shootSpecDumpEnabled {
cfg.PVCPath = "/testdata/kim"
Expand Down Expand Up @@ -238,3 +249,48 @@ func initGardenerClients(kubeconfigPath string, namespace string) (client.Client
}
return gardenerClient, shootClient, dynamicKubeconfigAPI, nil
}

func validateAuditLogConfiguration(tenantConfigPath string) error {
getReaderCloser := func() (io.ReadCloser, error) {
return os.Open(tenantConfigPath)
}

f, err := getReaderCloser()

defer func(f io.ReadCloser) {
_ = f.Close()
}(f)

if err != nil {
setupLog.Error(err, "unable to open Audit Log configuration file")
return err
}

var auditLogConfig map[string]map[string]auditlogging.AuditLogData

if err = json.NewDecoder(f).Decode(&auditLogConfig); err != nil {
setupLog.Error(err, "unable to decode Audit Log configuration")
return err
}

if err = validateAuditLogDataMap(auditLogConfig); err != nil {
setupLog.Error(err, "invalid audit log configuration")
return err
}

return err
}

func validateAuditLogDataMap(data map[string]map[string]auditlogging.AuditLogData) error {
validate := validator.New(validator.WithRequiredStructEnabled())

for _, nestedMap := range data {
for _, auditLogData := range nestedMap {
if err := validate.Struct(auditLogData); err != nil {
return err
}
}
}

return nil
}
6 changes: 3 additions & 3 deletions internal/auditlogging/auditlogging.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ type auditLogConfig struct {
}

type AuditLogData struct {
TenantID string `json:"tenantID"`
ServiceURL string `json:"serviceURL"`
SecretName string `json:"secretName"`
TenantID string `json:"tenantID" validate:"required"`
ServiceURL string `json:"serviceURL" validate:"required,url"`
SecretName string `json:"secretName" validate:"required"`
}

type AuditlogExtensionConfig struct {
Expand Down
3 changes: 2 additions & 1 deletion internal/auditlogging/tests/auditlogging_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ package tests
import (
"context"
"fmt"
"testing"

gardener "github.com/gardener/gardener/pkg/apis/core/v1beta1"
"github.com/kyma-project/infrastructure-manager/internal/auditlogging"
"github.com/kyma-project/infrastructure-manager/internal/auditlogging/mocks"
"github.com/stretchr/testify/require"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/utils/ptr"
"testing"
)

func TestEnable(t *testing.T) {
Expand Down
7 changes: 4 additions & 3 deletions internal/controller/runtime/fsm/runtime_fsm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@ type writerGetter = func(filePath string) (io.Writer, error)

// runtime reconciler specific configuration
type RCCfg struct {
Finalizer string
PVCPath string
ShootNamesapace string
Finalizer string
PVCPath string
ShootNamesapace string
AuditLogMandatory bool
shoot.ConverterConfig
}

Expand Down
45 changes: 36 additions & 9 deletions internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package fsm

import (
"context"
"strings"

imv1 "github.com/kyma-project/infrastructure-manager/api/v1"
ctrl "sigs.k8s.io/controller-runtime"
Expand All @@ -24,19 +25,45 @@ func sFnConfigureAuditLog(ctx context.Context, m *fsm, s *systemState) (stateFn,
return updateStatusAndRequeueAfter(gardenerRequeueDuration)
}

if err != nil {
m.log.Error(err, "Failed to configure Audit Log")
s.instance.UpdateStatePending(
imv1.ConditionTypeAuditLogConfigured,
imv1.ConditionReasonAuditLogError,
"False",
err.Error(),
)
if err != nil { //nolint:nestif
errorMessage := err.Error()
if strings.Contains(errorMessage, "auditlog config for region") {
Copy link
Member

Choose a reason for hiding this comment

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

Could you extract the "auditlog config for region" string to a const in the auditlogging.go and use the same const here? This line relies on the content of the error message and if someone would change the auditlogging.go then it would break this business logic (unit tests are not detecting this scenario).

Here's another approach on how it should be achieved

if m.RCCfg.AuditLogMandatory {
m.log.Error(err, "Failed to configure Audit Log, missing region mapping for this shoot")
s.instance.UpdateStatePending(
imv1.ConditionTypeAuditLogConfigured,
imv1.ConditionReasonAuditLogMissingRegionMapping,
"False",
errorMessage,
)
} else {
m.log.Info(errorMessage, "Failed to configure Audit Log, missing region mapping for this shoot, but is not mandatory to be configured")
Copy link
Member

@Disper Disper Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A tiny rewording proposal to make it more clear that it's a acceptable behavior. Feel free to ignore this suggestion if you don't agree.

Suggested change
m.log.Info(errorMessage, "Failed to configure Audit Log, missing region mapping for this shoot, but is not mandatory to be configured")
m.log.Info(errorMessage, "Audit Log was not configured, missing region mapping for this shoot. Continuing without error because flag `audit-log-mandatory` is disabled.")

s.instance.UpdateStateReady(
imv1.ConditionTypeAuditLogConfigured,
imv1.ConditionReasonAuditLogMissingRegionMapping,
"Missing region mapping for this shoot. Audit Log is not mandatory. Skipping configuration")
}
} else {
if m.RCCfg.AuditLogMandatory {
m.log.Error(err, "Failed to configure Audit Log")
s.instance.UpdateStatePending(
imv1.ConditionTypeAuditLogConfigured,
imv1.ConditionReasonAuditLogError,
"False",
errorMessage)
} else {
m.log.Info(errorMessage, "Failed to configure Audit Log, but is not mandatory to be configured")
Copy link
Member

Choose a reason for hiding this comment

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

During verification I've encountered a panic:

2024-09-09T15:29:14+02:00       INFO    Observed a panic in reconciler: odd number of arguments passed as key-value pairs for logging   {"controller": "runtime", "controllerGroup": "infrastructuremanager.kyma-project.io", "controllerKind": "Runtime", "Runtime": {"name":"kim-md-al6","namespace":"kcp-system"}, "namespace": "kcp-system", "name": "kim-md-al6", "reconcileID": "1aa26a9a-cc11-4cd7-bdf7-b6b022203af0"}
panic: odd number of arguments passed as key-value pairs for logging [recovered]
        panic: odd number of arguments passed as key-value pairs for logging

Quoting logger api docs:

The key/value pairs must alternate string keys and arbitrary// values.

I wonder if just m.log.Info(errorMessage) wouldn't be enough here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to check code. In my opinion this additional info need to be present to clearly describe that Audit Log was set as not mandatory.

Copy link
Member

Choose a reason for hiding this comment

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

It's possible to add either one parameter (errorMessage) or 3/5/7 etc with additional ones for key name and the value . (always uneven).

When I executed this part I noticed the Error during enabling Audit Logs on shoot: kim-md-al8: missing mapping for selected region in provider config was the errorMessage and passed as a string wasFailed to configure Audit Log, missing region mapping for this shoot, but is not mandatory to be configured . which indeed contains an additional information that's mandatory.

So how about such invocation:
m.log.Info(errorMessage, "AuditLogMandatory", m.RCCfg.AuditLogMandatory)?

The log would look like this
2024-09-10T07:31:51+02:00 INFO reqID 1 Error during enabling Audit Logs on shoot: kim-md-al8: missing mapping for selected region in provider config {"AuditLogMandatory": false}

Copy link
Member

@Disper Disper Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Logging additional information on region and provider type could make it easier to quickly fix such issues. m.log.Info(errorMessage, "AuditLogMandatory", m.RCCfg.AuditLogMandatory, "providerType", s.shoot.Spec.Provider.Type, "region", s.shoot.Spec.Region)

s.instance.UpdateStateReady(
imv1.ConditionTypeAuditLogConfigured,
imv1.ConditionReasonAuditLogError,
"Configuration of Audit Log is not mandatory, error for context: "+errorMessage)
}
}
} else {
s.instance.UpdateStateReady(
imv1.ConditionTypeAuditLogConfigured,
imv1.ConditionReasonAuditLogConfigured,
"Audit Log configured successfully",
"Audit Log state completed successfully",
)
}

Expand Down
125 changes: 121 additions & 4 deletions internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@ func TestAuditLogState(t *testing.T) {
},
}

fsm := &fsm{AuditLogging: auditLog}
fsm.RCCfg.AuditLogMandatory = true

auditLog.On("Enable", ctx, shoot).Return(true, nil).Once()

// when
fsm := &fsm{AuditLogging: auditLog}
stateFn, _, _ := sFnConfigureAuditLog(ctx, fsm, systemState)

// set the time to its zero value for comparison purposes
Expand Down Expand Up @@ -64,14 +66,90 @@ func TestAuditLogState(t *testing.T) {
Type: string(v1.ConditionTypeAuditLogConfigured),
Status: "True",
Reason: string(v1.ConditionReasonAuditLogConfigured),
Message: "Audit Log configured successfully",
Message: "Audit Log state completed successfully",
},
}

fsm := &fsm{AuditLogging: auditLog}
fsm.RCCfg.AuditLogMandatory = true

auditLog.On("Enable", ctx, shoot).Return(false, nil).Once()

// when
stateFn, _, _ := sFnConfigureAuditLog(ctx, fsm, systemState)

// set the time to its zero value for comparison purposes
systemState.instance.Status.Conditions[0].LastTransitionTime = metav1.Time{}

// then
auditLog.AssertExpectations(t)
require.Contains(t, stateFn.name(), "sFnUpdateStatus")
assert.Equal(t, v1.RuntimeStateReady, string(systemState.instance.Status.State))
assert.Equal(t, expectedRuntimeConditions, systemState.instance.Status.Conditions)
})

t.Run("Should set status on Runtime CR when region mapping is missing and Audit Log is mandatory (hard error)", func(t *testing.T) {
// given
ctx := context.Background()
auditLog := &mocks.AuditLogging{}
shoot := shootForTest()
instance := runtimeForTest()
systemState := &systemState{
instance: instance,
shoot: shoot,
}
expectedRuntimeConditions := []metav1.Condition{
{
Type: string(v1.ConditionTypeAuditLogConfigured),
Status: "False",
Reason: string(v1.ConditionReasonAuditLogMissingRegionMapping),
Message: "auditlog config for region",
},
}

fsm := &fsm{AuditLogging: auditLog}
fsm.RCCfg.AuditLogMandatory = true

auditLog.On("Enable", ctx, shoot).Return(false, errors.New("auditlog config for region")).Once()

// when
stateFn, _, _ := sFnConfigureAuditLog(ctx, fsm, systemState)

// set the time to its zero value for comparison purposes
systemState.instance.Status.Conditions[0].LastTransitionTime = metav1.Time{}

// then
auditLog.AssertExpectations(t)
require.Contains(t, stateFn.name(), "sFnUpdateStatus")
assert.Equal(t, v1.RuntimeStateFailed, string(systemState.instance.Status.State))
assert.Equal(t, expectedRuntimeConditions, systemState.instance.Status.Conditions)
})

t.Run("Should set status on Runtime CR when region mapping is missing and Audit Log is not mandatory (proceed to next step)", func(t *testing.T) {
// given
ctx := context.Background()
auditLog := &mocks.AuditLogging{}
shoot := shootForTest()
instance := runtimeForTest()
systemState := &systemState{
instance: instance,
shoot: shoot,
}
expectedRuntimeConditions := []metav1.Condition{
{
Type: string(v1.ConditionTypeAuditLogConfigured),
Status: "True",
Reason: string(v1.ConditionReasonAuditLogMissingRegionMapping),
Message: "Missing region mapping for this shoot. Audit Log is not mandatory. Skipping configuration",
},
}

fsm := &fsm{AuditLogging: auditLog}
fsm.RCCfg.AuditLogMandatory = false

auditLog.On("Enable", ctx, shoot).Return(false, errors.New("auditlog config for region")).Once()

// when
stateFn, _, _ := sFnConfigureAuditLog(ctx, fsm, systemState)

// set the time to its zero value for comparison purposes
Expand All @@ -84,7 +162,7 @@ func TestAuditLogState(t *testing.T) {
assert.Equal(t, expectedRuntimeConditions, systemState.instance.Status.Conditions)
})

t.Run("Should stop in case of error during configuration and set status on Runtime CR", func(t *testing.T) {
t.Run("Should stop in case of error during configuration if Audit Log is mandatory, and set status on Runtime CR (hard error)", func(t *testing.T) {
// given
ctx := context.Background()
auditLog := &mocks.AuditLogging{}
Expand All @@ -103,10 +181,12 @@ func TestAuditLogState(t *testing.T) {
},
}

fsm := &fsm{AuditLogging: auditLog}
fsm.RCCfg.AuditLogMandatory = true

auditLog.On("Enable", ctx, shoot).Return(false, errors.New("some error during configuration")).Once()

// when
fsm := &fsm{AuditLogging: auditLog}
stateFn, _, _ := sFnConfigureAuditLog(ctx, fsm, systemState)

// set the time to its zero value for comparison purposes
Expand All @@ -118,6 +198,43 @@ func TestAuditLogState(t *testing.T) {
assert.Equal(t, v1.RuntimeStateFailed, string(systemState.instance.Status.State))
assert.Equal(t, expectedRuntimeConditions, systemState.instance.Status.Conditions)
})

t.Run("Should stop in case of error during configuration if Audit Log is not mandatory, and set status on Runtime CR (proceed to next step)", func(t *testing.T) {
// given
ctx := context.Background()
auditLog := &mocks.AuditLogging{}
shoot := shootForTest()
instance := runtimeForTest()
systemState := &systemState{
instance: instance,
shoot: shoot,
}
expectedRuntimeConditions := []metav1.Condition{
{
Type: string(v1.ConditionTypeAuditLogConfigured),
Status: "True",
Reason: string(v1.ConditionReasonAuditLogError),
Message: "Configuration of Audit Log is not mandatory, error for context: some error during configuration",
},
}

fsm := &fsm{AuditLogging: auditLog}
fsm.RCCfg.AuditLogMandatory = false

auditLog.On("Enable", ctx, shoot).Return(false, errors.New("some error during configuration")).Once()

// when
stateFn, _, _ := sFnConfigureAuditLog(ctx, fsm, systemState)

// set the time to its zero value for comparison purposes
systemState.instance.Status.Conditions[0].LastTransitionTime = metav1.Time{}

// then
auditLog.AssertExpectations(t)
require.Contains(t, stateFn.name(), "sFnUpdateStatus")
assert.Equal(t, v1.RuntimeStateReady, string(systemState.instance.Status.State))
assert.Equal(t, expectedRuntimeConditions, systemState.instance.Status.Conditions)
})
}

func shootForTest() *gardener.Shoot {
Expand Down
Loading
Loading