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.

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

Add a DryRun CLI #298

Conversation

JustinKuli
Copy link
Member

See each commit, or this story: https://issues.redhat.com/browse/ACM-14161

pkg/mappings/mappings.go Outdated Show resolved Hide resolved
pkg/mappings/mappings.go Outdated Show resolved Hide resolved
pkg/mappings/mappings.go Outdated Show resolved Hide resolved
// and decodes them from YAML into k8s resources. Directories can be passed in
// arguments: in this case all files in that directory will be read and decoded
// (but no "deeper" directories are considered).
func (d *DryRunner) readInputResources(cmd *cobra.Command, args []string) (
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is useful, but this is how I pull up all nested YAML files from the current directory (could be modified for your case):

func getYAMLFiles() ([]string, error) {
	workingDir, err := os.Getwd()
	if err != nil {
		return nil, err
	}

	yamlFiles := []string{}

	err = filepath.WalkDir(workingDir, func(path string, d fs.DirEntry, err error) error {
		if err != nil {
			return nil
		}

		if d.IsDir() {
			return nil
		}

		fileExt := filepath.Ext(d.Name())
		if fileExt == ".yaml" || fileExt == ".yml" {
			yamlFiles = append(yamlFiles, path)
		}

		return nil
	})
	if err != nil {
		return nil, err
	}

	return yamlFiles, nil
}

pkg/mappings/mappings.go Outdated Show resolved Hide resolved
return err
}

for _, msg := range messages {
Copy link
Member

Choose a reason for hiding this comment

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

Is this already sorted by timestamp?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the items in the event list are sorted lexically, but since a timestamp is in the name, effectively it is sorted by time.

@mprahl
Copy link
Member

mprahl commented Sep 24, 2024

@JustinKuli this is how I got the template lookups to work:

diff --git a/cmd/dryrun/dryrun.go b/cmd/dryrun/dryrun.go
index e69978e..20e3723 100644
--- a/cmd/dryrun/dryrun.go
+++ b/cmd/dryrun/dryrun.go
@@ -447,14 +447,15 @@ func (d *DryRunner) setupReconciler(
 			return nil, err
 		}
 
-		rec.RestMapper = mappings.MapperFrom(apiMappings)
+		rec.RestMapper, clientset.Resources = mappings.MapperFrom(apiMappings)
 	} else {
-		mapper, err := mappings.DefaultMapper()
+		mapper, resourceList, err := mappings.DefaultMapper()
 		if err != nil {
 			return nil, err
 		}
 
 		rec.RestMapper = mapper
+		clientset.Resources = resourceList
 	}
 
 	// wait for dynamic watcher to have started
diff --git a/pkg/mappings/mappings.go b/pkg/mappings/mappings.go
index e4ea0f6..088b6a2 100644
--- a/pkg/mappings/mappings.go
+++ b/pkg/mappings/mappings.go
@@ -11,6 +11,7 @@ import (
 
 	"github.com/spf13/cobra"
 	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
@@ -32,9 +33,11 @@ type APIMapping struct {
 // MapperFrom takes a list of APIMappings and uses them to populate a
 // DefaultRESTMapper which can be used locally without connecting to a Discovery
 // client.
-func MapperFrom(mappings []APIMapping) meta.RESTMapper {
+func MapperFrom(mappings []APIMapping) (meta.RESTMapper, []*metav1.APIResourceList) {
 	mapper := meta.NewDefaultRESTMapper(scheme.Scheme.PreferredVersionAllGroups())
 
+	resourceListMapping := map[schema.GroupVersion][]metav1.APIResource{}
+
 	for _, mapping := range mappings {
 		gv := schema.GroupVersion{
 			Group:   mapping.Group,
@@ -56,9 +59,25 @@ func MapperFrom(mappings []APIMapping) meta.RESTMapper {
 				namespaced,
 			)
 		}
+
+		resourceListMapping[gv] = append(resourceListMapping[gv], metav1.APIResource{
+			Kind:         mapping.Kind,
+			Name:         mapping.Plural,
+			SingularName: mapping.Singular,
+			Namespaced:   mapping.Scope == "Namespaced",
+		})
+	}
+
+	resourceLists := []*metav1.APIResourceList{}
+
+	for gv := range resourceListMapping {
+		resourceLists = append(resourceLists, &metav1.APIResourceList{
+			GroupVersion: gv.String(),
+			APIResources: resourceListMapping[gv],
+		})
 	}
 
-	return mapper
+	return mapper, resourceLists
 }
 
 //go:embed default-mappings.json
@@ -66,14 +85,16 @@ var defaultMappingsJSON []byte
 
 // DefaultMapper returns a RESTMapper initialized on default mappings that were
 // embedded into this package. It should include built-in Kubernetes types.
-func DefaultMapper() (meta.RESTMapper, error) {
+func DefaultMapper() (meta.RESTMapper, []*metav1.APIResourceList, error) {
 	mappings := []APIMapping{}
 
 	if err := json.Unmarshal(defaultMappingsJSON, &mappings); err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	return MapperFrom(mappings), nil
+	mapper, resourceList := MapperFrom(mappings)
+
+	return mapper, resourceList, nil
 }
 
 // GenerateMappings connects to a Kubernetes cluster and discovers the available

Previously, this map was initialized when being setup with a controller
runtime manager. It is not necessary to wait until then, and it was
preventing any possibility of running the reconcilers without a real
manager (for example, in tests).

Signed-off-by: Justin Kulikauskas <[email protected]>
Storing both the rest.Config *and* multiple clients generated from it
could lead to confusion, and prevented the possibility of using clients
that we don't have the config for (potentially in tests).

Signed-off-by: Justin Kulikauskas <[email protected]>
v2 is much more configurable.

Signed-off-by: Justin Kulikauskas <[email protected]>
for gv, resList := range resources {
for _, res := range resList.APIResources {
if strings.Contains(res.Name, "/") {
continue // skip subresources
Copy link
Member Author

Choose a reason for hiding this comment

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

@mprahl FYI, GroupsAndMaybeResources includes the subresources, which I was ignoring when parsing /api and /apis "by hand"... I didn't see a better way to determine if something was a subresource, but if I included them in the mappings / resources used by dryrun, tests would fail and it looked like it was incorrectly checking things in subresources (eg a noncompliant message indicated that a "pod/status did not match")


filepaths := []string{}

if stdinInfo.Mode()&os.ModeCharDevice == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Not entirely sure if this is the correct fix, but when running it through the debugger where it has no stdin, it would just hang forever for the YAML decoder to get something from stdin.

Suggested change
if stdinInfo.Mode()&os.ModeCharDevice == 0 {
if stdinInfo.Mode()&os.ModeCharDevice == 0 && stdinInfo.Size() != 0 {

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, I don't have any issue when I run the cli locally without any stdin, but this change seems reasonable

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll need to look at this more. With && stdinInfo.Size() != 0, I found that the cli doesn't successfully read inputs from stdin anymore when you run it directly. The tests still pass, I think because I kind of hacked the inputs for the tests...

So based on the test_multiple_resources case:

❯ cat input_stdin.yaml | ./dryrun-original -p policy.yaml input_3.yaml input_4.yaml 
# Diffs:
v1 Pod default/nginx-pod-e2e:

v1 Pod default/nginx-pod-e2e-2:

v1 Pod default/nginx-pod-e2e-3:

v1 Pod default/nginx-pod-e2e-4:

# Compliance messages:
Compliant; notification - pods [nginx-pod-e2e] found as specified in namespace default; notification - pods [nginx-pod-e2e-2] found as specified in namespace default; notification - pods [nginx-pod-e2e-3] found as specified in namespace default; notification - pods [nginx-pod-e2e-4] found as specified in namespace default

❯ cat input_stdin.yaml | ./dryrun-sizene0 -p policy.yaml input_3.yaml input_4.yaml                 
# Diffs:
v1 Pod default/nginx-pod-e2e-3:

v1 Pod default/nginx-pod-e2e-4:

# Compliance messages:
Compliant; notification - pods [nginx-pod-e2e-3] found as specified in namespace default; notification - pods [nginx-pod-e2e-4] found as specified in namespace default

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a debugger setting where you can set stdin?

Copy link
Member

Choose a reason for hiding this comment

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

@JustinKuli I don't know of one but this is the launch.json section that I had with this issue:

        {
            "name": "Launch dryrun",
            "type": "go",
            "request": "launch",
            "mode": "auto",
            "program": "${workspaceFolder}/cmd/main.go",
            "args": [
                "-p",
                "${workspaceFolder}/cmd/dryrun/testdata/test_basic_compliant/policy.yaml",
                "${workspaceFolder}/cmd/dryrun/testdata/test_basic_compliant/input_1.yaml"
            ]
        }

Copy link
Member

Choose a reason for hiding this comment

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

We can leave that out for now though!

@mprahl
Copy link
Member

mprahl commented Sep 25, 2024

@JustinKuli could we please remove the RestMapper in the controller? I think this works pretty well:

diff --git a/cmd/dryrun/dryrun.go b/cmd/dryrun/dryrun.go
index 662e716..f93b601 100644
--- a/cmd/dryrun/dryrun.go
+++ b/cmd/dryrun/dryrun.go
@@ -22,7 +22,6 @@ import (
 	yaml "gopkg.in/yaml.v3"
 	corev1 "k8s.io/api/core/v1"
 	extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
-	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
@@ -71,17 +70,17 @@ func (d *DryRunner) dryRun(cmd *cobra.Command, args []string) error {
 	for _, obj := range inputObjects {
 		gvk := obj.GroupVersionKind()
 
-		mapping, err := rec.RestMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
+		scopedGVR, err := rec.DynamicWatcher.GVKToGVR(gvk)
 		if err != nil {
 			return err
 		}
 
 		var resInt dynamic.ResourceInterface
 
-		if mapping.Scope.Name() == meta.RESTScopeNameNamespace {
-			resInt = rec.TargetK8sDynamicClient.Resource(mapping.Resource).Namespace(obj.GetNamespace())
+		if scopedGVR.Namespaced {
+			resInt = rec.TargetK8sDynamicClient.Resource(scopedGVR.GroupVersionResource).Namespace(obj.GetNamespace())
 		} else {
-			resInt = rec.TargetK8sDynamicClient.Resource(mapping.Resource)
+			resInt = rec.TargetK8sDynamicClient.Resource(scopedGVR.GroupVersionResource)
 		}
 
 		if _, err := resInt.Create(ctx, obj, metav1.CreateOptions{}); err != nil {
@@ -247,7 +246,7 @@ func (d *DryRunner) readInputResources(cmd *cobra.Command, args []string) (
 
 	filepaths := []string{}
 
-	if stdinInfo.Mode()&os.ModeCharDevice == 0 {
+	if stdinInfo.Mode()&os.ModeCharDevice == 0 && stdinInfo.Size() != 0 {
 		filepaths = append(filepaths, "-")
 	}
 
@@ -447,9 +446,9 @@ func (d *DryRunner) setupReconciler(
 			return nil, err
 		}
 
-		rec.RestMapper, clientset.Resources = mappings.MapperFrom(apiMappings)
+		clientset.Resources = mappings.ResourceListFromMapping(apiMappings)
 	} else {
-		rec.RestMapper, clientset.Resources, err = mappings.DefaultMapper()
+		clientset.Resources, err = mappings.DefaultResourceList()
 		if err != nil {
 			return nil, err
 		}
diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go
index 135fccf..007aafd 100644
--- a/controllers/configurationpolicy_controller.go
+++ b/controllers/configurationpolicy_controller.go
@@ -27,7 +27,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	extensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
-	apimachinerymeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -203,7 +202,6 @@ type ConfigurationPolicyReconciler struct {
 	// This is a workaround to account for race conditions where the status is updated but the controller-runtime cache
 	// has not updated yet.
 	lastEvaluatedCache sync.Map
-	RestMapper         apimachinerymeta.RESTMapper
 }
 
 //+kubebuilder:rbac:groups=*,resources=*,verbs=*
@@ -1901,16 +1899,6 @@ func (r *ConfigurationPolicyReconciler) getMapping(
 		return depclient.ScopedGVR{}, err
 	}
 
-	if r.RestMapper != nil {
-		mapping, err := r.RestMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
-		if err == nil {
-			return depclient.ScopedGVR{
-				GroupVersionResource: mapping.Resource,
-				Namespaced:           mapping.Scope.Name() == apimachinerymeta.RESTScopeNameNamespace,
-			}, nil
-		}
-	}
-
 	scopedGVR, err := r.DynamicWatcher.GVKToGVR(gvk)
 	if err != nil {
 		if !errors.Is(err, depclient.ErrNoVersionedResource) {
diff --git a/pkg/mappings/mappings.go b/pkg/mappings/mappings.go
index 38fe86c..88998ba 100644
--- a/pkg/mappings/mappings.go
+++ b/pkg/mappings/mappings.go
@@ -13,7 +13,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/clientcmd"
-	"k8s.io/kubectl/pkg/scheme"
 	"sigs.k8s.io/yaml"
 )
 
@@ -32,12 +31,9 @@ func (a APIMapping) String() string {
 	return a.Group + "/" + a.Version + " " + a.Plural
 }
 
-// MapperFrom takes a list of APIMappings and uses them to populate a
-// DefaultRESTMapper which can be used locally without connecting to a Discovery
-// client.
-func MapperFrom(mappings []APIMapping) (meta.RESTMapper, []*metav1.APIResourceList) {
-	mapper := meta.NewDefaultRESTMapper(scheme.Scheme.PreferredVersionAllGroups())
-
+// ResourceListFromMapping takes a list of APIMappings and uses them to populate an
+// APIResourceList which can be used to populate the fake discovery client.
+func ResourceListFromMapping(mappings []APIMapping) []*metav1.APIResourceList {
 	resourceListMapping := map[schema.GroupVersion][]metav1.APIResource{}
 
 	for _, mapping := range mappings {
@@ -46,13 +42,6 @@ func MapperFrom(mappings []APIMapping) (meta.RESTMapper, []*metav1.APIResourceLi
 			Version: mapping.Version,
 		}
 
-		mapper.AddSpecific(
-			gv.WithKind(mapping.Kind),
-			gv.WithResource(mapping.Plural),
-			gv.WithResource(mapping.Singular),
-			mapping.Scope,
-		)
-
 		resourceListMapping[gv] = append(resourceListMapping[gv], metav1.APIResource{
 			Kind:         mapping.Kind,
 			Name:         mapping.Plural,
@@ -70,24 +59,24 @@ func MapperFrom(mappings []APIMapping) (meta.RESTMapper, []*metav1.APIResourceLi
 		})
 	}
 
-	return mapper, resourceLists
+	return resourceLists
 }
 
 //go:embed default-mappings.yaml
 var defaultMappings []byte
 
-// DefaultMapper returns a RESTMapper initialized on default mappings that were
-// embedded into this package. It should include built-in Kubernetes types.
-func DefaultMapper() (meta.RESTMapper, []*metav1.APIResourceList, error) {
+// ResourceListFromMapping uses the default list of APIMappings and uses them to populate an
+// APIResourceList which can be used to populate the fake discovery client.
+func DefaultResourceList() ([]*metav1.APIResourceList, error) {
 	mappings := []APIMapping{}
 
 	if err := yaml.Unmarshal(defaultMappings, &mappings); err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
-	mapper, resList := MapperFrom(mappings)
+	resList := ResourceListFromMapping(mappings)
 
-	return mapper, resList, nil
+	return resList, nil
 }
 
 // GenerateMappings connects to a Kubernetes cluster and discovers the available

This would allow a user to save a set of REST mappings from an existing
cluster which the config-policy-controller reconciler *could* use
instead of a "live" discovery API. Currently the setup in main.go for
the controller *does not* connect this, but it would be possible to add
in the future.

The included "default-mappings.yaml" file is the output of the tool from
a simple kind cluster.

Signed-off-by: Justin Kulikauskas <[email protected]>
This command allows for the testing of configuration policies without a
cluster. "Cluster state" is provided via input files, which are
simulated in a fake cluster. A real config-policy-controller reconciler
is run against this fake cluster, and the resulting policy status, as
well as the compliance events, are captured and can be displayed to the
user. If the policy is noncompliant, the command exits with error code 2
(to distinguish it from other possible exit reasons).

This also exposes the GenerateMappings command as a subcommand.

Refs:
 - https://issues.redhat.com/browse/ACM-14161

Signed-off-by: Justin Kulikauskas <[email protected]>
Copy link

openshift-ci bot commented Sep 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JustinKuli, mprahl

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-merge-bot openshift-merge-bot bot merged commit 6e86ab4 into open-cluster-management-io:main Sep 25, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants