Skip to content
Closed
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
235 changes: 235 additions & 0 deletions test/integration/operator_run_bundle_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,235 @@
// Copyright 2020 The Operator-SDK Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// Modified from https://github.com/kubernetes-sigs/kubebuilder/tree/39224f0/test/e2e/v3

package e2e
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Sep 4, 2020

Choose a reason for hiding this comment

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

// should be the name of the dir with _test
Assuming that it will be in /tests/e2e_olm as suggested:

Suggested change
package e2e
package e2e_olm_test


import (
"context"
"fmt"
"path"
"path/filepath"
"strings"
"testing"
"time"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/operator-framework/api/pkg/operators/v1alpha1"
"github.com/operator-framework/operator-sdk/internal/olm/operator"
"github.com/operator-framework/operator-sdk/internal/olm/operator/bundle"
testutils "github.com/operator-framework/operator-sdk/test/internal"
"github.com/sirupsen/logrus"
corev1 "k8s.io/api/core/v1"
wait "k8s.io/apimachinery/pkg/util/wait"
"sigs.k8s.io/controller-runtime/pkg/client"
kbtestutils "sigs.k8s.io/kubebuilder/test/e2e/utils"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the imports need to be sorted.

)

// TODO: After run bundle is complete, remove the boolean variable and the if statements.
var runBundleImplemented = false

var (
tc testutils.TestContext
err error
)

var _ = BeforeSuite(func() {
By("creating a new context")
// For this test suite, the bundle image name is equal to tc.ImageName
tc, err = testutils.NewTestContext("GO111MODULE=on")
Expect(err).NotTo(HaveOccurred())
Expect(tc.Prepare()).To(Succeed())

createOperator(tc, defaultOperatorName)
})

var _ = AfterSuite(func() {
By("removing container image and working dir")
tc.Destroy()
})

var _ = Describe("run bundle", func() {
It("Run Bundle Basic", func() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
It("Run Bundle Basic", func() {
It("AllNamespaces (default)", func() {

if !runBundleImplemented {
Skip("Run bundle not yet implemented")
}

cfg := &operator.Configuration{KubeconfigPath: kubeconfigPath}
err := cfg.Load()
Expect(err).NotTo(HaveOccurred())
Comment on lines +70 to +72
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why it is rerquired?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These are a part of run bundle arguments. The kubecfg (specifying the namespace if needed), and the bundle image is passed to the run bundle.
For reference: It is similar to how testing is done in packagemanifests-> https://github.com/operator-framework/operator-sdk/blob/master/test/integration/operator_olm_test.go#L106


i := bundle.NewInstall(cfg)
i.BundleImage = tc.ImageName
Comment on lines +74 to +75
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Sep 2, 2020

Choose a reason for hiding this comment

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

You are running the make bundle for he image. what the NewInstall does? Why it is required?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The bundle image is an argument to the run bundle command. When the command is passed, the Install struct is populated. It is referenced here: https://github.com/operator-framework/operator-sdk/blob/master/internal/olm/operator/bundle/install.go#L42

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not we do not use operator-sdk run bundle here? IMO : It is an e2e test so we should use the CLI commands otherwise we could cover these scenarios with unit tests.


err = doInstall(i)
Expect(err).NotTo(HaveOccurred())
Comment on lines +77 to +78
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What the doinstall does?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

doinstall is the wrapper, which in turn uses the OperatorInstaller and installs the csv using olm.
For reference, deinstall implementation is here, which in turn calls the bundle's Run implementation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same above. we need to use the CLI command instead.


uninstall(kubeconfigPath, tc)
})

It("Run bundle SingleNamespace", func() {
if !runBundleImplemented {
Skip("Run bundle not yet implemented")
}

cfg := &operator.Configuration{KubeconfigPath: kubeconfigPath, Namespace: tc.Kubectl.Namespace}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If tc.Kubectl.Namespace == i.InstallMode.TargetNamespaces then you're actually using the OwnNamespace install mode.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is an e2e test and we are creating the project. So, we need to test it as users would be performing this action. Have we a flag to define the run bundle is single namespace or it users need to update the manifest generated?

err := cfg.Load()
Expect(err).NotTo(HaveOccurred())

i := bundle.NewInstall(cfg)
i.BundleImage = tc.ImageName
i.InstallMode.InstallModeType = v1alpha1.InstallModeTypeSingleNamespace
i.InstallMode.TargetNamespaces = []string{tc.Kubectl.Namespace}

err = doInstall(i)
Expect(err).NotTo(HaveOccurred())

uninstall(kubeconfigPath, tc)
})

It("Run bundle OwnNamespace", func() {
if !runBundleImplemented {
Skip("Run bundle not yet implemented")
}

cfg := &operator.Configuration{KubeconfigPath: kubeconfigPath}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This may not be testing OwnNamespace since the default namespace used might not be the one created for this test (tc.Kubectl.Namespace).

err := cfg.Load()
Expect(err).NotTo(HaveOccurred())

i := bundle.NewInstall(cfg)
i.BundleImage = tc.ImageName
i.InstallMode.InstallModeType = v1alpha1.InstallModeTypeOwnNamespace
i.InstallMode.TargetNamespaces = []string{tc.Kubectl.Namespace}

err = doInstall(i)
Expect(err).NotTo(HaveOccurred())

uninstall(kubeconfigPath, tc)
})

It("Run Bundle - watching the specified namesapce", func() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you mean by "watching the specified namespace"? This case is testing SingleNamespace. I suggest you get rid of the above It() and just use this one.

if !runBundleImplemented {
Skip("Run bundle not yet implemented")
}

cfg := &operator.Configuration{KubeconfigPath: kubeconfigPath, Namespace: tc.Kubectl.Namespace}
err := cfg.Load()
Expect(err).NotTo(HaveOccurred())

By("create a namespace to watch")
_, err = tc.Kubectl.Command("create", "namespace", "bar")
Expect(err).NotTo(HaveOccurred())

i := bundle.NewInstall(cfg)
i.BundleImage = tc.ImageName
i.InstallMode.InstallModeType = v1alpha1.InstallModeTypeSingleNamespace
i.InstallMode.TargetNamespaces = []string{tc.Kubectl.Namespace, "bar"}

err = doInstall(i)
Expect(err).NotTo(HaveOccurred())

uninstall(kubeconfigPath, tc)
})

})

// createOperator scaffolds a new test operator, generates bundle and pushes it to a remote
// repository. The generated csv by default supports OwnNamespace, SingleNamespace and AllNamespaces.
func createOperator(tc testutils.TestContext, projectName string) {
By("initializing a project")
err := tc.Init(
"--project-version", "3-alpha",
"--repo", path.Join("github.com", "example", projectName),
"--domain", tc.Domain,
"--fetch-deps=false")
Expect(err).Should(Succeed())

By("creating an API definition")
err = tc.CreateAPI(
"--group", tc.Group,
"--version", tc.Version,
"--kind", tc.Kind,
"--namespaced",
"--resource",
"--controller",
"--make=false")
Expect(err).Should(Succeed())

By("implementing the API")
Expect(kbtestutils.InsertCode(
filepath.Join(tc.Dir, "api", tc.Version, fmt.Sprintf("%s_types.go", strings.ToLower(tc.Kind))),
fmt.Sprintf(`type %sSpec struct {
`, tc.Kind),
` // +optional
Count int `+"`"+`json:"count,omitempty"`+"`"+`
`)).Should(Succeed())
Comment on lines +171 to +178
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It shows not required for the test at all. Why we need to add an value in the types? Could we not use the foo?


By("generating the operator bundle")
// Turn off interactive prompts for all generation tasks.
replace := "operator-sdk generate kustomize manifests"
testutils.ReplaceInFile(filepath.Join(tc.Dir, "Makefile"), replace, replace+" --interactive=false")
err = tc.Make("bundle", "IMG="+tc.ImageName)
Expect(err).NotTo(HaveOccurred())

By("building the operator bundle image")
// Use the existing image tag but with a "-bundle" suffix.
err = tc.Make("bundle-build", "BUNDLE_IMG="+tc.ImageName)
Expect(err).NotTo(HaveOccurred())

By("push the bundle to a remote repository")
err = tc.Make("docker-push", "IMG="+tc.ImageName)
Expect(err).NotTo(HaveOccurred())
Comment on lines +192 to +194
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Sep 4, 2020

Choose a reason for hiding this comment

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

Suggested change
By("push the bundle to a remote repository")
err = tc.Make("docker-push", "IMG="+tc.ImageName)
Expect(err).NotTo(HaveOccurred())

we should not use the docker-push here. The tests are executed using Kind. We build the images and then we load them. See:

https://github.com/operator-framework/operator-sdk/blob/master/test/e2e-ansible/e2e_ansible_olm_test.go#L62-L68

}

func uninstall(kubeconfig string, tc testutils.TestContext) {
cfg := &operator.Configuration{KubeconfigPath: kubeconfig}
err := cfg.Load()
Expect(err).NotTo(HaveOccurred())

uninstallConfig := operator.NewUninstall(cfg)
uninstallConfig.DeleteAll = true
uninstallConfig.DeleteOperatorGroupNames = []string{tc.Group}
uninstallConfig.Package = defaultOperatorName
uninstallConfig.Logf = logrus.Infof
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what should be uninstalled here? Is the bundle?

Copy link
Copy Markdown
Member Author

@varshaprasad96 varshaprasad96 Sep 3, 2020

Choose a reason for hiding this comment

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

Yes, it is the bundle. As you said, after creating a new test context for the entire suite, before each test we uninstall the previous bundle (csv, crd and other created objects).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will not the tool provide a command to uninstall?


ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout)
defer cancel()

err = uninstallConfig.Run(ctx)
Expect(err).NotTo(HaveOccurred())

waitForConfigMapDeletion(ctx, cfg, defaultOperatorName)
}

func waitForConfigMapDeletion(ctx context.Context, cfg *operator.Configuration, packageName string) {
cfgmaps := corev1.ConfigMapList{}
opts := []client.ListOption{
client.InNamespace(cfg.Namespace),
client.MatchingLabels{"owner": "operator-sdk", "package-name": packageName},
}
err := wait.PollImmediateUntil(250*time.Millisecond, func() (bool, error) {
if err := cfg.Client.List(ctx, &cfgmaps, opts...); err != nil {
return false, err
}
return len(cfgmaps.Items) == 0, nil
}, ctx.Done())
Expect(err).NotTo(HaveOccurred())
}

func TestRunBundle(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Operator SDK run bundle test suite")
}
Comment on lines +232 to +235
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in order to follow up a standard each dir should have 1 suite test and the tests that will be called by it. IMO we need re-check if is really required to keep this dir.

Copy link
Copy Markdown
Member Author

@varshaprasad96 varshaprasad96 Sep 3, 2020

Choose a reason for hiding this comment

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

As discussed with @jmrodri and @estroz, this PR was structured to follow the run packagemanifests format. We add the generic run bundle test to operator e2e test suite which is done here: #3831 and the tests to check various scenarios in the olm integration suite. Similar to how package manifests does: test/integration/operator_olm_test.go.
I am not sure if these tests would be run when the user calls run bundle, because we are using a pre-generated bundle specific for this tests.

Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Sep 4, 2020

Choose a reason for hiding this comment

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

OK. See that :

  • we cover the features with unit test
  • we have e2e tests for each type of project where we will test the run bundle with ALL Namespaces.

So, this e2e test would just make sense if we would like to cover all possible scenarios. Is this the goal? If yes, OK. However, note that test/integration/operator_olm_test.go is old and is NOT follow the conventions and standards defined so far.

So, if we will add tests to cover all options of the run bundle command with e2e then:

  • we should use the libs defined and then follow the standards and conventions defined by them and applied in the ansible/helm tests already. (we have pr to cleanup go implementation waiting for reviews feat: cleanup go e2e tests and perform improvements )
  • the integration tests need to be in test/e2e-<name>. I suggest we have a dir test/e2e-olm. (we can leave for this integration dir, however, I'd like to see the new tests following the same standard and convections)
  • The directory requires an e2e_<name>_suite_test which will have the suite and then we have the files tests. Note that it is the standard of the libs adopted.

NOTE: You can create the dir and run the command $ ginkgo bootstrap ** and then, you will check the files in this way.

  • we should not re-create the project for each test. the project can be created in the before of the suite and used by all test and the removed on the after of the suite instead. See Ansible/Helm suite tests.
  • the go e2e test should have the suffix _test which has special meaning for the Go compiler

c/c @jmrodri @estroz

Comment on lines +232 to +235
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All integration tests in this package should be converted to ginkgo/gomega tests eventually like this integration test is, so the suite runner should be in a separate file and look like so:

Suggested change
func TestRunBundle(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Operator SDK run bundle test suite")
}
func TestIntegration(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Integration Suite")
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO we need to create the dir /test/e2e-olm/ in order to follow the project standard. The tests here are old and are not following the new definitions. Then, we should run ginkgo bootstrap. It will generated the files with the names following the lib convention. See: https://onsi.github.io/ginkgo/