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

[v0.11] Fix defaults from gitrepo restriction #3080

Merged
merged 1 commit into from
Nov 14, 2024
Merged

[v0.11] Fix defaults from gitrepo restriction #3080

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
3 changes: 1 addition & 2 deletions internal/cmd/controller/gitops/reconciler/gitjob_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,7 @@ func (r *GitJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr

// Restrictions / Overrides, gitrepo reconciler is responsible for setting error in status
oldStatus := gitrepo.Status.DeepCopy()
_, err := authorizeAndAssignDefaults(ctx, r.Client, gitrepo)
if err != nil {
if err := AuthorizeAndAssignDefaults(ctx, r.Client, gitrepo); err != nil {
r.Recorder.Event(gitrepo, fleetevent.Warning, "FailedToApplyRestrictions", err.Error())
return ctrl.Result{}, updateErrorStatus(ctx, r.Client, req.NamespacedName, *oldStatus, err)
}
Expand Down
36 changes: 21 additions & 15 deletions internal/cmd/controller/gitops/reconciler/restrictions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,50 +11,56 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

// authorizeAndAssignDefaults applies restrictions and returns a new GitRepo if it passes the restrictions
func authorizeAndAssignDefaults(ctx context.Context, c client.Client, gitrepo *fleet.GitRepo) (*fleet.GitRepo, error) {
// AuthorizeAndAssignDefaults applies restrictions and mutates the passed in
// GitRepo if it passes the restrictions
func AuthorizeAndAssignDefaults(ctx context.Context, c client.Client, gitrepo *fleet.GitRepo) error {
restrictions := &fleet.GitRepoRestrictionList{}
err := c.List(ctx, restrictions, client.InNamespace(gitrepo.Namespace))
if err != nil {
return nil, err
return err
}

if len(restrictions.Items) == 0 {
return gitrepo, nil
return nil
}

restriction := aggregate(restrictions.Items)
gitrepo = gitrepo.DeepCopy()

if len(restriction.AllowedTargetNamespaces) > 0 && gitrepo.Spec.TargetNamespace == "" {
return nil, fmt.Errorf("empty targetNamespace denied, because allowedTargetNamespaces restriction is present")
return fmt.Errorf("empty targetNamespace denied, because allowedTargetNamespaces restriction is present")
}

gitrepo.Spec.TargetNamespace, err = isAllowed(gitrepo.Spec.TargetNamespace, "", restriction.AllowedTargetNamespaces)
targetNamespace, err := isAllowed(gitrepo.Spec.TargetNamespace, "", restriction.AllowedTargetNamespaces)
if err != nil {
return nil, fmt.Errorf("disallowed targetNamespace %s: %w", gitrepo.Spec.TargetNamespace, err)
return fmt.Errorf("disallowed targetNamespace %s: %w", gitrepo.Spec.TargetNamespace, err)
}

gitrepo.Spec.ServiceAccount, err = isAllowed(gitrepo.Spec.ServiceAccount,
serviceAccount, err := isAllowed(gitrepo.Spec.ServiceAccount,
restriction.DefaultServiceAccount,
restriction.AllowedServiceAccounts)
if err != nil {
return nil, fmt.Errorf("disallowed serviceAccount %s: %w", gitrepo.Spec.ServiceAccount, err)
return fmt.Errorf("disallowed serviceAccount %s: %w", gitrepo.Spec.ServiceAccount, err)
}

gitrepo.Spec.Repo, err = isAllowedByRegex(gitrepo.Spec.Repo, "", restriction.AllowedRepoPatterns)
repo, err := isAllowedByRegex(gitrepo.Spec.Repo, "", restriction.AllowedRepoPatterns)
if err != nil {
return nil, fmt.Errorf("disallowed repo %s: %w", gitrepo.Spec.ServiceAccount, err)
return fmt.Errorf("disallowed repo %s: %w", gitrepo.Spec.Repo, err)
}

gitrepo.Spec.ClientSecretName, err = isAllowed(gitrepo.Spec.ClientSecretName,
clientSecretName, err := isAllowed(gitrepo.Spec.ClientSecretName,
restriction.DefaultClientSecretName,
restriction.AllowedClientSecretNames)
if err != nil {
return nil, fmt.Errorf("disallowed clientSecretName %s: %w", gitrepo.Spec.ServiceAccount, err)
return fmt.Errorf("disallowed clientSecretName %s: %w", gitrepo.Spec.ClientSecretName, err)
}

return gitrepo, nil
// set the defaults back to the GitRepo
gitrepo.Spec.TargetNamespace = targetNamespace
gitrepo.Spec.ServiceAccount = serviceAccount
gitrepo.Spec.Repo = repo
gitrepo.Spec.ClientSecretName = clientSecretName

return nil
}

func aggregate(restrictions []fleet.GitRepoRestriction) (result fleet.GitRepoRestriction) {
Expand Down
249 changes: 249 additions & 0 deletions internal/cmd/controller/gitops/reconciler/restrictions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,249 @@
//go:generate mockgen --build_flags=--mod=mod -destination=../../../../mocks/client_mock.go -package=mocks sigs.k8s.io/controller-runtime/pkg/client Client

package reconciler_test

import (
"context"
"errors"
"reflect"
"regexp"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/mock/gomock"
crclient "sigs.k8s.io/controller-runtime/pkg/client"

"github.com/rancher/fleet/internal/cmd/controller/gitops/reconciler"
"github.com/rancher/fleet/internal/mocks"
fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
fleetv1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
)

func TestAuthorizeAndAssignDefaults(t *testing.T) {
dummyErrMsg := "something happened"
dummyErr := errors.New(dummyErrMsg)
cases := []struct {
name string
inputGr fleet.GitRepo
restrictions *fleet.GitRepoRestrictionList
restrictionsListErr error
expectedGr fleet.GitRepo
expectedErr string
}{
{
name: "fail when listing GitRepo restrictions errors",
inputGr: fleet.GitRepo{},
restrictionsListErr: dummyErr,
expectedGr: fleet.GitRepo{},
expectedErr: dummyErrMsg,
},
{
name: "pass when list of GitRepo restrictions is empty",
inputGr: fleet.GitRepo{},
restrictions: &fleet.GitRepoRestrictionList{},
expectedGr: fleet.GitRepo{},
},
{
name: "deny empty targetNamespace when allowedTargetNamespaces restriction present",
inputGr: fleet.GitRepo{}, // no target ns provided
restrictions: &fleet.GitRepoRestrictionList{
Items: []fleet.GitRepoRestriction{
{
AllowedTargetNamespaces: []string{"foo"},
},
},
},
expectedGr: fleet.GitRepo{},
expectedErr: "empty targetNamespace denied.*allowedTargetNamespaces restriction is present",
},
{
name: "deny disallowed targetNamespace",
inputGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
TargetNamespace: "not-foo",
},
},
restrictions: &fleet.GitRepoRestrictionList{
Items: []fleet.GitRepoRestriction{
{
AllowedTargetNamespaces: []string{"foo"},
},
},
},
expectedGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
TargetNamespace: "not-foo",
},
},
expectedErr: "disallowed targetNamespace.*",
},
{
name: "deny disallowed service account",
inputGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
ServiceAccount: "not-foo",
},
},
restrictions: &fleet.GitRepoRestrictionList{
Items: []fleet.GitRepoRestriction{
{
AllowedServiceAccounts: []string{"foo"},
},
},
},
expectedGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
ServiceAccount: "not-foo",
},
},
expectedErr: "disallowed serviceAccount.*",
},
{
name: "deny disallowed repo pattern",
inputGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
Repo: "bar",
},
},
restrictions: &fleet.GitRepoRestrictionList{
Items: []fleet.GitRepoRestriction{
{
AllowedRepoPatterns: []string{"baz"},
},
},
},
expectedGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
Repo: "bar",
},
},
expectedErr: "disallowed repo.*",
},
{
name: "deny disallowed client secret name",
inputGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
ClientSecretName: "not-foo",
},
},
restrictions: &fleet.GitRepoRestrictionList{
Items: []fleet.GitRepoRestriction{
{
AllowedClientSecretNames: []string{"foo"},
},
},
},
expectedGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
ClientSecretName: "not-foo",
},
},
expectedErr: "disallowed clientSecretName.*",
},
{
name: "pass when no restrictions nor disallowed values exist",
inputGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
Repo: "http://foo.bar/baz",
},
},
restrictions: &fleet.GitRepoRestrictionList{},
expectedGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
Repo: "http://foo.bar/baz",
},
},
},
{
name: "pass when restrictions exist and the GitRepo matches allowed values",
inputGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
ClientSecretName: "csn",
Repo: "http://foo.bar/baz",
ServiceAccount: "sacc",
TargetNamespace: "tns",
},
},
restrictions: &fleet.GitRepoRestrictionList{
Items: []fleet.GitRepoRestriction{
{
AllowedClientSecretNames: []string{"csn"},
AllowedRepoPatterns: []string{".*foo.bar.*"},
AllowedServiceAccounts: []string{"sacc"},
AllowedTargetNamespaces: []string{"tns"},
},
},
},
expectedGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
ClientSecretName: "csn",
Repo: "http://foo.bar/baz",
ServiceAccount: "sacc",
TargetNamespace: "tns",
},
},
},
{
name: "pass and mutate repo with defaults when restrictions exist and the GitRepo matches allowed values",
inputGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
Repo: "http://foo.bar/baz",
TargetNamespace: "tns",
},
},
restrictions: &fleet.GitRepoRestrictionList{
Items: []fleet.GitRepoRestriction{
{
AllowedClientSecretNames: []string{"csn"},
AllowedRepoPatterns: []string{".*foo.bar.*"},
AllowedServiceAccounts: []string{"sacc"},
AllowedTargetNamespaces: []string{"tns"},
DefaultClientSecretName: "dcsn",
DefaultServiceAccount: "dsacc",
},
},
},
expectedGr: fleet.GitRepo{
Spec: fleet.GitRepoSpec{
ClientSecretName: "dcsn",
Repo: "http://foo.bar/baz",
ServiceAccount: "dsacc",
TargetNamespace: "tns",
},
},
},
}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()

client := mocks.NewMockClient(mockCtrl)

client.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn(
func(_ context.Context, rl *fleetv1.GitRepoRestrictionList, ns crclient.InNamespace) error {
if c.restrictions != nil && len(c.restrictions.Items) > 0 {
rl.Items = c.restrictions.Items
}

return c.restrictionsListErr
},
)

err := reconciler.AuthorizeAndAssignDefaults(context.TODO(), client, &c.inputGr)

if len(c.expectedErr) > 0 {
require.NotNil(t, err)
assert.Regexp(t, regexp.MustCompile(c.expectedErr), err.Error())
} else {
assert.Nil(t, err)
}

if !reflect.DeepEqual(c.inputGr, c.expectedGr) {
t.Errorf("Expected res %v, got %v", c.expectedGr, c.inputGr)
}
})
}
}
5 changes: 2 additions & 3 deletions internal/cmd/controller/gitops/reconciler/status_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,7 @@ func (r *StatusReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
}

// Restrictions / Overrides, gitrepo reconciler is responsible for setting error in status
_, err := authorizeAndAssignDefaults(ctx, r.Client, gitrepo)
if err != nil {
if err := AuthorizeAndAssignDefaults(ctx, r.Client, gitrepo); err != nil {
// the gitjob_controller will handle the error
return ctrl.Result{}, nil
}
Expand All @@ -94,7 +93,7 @@ func (r *StatusReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
logger.V(1).Info("Reconciling GitRepo status")

bdList := &v1alpha1.BundleDeploymentList{}
err = r.List(ctx, bdList, client.MatchingLabels{
err := r.List(ctx, bdList, client.MatchingLabels{
v1alpha1.RepoLabel: gitrepo.Name,
v1alpha1.BundleNamespaceLabel: gitrepo.Namespace,
})
Expand Down
Loading