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 constructor for conversion webhooks, rm admission.GetDecoder() #2144

Merged
merged 1 commit into from Jan 26, 2023
Merged

⚠️ Add constructor for conversion webhooks, rm admission.GetDecoder() #2144

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
2 changes: 1 addition & 1 deletion pkg/builder/webhook.go
View file
Open in desktop
Expand Up @@ -213,7 +213,7 @@ func (blder *WebhookBuilder) registerConversionWebhook() error {
}
if ok {
if !blder.isAlreadyHandled("/convert") {
blder.mgr.GetWebhookServer().Register("/convert", &conversion.Webhook{})
blder.mgr.GetWebhookServer().Register("/convert", conversion.NewWebhookHandler(blder.mgr.GetScheme()))
}
log.Info("Conversion webhook enabled", "GVK", blder.gvk)
}
Expand Down
9 changes: 0 additions & 9 deletions pkg/webhook/admission/webhook.go
View file
Open in desktop
Expand Up @@ -135,9 +135,6 @@ type Webhook struct {
// outside the context of requests.
LogConstructor func(base logr.Logger, req *Request) logr.Logger

// decoder is constructed on receiving a scheme and passed down to then handler
decoder *Decoder

setupLogOnce sync.Once
log logr.Logger
}
Expand Down Expand Up @@ -204,12 +201,6 @@ func DefaultLogConstructor(base logr.Logger, req *Request) logr.Logger {
return base
}

// GetDecoder returns a decoder to decode the objects embedded in admission requests.
// It may be nil if we haven't received a scheme to use to determine object types yet.
func (wh *Webhook) GetDecoder() *Decoder {
return wh.decoder
}

// StandaloneOptions let you configure a StandaloneWebhook.
type StandaloneOptions struct {
// Logger to be used by the webhook.
Expand Down
22 changes: 13 additions & 9 deletions pkg/webhook/conversion/conversion.go
View file
Open in desktop
Expand Up @@ -39,16 +39,20 @@ var (
log = logf.Log.WithName("conversion-webhook")
)

// Webhook implements a CRD conversion webhook HTTP handler.
type Webhook struct {
func NewWebhookHandler(scheme *runtime.Scheme) http.Handler {
return &webhook{scheme: scheme, decoder: NewDecoder(scheme)}
}

// webhook implements a CRD conversion webhook HTTP handler.
type webhook struct {
scheme *runtime.Scheme
decoder *Decoder
}

// ensure Webhook implements http.Handler
var _ http.Handler = &Webhook{}
var _ http.Handler = &webhook{}

func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
func (wh *webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
convertReview := &apix.ConversionReview{}
err := json.NewDecoder(r.Body).Decode(convertReview)
if err != nil {
Expand Down Expand Up @@ -83,7 +87,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}

// handles a version conversion request.
func (wh *Webhook) handleConvertRequest(req *apix.ConversionRequest) (*apix.ConversionResponse, error) {
func (wh *webhook) handleConvertRequest(req *apix.ConversionRequest) (*apix.ConversionResponse, error) {
if req == nil {
return nil, fmt.Errorf("conversion request is nil")
}
Expand Down Expand Up @@ -116,7 +120,7 @@ func (wh *Webhook) handleConvertRequest(req *apix.ConversionRequest) (*apix.Conv
// convertObject will convert given a src object to dst object.
// Note(droot): couldn't find a way to reduce the cyclomatic complexity under 10
// without compromising readability, so disabling gocyclo linter
func (wh *Webhook) convertObject(src, dst runtime.Object) error {
func (wh *webhook) convertObject(src, dst runtime.Object) error {
srcGVK := src.GetObjectKind().GroupVersionKind()
dstGVK := dst.GetObjectKind().GroupVersionKind()

Expand All @@ -143,7 +147,7 @@ func (wh *Webhook) convertObject(src, dst runtime.Object) error {
}
}

func (wh *Webhook) convertViaHub(src, dst conversion.Convertible) error {
func (wh *webhook) convertViaHub(src, dst conversion.Convertible) error {
hub, err := wh.getHub(src)
if err != nil {
return err
Expand All @@ -167,7 +171,7 @@ func (wh *Webhook) convertViaHub(src, dst conversion.Convertible) error {
}

// getHub returns an instance of the Hub for passed-in object's group/kind.
func (wh *Webhook) getHub(obj runtime.Object) (conversion.Hub, error) {
func (wh *webhook) getHub(obj runtime.Object) (conversion.Hub, error) {
gvks, err := objectGVKs(wh.scheme, obj)
if err != nil {
return nil, err
Expand Down Expand Up @@ -195,7 +199,7 @@ func (wh *Webhook) getHub(obj runtime.Object) (conversion.Hub, error) {
}

// allocateDstObject returns an instance for a given GVK.
func (wh *Webhook) allocateDstObject(apiVersion, kind string) (runtime.Object, error) {
func (wh *webhook) allocateDstObject(apiVersion, kind string) (runtime.Object, error) {
gvk := schema.FromAPIVersionAndKind(apiVersion, kind)

obj, err := wh.scheme.New(gvk)
Expand Down
21 changes: 11 additions & 10 deletions pkg/webhook/conversion/conversion_test.go
View file
Open in desktop
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package conversion
package conversion_test

import (
"bytes"
Expand All @@ -33,6 +33,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
kscheme "k8s.io/client-go/kubernetes/scheme"

"sigs.k8s.io/controller-runtime/pkg/webhook/conversion"
jobsv1 "sigs.k8s.io/controller-runtime/pkg/webhook/conversion/testdata/api/v1"
jobsv2 "sigs.k8s.io/controller-runtime/pkg/webhook/conversion/testdata/api/v2"
jobsv3 "sigs.k8s.io/controller-runtime/pkg/webhook/conversion/testdata/api/v3"
Expand All @@ -41,9 +42,9 @@ import (
var _ = Describe("Conversion Webhook", func() {

var respRecorder *httptest.ResponseRecorder
var decoder *Decoder
var decoder *conversion.Decoder
var scheme *runtime.Scheme
var webhook *Webhook
var wh http.Handler

BeforeEach(func() {
respRecorder = &httptest.ResponseRecorder{
Expand All @@ -56,8 +57,8 @@ var _ = Describe("Conversion Webhook", func() {
Expect(jobsv2.AddToScheme(scheme)).To(Succeed())
Expect(jobsv3.AddToScheme(scheme)).To(Succeed())

decoder = NewDecoder(scheme)
webhook = &Webhook{scheme: scheme, decoder: decoder}
decoder = conversion.NewDecoder(scheme)
wh = conversion.NewWebhookHandler(scheme)
})

doRequest := func(convReq *apix.ConversionReview) *apix.ConversionReview {
Expand All @@ -69,7 +70,7 @@ var _ = Describe("Conversion Webhook", func() {
req := &http.Request{
Body: io.NopCloser(bytes.NewReader(payload.Bytes())),
}
webhook.ServeHTTP(respRecorder, req)
wh.ServeHTTP(respRecorder, req)
Expect(json.NewDecoder(respRecorder.Result().Body).Decode(convReview)).To(Succeed())
return convReview
}
Expand Down Expand Up @@ -313,7 +314,7 @@ var _ = Describe("IsConvertible", func() {
It("should not error for uninitialized types", func() {
obj := &jobsv2.ExternalJob{}

ok, err := IsConvertible(scheme, obj)
ok, err := conversion.IsConvertible(scheme, obj)
Expect(err).NotTo(HaveOccurred())
Expect(ok).To(BeTrue())
})
Expand All @@ -326,7 +327,7 @@ var _ = Describe("IsConvertible", func() {
},
}

ok, err := IsConvertible(scheme, obj)
ok, err := conversion.IsConvertible(scheme, obj)
Expect(err).NotTo(HaveOccurred())
Expect(ok).To(BeTrue())
})
Expand All @@ -339,7 +340,7 @@ var _ = Describe("IsConvertible", func() {
},
}

ok, err := IsConvertible(scheme, obj)
ok, err := conversion.IsConvertible(scheme, obj)
Expect(err).NotTo(HaveOccurred())
Expect(ok).To(BeTrue())
})
Expand All @@ -352,7 +353,7 @@ var _ = Describe("IsConvertible", func() {
},
}

ok, err := IsConvertible(scheme, obj)
ok, err := conversion.IsConvertible(scheme, obj)
Expect(err).NotTo(HaveOccurred())
Expect(ok).ToNot(BeTrue())
})
Expand Down