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

Complete auth support #8

Merged
merged 4 commits into from
Jan 21, 2022
Merged

Complete auth support #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
46779b8
Merge branch 'main' into complete-auth-support
Jan 18, 2022
dbe7c9d
Adding Unit teste
Jan 18, 2022
eaf1f96
resolve golinbt issues
Jan 18, 2022
9c698fa
Merge branch 'complete-auth-support' of https://github.com/dell/csm-o…
Jan 18, 2022
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 controllers/csm_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ var configVersionKey = fmt.Sprintf("%s/%s", MetadataPrefix, "CSIDriverConfigVers
// +kubebuilder:rbac:groups="storage.k8s.io",resources=storageclasses,verbs=get;list;watch;create;update;delete
// +kubebuilder:rbac:groups="storage.k8s.io",resources=volumeattachments,verbs=get;list;watch;create;update;patch
// +kubebuilder:rbac:groups="storage.k8s.io",resources=csinodes,verbs=get;list;watch;create;update
// +kubebuilder:rbac:groups="csi.storage.k8s.io",resources=csinodeinfos,verbs=get;list;watch
// +kubebuilder:rbac:groups="snapshot.storage.k8s.io",resources=volumesnapshotclasses;volumesnapshotcontents,verbs=get;list;watch;create;update;delete
// +kubebuilder:rbac:groups="snapshot.storage.k8s.io",resources=volumesnapshotcontents/status,verbs=update
// +kubebuilder:rbac:groups="snapshot.storage.k8s.io",resources=volumesnapshots;volumesnapshots/status,verbs=get;list;watch;update
Expand Down Expand Up @@ -145,7 +146,7 @@ func (r *ContainerStorageModuleReconciler) Reconcile(ctx context.Context, req ct
}

// Before doing anything else, check for config version and apply annotation if not set
isUpdated, err := checkAndApplyConfigVersionAnnotations(*csm, log, false)
isUpdated, err := checkAndApplyConfigVersionAnnotations(csm, log, false)
if err != nil {
return utils.HandleValidationError(ctx, csm, r, reqLogger, err)
} else if isUpdated {
Expand Down Expand Up @@ -525,7 +526,7 @@ func (r *ContainerStorageModuleReconciler) PreChecks(ctx context.Context, cr *cs
if m.Enabled {
switch m.Name {
case csmv1.Authorization:
err := modules.AuthorizationPrecheck(ctx, cr, m, r, reqLogger)
err := modules.AuthorizationPrecheck(ctx, cr.GetNamespace(), string(cr.Spec.Driver.CSIDriverType), operatorConfig, m, r.GetClient(), reqLogger)
if err != nil {
return fmt.Errorf("failed authorization validation: %v", err)
}
Expand All @@ -542,7 +543,7 @@ func (r *ContainerStorageModuleReconciler) PreChecks(ctx context.Context, cr *cs

}

func checkAndApplyConfigVersionAnnotations(instance csmv1.ContainerStorageModule, log logr.Logger, update bool) (bool, error) {
func checkAndApplyConfigVersionAnnotations(instance *csmv1.ContainerStorageModule, log logr.Logger, update bool) (bool, error) {
if instance.Spec.Driver.ConfigVersion == "" {
// fail immediately
return false, fmt.Errorf("mandatory argument: ConfigVersion missing")
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ require (
github.com/kubernetes-csi/external-snapshotter/client/v3 v3.0.0
github.com/onsi/ginkgo v1.16.4
github.com/onsi/gomega v1.13.0
github.com/stretchr/testify v1.6.1 // indirect
k8s.io/api v0.21.2
k8s.io/apimachinery v0.21.2
k8s.io/client-go v0.21.2
Expand Down
47 changes: 11 additions & 36 deletions k8s/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,49 +1,24 @@
package k8s_test
package k8s

/*import (
"context"
"encoding/json"
import (
"errors"
"fmt"
"reflect"
"testing"
"time"

"github.com/dell/csm-deployment/utils"
"github.com/labstack/echo/v4"
"github.com/stretchr/testify/assert"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/kubernetes/fake"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
k8stesting "k8s.io/client-go/testing"

"k8s.io/apimachinery/pkg/version"
discoveryfake "k8s.io/client-go/discovery/fake"
"k8s.io/client-go/tools/clientcmd"
clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
ctrlClient "sigs.k8s.io/controller-runtime/pkg/client"
ctrlClientFake "sigs.k8s.io/controller-runtime/pkg/client/fake"
)

type testOverrides struct {
getClientSetWrapper func() (kubernetes.Interface, error)
ignoreError bool
}

var (
namespace = "csm"
csmDataCollector = "csm-data-collector"
secretName = "testing-secret"
configMapName = "testing-configMap"
)

func Test_IsOpenShift(t *testing.T) {
tests := map[string]func(t *testing.T) (bool, testOverrides){
"success ": func(*testing.T) (bool, testOverrides) {
Expand All @@ -66,6 +41,9 @@ func Test_IsOpenShift(t *testing.T) {
},
}
},
"bad config data ": func(*testing.T) (bool, testOverrides) {
return false, testOverrides{ignoreError: true}
},
"fail - not found ": func(*testing.T) (bool, testOverrides) {
return false, testOverrides{
getClientSetWrapper: func() (kubernetes.Interface, error) {
Expand Down Expand Up @@ -106,9 +84,6 @@ func Test_IsOpenShift(t *testing.T) {
},
}
},
"bad config data ": func(*testing.T) (bool, testOverrides) {
return false, testOverrides{}
},
"fail - to get client set": func(*testing.T) (bool, testOverrides) {
return false, testOverrides{
getClientSetWrapper: func() (kubernetes.Interface, error) {
Expand All @@ -129,7 +104,9 @@ func Test_IsOpenShift(t *testing.T) {
}

isOpenshift, err := IsOpenShift()
if !success {
if patch.ignoreError {
t.Log("cover real Openshift setup")
} else if !success {
assert.False(t, isOpenshift)
} else {
assert.NoError(t, err)
Expand All @@ -138,8 +115,8 @@ func Test_IsOpenShift(t *testing.T) {

})
}

}

func Test_GetVersion(t *testing.T) {
tests := map[string]func(t *testing.T) (bool, string, string, testOverrides){
"success ": func(*testing.T) (bool, string, string, testOverrides) {
Expand All @@ -163,7 +140,6 @@ func Test_GetVersion(t *testing.T) {
return fake.NewSimpleClientset(), errors.New(" error listing pods")
},
}

},
}
for name, tc := range tests {
Expand All @@ -176,7 +152,7 @@ func Test_GetVersion(t *testing.T) {
GetClientSetWrapper = patch.getClientSetWrapper
}

out, err := GetVersion([]byte(""))
out, err := GetVersion()
if !success {
assert.Error(t, err)
} else {
Expand All @@ -187,4 +163,3 @@ func Test_GetVersion(t *testing.T) {
})
}
}
*/
2 changes: 1 addition & 1 deletion operatorconfig/driverconfig/powerscale/v2.0.0/driver-config-params.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ metadata:
namespace: <DriverDefaultReleaseNamespace>
data:
driver-config-params.yaml: |
CSI_LOG_LEVEL: debug
CSI_LOG_LEVEL: debug
50 changes: 31 additions & 19 deletions pkg/modules/authorization.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/types"
crclient "sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/yaml"
)

const (
// DefaultPluginIdentifier constant
// DefaultPluginIdentifier - spring placeholder for driver plugin
DefaultPluginIdentifier = "<DriverPluginIdentifier>"
)

Expand All @@ -29,10 +30,6 @@ var SupportedDrivers = map[string]string{

func getAuthCR(cr csmv1.ContainerStorageModule, op utils.OperatorConfig) (*csmv1.Module, *corev1.Container, error) {
var err error
configMapName := map[string]string{
"powerscale": "csi-isilon",
"isilon": "csi-isilon",
}
authModule := csmv1.Module{}
for _, m := range cr.Spec.Modules {
if m.Name == csmv1.Authorization {
Expand All @@ -55,10 +52,7 @@ func getAuthCR(cr csmv1.ContainerStorageModule, op utils.OperatorConfig) (*csmv1
return nil, nil, err
}

YamlString := string(buf)
if cr.Name != "" {
YamlString = strings.ReplaceAll(YamlString, utils.DefaultReleaseName, configMapName[string(cr.Spec.Driver.CSIDriverType)])
}
YamlString := utils.ModifyCommonCR(string(buf), cr)

if string(cr.Spec.Driver.Common.ImagePullPolicy) != "" {
YamlString = strings.ReplaceAll(YamlString, utils.DefaultImagePullPolicy, string(cr.Spec.Driver.Common.ImagePullPolicy))
Expand Down Expand Up @@ -133,7 +127,7 @@ func getAuthVolumes(cr csmv1.ContainerStorageModule, op utils.OperatorConfig, au
return vols, nil
}

// AuthInjectDaemonset auth module
// AuthInjectDaemonset - inject authorization into daemonset
func AuthInjectDaemonset(ds appsv1.DaemonSet, cr csmv1.ContainerStorageModule, op utils.OperatorConfig) (*appsv1.DaemonSet, error) {
authModule, containerPtr, err := getAuthCR(cr, op)
if err != nil {
Expand Down Expand Up @@ -161,7 +155,7 @@ func AuthInjectDaemonset(ds appsv1.DaemonSet, cr csmv1.ContainerStorageModule, o
return &ds, nil
}

// AuthInjectDeployment deploy
// AuthInjectDeployment - inject authorization into deployment
func AuthInjectDeployment(dp appsv1.Deployment, cr csmv1.ContainerStorageModule, op utils.OperatorConfig) (*appsv1.Deployment, error) {
authModule, containerPtr, err := getAuthCR(cr, op)
if err != nil {
Expand Down Expand Up @@ -190,10 +184,30 @@ func AuthInjectDeployment(dp appsv1.Deployment, cr csmv1.ContainerStorageModule,

}

// AuthorizationPrecheck precheck
func AuthorizationPrecheck(ctx context.Context, cr *csmv1.ContainerStorageModule, auth csmv1.Module, r utils.ReconcileCSM, log logr.Logger) error {
if _, ok := SupportedDrivers[string(cr.Spec.Driver.CSIDriverType)]; !ok {
return fmt.Errorf("csm authorization does not support %s driver", string(cr.Spec.Driver.CSIDriverType))
// AuthorizationPrecheck - runs precheck for CSM Authorization
func AuthorizationPrecheck(ctx context.Context, namespace, driverType string, op utils.OperatorConfig, auth csmv1.Module, ctrlClient crclient.Client, log logr.Logger) error {
if _, ok := SupportedDrivers[driverType]; !ok {
return fmt.Errorf("CSM Authorization does not support %s driver", driverType)
}

// check if provided version is supported
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we support the case where I specify a version of the PowerScale driver but don't specify the version of Authorization? In that case, the operator should know which version of Authorization should be used based on our support matrix.

Copy link
Contributor Author

@medegw01 medegw01 Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that's the default behavior I am testing. Please see the test file: https://github.com/dell/csm-operator/blob/complete-auth-support/pkg/modules/testdata/cr_powerscale_auth.yaml and corresponding prechecks testing it:

csm-operator/pkg/modules/authorization_test.go

Line 276 in 9c698fa

func TestAuthorizationPrecheck(t *testing.T) {

And finally, here is the line in the code that calls the default:

csm-operator/pkg/modules/authorization.go

Line 42 in 9c698fa

if authConfigVersion == "" {

if auth.ConfigVersion != "" {
files, err := ioutil.ReadDir(fmt.Sprintf("%s/moduleconfig/authorization/", op.ConfigDirectory))
if err != nil {
return err
}
found := false
authVersions := ""
for _, file := range files {
authVersions += (file.Name() + ",")
if file.Name() == auth.ConfigVersion {
found = true
}
}
if !found {
return fmt.Errorf("CSM Authorization does not have %s version. The following are supported versions: %s", auth.ConfigVersion, authVersions[:len(authVersions)-1])
}

}

// Check for secrets
Expand All @@ -219,8 +233,8 @@ func AuthorizationPrecheck(ctx context.Context, cr *csmv1.ContainerStorageModule

for _, name := range secrets {
found := &corev1.Secret{}
err := r.GetClient().Get(ctx, types.NamespacedName{Name: name,
Namespace: cr.GetNamespace()}, found)
err := ctrlClient.Get(ctx, types.NamespacedName{Name: name,
Namespace: namespace}, found)
if err != nil {
if errors.IsNotFound(err) {
return fmt.Errorf("failed to find secret %s and certificate validation is requested", name)
Expand All @@ -229,7 +243,5 @@ func AuthorizationPrecheck(ctx context.Context, cr *csmv1.ContainerStorageModule
}
}

// TODO(Michael): Do Other configuration checks if needed

return nil
}
Loading