Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion Makefile
Copy link
Member

Choose a reason for hiding this comment

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

SKIP_SECRET_GENERATION ENV can be exported with false

Copy link
Member Author

Choose a reason for hiding this comment

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

Explained in #11 (comment).

Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,12 @@ test-e2e-teardown: $(KIND)
$(e2e_targets):: test-e2e-setup
test-e2e:: $(e2e_tests) ## Run e2e tests

test-e2e-ansible:: image/ansible-operator ## Run Ansible e2e tests
test-e2e-ansible:: image/ansible-operator test-e2e-ansible-run ## Run Ansible e2e tests

.PHONY: test-e2e-ansible-run
test-e2e-ansible-run:
go test ./test/e2e/ansible -v -ginkgo.v
Comment on lines +165 to 167
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to move this target above # Double colon rules allow repeated rule declarations. comment line for better flow understanding.

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 thought adding the test-e2e-ansible-run target just below the test-e2e-ansible target would make it easier to understand the flow.


test-e2e-ansible-molecule:: install dev-install image/ansible-operator ## Run molecule-based Ansible e2e tests
go run ./hack/generate/samples/molecule/generate.go
./hack/tests/e2e-ansible-molecule.sh
Comment on lines 169 to 171
Copy link
Member

Choose a reason for hiding this comment

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

we should also try adding test-e2e-ansible-molecule target in the CI, as a followup PR may be.

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 can take a look at it.

Expand Down
19 changes: 19 additions & 0 deletions hack/generate/samples/ansible/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,25 @@ const rolesForBaseOperator = `
- watch
#+kubebuilder:scaffold:rules
`
const rolesForProject = `
##
## Apply customize roles related to project.openshift.io
##
- apiGroups:
- project.openshift.io
resources:
- projectrequests
- projects
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
#+kubebuilder:scaffold:rules
`

const customMetricsTest = `
- name: Search for all running pods
Expand Down
45 changes: 27 additions & 18 deletions hack/generate/samples/ansible/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ var memcachedGVK = schema.GroupVersionKind{
Kind: "Memcached",
}

var skipSecretGeneration = ""
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to make it a global variable? If this variable is getting used only in GenerateMoleculeSample, we can make it local to that function to avoid unexpected use by other files inside this package.

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 didn't want to make changes to the existing function definitions. That's why I used the global variable.


func getCli() *cli.CLI {
ansibleBundle, _ := plugin.NewBundleWithOptions(
plugin.WithName(golang.DefaultNameQualifier),
Expand Down Expand Up @@ -127,21 +129,26 @@ func GenerateMoleculeSample(samplesPath string) []sample.Sample {
)
pkg.CheckError("attempting to create sample cli", err)

addIgnore, err := samplecli.NewCliSample(
samplecli.WithCLI(getCli()),
samplecli.WithCommandContext(ansibleMoleculeMemcached.CommandContext()),
samplecli.WithGvk(
schema.GroupVersionKind{
Group: "ignore",
Version: "v1",
Kind: "Secret",
},
),
samplecli.WithPlugins("ansible"),
samplecli.WithExtraApiOptions("--generate-role"),
samplecli.WithName(ansibleMoleculeMemcached.Name()),
)
pkg.CheckError("creating ignore samples", err)
skipSecretGeneration = os.Getenv("SKIP_SECRET_GENERATION")
Copy link
Member

Choose a reason for hiding this comment

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

skipSecretGeneration would hold only boolean values, and the default should be false. Would it make sense to update the type to bool? The following helper functions could be useful.

// env returns an environment variable or a default value if not specified.
func env(key string, defaultValue string) string {
	val := os.Getenv(key)
	if len(val) == 0 {
		return defaultValue
	}
	return val
}
func isTrue(s string) bool {
	v, _ := strconv.ParseBool(s)
	return v
}
Suggested change
skipSecretGeneration = os.Getenv("SKIP_SECRET_GENERATION")
skipSecretGeneration := isTrue(env("SKIP_SECRET_GENERATION", "")

Copy link
Member Author

Choose a reason for hiding this comment

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

The usage of strconv.ParseBool() will introduce the issue of error handling. The idea is to add code with minimal/no chances of erroring out. The current code simply says that if the SKIP_SECRET_GENERATION env var is not set to any value, then the operation will work as it is working currently. However, if you need to skip the generation of the secret related code, then set the env var to any value (need not be boolean). This avoids the additional checking of the value being a valid boolean or not.

var addIgnore sample.Sample

if skipSecretGeneration == "" {
addIgnore, err = samplecli.NewCliSample(
samplecli.WithCLI(getCli()),
samplecli.WithCommandContext(ansibleMoleculeMemcached.CommandContext()),
samplecli.WithGvk(
schema.GroupVersionKind{
Group: "ignore",
Version: "v1",
Kind: "Secret",
},
),
samplecli.WithPlugins("ansible"),
samplecli.WithExtraApiOptions("--generate-role"),
samplecli.WithName(ansibleMoleculeMemcached.Name()),
)
pkg.CheckError("creating ignore samples", err)
}

// remove sample directory if it already exists
err = os.RemoveAll(ansibleMoleculeMemcached.Dir())
Expand All @@ -158,9 +165,11 @@ func GenerateMoleculeSample(samplesPath string) []sample.Sample {
err = e2e.AllowProjectBeMultiGroup(ansibleMoleculeMemcached)
pkg.CheckError("updating PROJECT file", err)

ignoreGen := sample.NewGenerator(sample.WithNoInit(), sample.WithNoWebhook())
err = ignoreGen.GenerateSamples(addIgnore)
pkg.CheckError("generating ansible molecule sample - ignore", err)
if skipSecretGeneration == "" {
ignoreGen := sample.NewGenerator(sample.WithNoInit(), sample.WithNoWebhook())
err = ignoreGen.GenerateSamples(addIgnore)
pkg.CheckError("generating ansible molecule sample - ignore", err)
}

ImplementMemcached(ansibleMoleculeMemcached, bundleImage)
Copy link
Member

Choose a reason for hiding this comment

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

ImplementMemcached can take skipSecretGeneration as an arg to avoid relying on global variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as #11 (comment).


Expand Down
77 changes: 43 additions & 34 deletions hack/generate/samples/ansible/memcached_molecule.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,14 @@ func ImplementMemcachedMolecule(sample sample.Sample, image string) {
err := kbutil.InsertCode(moleculeTaskPath, targetMoleculeCheckDeployment, molecuTaskToCheckConfigMap)
pkg.CheckError("replacing memcached task to add config map check", err)

log.Info("insert molecule task to ensure to check secret")
err = kbutil.InsertCode(moleculeTaskPath, memcachedCustomStatusMoleculeTarget, testSecretMoleculeCheck)
pkg.CheckError("replacing memcached task to add secret check", err)
if skipSecretGeneration == "" {
log.Info("insert molecule task to ensure to check secret")
err = kbutil.InsertCode(moleculeTaskPath, memcachedCustomStatusMoleculeTarget, testSecretMoleculeCheck)
pkg.CheckError("replacing memcached task to add secret check", err)
}

log.Info("insert molecule task to ensure to foo ")
err = kbutil.InsertCode(moleculeTaskPath, testSecretMoleculeCheck, testFooMoleculeCheck)
err = kbutil.InsertCode(moleculeTaskPath, memcachedCustomStatusMoleculeTarget, testFooMoleculeCheck)
Copy link
Member

Choose a reason for hiding this comment

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

Does the sequence of the tasks matter? If yes, then this might change the sequence of the code insertion for regular flow.

Copy link
Member Author

Choose a reason for hiding this comment

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

The task related to secret is independent of the other tasks. The secrets will get reconciled by default whenever the operator starts, and it will create the corresponding service. This is independent of the tasks related to the reconciliation of the Memcached CR, which will have it's own set of reconciliation related logic.

pkg.CheckError("replacing memcached task to add foo check", err)

log.Info("insert molecule task to check custom metrics")
Expand All @@ -58,6 +60,11 @@ func ImplementMemcachedMolecule(sample sample.Sample, image string) {
err = kbutil.InsertCode(filepath.Join(sample.Dir(), "roles", strings.ToLower(gvk.Kind), "tasks", "main.yml"),
roleFragment, memcachedWithBlackListTask)
pkg.CheckError("replacing in tasks/main.yml", err)

log.Info("adding RBAC permissions for project.openshift.io")
err = kbutil.ReplaceInFile(filepath.Join(sample.Dir(), "config", "rbac", "role.yaml"),
"#+kubebuilder:scaffold:rules", rolesForProject)
pkg.CheckError("replacing in role.yml", err)
}

if gvk.Kind != "Memcached" {
Expand Down Expand Up @@ -95,34 +102,36 @@ func ImplementMemcachedMolecule(sample sample.Sample, image string) {
"playbook: playbooks/memcached.yml", memcachedWatchCustomizations)
pkg.CheckError("replacing in watches", err)

log.Info("removing ignore group for the secret from watches as an workaround to work with core types")
err = kbutil.ReplaceInFile(filepath.Join(sample.Dir(), "watches.yaml"),
"ignore.example.com", "\"\"")
pkg.CheckError("replacing the watches file", err)

log.Info("removing molecule test for the Secret since it is a core type")
cmd := exec.Command("rm", "-rf", filepath.Join(sample.Dir(), "molecule", "default", "tasks", "secret_test.yml"))
_, err = sample.CommandContext().Run(cmd)
pkg.CheckError("removing secret test file", err)

log.Info("adding Secret task to the role")
err = kbutil.ReplaceInFile(filepath.Join(sample.Dir(), "roles", "secret", "tasks", "main.yml"),
originalTaskSecret, taskForSecret)
pkg.CheckError("replacing in secret/tasks/main.yml file", err)

log.Info("adding ManageStatus == false for role secret")
err = kbutil.ReplaceInFile(filepath.Join(sample.Dir(), "watches.yaml"),
"role: secret", manageStatusFalseForRoleSecret)
pkg.CheckError("replacing in watches.yaml", err)

// prevent high load of controller caused by watching all the secrets in the cluster
watchNamespacePatchFileName := "watch_namespace_patch.yaml"
log.Info("adding WATCH_NAMESPACE env patch to watch own namespace")
err = os.WriteFile(filepath.Join(sample.Dir(), "config", "testing", watchNamespacePatchFileName), []byte(watchNamespacePatch), 0644)
pkg.CheckError("adding watch_namespace_patch.yaml", err)

log.Info("adding WATCH_NAMESPACE env patch to patch list to be applied")
err = kbutil.InsertCode(filepath.Join(sample.Dir(), "config", "testing", "kustomization.yaml"), "patchesStrategicMerge:",
fmt.Sprintf("\n- %s", watchNamespacePatchFileName))
pkg.CheckError("inserting in kustomization.yaml", err)
if skipSecretGeneration == "" {
log.Info("removing ignore group for the secret from watches as an workaround to work with core types")
err = kbutil.ReplaceInFile(filepath.Join(sample.Dir(), "watches.yaml"),
"ignore.example.com", "\"\"")
pkg.CheckError("replacing the watches file", err)

log.Info("removing molecule test for the Secret since it is a core type")
cmd := exec.Command("rm", "-rf", filepath.Join(sample.Dir(), "molecule", "default", "tasks", "secret_test.yml"))
_, err = sample.CommandContext().Run(cmd)
pkg.CheckError("removing secret test file", err)

log.Info("adding Secret task to the role")
err = kbutil.ReplaceInFile(filepath.Join(sample.Dir(), "roles", "secret", "tasks", "main.yml"),
originalTaskSecret, taskForSecret)
pkg.CheckError("replacing in secret/tasks/main.yml file", err)

log.Info("adding ManageStatus == false for role secret")
err = kbutil.ReplaceInFile(filepath.Join(sample.Dir(), "watches.yaml"),
"role: secret", manageStatusFalseForRoleSecret)
pkg.CheckError("replacing in watches.yaml", err)

// prevent high load of controller caused by watching all the secrets in the cluster
watchNamespacePatchFileName := "watch_namespace_patch.yaml"
log.Info("adding WATCH_NAMESPACE env patch to watch own namespace")
err = os.WriteFile(filepath.Join(sample.Dir(), "config", "testing", watchNamespacePatchFileName), []byte(watchNamespacePatch), 0644)
pkg.CheckError("adding watch_namespace_patch.yaml", err)
Comment on lines +127 to +130
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me why watch_namespace_patch.yaml needs to be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

The operator will reconcile secrets from all namespaces and this will overburden the operator with continuous events related to all the secrets in the cluster. To reduce this reconciliation burden, a watch is added to a specific namespace. That's why this is part of the code generation related to secret.


log.Info("adding WATCH_NAMESPACE env patch to patch list to be applied")
err = kbutil.InsertCode(filepath.Join(sample.Dir(), "config", "testing", "kustomization.yaml"), "patchesStrategicMerge:",
fmt.Sprintf("\n- %s", watchNamespacePatchFileName))
pkg.CheckError("inserting in kustomization.yaml", err)
}
}
20 changes: 20 additions & 0 deletions openshift/Dockerfile.e2e-ansible
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
FROM basebuilder AS builder
Copy link
Member

Choose a reason for hiding this comment

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

Should we directly use the image tag to avoid overwriting in release config?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems easier to maintain it like this. We won't have to update it on every branch cut and it will minimize the effort going forward. Also we just need the basebulider to re-generate the testdata directory. We are not building any binary which will be impacted by the base image.


COPY . /go/src/github.com/openshift/ansible-operator-plugins

ENV SKIP_SECRET_GENERATION=true

RUN cd /go/src/github.com/openshift/ansible-operator-plugins && \
make generate

FROM openshift-ansible-operator-plugins

COPY --from=builder /go/src/github.com/openshift/ansible-operator-plugins/testdata/memcached-molecule-operator/requirements.yml ${HOME}/requirements.yml

RUN ansible-galaxy collection install -r ${HOME}/requirements.yml \
&& chmod -R ug+rwx ${HOME}/.ansible

COPY --from=builder /go/src/github.com/openshift/ansible-operator-plugins/testdata/memcached-molecule-operator/watches.yaml ${HOME}/watches.yaml
COPY --from=builder /go/src/github.com/openshift/ansible-operator-plugins/testdata/memcached-molecule-operator/roles/ ${HOME}/roles/
COPY --from=builder /go/src/github.com/openshift/ansible-operator-plugins/testdata/memcached-molecule-operator/playbooks/ ${HOME}/playbooks/

19 changes: 18 additions & 1 deletion pkg/testutils/e2e/metrics/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,25 @@ func GetMetrics(sample sample.Sample, kubectl kubernetes.Kubectl, metricsCluster
gomega.Expect(len(token)).To(gomega.BeNumerically(">", 0))

ginkgo.By("creating a curl pod")
securityContext := `
{
"apiVersion": "v1",
"spec": {
"securityContext": {
"runAsNonRoot": true,
"runAsUser": 1000,
"runAsGroup": 1000,
"fsGroup": 1000,
"seccompProfile": {
"type": "RuntimeDefault"
}
}
}
}
`
overrides := fmt.Sprintf("--overrides=%s", securityContext)
cmdOpts := []string{
"run", "curl", "--image=curlimages/curl:7.68.0", "--restart=OnFailure", "--",
"run", "curl", "--image=curlimages/curl:7.68.0", "--restart=OnFailure", overrides, "--",
"curl", "-v", "-k", "-H", fmt.Sprintf(`Authorization: Bearer %s`, token),
fmt.Sprintf("https://%s-controller-manager-metrics-service.%s.svc:8443/metrics", sample.Name(), kubectl.Namespace()),
}
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/ansible/cluster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ var _ = Describe("Running ansible projects", func() {
Expect(metrics.CleanUpMetrics(kctl, metricsClusterRoleBindingName)).To(Succeed())

By("cleaning up created API objects during test process")
Expect(operator.UndeployOperator(ansibleSample)).To(Succeed())
testutils.WrapWarnOutput("", operator.UndeployOperator(ansibleSample))
Copy link
Member

Choose a reason for hiding this comment

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

Is Undeploy causing some issues? Currently, there is only one It, so it's okay to use Warn instead of Error, but if the test grows, it could be an issue, and I think upstream won't accept this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

The make undeploy removes the objects in the same order they are created. The namespace is deleted first and as it gets deleted all the resources created in the namespaces also gets deleted along with it. This always fails when running in an OCP cluster. However, on a kind cluster, this doesn't fail as the deletion of namespace takes some time and thus the make undeploy completes successfully.

I think it's okay to not fail a test if the cleanup fails. It's not related to the actual e2e test anyway.


By("ensuring that the namespace was deleted")
testutils.WrapWarnOutput(kctl.Wait(false, "namespace", "foo", "--for", "delete", "--timeout", "2m"))
Expand Down
23 changes: 17 additions & 6 deletions test/e2e/ansible/local_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package e2e_ansible_test

import (
"os"
"os/exec"

. "github.com/onsi/ginkgo/v2"
Expand All @@ -25,18 +26,28 @@ import (
var _ = Describe("Running Ansible projects", func() {
Context("built with operator-sdk", func() {
BeforeEach(func() {
By("Installing CRD's")
err := operator.InstallCRDs(ansibleSample)
Expect(err).NotTo(HaveOccurred())
skip_local_test := os.Getenv("SKIP_LOCAL_TEST")
if skip_local_test == "" {
By("Installing CRD's")
err := operator.InstallCRDs(ansibleSample)
Expect(err).NotTo(HaveOccurred())
}
})

AfterEach(func() {
By("Uninstalling CRD's")
err := operator.UninstallCRDs(ansibleSample)
Expect(err).NotTo(HaveOccurred())
skip_local_test := os.Getenv("SKIP_LOCAL_TEST")
if skip_local_test == "" {
By("Uninstalling CRD's")
err := operator.UninstallCRDs(ansibleSample)
Expect(err).NotTo(HaveOccurred())
}
})

It("Should run correctly when run locally", func() {
skip_local_test := os.Getenv("SKIP_LOCAL_TEST")
if skip_local_test != "" {
Skip("Skipping local test")
}
By("Running the project")
cmd := exec.Command("make", "run")
cmd.Dir = ansibleSample.Dir()
Expand Down
18 changes: 12 additions & 6 deletions test/e2e/ansible/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,13 @@ var _ = BeforeSuite(func() {
}, 3*time.Minute, time.Second).Should(Succeed())
}

By("building the project image")
err = operator.BuildOperatorImage(ansibleSample, image)
Expect(err).NotTo(HaveOccurred())
if image_env_var, exists := os.LookupEnv("IMAGE_FORMAT"); exists {
image = image_env_var
} else {
By("building the project image")
err = operator.BuildOperatorImage(ansibleSample, image)
Expect(err).NotTo(HaveOccurred())
}

onKind, err := kind.IsRunningOnKind(kctl)
Expect(err).NotTo(HaveOccurred())
Expand All @@ -129,9 +133,11 @@ var _ = AfterSuite(func() {
}

By("destroying container image and work dir")
cmd := exec.Command("docker", "rmi", "-f", image)
if _, err := ansibleSample.CommandContext().Run(cmd); err != nil {
Expect(err).To(BeNil())
if _, exists := os.LookupEnv("IMAGE_FORMAT"); !exists {
cmd := exec.Command("docker", "rmi", "-f", image)
if _, err := ansibleSample.CommandContext().Run(cmd); err != nil {
Expect(err).To(BeNil())
}
}
if err := os.RemoveAll(testdir); err != nil {
Expect(err).To(BeNil())
Expand Down
18 changes: 18 additions & 0 deletions testdata/memcached-molecule-operator/config/rbac/role.yaml
Copy link
Member

Choose a reason for hiding this comment

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

The changes should happen in runtime inside the container, do we need to commit the changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is updated as a result of running make generate. This additional permission is anyway required and it was kind of a bug not to include the permissions in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also if we don't run make generate before pushing the changes to the PR, it'll fail the sanity check.

Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,23 @@ rules:
- update
- watch

##
## Apply customize roles related to project.openshift.io
##
- apiGroups:
- project.openshift.io
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the api groups installed via a CR on kind cluster?

resources:
- projectrequests
- projects
verbs:
- create
- delete
- get
- list
- patch
- update
- watch

##
## Apply customize roles for base operator
##
Expand All @@ -124,3 +141,4 @@ rules:
- watch
#+kubebuilder:scaffold:rules


Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, this is updated as a result of running make generate. If we don't run make generate before pushing the changes to the PR, it'll fail the sanity check.

Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,6 @@
resource_name=custom_resource.metadata.name
)}}'

# This will verify that the secret role was executed
- name: Verify that test-service was created
assert:
that: lookup('k8s', kind='Service', api_version='v1', namespace=namespace, resource_name='test-service')


- name: Verify that project testing-foo was created
assert:
that: lookup('k8s', kind='Namespace', api_version='v1', resource_name='testing-foo')
Expand Down Expand Up @@ -116,6 +110,12 @@



# This will verify that the secret role was executed
- name: Verify that test-service was created
assert:
that: lookup('k8s', kind='Service', api_version='v1', namespace=namespace, resource_name='test-service')


- when: molecule_yml.scenario.name == "test-local"
block:
- name: Restart the operator by killing the pod
Expand Down