Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
use a single client in webhook
Browse files Browse the repository at this point in the history
By using a single client that is injected to the webooks we not only
decrease the execution time of webhooks that interact with the cluster
(and thus need the client) but also resolve an issue that happens in
some environments where it takes relatively long time to instantiate the
client (few seconds) and then the execution of the webhook reaches a
13-seconds timeout. The deployment of Forklift gets stuck when this
repeatedly happens when trying to add the "default host provider".

Signed-off-by: Arik Hadas <[email protected]>
ahadas committed Oct 2, 2023
1 parent 0236256 commit 8d54bf7
Showing 14 changed files with 79 additions and 148 deletions.
4 changes: 4 additions & 0 deletions cmd/forklift-api/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -10,7 +10,11 @@ go_library(
"//pkg/forklift-api",
"//pkg/lib/logging",
"//vendor/github.com/go-logr/logr",
"//vendor/github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1:k8s_cni_cncf_io",
"//vendor/k8s.io/client-go/kubernetes/scheme",
"//vendor/k8s.io/client-go/rest",
"//vendor/k8s.io/client-go/tools/clientcmd/api",
"//vendor/sigs.k8s.io/controller-runtime/pkg/client",
"//vendor/sigs.k8s.io/controller-runtime/pkg/log",
],
)
31 changes: 30 additions & 1 deletion cmd/forklift-api/forklift-api.go
Original file line number Diff line number Diff line change
@@ -20,10 +20,15 @@ import (
"os"

"github.com/go-logr/logr"
net "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
"github.com/konveyor/forklift-controller/pkg/apis"
forklift_api "github.com/konveyor/forklift-controller/pkg/forklift-api"
"github.com/konveyor/forklift-controller/pkg/lib/logging"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"

"k8s.io/client-go/tools/clientcmd/api"
"sigs.k8s.io/controller-runtime/pkg/client"
logf "sigs.k8s.io/controller-runtime/pkg/log"
)

@@ -37,13 +42,37 @@ func init() {

func main() {
log.Info("start forklift-api")
app := forklift_api.NewForkliftApi()

err := apis.AddToScheme(scheme.Scheme)
if err != nil {
log.Error(err, "unable to add forklift API to scheme")
os.Exit(1)
}

err = api.SchemeBuilder.AddToScheme(scheme.Scheme)
if err != nil {
log.Error(err, "Couldn't build the scheme")
os.Exit(1)
}

err = net.AddToScheme(scheme.Scheme)
if err != nil {
log.Error(err, "Couldn't add network-attachment-definition-client to the scheme")
os.Exit(1)
}

config, err := rest.InClusterConfig()
if err != nil {
log.Error(err, "Couldn't get the cluster configuration")
os.Exit(1)
}

client, err := client.New(config, client.Options{Scheme: scheme.Scheme})
if err != nil {
log.Error(err, "Couldn't create a cluster client")
os.Exit(1)
}

app := forklift_api.NewForkliftApi(client)
app.Execute()
}
1 change: 1 addition & 0 deletions pkg/forklift-api/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -8,5 +8,6 @@ go_library(
deps = [
"//pkg/forklift-api/webhooks",
"//pkg/lib/logging",
"//vendor/sigs.k8s.io/controller-runtime/pkg/client",
],
)
9 changes: 6 additions & 3 deletions pkg/forklift-api/api.go
Original file line number Diff line number Diff line change
@@ -22,6 +22,7 @@ import (

webhooks "github.com/konveyor/forklift-controller/pkg/forklift-api/webhooks"
"github.com/konveyor/forklift-controller/pkg/lib/logging"
"sigs.k8s.io/controller-runtime/pkg/client"
)

const (
@@ -42,13 +43,15 @@ type forkliftAPIApp struct {
Name string
BindAddress string
Port int
client client.Client
}

func NewForkliftApi() ForkliftApi {
func NewForkliftApi(client client.Client) ForkliftApi {

app := &forkliftAPIApp{}
app.BindAddress = defaultHost
app.Port = defaultPort
app.client = client

return app
}
@@ -66,8 +69,8 @@ func (app *forkliftAPIApp) Execute() {
}

mux := http.NewServeMux()
webhooks.RegisterMutatingWebhooks(mux)
webhooks.RegisterValidatingWebhooks(mux)
webhooks.RegisterMutatingWebhooks(mux, app.client)
webhooks.RegisterValidatingWebhooks(mux, app.client)
server := http.Server{
Addr: ":8443",
Handler: mux,
1 change: 1 addition & 0 deletions pkg/forklift-api/webhooks/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@ go_library(
"//pkg/forklift-api/webhooks/validating-webhook",
"//pkg/forklift-api/webhooks/validating-webhook/admitters",
"//pkg/lib/logging",
"//vendor/sigs.k8s.io/controller-runtime/pkg/client",
"//vendor/sigs.k8s.io/controller-runtime/pkg/manager",
],
)
5 changes: 3 additions & 2 deletions pkg/forklift-api/webhooks/mutating-webhook.go
Original file line number Diff line number Diff line change
@@ -5,12 +5,13 @@ import (

mutating_webhooks "github.com/konveyor/forklift-controller/pkg/forklift-api/webhooks/mutating-webhook"
"github.com/konveyor/forklift-controller/pkg/forklift-api/webhooks/mutating-webhook/mutators"
"sigs.k8s.io/controller-runtime/pkg/client"
)

func ServeSecretMutator(resp http.ResponseWriter, req *http.Request) {
mutating_webhooks.Serve(resp, req, &mutators.SecretMutator{})
}

func ServePlanMutator(resp http.ResponseWriter, req *http.Request) {
mutating_webhooks.Serve(resp, req, &mutators.PlanMutator{})
func ServePlanMutator(resp http.ResponseWriter, req *http.Request, client client.Client) {
mutating_webhooks.Serve(resp, req, &mutators.PlanMutator{Client: client})
}
Original file line number Diff line number Diff line change
@@ -9,7 +9,6 @@ go_library(
importpath = "github.com/konveyor/forklift-controller/pkg/forklift-api/webhooks/mutating-webhook/mutators",
visibility = ["//visibility:public"],
deps = [
"//pkg/apis",
"//pkg/apis/forklift/v1beta1",
"//pkg/forklift-api/webhooks/util",
"//pkg/lib/error",
@@ -19,8 +18,6 @@ go_library(
"//vendor/k8s.io/api/core/v1:core",
"//vendor/k8s.io/apimachinery/pkg/api/errors",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:meta",
"//vendor/k8s.io/client-go/kubernetes/scheme",
"//vendor/k8s.io/client-go/rest",
"//vendor/sigs.k8s.io/controller-runtime/pkg/client",
],
)
Original file line number Diff line number Diff line change
@@ -6,15 +6,12 @@ import (
"net/http"

net "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
"github.com/konveyor/forklift-controller/pkg/apis"
api "github.com/konveyor/forklift-controller/pkg/apis/forklift/v1beta1"
"github.com/konveyor/forklift-controller/pkg/forklift-api/webhooks/util"
admissionv1 "k8s.io/api/admission/v1beta1"
core "k8s.io/api/core/v1"
k8serr "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/client"
)

@@ -23,8 +20,9 @@ const (
)

type PlanMutator struct {
ar *admissionv1.AdmissionReview
plan api.Plan
ar *admissionv1.AdmissionReview
plan api.Plan
Client client.Client
}

func (mutator *PlanMutator) Mutate(ar *admissionv1.AdmissionReview) *admissionv1.AdmissionResponse {
@@ -71,36 +69,8 @@ func (mutator *PlanMutator) setTransferNetworkIfNotSet() (bool, error) {
var planChanged bool

if mutator.plan.Spec.TransferNetwork == nil {
config, err := rest.InClusterConfig()
if err != nil {
log.Error(err, "Couldn't get the cluster configuration")
return false, err
}

err = api.SchemeBuilder.AddToScheme(scheme.Scheme)
if err != nil {
log.Error(err, "Couldn't build the scheme")
return false, err
}
err = apis.AddToScheme(scheme.Scheme)
if err != nil {
log.Error(err, "Couldn't add forklift API to the scheme")
return false, err
}
err = net.AddToScheme(scheme.Scheme)
if err != nil {
log.Error(err, "Couldn't add network-attachment-definition-client to the scheme")
return false, err
}

cl, err := client.New(config, client.Options{Scheme: scheme.Scheme})
if err != nil {
log.Error(err, "Couldn't create a cluster client")
return false, err
}

targetProvider := api.Provider{}
err = cl.Get(context.TODO(), client.ObjectKey{Namespace: mutator.plan.Spec.Provider.Destination.Namespace, Name: mutator.plan.Spec.Provider.Destination.Name}, &targetProvider)
err := mutator.Client.Get(context.TODO(), client.ObjectKey{Namespace: mutator.plan.Spec.Provider.Destination.Namespace, Name: mutator.plan.Spec.Provider.Destination.Name}, &targetProvider)
if err != nil {
log.Error(err, "Couldn't get the target provider")
return false, err
@@ -114,11 +84,11 @@ func (mutator *PlanMutator) setTransferNetworkIfNotSet() (bool, error) {

var tcl client.Client // target client, i.e., client to a possibly remote cluster
if targetProvider.IsHost() {
tcl = cl
tcl = mutator.Client
} else {
ref := targetProvider.Spec.Secret
secret := &core.Secret{}
err = cl.Get(
err = mutator.Client.Get(
context.TODO(),
client.ObjectKey{
Namespace: ref.Namespace,
13 changes: 7 additions & 6 deletions pkg/forklift-api/webhooks/validating-webhook.go
Original file line number Diff line number Diff line change
@@ -5,16 +5,17 @@ import (

validating_webhooks "github.com/konveyor/forklift-controller/pkg/forklift-api/webhooks/validating-webhook"
"github.com/konveyor/forklift-controller/pkg/forklift-api/webhooks/validating-webhook/admitters"
"sigs.k8s.io/controller-runtime/pkg/client"
)

func ServeSecretCreate(resp http.ResponseWriter, req *http.Request) {
validating_webhooks.Serve(resp, req, &admitters.SecretAdmitter{})
func ServeSecretCreate(resp http.ResponseWriter, req *http.Request, client client.Client) {
validating_webhooks.Serve(resp, req, &admitters.SecretAdmitter{Client: client})
}

func ServePlanCreate(resp http.ResponseWriter, req *http.Request) {
validating_webhooks.Serve(resp, req, &admitters.PlanAdmitter{})
func ServePlanCreate(resp http.ResponseWriter, req *http.Request, client client.Client) {
validating_webhooks.Serve(resp, req, &admitters.PlanAdmitter{Client: client})
}

func ServeProviderCreate(resp http.ResponseWriter, req *http.Request) {
validating_webhooks.Serve(resp, req, &admitters.ProviderAdmitter{})
func ServeProviderCreate(resp http.ResponseWriter, req *http.Request, client client.Client) {
validating_webhooks.Serve(resp, req, &admitters.ProviderAdmitter{Client: client})
}
Original file line number Diff line number Diff line change
@@ -10,7 +10,6 @@ go_library(
importpath = "github.com/konveyor/forklift-controller/pkg/forklift-api/webhooks/validating-webhook/admitters",
visibility = ["//visibility:public"],
deps = [
"//pkg/apis",
"//pkg/apis/forklift/v1beta1",
"//pkg/controller/plan/adapter/vsphere",
"//pkg/controller/provider/container",
@@ -26,8 +25,6 @@ go_library(
"//vendor/k8s.io/api/storage/v1:storage",
"//vendor/k8s.io/apimachinery/pkg/api/errors",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:meta",
"//vendor/k8s.io/client-go/kubernetes/scheme",
"//vendor/k8s.io/client-go/rest",
"//vendor/sigs.k8s.io/controller-runtime/pkg/client",
],
)
Original file line number Diff line number Diff line change
@@ -2,24 +2,22 @@ package admitters

import (
"context"

v1 "k8s.io/api/storage/v1"
"sigs.k8s.io/controller-runtime/pkg/client"

"encoding/json"
"fmt"

admissionv1 "k8s.io/api/admission/v1beta1"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"

"github.com/konveyor/forklift-controller/pkg/apis"
api "github.com/konveyor/forklift-controller/pkg/apis/forklift/v1beta1"
"github.com/konveyor/forklift-controller/pkg/forklift-api/webhooks/util"
liberr "github.com/konveyor/forklift-controller/pkg/lib/error"
)

type PlanAdmitter struct {
client client.Client
Client client.Client
plan api.Plan
sourceProvider api.Provider
destinationProvider api.Provider
@@ -43,14 +41,14 @@ func (admitter *PlanAdmitter) validateStorage() error {
}

storageClasses := v1.StorageClassList{}
err := admitter.client.List(context.TODO(), &storageClasses, &client.ListOptions{})
err := admitter.Client.List(context.TODO(), &storageClasses, &client.ListOptions{})
if err != nil {
log.Error(err, "Couldn't get the cluster storage classes")
return err
}

storageMap := api.StorageMap{}
err = admitter.client.Get(
err = admitter.Client.Get(
context.TODO(),
client.ObjectKey{
Namespace: admitter.plan.Spec.Map.Storage.Namespace,
@@ -127,30 +125,7 @@ func (admitter *PlanAdmitter) Admit(ar *admissionv1.AdmissionReview) *admissionv
return util.ToAdmissionResponseError(err)
}

config, err := rest.InClusterConfig()
if err != nil {
log.Error(err, "Couldn't get the cluster configuration")
return util.ToAdmissionResponseError(err)
}

err = api.SchemeBuilder.AddToScheme(scheme.Scheme)
if err != nil {
log.Error(err, "Couldn't build the scheme")
return util.ToAdmissionResponseError(err)
}
err = apis.AddToScheme(scheme.Scheme)
if err != nil {
log.Error(err, "Couldn't add forklift API to the scheme")
return util.ToAdmissionResponseError(err)
}

admitter.client, err = client.New(config, client.Options{Scheme: scheme.Scheme})
if err != nil {
log.Error(err, "Couldn't create a cluster client")
return util.ToAdmissionResponseError(err)
}

err = admitter.client.Get(
err = admitter.Client.Get(
context.TODO(),
client.ObjectKey{
Namespace: admitter.plan.Spec.Provider.Source.Namespace,
@@ -162,7 +137,7 @@ func (admitter *PlanAdmitter) Admit(ar *admissionv1.AdmissionReview) *admissionv
return util.ToAdmissionResponseAllow()
}

err = admitter.client.Get(
err = admitter.Client.Get(
context.TODO(),
client.ObjectKey{
Namespace: admitter.plan.Spec.Provider.Destination.Namespace,
Original file line number Diff line number Diff line change
@@ -4,22 +4,21 @@ import (
"context"
"encoding/json"

"github.com/konveyor/forklift-controller/pkg/apis"
api "github.com/konveyor/forklift-controller/pkg/apis/forklift/v1beta1"
"github.com/konveyor/forklift-controller/pkg/forklift-api/webhooks/util"
liberr "github.com/konveyor/forklift-controller/pkg/lib/error"
admissionv1 "k8s.io/api/admission/v1beta1"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/client"
)

type ProviderAdmitter struct {
client client.Client
Client client.Client
provider api.Provider
}

func (admitter *ProviderAdmitter) validateVDDK() error {
log.Info("not sleeping for 10 seconds")
//time.Sleep(10 * time.Second)
if admitter.provider.Type() != api.VSphere {
log.Info("Provider of this type does not require VDDK, passing", "type", admitter.provider.Type())
return nil
@@ -31,7 +30,7 @@ func (admitter *ProviderAdmitter) validateVDDK() error {
}

plans := api.PlanList{}
err := admitter.client.List(context.TODO(), &plans, &client.ListOptions{})
err := admitter.Client.List(context.TODO(), &plans, &client.ListOptions{})
if err != nil {
log.Error(err, "Couldn't get all plans", "namespace", admitter.provider.Namespace)
return err
@@ -53,7 +52,7 @@ func (admitter *ProviderAdmitter) validateVDDK() error {
}

var destinationProvider api.Provider
err = admitter.client.Get(
err = admitter.Client.Get(
context.TODO(),
client.ObjectKey{
Namespace: plan.Spec.Provider.Destination.Namespace,
@@ -96,29 +95,6 @@ func (admitter *ProviderAdmitter) Admit(ar *admissionv1.AdmissionReview) *admiss
return util.ToAdmissionResponseError(err)
}

config, err := rest.InClusterConfig()
if err != nil {
log.Error(err, "Couldn't get the cluster configuration")
return util.ToAdmissionResponseError(err)
}

err = api.SchemeBuilder.AddToScheme(scheme.Scheme)
if err != nil {
log.Error(err, "Couldn't build the scheme")
return util.ToAdmissionResponseError(err)
}
err = apis.AddToScheme(scheme.Scheme)
if err != nil {
log.Error(err, "Couldn't add forklift API to the scheme")
return util.ToAdmissionResponseError(err)
}

admitter.client, err = client.New(config, client.Options{Scheme: scheme.Scheme})
if err != nil {
log.Error(err, "Couldn't create a cluster client")
return util.ToAdmissionResponseError(err)
}

err = admitter.validateVDDK()
if err != nil {
return util.ToAdmissionResponseError(err)
Original file line number Diff line number Diff line change
@@ -21,8 +21,6 @@ import (
core "k8s.io/api/core/v1"
k8serr "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/client"
)

@@ -41,6 +39,7 @@ func init() {
type SecretAdmitter struct {
ar *admissionv1.AdmissionReview
secret core.Secret
Client client.Client
}

var resourceTypeToValidateFunc = map[string]func(*SecretAdmitter) *admissionv1.AdmissionResponse{
@@ -156,25 +155,13 @@ func (admitter *SecretAdmitter) buildProviderCollector(providerType *api.Provide
}

func (admitter *SecretAdmitter) testConnectionToHost(hostName string) (tested bool, err error) {
config, err := rest.InClusterConfig()
if err != nil {
log.Error(err, "Couldn't get the cluster configuration")
return
}

cl, err := client.New(config, client.Options{Scheme: scheme.Scheme})
if err != nil {
log.Error(err, "Couldn't create a cluster client")
return
}

provider := &api.Provider{}
providerName := string(admitter.secret.Data["provider"])
// there is an assumption that the provider resides within the same namespace as the secret
// which is reasonable as the hosts are also created on the same namespace as the provider
// but anyway, if that's not the case, we would likely pass the validation (due to IsNotFound check)
providerNamespace := admitter.secret.Namespace
err = cl.Get(context.TODO(), client.ObjectKey{Namespace: providerNamespace, Name: providerName}, provider)
err = admitter.Client.Get(context.TODO(), client.ObjectKey{Namespace: providerNamespace, Name: providerName}, provider)
if err != nil {
if k8serr.IsNotFound(err) {
log.Info("Failed to find provider of host, passing")
@@ -231,20 +218,8 @@ func (admitter *SecretAdmitter) validateUpdateOfOVAProviderSecret() *admissionv1
}

func (admitter *SecretAdmitter) isOvaUrlChanged() (bool, error) {
config, err := rest.InClusterConfig()
if err != nil {
log.Error(err, "Couldn't get the cluster configuration")
return false, err
}

cl, err := client.New(config, client.Options{Scheme: scheme.Scheme})
if err != nil {
log.Error(err, "Couldn't create a cluster client")
return false, err
}

oldSecret := core.Secret{}
err = cl.Get(context.TODO(), client.ObjectKey{Namespace: admitter.secret.Namespace, Name: admitter.secret.Name}, &oldSecret)
err := admitter.Client.Get(context.TODO(), client.ObjectKey{Namespace: admitter.secret.Namespace, Name: admitter.secret.Name}, &oldSecret)
if err != nil {
log.Error(err, "Couldn't get the target provider secret")
return false, err
13 changes: 7 additions & 6 deletions pkg/forklift-api/webhooks/webhooks.go
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@ import (
"net/http"

"github.com/konveyor/forklift-controller/pkg/lib/logging"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/manager"
)

@@ -28,25 +29,25 @@ func AddToManager(m manager.Manager) error {
return nil
}

func RegisterValidatingWebhooks(mux *http.ServeMux) {
func RegisterValidatingWebhooks(mux *http.ServeMux, client client.Client) {
log.Info("register validation webhooks")
mux.HandleFunc(SecretValidatePath, func(w http.ResponseWriter, r *http.Request) {
ServeSecretCreate(w, r)
ServeSecretCreate(w, r, client)
})
mux.HandleFunc(PlanValidatePath, func(w http.ResponseWriter, r *http.Request) {
ServePlanCreate(w, r)
ServePlanCreate(w, r, client)
})
mux.HandleFunc(ProviderValidatePath, func(w http.ResponseWriter, r *http.Request) {
ServeProviderCreate(w, r)
ServeProviderCreate(w, r, client)
})
}

func RegisterMutatingWebhooks(mux *http.ServeMux) {
func RegisterMutatingWebhooks(mux *http.ServeMux, client client.Client) {
log.Info("register mutation webhook")
mux.HandleFunc(SecretMutatorPath, func(w http.ResponseWriter, r *http.Request) {
ServeSecretMutator(w, r)
})
mux.HandleFunc(PlanMutatorPath, func(w http.ResponseWriter, r *http.Request) {
ServePlanMutator(w, r)
ServePlanMutator(w, r, client)
})
}

0 comments on commit 8d54bf7

Please sign in to comment.