controllers: create SCCs on Openshift #52

nbalacha · 2021-12-27T11:43:51Z

Create SCCs for topolvm-node and vgmanager on openshift.

Signed-off-by: N Balachandran [email protected]

nbalacha · 2021-12-27T11:44:34Z

/hold

leelavg · 2021-12-27T11:58:47Z

main.go

-		Scheme: mgr.GetScheme(),
+		Client:         mgr.GetClient(),
+		Scheme:         mgr.GetScheme(),
+		IsOpenshift:    true,


any possibility to avoid this, since secv1 is just another api, if there's any possibility to check the availability of that api and set this option?

Openshift requires that this api be used for security.

I meant it differently, yes OCP uses this api for security, my ask here is, can this not be hardcoded and set the value based on api availability?

leelavg · 2021-12-27T12:00:23Z

controllers/scc.go

@@ -0,0 +1,169 @@
+/*


please add this as one of resource unit, just reminding

leelavg · 2021-12-27T12:09:38Z

controllers/scc.go

+limitations under the License.
+*/
+
+package controllers


can you try below once?

diff --git a/controllers/scc.go b/controllers/scc.go index a987590..f0a9d62 100644 --- a/controllers/scc.go +++ b/controllers/scc.go @@ -45,17 +45,17 @@ func (c openshiftSccs) ensureCreated(r *LVMClusterReconciler, ctx context.Contex sccs := getAllSCCs(lvmCluster.Namespace) for _, scc := range sccs { - found, err := r.SecurityClient.SecurityContextConstraints().Get(scc.Name, metav1.GetOptions{}) + found, err := r.SecurityClient.SecurityContextConstraints().Get(ctx, scc.Name, metav1.GetOptions{}) if err != nil && errors.IsNotFound(err) { r.Log.Info("Creating SecurityContextConstraint.", "SecurityContextConstraint", scc.Name) - _, err := r.SecurityClient.SecurityContextConstraints().Create(scc) + _, err := r.SecurityClient.SecurityContextConstraints().Create(ctx, scc, metav1.CreateOptions{}) if err != nil { return fmt.Errorf("unable to create SCC %+v: %v", scc, err) } } else if err == nil { scc.ObjectMeta = found.ObjectMeta r.Log.Info("Updating SecurityContextConstraint.", "SecurityContextConstraint", scc.Name) - _, err := r.SecurityClient.SecurityContextConstraints().Update(scc) + _, err := r.SecurityClient.SecurityContextConstraints().Update(ctx, scc, metav1.UpdateOptions{}) if err != nil { r.Log.Error(err, "Unable to update SecurityContextConstraint.", "SecurityContextConstraint", scc.Name) return fmt.Errorf("unable to update SCC %+v: %v", scc, err)

That was what I had originally but that did not match the v3.9 signatures

It does work with the updated packages .

leelavg · 2021-12-27T12:10:11Z

go.mod

@@ -7,6 +7,8 @@ require (
 	github.com/google/go-cmp v0.5.6
 	github.com/onsi/ginkgo v1.16.4
 	github.com/onsi/gomega v1.15.0
+	github.com/openshift/api v3.9.0+incompatible


can you try with below?

diff --git a/go.mod b/go.mod index ddfee36..613087f 100644 --- a/go.mod +++ b/go.mod @@ -7,8 +7,8 @@ require ( github.com/google/go-cmp v0.5.6 github.com/onsi/ginkgo v1.16.4 github.com/onsi/gomega v1.15.0 - github.com/openshift/api v3.9.0+incompatible - github.com/openshift/client-go v3.9.0+incompatible + github.com/openshift/api v0.0.0-20211028023115-7224b732cc14 + github.com/openshift/client-go v0.0.0-20210831095141-e19a065e79f7 github.com/topolvm/topolvm v0.10.3 gotest.tools/v3 v3.0.3 k8s.io/api v0.22.2

above is the result of

-> go get github.com/openshift/[email protected] -> go get github.com/openshift/[email protected]

That works. Thanks.

sp98

nit

sp98 · 2021-12-27T12:18:20Z

controllers/scc.go

+
+func (c openshiftSccs) ensureCreated(r *LVMClusterReconciler, ctx context.Context, lvmCluster *lvmv1alpha1.LVMCluster) error {
+	if r.IsOpenshift == true {
+


whitespace?

sp98 · 2021-12-27T12:21:15Z

controllers/scc.go

+				r.Log.Info("Updating SecurityContextConstraint.", "SecurityContextConstraint", scc.Name)
+				_, err := r.SecurityClient.SecurityContextConstraints().Update(scc)
+				if err != nil {
+					r.Log.Error(err, "Unable to update SecurityContextConstraint.", "SecurityContextConstraint", scc.Name)


Suggested change

r.Log.Error(err, "Unable to update SecurityContextConstraint.", "SecurityContextConstraint", scc.Name)

r.Log.Error(err, "failed to update SecurityContextConstraint.", "SecurityContextConstraint", scc.Name)

just to be consistent with other error messages.

sp98 · 2021-12-27T12:21:49Z

controllers/scc.go

+				_, err := r.SecurityClient.SecurityContextConstraints().Update(scc)
+				if err != nil {
+					r.Log.Error(err, "Unable to update SecurityContextConstraint.", "SecurityContextConstraint", scc.Name)
+					return fmt.Errorf("unable to update SCC %+v: %v", scc, err)


Suggested change

return fmt.Errorf("unable to update SCC %+v: %v", scc, err)

return fmt.Errorf("failed to update SCC %q: %v", scc.Name, err)

sp98 · 2021-12-27T12:22:38Z

controllers/scc.go

+				}
+			} else {
+				r.Log.Error(err, "Something went wrong when checking for SecurityContextConstraint.", "SecurityContextConstraint", scc.Name)
+				return fmt.Errorf("something went wrong when checking for SCC %+v: %v", scc, err)


Suggested change

return fmt.Errorf("something went wrong when checking for SCC %+v: %v", scc, err)

return fmt.Errorf("something went wrong when checking for SCC %q: %v", scc.Name, err)

sp98 · 2021-12-27T12:23:09Z

controllers/scc.go

+}
+
+func (c openshiftSccs) updateStatus(r *LVMClusterReconciler, ctx context.Context, lvmCluster *lvmv1alpha1.LVMCluster) error {
+	// intentionally empty as there'll be no status field on CSIDriver resource


need to update this comment for SCC.

nbalacha · 2021-12-29T16:22:28Z

/unhold

nbalacha · 2021-12-29T16:23:08Z

Cannot create SCCs using the ENV tests so skipping that for now.

leelavg · 2021-12-30T07:36:37Z

config/rbac/kustomization.yaml

+#- topolvm_node_scc.yaml
+#- topolvm_node_scc_role.yaml
+#- topolvm_node_scc_role_bindings.yaml


as the files are deleted, better remove these lines rather than commenting them?

Ack. Missed this.

leelavg · 2021-12-30T07:40:18Z

controllers/scc.go

+			Kind:       "SecurityContextConstraints",
+		},
+	}
+	scc.Name = "odf-lvm-topolvm-node"


is the prefix odf intentional here?

Yes. SCCs will be created only on Openshift so Odf is fine.

leelavg · 2021-12-30T07:41:19Z

controllers/scc.go

+			Kind:       "SecurityContextConstraints",
+		},
+	}
+	scc.Name = "odf-lvm-vgmanager"


intentional odf prefix?

same as above.

sp98 · 2021-12-30T08:50:19Z

controllers/lvmcluster_controller.go

+// It does this by querying for the "privileged" SCC which exists on all OCP clusters.
+func (r *LVMClusterReconciler) checkIfOpenshift(ctx context.Context) error {
+	if r.ClusterType == "" {
+		//has not been determined yet


Suggested change

//has not been determined yet

// has not been determined yet

sp98 · 2021-12-30T11:47:03Z

controllers/scc.go

+				return fmt.Errorf("failed to create SCC %q: %v", scc.Name, err)
+			}
+		} else if err == nil {
+			//Don't update the SCC


Suggested change

//Don't update the SCC

// Don't update the SCC

sp98 · 2021-12-30T11:47:45Z

controllers/lvmcluster_controller.go

+
+const (
+	ClusterTypeOCP   ClusterType = "openshift"
+	ClusterTypeOther ClusterType = "other"


do we really need other option here? I think only ClusterTypeOCP should do the job.

other looks a bit ambiguous as well

It is set to a value to indicate that the check has been performed and a result has been received.

sp98 · 2021-12-30T11:48:02Z

controllers/lvmcluster_controller.go

-
+	err = r.checkIfOpenshift(ctx)
+	if err != nil {
+		//Ignore the error for now


Suggested change

//Ignore the error for now

// Ignore the error for now

why ignore the error? error should be propagated if we can't detect the platform, IMO.

For non-Openshift clusters it should still work so take a chance and go ahead?

Bundle manifests cannot include SCCs.The SCCs required by the topolvm-node and vgmanager have to be created post the operator installation by the LVM operator. Signed-off-by: N Balachandran <[email protected]>

This commit removes the topolvm-node and vgmanager specific scc related rbacs from config/ and includes the changes required for the lvm controller to create the required SCCs programatically. Signed-off-by: N Balachandran <[email protected]>

Updated the env tests with scc changes. Signed-off-by: N Balachandran <[email protected]>

nbalacha · 2021-12-30T12:38:29Z

/approve

openshift-ci · 2021-12-30T12:38:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: leelavg, nbalacha, sp98

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 27, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 27, 2021

leelavg suggested changes Dec 27, 2021

View reviewed changes

openshift-ci bot assigned leelavg Dec 27, 2021

sp98 suggested changes Dec 27, 2021

View reviewed changes

openshift-ci bot assigned sp98 Dec 27, 2021

nbalacha changed the title ~~WIP: create SCCs on Openshift~~ controllers: create SCCs on Openshift Dec 29, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 29, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 29, 2021

leelavg reviewed Dec 30, 2021

View reviewed changes

leelavg approved these changes Dec 30, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 30, 2021

sp98 suggested changes Dec 30, 2021

View reviewed changes

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 30, 2021

nbalacha added 3 commits December 30, 2021 17:38

feat: add scc resource manager

9e982bb

Bundle manifests cannot include SCCs.The SCCs required by the topolvm-node and vgmanager have to be created post the operator installation by the LVM operator. Signed-off-by: N Balachandran <[email protected]>

feat: updates the rbacs for sccs

948104b

This commit removes the topolvm-node and vgmanager specific scc related rbacs from config/ and includes the changes required for the lvm controller to create the required SCCs programatically. Signed-off-by: N Balachandran <[email protected]>

tests: updates env tests

967f03f

Updated the env tests with scc changes. Signed-off-by: N Balachandran <[email protected]>

sp98 approved these changes Dec 30, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 30, 2021

nbalacha merged commit 9b6bbc8 into openshift:main Dec 30, 2021

nbalacha deleted the scc branch December 31, 2021 12:31


		func (c openshiftSccs) ensureCreated(r LVMClusterReconciler, ctx context.Context, lvmCluster lvmv1alpha1.LVMCluster) error {
		if r.IsOpenshift == true {

	r.Log.Error(err, "Unable to update SecurityContextConstraint.", "SecurityContextConstraint", scc.Name)
	r.Log.Error(err, "failed to update SecurityContextConstraint.", "SecurityContextConstraint", scc.Name)

	return fmt.Errorf("unable to update SCC %+v: %v", scc, err)
	return fmt.Errorf("failed to update SCC %q: %v", scc.Name, err)

	return fmt.Errorf("something went wrong when checking for SCC %+v: %v", scc, err)
	return fmt.Errorf("something went wrong when checking for SCC %q: %v", scc.Name, err)

controllers: create SCCs on Openshift #52

controllers: create SCCs on Openshift #52

Conversation

nbalacha commented Dec 27, 2021

nbalacha commented Dec 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbalacha commented Dec 29, 2021

nbalacha commented Dec 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sp98 Dec 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbalacha commented Dec 30, 2021

openshift-ci bot commented Dec 30, 2021

sp98 Dec 30, 2021 •

edited

Loading