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

added exempted user to guardrails #3319

Merged
merged 4 commits into from
Jan 3, 2024
Merged

added exempted user to guardrails #3319

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
0312703
added exempted user to guardrails
yjst2012 Dec 8, 2023
1b8adb2
let the cleanup continue when failure occurs
yjst2012 Dec 11, 2023
17ad431
update test cases
yjst2012 Dec 11, 2023
7bf277b
iterate over all namespaces to find out if another gatekeeper is depl…
yjst2012 Dec 21, 2023
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
2 changes: 1 addition & 1 deletion cmd/aro/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ func operator(ctx context.Context, log *logrus.Entry) error {
}
if err = (guardrails.NewReconciler(
log.WithField("controller", guardrails.ControllerName),
client, dh)).SetupWithManager(mgr); err != nil {
client, dh, kubernetescli)).SetupWithManager(mgr); err != nil {
return fmt.Errorf("unable to create controller %s: %v", guardrails.ControllerName, err)
}
if err = (cloudproviderconfig.NewReconciler(
Expand Down
41 changes: 41 additions & 0 deletions pkg/operator/controllers/guardrails/guardrails_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ import (
"context"
"embed"
"strings"
"time"

configv1 "github.com/openshift/api/config/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"

arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
Expand Down Expand Up @@ -147,3 +149,42 @@ func (r *Reconciler) VersionLT411(ctx context.Context) (bool, error) {
ver411, _ := version.ParseVersion("4.11.0")
return clusterVersion.Lt(ver411), nil
}

func (r *Reconciler) getGatekeeperDeployedNs(ctx context.Context, instance *arov1alpha1.Cluster) (string, error) {
name := ""
if r.kubernetescli == nil {
r.log.Debug("nil kubernetescli object")
return "", nil
}
start := time.Now()
namespaces, err := r.kubernetescli.CoreV1().Namespaces().List(ctx, metav1.ListOptions{})
if err != nil {
r.log.Warnf("Error retrieving namespaces: %v", err)
return "", err
}
Comment on lines +160 to +164
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want a retry 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.

Thanks for looking into this, didn't do that as 1) retry makes code a bit less readable, 2) in case of a failure the reconcile process will be retried.

guardrails_namespace := instance.Spec.OperatorFlags.GetWithDefault(controllerNamespace, defaultNamespace)
for _, ns := range namespaces.Items {
if ns.Name == guardrails_namespace {
// skip guardrails ns
continue
}
deployments, err := r.kubernetescli.AppsV1().Deployments(ns.Name).List(ctx, metav1.ListOptions{
LabelSelector: "gatekeeper.sh/system=yes",
})
Comment on lines +171 to +173
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to have a try here as well

if err != nil {
r.log.Warnf("Error retrieving deployments in namespace %s: %v", ns.Name, err)
continue
}
if len(deployments.Items) > 0 {
name = ns.Name
break
}
}
dura := time.Since(start)
msg := "Gatekeeper not found"
if name != "" {
msg = "Found another gatekeeper deployed in namespace " + name
}
r.log.Infof("%s, search took %s.", msg, dura.String())
return name, nil
}
31 changes: 25 additions & 6 deletions pkg/operator/controllers/guardrails/guardrails_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/sirupsen/logrus"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/kubernetes"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -36,9 +37,11 @@ type Reconciler struct {
namespace string
policyTickerDone chan bool
reconciliationMinutes int
cleanupNeeded bool
kubernetescli kubernetes.Interface
}

func NewReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface) *Reconciler {
func NewReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Interface, k8scli kubernetes.Interface) *Reconciler {
return &Reconciler{
log: log,

Expand All @@ -50,6 +53,8 @@ func NewReconciler(log *logrus.Entry, client client.Client, dh dynamichelper.Int

readinessPollTime: 10 * time.Second,
readinessTimeout: 5 * time.Minute,
cleanupNeeded: false,
kubernetescli: k8scli,
}
}

Expand All @@ -74,9 +79,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
// If enabled and managed=false, remove the GuardRails deployment
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there's no need to comment how the code block works. The codes already explains what it does.

// If enabled and managed is missing, do nothing
if strings.EqualFold(managed, "true") {
// Check if standard GateKeeper is already running
if running, err := r.deployer.IsReady(ctx, "gatekeeper-system", "gatekeeper-audit"); err != nil && running {
r.log.Warn("standard GateKeeper is running, skipping Guardrails deployment")
if ns, err := r.getGatekeeperDeployedNs(ctx, instance); err == nil && ns != "" {
r.log.Warnf("Found another GateKeeper deployed in ns %s, aborting Guardrails", ns)
return reconcile.Result{}, nil
}

Expand All @@ -97,6 +101,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
if err != nil {
return reconcile.Result{}, fmt.Errorf("GateKeeper deployment timed out on Ready: %w", err)
}
r.cleanupNeeded = true
policyConfig := &config.GuardRailsPolicyConfig{}
if r.gkPolicyTemplate != nil {
// Deploy the GateKeeper ConstraintTemplate
Expand All @@ -122,24 +127,38 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
// start a ticker to re-enforce gatekeeper policies periodically
r.startTicker(ctx, instance)
} else if strings.EqualFold(managed, "false") {
if !r.cleanupNeeded {
if ns, err := r.getGatekeeperDeployedNs(ctx, instance); err == nil && ns != "" {
// resources were *not* created by guardrails, plus another gatekeeper deployed
//
// guardrails didn't get deployed most likely due to another gatekeeper is deployed by customer
// this is to avoid the accidental deletion of gatekeeper CRDs that were deployed by customer
// the unnamespaced gatekeeper CRDs were possibly created by a customised gatekeeper, hence cannot ramdomly delete them.
r.log.Warn("Skipping cleanup as it is not safe and may destroy customer's gatekeeper resources")
return reconcile.Result{}, nil
}
}
Comment on lines +130 to +140
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can combine the if statement into 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried that, but end up making code either less readable or have to invoke the cumbersome r.getGatekeeperDeployedNs() unnecessarily


if r.gkPolicyTemplate != nil {
// stop the gatekeeper policies re-enforce ticker
r.stopTicker()

err = r.removePolicy(ctx, gkPolicyConstraints, gkConstraintsPath)
if err != nil {
return reconcile.Result{}, err
r.log.Warnf("failed to remove Constraints with error %s", err.Error())
}

err = r.gkPolicyTemplate.Remove(ctx, config.GuardRailsPolicyConfig{})
if err != nil {
return reconcile.Result{}, err
r.log.Warnf("failed to remove ConstraintTemplates with error %s", err.Error())
}
}
err = r.deployer.Remove(ctx, config.GuardRailsDeploymentConfig{Namespace: r.namespace})
if err != nil {
r.log.Warnf("failed to remove deployer with error %s", err.Error())
return reconcile.Result{}, err
}
r.cleanupNeeded = false
}

return reconcile.Result{}, nil
Expand Down
14 changes: 7 additions & 7 deletions pkg/operator/controllers/guardrails/guardrails_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ import (

func TestGuardRailsReconciler(t *testing.T) {
tests := []struct {
name string
mocks func(*mock_deployer.MockDeployer, *arov1alpha1.Cluster)
flags arov1alpha1.OperatorFlags
name string
mocks func(*mock_deployer.MockDeployer, *arov1alpha1.Cluster)
flags arov1alpha1.OperatorFlags
cleanupNeeded bool
// errors
wantErr string
}{
Expand Down Expand Up @@ -71,7 +72,6 @@ func TestGuardRailsReconciler(t *testing.T) {
RoleSCCResourceName: "restricted-v2",
}
md.EXPECT().CreateOrUpdate(gomock.Any(), cluster, expectedConfig).Return(nil)
md.EXPECT().IsReady(gomock.Any(), "gatekeeper-system", "gatekeeper-audit").Return(true, nil)
md.EXPECT().IsReady(gomock.Any(), "wonderful-namespace", "gatekeeper-audit").Return(true, nil)
md.EXPECT().IsReady(gomock.Any(), "wonderful-namespace", "gatekeeper-controller-manager").Return(true, nil)
},
Expand Down Expand Up @@ -101,7 +101,6 @@ func TestGuardRailsReconciler(t *testing.T) {
RoleSCCResourceName: "restricted-v2",
}
md.EXPECT().CreateOrUpdate(gomock.Any(), cluster, expectedConfig).Return(nil)
md.EXPECT().IsReady(gomock.Any(), "gatekeeper-system", "gatekeeper-audit").Return(true, nil)
md.EXPECT().IsReady(gomock.Any(), "openshift-azure-guardrails", "gatekeeper-audit").Return(true, nil)
md.EXPECT().IsReady(gomock.Any(), "openshift-azure-guardrails", "gatekeeper-controller-manager").Return(true, nil)
},
Expand Down Expand Up @@ -132,7 +131,6 @@ func TestGuardRailsReconciler(t *testing.T) {
RoleSCCResourceName: "restricted-v2",
}
md.EXPECT().CreateOrUpdate(gomock.Any(), cluster, expectedConfig).Return(nil)
md.EXPECT().IsReady(gomock.Any(), "gatekeeper-system", "gatekeeper-audit").Return(true, nil)
md.EXPECT().IsReady(gomock.Any(), gomock.Any(), gomock.Any()).Return(false, nil)
},
wantErr: "GateKeeper deployment timed out on Ready: timed out waiting for the condition",
Expand All @@ -145,7 +143,6 @@ func TestGuardRailsReconciler(t *testing.T) {
controllerPullSpec: "wonderfulPullspec",
},
mocks: func(md *mock_deployer.MockDeployer, cluster *arov1alpha1.Cluster) {
md.EXPECT().IsReady(gomock.Any(), "gatekeeper-system", "gatekeeper-audit").Return(true, nil)
md.EXPECT().CreateOrUpdate(gomock.Any(), cluster, gomock.AssignableToTypeOf(&config.GuardRailsDeploymentConfig{})).Return(errors.New("failed ensure"))
},
wantErr: "failed ensure",
Expand All @@ -157,6 +154,7 @@ func TestGuardRailsReconciler(t *testing.T) {
controllerManaged: "false",
controllerPullSpec: "wonderfulPullspec",
},
cleanupNeeded: true,
mocks: func(md *mock_deployer.MockDeployer, cluster *arov1alpha1.Cluster) {
md.EXPECT().Remove(gomock.Any(), gomock.Any()).Return(nil)
},
Expand All @@ -168,6 +166,7 @@ func TestGuardRailsReconciler(t *testing.T) {
controllerManaged: "false",
controllerPullSpec: "wonderfulPullspec",
},
cleanupNeeded: true,
mocks: func(md *mock_deployer.MockDeployer, cluster *arov1alpha1.Cluster) {
md.EXPECT().Remove(gomock.Any(), gomock.Any()).Return(errors.New("failed delete"))
},
Expand Down Expand Up @@ -213,6 +212,7 @@ func TestGuardRailsReconciler(t *testing.T) {
client: clientBuilder.Build(),
readinessTimeout: 0 * time.Second,
readinessPollTime: 1 * time.Second,
cleanupNeeded: tt.cleanupNeeded,
}
_, err := r.Reconcile(context.Background(), reconcile.Request{})
if err != nil && err.Error() != tt.wantErr {
Expand Down
1 change: 1 addition & 0 deletions pkg/operator/controllers/guardrails/policies/gktemplates-src/library/common.rego
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ is_priv_namespace(ns) = true {

exempted_user = {
"system:kube-controller-manager",
"system:kube-scheduler",
"system:admin" # comment out temporarily for testing in console
}

Expand Down
1 change: 1 addition & 0 deletions pkg/operator/controllers/guardrails/policies/gktemplates/aro-deny-delete-pull-secret.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ spec:

exempted_user = {
"system:kube-controller-manager",
"system:kube-scheduler",
"system:admin" # comment out temporarily for testing in console
}

Expand Down
1 change: 1 addition & 0 deletions pkg/operator/controllers/guardrails/policies/gktemplates/aro-deny-machine-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ spec:

exempted_user = {
"system:kube-controller-manager",
"system:kube-scheduler",
"system:admin" # comment out temporarily for testing in console
}

Expand Down
1 change: 1 addition & 0 deletions ...erator/controllers/guardrails/policies/gktemplates/aro-deny-master-toleration-taints.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ spec:

exempted_user = {
"system:kube-controller-manager",
"system:kube-scheduler",
"system:admin" # comment out temporarily for testing in console
}

Expand Down
1 change: 1 addition & 0 deletions pkg/operator/controllers/guardrails/policies/gktemplates/aro-deny-privileged-namespace.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ spec:

exempted_user = {
"system:kube-controller-manager",
"system:kube-scheduler",
"system:admin" # comment out temporarily for testing in console
}

Expand Down
Loading