Skip to content

Commit

Permalink
use a single client in webhook
Browse files Browse the repository at this point in the history 
By using a single client that is injection 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]>
  • Loading branch information
@ahadas
ahadas committed Oct 1, 2023
1 parent b7b33f1 commit 0103866
Show file tree
Hide file tree
Showing 14 changed files with 79 additions and 148 deletions.
4 changes: 4 additions & 0 deletions cmd/forklift-api/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)
Expand Down
31 changes: 30 additions & 1 deletion cmd/forklift-api/forklift-api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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
}
Expand All @@ -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,
Expand Down
1 change: 1 addition & 0 deletions pkg/forklift-api/webhooks/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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})
}
3 changes: 0 additions & 3 deletions pkg/forklift-api/webhooks/mutating-webhook/mutators/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
],
)
Expand Down
42 changes: 6 additions & 36 deletions pkg/forklift-api/webhooks/mutating-webhook/mutators/plan-mutator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down
13 changes: 7 additions & 6 deletions pkg/forklift-api/webhooks/validating-webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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})
}
3 changes: 0 additions & 3 deletions pkg/forklift-api/webhooks/validating-webhook/admitters/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
],
)
37 changes: 6 additions & 31 deletions pkg/forklift-api/webhooks/validating-webhook/admitters/plan-admitter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
Loading

0 comments on commit 0103866

Please sign in to comment.