Skip to content

Commit

Permalink
fix: use margo and cloudianAPIDefaults to clarify and correct desired…
Browse files Browse the repository at this point in the history 
… vs observed group reconcile logic
  • Loading branch information
@mariatsji
mariatsji committed Dec 11, 2024
1 parent f141533 commit ea6cceb
Show file tree
Hide file tree
Showing 3 changed files with 206 additions and 13 deletions.
4 changes: 2 additions & 2 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,20 @@ module github.com/statnett/provider-cloudian
go 1.23.0

require (
dario.cat/mergo v1.0.1
github.com/crossplane/crossplane-runtime v1.18.0
github.com/crossplane/crossplane-tools v0.0.0-20240522174801-1ad3d4c87f21
github.com/google/go-cmp v0.6.0
github.com/pkg/errors v0.9.1
gopkg.in/alecthomas/kingpin.v2 v2.2.6
k8s.io/apimachinery v0.31.3
k8s.io/client-go v0.31.3
k8s.io/utils v0.0.0-20241104163129-6fe5fd82f078
sigs.k8s.io/controller-runtime v0.19.2
sigs.k8s.io/controller-tools v0.16.5
)

require (
dario.cat/mergo v1.0.1 // indirect
github.com/alecthomas/kingpin/v2 v2.4.0 // indirect
github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751 // indirect
github.com/alecthomas/units v0.0.0-20211218093645-b94a6e3cc137 // indirect
Expand Down Expand Up @@ -86,7 +87,6 @@ require (
k8s.io/component-base v0.31.3 // indirect
k8s.io/klog/v2 v2.130.1 // indirect
k8s.io/kube-openapi v0.0.0-20241105132330-32ad38e42d3f // indirect
k8s.io/utils v0.0.0-20241104163129-6fe5fd82f078 // indirect
sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8 // indirect
sigs.k8s.io/structured-merge-diff/v4 v4.4.3 // indirect
sigs.k8s.io/yaml v1.4.0 // indirect
Expand Down
55 changes: 44 additions & 11 deletions internal/controller/group/group.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ package group
import (
"context"

"dario.cat/mergo"
"github.com/google/go-cmp/cmp"
"github.com/pkg/errors"
"k8s.io/apimachinery/pkg/types"
"k8s.io/utils/ptr"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"

Expand Down Expand Up @@ -156,7 +159,7 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex

cr.SetConditions(xpv1.Available())

upToDate := isUpToDate(cr.Spec.ForProvider, *observedGroup)
upToDate, _, err := isUpToDate(cr.Spec.ForProvider, toAPIGroup(*observedGroup))

return managed.ExternalObservation{
// Return false when the external resource does not exist. This lets
Expand All @@ -172,20 +175,50 @@ func (c *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
// Return any details that may be required to connect to the external
// resource. These will be stored as the connection secret.
ConnectionDetails: managed.ConnectionDetails{},
}, nil
}, err
}

func isUpToDate(desired v1alpha1.GroupParameters, observed cloudian.Group) bool {
// Only compare groupId and groupName for now - use defaults for the rest
if desired.GroupID != observed.GroupID {
return false
func toAPIGroup(g cloudian.Group) v1alpha1.GroupParameters {
return v1alpha1.GroupParameters{
Active: g.Active,
GroupID: g.GroupID,
GroupName: g.GroupName,
LDAPEnabled: g.LDAPEnabled,
LDAPGroup: g.LDAPGroup,
LDAPMatchAttribute: g.LDAPMatchAttribute,
LDAPSearch: g.LDAPSearch,
LDAPSearchUserBase: g.LDAPSearchUserBase,
LDAPServerURL: g.LDAPServerURL,
LDAPUserDNTemplate: g.LDAPUserDNTemplate,
S3EndpointsHTTP: g.S3EndpointsHTTP,
S3EndpointsHTTPS: g.S3EndpointsHTTPS,
S3WebsiteEndpoints: g.S3WebsiteEndpoints,
}
if desired.GroupName != nil &&
observed.GroupName != nil &&
*desired.GroupName != *observed.GroupName {
return false
}

func isUpToDate(desired v1alpha1.GroupParameters, observed v1alpha1.GroupParameters) (bool, string, error) {
// These values are the Cloudian API defaults
// We can find them by POSTing a minimal gropuData {"groupId" = "testgroup"}
// and then reading this group back through a GET
cloudianAPIDefaults := v1alpha1.GroupParameters{
Active: ptr.To("true"),
GroupName: ptr.To(""),
LDAPGroup: nil,
LDAPEnabled: ptr.To(false),
LDAPServerURL: nil,
LDAPMatchAttribute: nil,
LDAPSearch: nil,
LDAPSearchUserBase: nil,
LDAPUserDNTemplate: nil,
S3EndpointsHTTP: []string{"ALL"},
S3EndpointsHTTPS: []string{"ALL"},
S3WebsiteEndpoints: []string{"ALL"},
}
err := mergo.Merge(&desired, cloudianAPIDefaults)
if err != nil {
return false, "", err
}
return true
return cmp.Equal(desired, observed), cmp.Diff(desired, observed), err
}

func newGroupFromCR(cr *v1alpha1.Group) cloudian.Group {
Expand Down
160 changes: 160 additions & 0 deletions internal/controller/group/group_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,13 @@ import (
"testing"

"github.com/google/go-cmp/cmp"
"k8s.io/utils/ptr"

"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
"github.com/crossplane/crossplane-runtime/pkg/resource"
"github.com/crossplane/crossplane-runtime/pkg/test"

"github.com/statnett/provider-cloudian/apis/user/v1alpha1"
)

// Unlike many Kubernetes projects Crossplane does not use third party testing
Expand Down Expand Up @@ -72,3 +75,160 @@ func TestObserve(t *testing.T) {
})
}
}

func TestGroupIsConsideredUpToDate(t *testing.T) {
tests := []struct {
name string
desired, observed v1alpha1.GroupParameters
wantEquality bool
}{
{
name: "GroupID and GroupName is set",
desired: v1alpha1.GroupParameters{
GroupID: "QA",
GroupName: ptr.To("Hello"),
},
observed: v1alpha1.GroupParameters{
Active: ptr.To("true"),
GroupID: "QA",
GroupName: ptr.To("Hello"),
LDAPEnabled: ptr.To(false),
LDAPGroup: nil,
LDAPMatchAttribute: nil,
LDAPSearch: nil,
LDAPSearchUserBase: nil,
LDAPServerURL: nil,
LDAPUserDNTemplate: nil,
S3EndpointsHTTP: []string{"ALL"},
S3EndpointsHTTPS: []string{"ALL"},
S3WebsiteEndpoints: []string{"ALL"},
},
wantEquality: true,
},
{
name: "GroupID is set GroupName is not set",
desired: v1alpha1.GroupParameters{
GroupID: "QA",
},
observed: v1alpha1.GroupParameters{
Active: ptr.To("true"),
GroupID: "QA",
GroupName: ptr.To(""),
LDAPEnabled: ptr.To(false),
LDAPGroup: nil,
LDAPMatchAttribute: nil,
LDAPSearch: nil,
LDAPSearchUserBase: nil,
LDAPServerURL: nil,
LDAPUserDNTemplate: nil,
S3EndpointsHTTP: []string{"ALL"},
S3EndpointsHTTPS: []string{"ALL"},
S3WebsiteEndpoints: []string{"ALL"},
},
wantEquality: true,
},
{
name: "Desired GroupID is unlike observed GroupID",
desired: v1alpha1.GroupParameters{
GroupID: "desired",
},
observed: v1alpha1.GroupParameters{
Active: ptr.To("true"),
GroupID: "observed",
GroupName: ptr.To(""),
LDAPEnabled: ptr.To(false),
LDAPGroup: nil,
LDAPMatchAttribute: nil,
LDAPSearch: nil,
LDAPSearchUserBase: nil,
LDAPServerURL: nil,
LDAPUserDNTemplate: nil,
S3EndpointsHTTP: []string{"ALL"},
S3EndpointsHTTPS: []string{"ALL"},
S3WebsiteEndpoints: []string{"ALL"},
},
wantEquality: false,
},
{
name: "Desired GroupName is unlike observed GroupName",
desired: v1alpha1.GroupParameters{
GroupID: "desired",
GroupName: ptr.To("desired description"),
},
observed: v1alpha1.GroupParameters{
Active: ptr.To("true"),
GroupID: "desired",
GroupName: ptr.To(""),
LDAPEnabled: ptr.To(false),
LDAPGroup: nil,
LDAPMatchAttribute: nil,
LDAPSearch: nil,
LDAPSearchUserBase: nil,
LDAPServerURL: nil,
LDAPUserDNTemplate: nil,
S3EndpointsHTTP: []string{"ALL"},
S3EndpointsHTTPS: []string{"ALL"},
S3WebsiteEndpoints: []string{"ALL"},
},
wantEquality: false,
},
{
name: "We have not set a GroupName in the desired state, and the observed GroupName is empty string",
desired: v1alpha1.GroupParameters{
GroupID: "desired",
GroupName: nil,
},
observed: v1alpha1.GroupParameters{
Active: ptr.To("true"),
GroupID: "desired",
GroupName: ptr.To(""),
LDAPEnabled: ptr.To(false),
LDAPGroup: nil,
LDAPMatchAttribute: nil,
LDAPSearch: nil,
LDAPSearchUserBase: nil,
LDAPServerURL: nil,
LDAPUserDNTemplate: nil,
S3EndpointsHTTP: []string{"ALL"},
S3EndpointsHTTPS: []string{"ALL"},
S3WebsiteEndpoints: []string{"ALL"},
},
wantEquality: true,
},
{
name: "We can desire a LDAPServerURL and get a diff if observed does not have one",
desired: v1alpha1.GroupParameters{
GroupID: "desired",
GroupName: nil,
LDAPServerURL: ptr.To("ldap://example.com"),
},
observed: v1alpha1.GroupParameters{
Active: ptr.To("true"),
GroupID: "desired",
GroupName: ptr.To(""),
LDAPEnabled: ptr.To(false),
LDAPGroup: nil,
LDAPMatchAttribute: nil,
LDAPSearch: nil,
LDAPSearchUserBase: nil,
LDAPServerURL: nil,
LDAPUserDNTemplate: nil,
S3EndpointsHTTP: []string{"ALL"},
S3EndpointsHTTPS: []string{"ALL"},
S3WebsiteEndpoints: []string{"ALL"},
},
wantEquality: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
isUpToDate, diff, err := isUpToDate(tt.desired, tt.observed)
if err != nil {
t.Errorf("isUpToDate() error = %v", err)
}
if isUpToDate != tt.wantEquality {
t.Errorf("isUpToDate() = %v, want %v, but the diff was %s", isUpToDate, tt.wantEquality, diff)
}
})
}
}

0 comments on commit ea6cceb

Please sign in to comment.