From ddc427e30e5a60d2f4ee0e9ccbaca6ec327c71ed Mon Sep 17 00:00:00 2001
From: Anders Eknert <anders@eknert.com>
Date: Sat, 14 Oct 2023 18:51:45 +0200
Subject: [PATCH] Add Regal for linting Rego
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This PR adds [Regal](https://github.com/styraInc/regal) for linting the Rego
contained in this repo.

Practically, this PR adds a Regal configuration file adapted to the project,
where some of the most reported style issues are ignoed for the time being.
Additionally, we're adding a linter job to the CI pipeline to ensure future
updates to policy if compliant as well.

A few (hopefully) uncontroversial issues reported have also been fixed:

* [constant-condition](https://docs.styra.com/regal/rules/bugs/constant-condition)
* [use-assignment-operator](https://docs.styra.com/regal/rules/style/use-assignment-operator)
* [use-in-operator](https://docs.styra.com/regal/rules/idiomatic/use-in-operator)
* [use-some-for-output-vars](https://docs.styra.com/regal/rules/idiomatic/use-some-for-output-vars)
* [non-raw-regex-pattern](https://docs.styra.com/regal/rules/idiomatic/non-raw-regex-pattern)

While the changes are non-intrusive, and should have no impact on evaluation,
I naturally wanted to run the tests included in this repo and followed the instructions
in TESTING.md. However, even after having installed all of the dependencies listed as
required, the test command fails due to a missing `oc` command. I tried to find that
and possibly update the TESTING.md file to include it, but the instructions I followed
suggested logging in to a "RedHat Customer Portal" to get it. If being a customer is required
to run the tests, that requirement would be good to add to the docs as well.

Signed-off-by: Anders Eknert <anders@eknert.com>
---
 .github/workflows/regal-lint.yaml             | 18 +++++++
 .regal/config.yaml                            | 22 ++++++++
 policy/lib/konstraint/core.rego               | 43 ++++++++-------
 policy/lib/konstraint/measurements.rego       | 54 +++++++++----------
 policy/lib/konstraint/pods.rego               | 16 +++---
 policy/lib/konstraint/rbac.rego               |  4 +-
 policy/lib/openshift.rego                     | 18 ++++---
 .../src.rego                                  |  6 ++-
 .../src.rego                                  | 10 ++--
 .../src.rego                                  | 11 ++--
 .../src.rego                                  |  1 +
 11 files changed, 129 insertions(+), 74 deletions(-)
 create mode 100644 .github/workflows/regal-lint.yaml
 create mode 100644 .regal/config.yaml

diff --git a/.github/workflows/regal-lint.yaml b/.github/workflows/regal-lint.yaml
new file mode 100644
index 00000000..c11468b9
--- /dev/null
+++ b/.github/workflows/regal-lint.yaml
@@ -0,0 +1,18 @@
+name: Lint policies with Regal
+
+on: [push, pull_request]
+
+jobs:
+  lint-policy:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Check out code
+        uses: actions/checkout@v4
+
+      - name: Setup Regal
+        uses: StyraInc/setup-regal@v0.2.0
+        with:
+          version: v0.10.1
+
+      - name: Run Regal lint
+        run: regal lint --format github policy
diff --git a/.regal/config.yaml b/.regal/config.yaml
new file mode 100644
index 00000000..8d09d7c3
--- /dev/null
+++ b/.regal/config.yaml
@@ -0,0 +1,22 @@
+rules:
+  idiomatic:
+    no-defined-entrypoint:
+      level: ignore
+  style:
+    avoid-get-and-list-prefix:
+      level: ignore
+    external-reference:
+      level: ignore
+    line-length:
+      level: ignore
+    no-whitespace-comment:
+      level: ignore
+    opa-fmt:
+      level: ignore
+    prefer-some-in-iteration:
+      level: ignore
+    prefer-snake-case:
+      # 13 violations in the repo seems easy to fix
+      level: ignore
+    todo-comment:
+      level: ignore
diff --git a/policy/lib/konstraint/core.rego b/policy/lib/konstraint/core.rego
index 8ead2741..14adb6bb 100644
--- a/policy/lib/konstraint/core.rego
+++ b/policy/lib/konstraint/core.rego
@@ -1,41 +1,44 @@
 package lib.konstraint.core
 
-default is_gatekeeper = false
+default is_gatekeeper := false
 
 is_gatekeeper {
     has_field(input, "review")
     has_field(input.review, "object")
 }
 
-resource = input.review.object {
+resource := input.review.object {
     is_gatekeeper
 }
 
-resource = input {
+# This is a bug in Regal causing a false positive
+# to be reported. It's been fixed, and this can
+# safely be removed following the next (above
+# v0.10.1) release.
+# https://github.com/StyraInc/regal/issues/401
+#
+# regal ignore:top-level-iteration
+resource := input {
     not is_gatekeeper
 }
 
-format(msg) = {"msg": msg} {
-    true
-}
+format(msg) := {"msg": msg}
 
-format_with_id(msg, id) = msg_fmt {
-    msg_fmt := {
-        "msg": sprintf("%s: %s", [id, msg]),
-        "details": {"policyID": id}
-    }
+format_with_id(msg, id) := {
+    "msg": sprintf("%s: %s", [id, msg]),
+    "details": {"policyID": id}
 }
 
-apiVersion = resource.apiVersion
-name = resource.metadata.name
-kind = resource.kind
-labels = resource.metadata.labels
-annotations = resource.metadata.annotations
+apiVersion := resource.apiVersion
+name := resource.metadata.name
+kind := resource.kind
+labels := resource.metadata.labels
+annotations := resource.metadata.annotations
 gv := split(apiVersion, "/")
-group = gv[0] {
+group := gv[0] {
     contains(apiVersion, "/")
 }
-group = "core" {
+group := "core" {
     not contains(apiVersion, "/")
 }
 version := gv[count(gv) - 1]
@@ -52,10 +55,10 @@ has_field(obj, field) {
     not object.get(obj, field, "N_DEFINED") == "N_DEFINED"
 }
 
-missing_field(obj, field) = true {
+missing_field(obj, field) := true {
     obj[field] == ""
 }
 
-missing_field(obj, field) = true {
+missing_field(obj, field) := true {
     not has_field(obj, field)
 }
diff --git a/policy/lib/konstraint/measurements.rego b/policy/lib/konstraint/measurements.rego
index 2ba38085..1201c931 100644
--- a/policy/lib/konstraint/measurements.rego
+++ b/policy/lib/konstraint/measurements.rego
@@ -1,69 +1,69 @@
 package lib.konstraint.measurements
 
 # 10 ** 21
-mem_multiple("E") = 1000000000000000000000 { true }
+mem_multiple("E") := 1000000000000000000000
 
 # 10 ** 18
-mem_multiple("P") = 1000000000000000000 { true }
+mem_multiple("P") := 1000000000000000000
 
 # 10 ** 15
-mem_multiple("T") = 1000000000000000 { true }
+mem_multiple("T") := 1000000000000000
 
 # 10 ** 12
-mem_multiple("G") = 1000000000000 { true }
+mem_multiple("G") := 1000000000000
 
 # 10 ** 9
-mem_multiple("M") = 1000000000 { true }
+mem_multiple("M") := 1000000000
 
 # 10 ** 6
-mem_multiple("k") = 1000000 { true }
+mem_multiple("k") := 1000000
 
 # 10 ** 3
-mem_multiple("") = 1000 { true }
+mem_multiple("") := 1000
 
 # Kubernetes accepts millibyte precision when it probably shouldn't.
 # https://github.com/kubernetes/kubernetes/issues/28741
 # 10 ** 0
-mem_multiple("m") = 1 { true }
+mem_multiple("m") := 1
 
 # 1000 * 2 ** 10
-mem_multiple("Ki") = 1024000 { true }
+mem_multiple("Ki") := 1024000
 
 # 1000 * 2 ** 20
-mem_multiple("Mi") = 1048576000 { true }
+mem_multiple("Mi") := 1048576000
 
 # 1000 * 2 ** 30
-mem_multiple("Gi") = 1073741824000 { true }
+mem_multiple("Gi") := 1073741824000
 
 # 1000 * 2 ** 40
-mem_multiple("Ti") = 1099511627776000 { true }
+mem_multiple("Ti") := 1099511627776000
 
 # 1000 * 2 ** 50
-mem_multiple("Pi") = 1125899906842624000 { true }
+mem_multiple("Pi") := 1125899906842624000
 
 # 1000 * 2 ** 60
-mem_multiple("Ei") = 1152921504606846976000 { true }
+mem_multiple("Ei") := 1152921504606846976000
 
-get_suffix(mem) = suffix {
+get_suffix(mem) := suffix {
     not is_string(mem)
     suffix := ""
 }
 
-get_suffix(mem) = suffix {
+get_suffix(mem) := suffix {
     is_string(mem)
     count(mem) > 0
     suffix := substring(mem, count(mem) - 1, -1)
     mem_multiple(suffix)
 }
 
-get_suffix(mem) = suffix {
+get_suffix(mem) := suffix {
     is_string(mem)
     count(mem) > 1
     suffix := substring(mem, count(mem) - 2, -1)
     mem_multiple(suffix)
 }
 
-get_suffix(mem) = suffix {
+get_suffix(mem) := suffix {
     is_string(mem)
     count(mem) > 1
     not mem_multiple(substring(mem, count(mem) - 1, -1))
@@ -71,25 +71,25 @@ get_suffix(mem) = suffix {
     suffix := ""
 }
 
-get_suffix(mem) = suffix {
+get_suffix(mem) := suffix {
     is_string(mem)
     count(mem) == 1
     not mem_multiple(substring(mem, count(mem) - 1, -1))
     suffix := ""
 }
 
-get_suffix(mem) = suffix {
+get_suffix(mem) := suffix {
     is_string(mem)
     count(mem) == 0
     suffix := ""
 }
 
-canonify_mem(orig) = new {
+canonify_mem(orig) := new {
     is_number(orig)
     new := orig * 1000
 }
 
-canonify_mem(orig) = new {
+canonify_mem(orig) := new {
     not is_number(orig)
     suffix := get_suffix(orig)
     raw := replace(orig, suffix, "")
@@ -97,12 +97,12 @@ canonify_mem(orig) = new {
     new := to_number(raw) * mem_multiple(suffix)
 }
 
-canonify_storage(orig) = new {
+canonify_storage(orig) := new {
     is_number(orig)
     new := orig
 }
 
-canonify_storage(orig) = new {
+canonify_storage(orig) := new {
     not is_number(orig)
     suffix := get_suffix(orig)
     raw := replace(orig, suffix, "")
@@ -110,18 +110,18 @@ canonify_storage(orig) = new {
     new := to_number(raw) * mem_multiple(suffix)
 }
 
-canonify_cpu(orig) = new {
+canonify_cpu(orig) := new {
     is_number(orig)
     new := orig * 1000
 }
 
-canonify_cpu(orig) = new {
+canonify_cpu(orig) := new {
     not is_number(orig)
     endswith(orig, "m")
     new := to_number(replace(orig, "m", ""))
 }
 
-canonify_cpu(orig) = new {
+canonify_cpu(orig) := new {
     not is_number(orig)
     not endswith(orig, "m")
     re_match("^[0-9]+$", orig)
diff --git a/policy/lib/konstraint/pods.rego b/policy/lib/konstraint/pods.rego
index a4432d78..1821e308 100644
--- a/policy/lib/konstraint/pods.rego
+++ b/policy/lib/konstraint/pods.rego
@@ -2,27 +2,27 @@ package lib.konstraint.pods
 
 import data.lib.konstraint.core
 
-default pod = false
+default pod := false
 
-pod = core.resource.spec.template {
+pod := core.resource.spec.template {
     pod_templates := ["daemonset","deployment","job","replicaset","replicationcontroller","statefulset"]
     lower(core.kind) == pod_templates[_]
 }
 
-pod = core.resource {
+pod := core.resource {
     lower(core.kind) == "pod"
 }
 
-pod = core.resource.spec.jobTemplate.spec.template {
+pod := core.resource.spec.jobTemplate.spec.template {
     lower(core.kind) == "cronjob"
 }
 
 containers[container] {
-    keys = {"containers", "initContainers"}
-    all_containers = [c | keys[k]; c = pod.spec[k][_]]
-    container = all_containers[_]
+    keys := {"containers", "initContainers"}
+    all_containers = [c | some k; keys[k]; c := pod.spec[k][_]]
+    container := all_containers[_]
 }
 
 volumes[volume] {
-    volume = pod.spec.volumes[_]
+    volume := pod.spec.volumes[_]
 }
diff --git a/policy/lib/konstraint/rbac.rego b/policy/lib/konstraint/rbac.rego
index 2f331671..9b649a7c 100644
--- a/policy/lib/konstraint/rbac.rego
+++ b/policy/lib/konstraint/rbac.rego
@@ -1,5 +1,7 @@
 package lib.konstraint.rbac
 
+import future.keywords.in
+
 import data.lib.konstraint.core
 
 rule_has_verb(rule, verb) {
@@ -13,7 +15,7 @@ rule_has_resource_type(rule, type) {
 }
 
 rule_has_resource_name(rule, name) {
-    name == rule.resourceNames[_]
+    name in rule.resourceNames
 }
 
 rule_has_resource_name(rule, name) {
diff --git a/policy/lib/openshift.rego b/policy/lib/openshift.rego
index b76e57f7..c284ce82 100644
--- a/policy/lib/openshift.rego
+++ b/policy/lib/openshift.rego
@@ -1,21 +1,23 @@
 package lib.openshift
 
+import future.keywords.in
+
 import data.lib.konstraint.core as konstraint_core
 import data.lib.konstraint.pods as konstraint_pods
 import data.lib.kubernetes
 
-pod = konstraint_pods.pod {
+pod := konstraint_pods.pod {
     konstraint_pods.pod
 }
 
-pod = konstraint_core.resource.spec.template {
+pod := konstraint_core.resource.spec.template {
     is_deploymentconfig
 }
 
 containers[container] {
-    keys = {"containers", "initContainers"}
-    all_containers = [c | keys[k]; c = pod.spec[k][_]]
-    container = all_containers[_]
+    keys := {"containers", "initContainers"}
+    all_containers := [c | some k; keys[k]; c := pod.spec[k][_]]
+    container := all_containers[_]
 }
 
 is_deploymentconfig {
@@ -52,16 +54,16 @@ is_policy_active(policyId) {
 }
 
 label_contains(disabledpolicies, policyId) {
-    policyId == disabledpolicies[_]
+    policyId in disabledpolicies
 }
 
-namespace_disabled_policies_label = disabledpolicies {
+namespace_disabled_policies_label := disabledpolicies {
     namepace := data.inventory.cluster["v1"].Namespace[konstraint_core.resource.metadata.namespace]
     label := namepace.metadata.labels["redhat-cop.github.com/gatekeeper-disabled-policies"]
     disabledpolicies := split(label, ",")
 }
 
-namespace_disabled_policies_label = [""] {
+namespace_disabled_policies_label := [""] {
     namepace := data.inventory.cluster["v1"].Namespace[konstraint_core.resource.metadata.namespace]
     not namepace.metadata.labels["redhat-cop.github.com/gatekeeper-disabled-policies"]
 }
\ No newline at end of file
diff --git a/policy/ocp/bestpractices/container-image-unknownregistries/src.rego b/policy/ocp/bestpractices/container-image-unknownregistries/src.rego
index a1817e3f..ad7da02c 100644
--- a/policy/ocp/bestpractices/container-image-unknownregistries/src.rego
+++ b/policy/ocp/bestpractices/container-image-unknownregistries/src.rego
@@ -5,6 +5,8 @@
 # @kinds apps.openshift.io/DeploymentConfig apps/DaemonSet apps/Deployment apps/Job apps/ReplicaSet core/ReplicationController apps/StatefulSet core/Pod batch/CronJob
 package ocp.bestpractices.container_image_unknownregistries
 
+import future.keywords.in
+
 import data.lib.konstraint.core as konstraint_core
 import data.lib.openshift
 
@@ -18,7 +20,7 @@ violation[msg] {
   msg := konstraint_core.format_with_id(sprintf("%s/%s: container '%s' is from (%s), which is an unknown registry.", [konstraint_core.kind, konstraint_core.name, container.name, container.image]), "RHCOP-OCP_BESTPRACT-00004")
 }
 
-get_registry(image) = registry {
+get_registry(image) := registry {
   contains(image, "/")
   possible_registry := lower(split(image, "/")[0])
   contains(possible_registry, ".")
@@ -28,5 +30,5 @@ get_registry(image) = registry {
 
 known_registry(image, registry) {
   known_registries := ["image-registry.openshift-image-registry.svc", "registry.redhat.io", "registry.connect.redhat.com", "quay.io"]
-  registry == known_registries[_]
+  registry in known_registries
 }
\ No newline at end of file
diff --git a/policy/ocp/bestpractices/container-resources-memoryunit-incorrect/src.rego b/policy/ocp/bestpractices/container-resources-memoryunit-incorrect/src.rego
index 55d33e66..3157b571 100644
--- a/policy/ocp/bestpractices/container-resources-memoryunit-incorrect/src.rego
+++ b/policy/ocp/bestpractices/container-resources-memoryunit-incorrect/src.rego
@@ -8,6 +8,8 @@
 # @kinds apps.openshift.io/DeploymentConfig apps/DaemonSet apps/Deployment apps/Job apps/ReplicaSet core/ReplicationController apps/StatefulSet core/Pod batch/CronJob
 package ocp.bestpractices.container_resources_memoryunit_incorrect
 
+import future.keywords.in
+
 import data.lib.konstraint.core as konstraint_core
 import data.lib.openshift
 
@@ -23,10 +25,10 @@ violation[msg] {
 }
 
 is_resource_memory_units_valid(container) {
-  memoryLimitsUnit := regex.find_n("[A-Za-z]+", container.resources.limits.memory, 1)[0]
-  memoryRequestsUnit := regex.find_n("[A-Za-z]+", container.resources.requests.memory, 1)[0]
+  memoryLimitsUnit := regex.find_n(`[A-Za-z]+`, container.resources.limits.memory, 1)[0]
+  memoryRequestsUnit := regex.find_n(`[A-Za-z]+`, container.resources.requests.memory, 1)[0]
 
   units := ["Ei", "Pi", "Ti", "Gi", "Mi", "Ki", "E", "P", "T", "G", "M", "K"]
-  memoryLimitsUnit == units[_]
-  memoryRequestsUnit == units[_]
+  memoryLimitsUnit in units
+  memoryRequestsUnit in units
 }
\ No newline at end of file
diff --git a/policy/ocp/bestpractices/container-resources-requests-cpuunit-incorrect/src.rego b/policy/ocp/bestpractices/container-resources-requests-cpuunit-incorrect/src.rego
index 2d204f56..46b33093 100644
--- a/policy/ocp/bestpractices/container-resources-requests-cpuunit-incorrect/src.rego
+++ b/policy/ocp/bestpractices/container-resources-requests-cpuunit-incorrect/src.rego
@@ -6,6 +6,8 @@
 # @kinds apps.openshift.io/DeploymentConfig apps/DaemonSet apps/Deployment apps/Job apps/ReplicaSet core/ReplicationController apps/StatefulSet core/Pod batch/CronJob
 package ocp.bestpractices.container_resources_requests_cpuunit_incorrect
 
+import future.keywords.in
+
 import data.lib.konstraint.core as konstraint_core
 import data.lib.openshift
 
@@ -24,9 +26,11 @@ is_resource_requests_cpu_contains_dollar(container) {
   startswith(container.resources.requests.cpu, "$")
 }
 
-#TODO: is input correct?
+# TODO: is input correct?
 is_resource_requests_cpu_a_core(container)  {
   is_number(input.resources.requests.cpu)
+  # This should never fail given that is_number succeeds
+  # regal ignore:unused-return-value
   to_number(input.resources.requests.cpu)
 }
 
@@ -38,8 +42,7 @@ is_resource_requests_cpu_units_valid(container)  {
   not is_resource_requests_cpu_a_core(container)
 
   # 'cpu' can be a quoted number, which is why we concat an empty string[] to match whole cpu cores
-  cpuRequestsUnit := array.concat(regex.find_n("[A-Za-z]+", container.resources.requests.cpu, 1), [""])[0]
+  cpuRequestsUnit := array.concat(regex.find_n(`[A-Za-z]+`, container.resources.requests.cpu, 1), [""])[0]
 
-  units := ["m", ""]
-  cpuRequestsUnit == units[_]
+  cpuRequestsUnit in {"m", ""}
 }
\ No newline at end of file
diff --git a/policy/ocp/bestpractices/deploymentconfig-triggers-containername/src.rego b/policy/ocp/bestpractices/deploymentconfig-triggers-containername/src.rego
index 812b0a74..f0764b27 100644
--- a/policy/ocp/bestpractices/deploymentconfig-triggers-containername/src.rego
+++ b/policy/ocp/bestpractices/deploymentconfig-triggers-containername/src.rego
@@ -21,6 +21,7 @@ violation[msg] {
   msg := konstraint_core.format_with_id(sprintf("%s/%s: has a imageChangeParams trigger with a miss-matching container name for '%s'", [konstraint_core.kind, konstraint_core.name, triggerContainerName]), "RHCOP-OCP_BESTPRACT-00027")
 }
 
+# regal ignore:custom-in-construct
 containers_contains_trigger(containers, triggerContainerName) {
   containers[_].name == triggerContainerName
 }
\ No newline at end of file