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

✨ Add scaffolding for the webhook test suite (go/v3-alpha) #1710

Merged
merged 1 commit into from
Oct 28, 2020
Merged

✨ Add scaffolding for the webhook test suite (go/v3-alpha) #1710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
225 changes: 225 additions & 0 deletions pkg/plugin/v3/scaffolds/internal/templates/config/api/webhook_suitetest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,225 @@
package api

import (
"fmt"
"path/filepath"

"sigs.k8s.io/kubebuilder/pkg/model/file"
)

var _ file.Template = &WebhookSuite{}
var _ file.Inserter = &WebhookSuite{}

// WebhookSuite scaffolds the webhook_suite.go file to setup the webhook test
type WebhookSuite struct {
file.TemplateMixin
file.MultiGroupMixin
file.BoilerplateMixin
file.ResourceMixin

// BaseDirectoryRelativePath define the Path for the base directory when it is multigroup
BaseDirectoryRelativePath string
}

// SetTemplateDefaults implements file.Template
func (f *WebhookSuite) SetTemplateDefaults() error {
if f.Path == "" {
if f.MultiGroup {
if f.Resource.Group != "" {
f.Path = filepath.Join("apis", "%[group]", "%[version]", "webhook_suite_test.go")
} else {
f.Path = filepath.Join("apis", "%[version]", "webhook_suite_test.go")
}
} else {
f.Path = filepath.Join("api", "%[version]", "webhook_suite_test.go")
}
}
f.Path = f.Resource.Replacer().Replace(f.Path)

f.TemplateBody = fmt.Sprintf(webhookTestSuiteTemplate,
file.NewMarkerFor(f.Path, importMarker),
file.NewMarkerFor(f.Path, addSchemeMarker),
file.NewMarkerFor(f.Path, addWebhookManagerMarker),
prafull01 marked this conversation as resolved.
Show resolved Hide resolved
"%s",
"%d",
)

// If is multigroup the path needs to be ../../.. since it has the group dir.
f.BaseDirectoryRelativePath = `"..", ".."`
if f.MultiGroup && f.Resource.Group != "" {
f.BaseDirectoryRelativePath = `"..", "..",".."`
}

return nil
}

const (
// TODO: admission webhook versions should be based on the input of the user. For More Info #1664
admissionImportAlias = "admissionv1beta1"
admissionPath = "k8s.io/api/admission/v1beta1"
importMarker = "imports"
addWebhookManagerMarker = "webhook"
addSchemeMarker = "scheme"
)

// GetMarkers implements file.Inserter
func (f *WebhookSuite) GetMarkers() []file.Marker {
return []file.Marker{
file.NewMarkerFor(f.Path, importMarker),
file.NewMarkerFor(f.Path, addSchemeMarker),
file.NewMarkerFor(f.Path, addWebhookManagerMarker),
}
}

const (
apiImportCodeFragment = `%s "%s"
`
addschemeCodeFragment = `err = %s.AddToScheme(scheme )
Expect(err).NotTo(HaveOccurred())

`
addWebhookManagerCodeFragment = `err = (&%s{}).SetupWebhookWithManager(mgr)
Expect(err).NotTo(HaveOccurred())

`
)

// GetCodeFragments implements file.Inserter
func (f *WebhookSuite) GetCodeFragments() file.CodeFragmentsMap {
fragments := make(file.CodeFragmentsMap, 3)

// Generate import code fragments
imports := make([]string, 0)
imports = append(imports, fmt.Sprintf(apiImportCodeFragment, admissionImportAlias, admissionPath))

// Generate add scheme code fragments
addScheme := make([]string, 0)
addScheme = append(addScheme, fmt.Sprintf(addschemeCodeFragment, admissionImportAlias))

// Generate add webhookManager code fragments
addWebhookManager := make([]string, 0)
addWebhookManager = append(addWebhookManager, fmt.Sprintf(addWebhookManagerCodeFragment, f.Resource.Kind))

// Only store code fragments in the map if the slices are non-empty
if len(addWebhookManager) != 0 {
fragments[file.NewMarkerFor(f.Path, addWebhookManagerMarker)] = addWebhookManager
}
if len(imports) != 0 {
fragments[file.NewMarkerFor(f.Path, importMarker)] = imports
}
if len(addScheme) != 0 {
fragments[file.NewMarkerFor(f.Path, addSchemeMarker)] = addScheme
}

return fragments
}

const (
webhookTestSuiteTemplate = `
package {{ .Resource.Version }}

import (
"path/filepath"
"testing"
"fmt"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
%s
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"k8s.io/apimachinery/pkg/runtime"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/envtest"
"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
)

// These tests use Ginkgo (BDD-style Go testing framework). Refer to
// http://onsi.github.io/ginkgo/ to learn more about Ginkgo.

var cfg *rest.Config
var k8sClient client.Client
var testEnv *envtest.Environment
var stopCh = make(chan struct{})

func TestAPIs(t *testing.T) {
RegisterFailHandler(Fail)

RunSpecsWithDefaultAndCustomReporters(t,
"Webhook Suite",
[]Reporter{printer.NewlineReporter{}})
}

var _ = BeforeSuite(func(done Done) {
Copy link

Choose a reason for hiding this comment

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

I might use the form without done given that done isn't being used.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with that. However, the same is applied to https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugin/v3/scaffolds/internal/templates/config/controller/controller_suitetest.go#L146. So, I think it might better be addressed in a follow up for both scenarios.

Copy link

Choose a reason for hiding this comment

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

For reference, many of my comments here came from my thoughts when I was "cleaning up" the kb generated code that's setup elsewhere. Of course, everyone is entitled to their own code style and opinions. I thought I'd raise the bits I found to be awkward. I agree with consistent style. If you agree that it's awkward, a followup to address all cases sounds good.

logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))

By("bootstrapping test environment")
testEnv = &envtest.Environment{
CRDDirectoryPaths: []string{filepath.Join({{ .BaseDirectoryRelativePath }}, "config", "crd", "bases")},
WebhookInstallOptions: envtest.WebhookInstallOptions{
DirectoryPaths: []string{filepath.Join({{ .BaseDirectoryRelativePath }}, "config", "webhook")},
},
}

var err error
Copy link

Choose a reason for hiding this comment

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

Rather that mutating the same error every time, it's probably more idiomatic to use the form

foo, err := Func()

Copy link
Member

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Yep :)

Copy link
Contributor

Choose a reason for hiding this comment

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

You are not mutating an error, you are re-assigning it. The current approach only allocates memory once while the other needs to allocate memory for every function so I don't think it is that good of an idea.

Copy link
Member

Choose a reason for hiding this comment

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

HI @Adirio,

The community suggestion was :
Replace

var err error
cfg, err = testEnv.Start()

With

cfg, err := testEnv.Start()

In order to keep the simplicity in the code which shows totally fine for me.
However, the most important in my POV is to keep both suite tests for controller and webhook as closer as possible, so, in this way a new issue was tracked to address this nit in both : #1733

So, if you do not agree with the above change could you please add your inputs to #1733?

Copy link

Choose a reason for hiding this comment

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

+1 for not mutating the same error every time.

Copy link
Contributor

Choose a reason for hiding this comment

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

The proposed changed is more idiomatic but doesn't change the behavior you mentioned. You are still using the same err variable and assigning different values to it. You are not actually mutating an error in any of the two cases. You just declare the variable the first time you use it instead of explicitly declaring the variable. So +1 for the change, but the explanation lead me to thinking the change you suggested was different.

cfg, err = testEnv.Start()
Expect(err).NotTo(HaveOccurred())
Expect(cfg).NotTo(BeNil())

scheme := runtime.NewScheme()
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
err = AddToScheme(scheme)
Expect(err).NotTo(HaveOccurred())
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved

%s

k8sClient, err = client.New(cfg, client.Options{Scheme: scheme})
Expect(err).NotTo(HaveOccurred())
Expect(k8sClient).NotTo(BeNil())

// start webhook server using Manager
webhookInstallOptions := &testEnv.WebhookInstallOptions
mgr, err := ctrl.NewManager(cfg, ctrl.Options{
Scheme: scheme,
Host: webhookInstallOptions.LocalServingHost,
Port: webhookInstallOptions.LocalServingPort,
CertDir: webhookInstallOptions.LocalServingCertDir,
LeaderElection: false,
MetricsBindAddress: "0",
})
Expect(err).NotTo(HaveOccurred())

%s

go func() {
err = mgr.Start(stopCh)
if err != nil {
Expect(err).NotTo(HaveOccurred())
}
}()

// wait for the webhook server to get ready
dialer := &net.Dialer{Timeout: time.Second}
addrPort := fmt.Sprintf("%s:%s", webhookInstallOptions.LocalServingHost, webhookInstallOptions.LocalServingPort)
Eventually(func() error {
Copy link

Choose a reason for hiding this comment

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

Suggestion:
Consider relying on https://godoc.org/k8s.io/apimachinery/pkg/util/wait#ExponentialBackoff. In my controller, i extracted all this setup logic into a more fully featured "envtest+manager" abstraction. It would be nice to have this logic less reliant on gomega and instead rely on apimachinery. If KB were to ever make an abstraction like this (to avoid this code in every suite_test), you wouldn't want that code to rely on gomega.

Copy link
Member

Choose a reason for hiding this comment

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

Really thank you for your input. I think that the best way for it be addressed would be might via a follow-up PR. So, WDYT about after it gets merged you push a PR with your suggestion? Then, it might clarify better as well as your thoughts.

conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true})
if err != nil {
return err
}
conn.Close()
return nil
}).Should(Succeed())

close(done)
}, 60)

var _ = AfterSuite(func() {
close(stopCh)
By("tearing down the test environment")
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
err := testEnv.Stop()
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
Expect(err).NotTo(HaveOccurred())
})
`
)
10 changes: 10 additions & 0 deletions pkg/plugin/v3/scaffolds/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,5 +88,15 @@ You need to implement the conversion.Hub and conversion.Convertible interfaces f
return err
}

// TODO: Add test suite for conversion webhook after #1664 has been merged & conversion tests supported in envtest.
prafull01 marked this conversation as resolved.
Show resolved Hide resolved
if s.defaulting || s.validation {
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
if err := machinery.NewScaffold().Execute(
s.newUniverse(),
&api.WebhookSuite{},
); err != nil {
return err
}
}

return nil
}
115 changes: 115 additions & 0 deletions testdata/project-v3-multigroup/apis/crew/v1/webhook_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
package v1

import (
"crypto/tls"
"fmt"
"net"
"path/filepath"
"testing"
"time"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

admissionv1beta1 "k8s.io/api/admission/v1beta1"
// +kubebuilder:scaffold:imports
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/rest"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/envtest"
"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
)

// These tests use Ginkgo (BDD-style Go testing framework). Refer to
// http://onsi.github.io/ginkgo/ to learn more about Ginkgo.

var cfg *rest.Config
var k8sClient client.Client
var testEnv *envtest.Environment
var stopCh = make(chan struct{})

func TestAPIs(t *testing.T) {
RegisterFailHandler(Fail)

RunSpecsWithDefaultAndCustomReporters(t,
"Webhook Suite",
[]Reporter{printer.NewlineReporter{}})
}

var _ = BeforeSuite(func(done Done) {
logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))

By("bootstrapping test environment")
testEnv = &envtest.Environment{
CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "config", "crd", "bases")},
WebhookInstallOptions: envtest.WebhookInstallOptions{
DirectoryPaths: []string{filepath.Join("..", "..", "..", "config", "webhook")},
},
}

var err error
cfg, err = testEnv.Start()
Expect(err).NotTo(HaveOccurred())
Expect(cfg).NotTo(BeNil())

scheme := runtime.NewScheme()
err = AddToScheme(scheme)
Expect(err).NotTo(HaveOccurred())

err = admissionv1beta1.AddToScheme(scheme)
Expect(err).NotTo(HaveOccurred())

// +kubebuilder:scaffold:scheme

k8sClient, err = client.New(cfg, client.Options{Scheme: scheme})
Expect(err).NotTo(HaveOccurred())
Expect(k8sClient).NotTo(BeNil())

// start webhook server using Manager
webhookInstallOptions := &testEnv.WebhookInstallOptions
mgr, err := ctrl.NewManager(cfg, ctrl.Options{
Scheme: scheme,
Host: webhookInstallOptions.LocalServingHost,
Port: webhookInstallOptions.LocalServingPort,
CertDir: webhookInstallOptions.LocalServingCertDir,
LeaderElection: false,
MetricsBindAddress: "0",
})
Expect(err).NotTo(HaveOccurred())

err = (&Captain{}).SetupWebhookWithManager(mgr)
Expect(err).NotTo(HaveOccurred())

// +kubebuilder:scaffold:webhook

go func() {
err = mgr.Start(stopCh)
if err != nil {
Expect(err).NotTo(HaveOccurred())
}
}()

// wait for the webhook server to get ready
dialer := &net.Dialer{Timeout: time.Second}
addrPort := fmt.Sprintf("%s:%d", webhookInstallOptions.LocalServingHost, webhookInstallOptions.LocalServingPort)
Eventually(func() error {
conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true})
if err != nil {
return err
}
conn.Close()
return nil
}).Should(Succeed())

close(done)
}, 60)

var _ = AfterSuite(func() {
close(stopCh)
By("tearing down the test environment")
err := testEnv.Stop()
Expect(err).NotTo(HaveOccurred())
})