From d39bc203ec6b683e70585760ba2429434132ba82 Mon Sep 17 00:00:00 2001 From: Bryce Soghigian <49734722+Bryce-Soghigian@users.noreply.github.com> Date: Thu, 22 Feb 2024 20:29:50 -0800 Subject: [PATCH] chore: removing webhooks from existence for the AKS provider (#156) * chore: removing all references to aks provider webhooks * chore: removing webhook role * fix: removing last reference to utils * fix: removing image id * fix: webhooks --- charts/karpenter/templates/webhooks.yaml | 67 ------------ cmd/controller/main.go | 2 - pkg/apis/v1alpha2/aksnodeclass_defaults.go | 25 ----- ...odeclass_util.go => aksnodeclass_utils.go} | 4 - pkg/apis/v1alpha2/aksnodeclass_validation.go | 69 ------------ pkg/cloudprovider/drift.go | 5 - pkg/providers/imagefamily/image.go | 5 - pkg/providers/imagefamily/image_test.go | 103 ------------------ pkg/providers/instance/instance.go | 7 +- pkg/webhooks/webhooks.go | 62 ----------- 10 files changed, 2 insertions(+), 347 deletions(-) delete mode 100644 charts/karpenter/templates/webhooks.yaml delete mode 100644 pkg/apis/v1alpha2/aksnodeclass_defaults.go rename pkg/apis/v1alpha2/{aksnodeclass_util.go => aksnodeclass_utils.go} (87%) delete mode 100644 pkg/apis/v1alpha2/aksnodeclass_validation.go delete mode 100644 pkg/webhooks/webhooks.go diff --git a/charts/karpenter/templates/webhooks.yaml b/charts/karpenter/templates/webhooks.yaml deleted file mode 100644 index 6af7df8bf..000000000 --- a/charts/karpenter/templates/webhooks.yaml +++ /dev/null @@ -1,67 +0,0 @@ -{{- if .Values.webhook.enabled }} -apiVersion: admissionregistration.k8s.io/v1 -kind: MutatingWebhookConfiguration -metadata: - name: defaulting.webhook.karpenter.azure.com - labels: - {{- include "karpenter.labels" . | nindent 4 }} - {{- with .Values.additionalAnnotations }} - annotations: - {{- toYaml . | nindent 4 }} - {{- end }} -webhooks: - - name: defaulting.webhook.karpenter.azure.com - admissionReviewVersions: ["v1"] - clientConfig: - service: - name: {{ include "karpenter.fullname" . }} - namespace: {{ .Release.Namespace }} - port: {{ .Values.webhook.port }} - failurePolicy: Fail - sideEffects: None - rules: - - apiGroups: - - karpenter.azure.com - apiVersions: - - v1alpha2 - operations: - - CREATE - - UPDATE - resources: - - aksnodeclasses - - aksnodeclasses/status - scope: '*' ---- -apiVersion: admissionregistration.k8s.io/v1 -kind: ValidatingWebhookConfiguration -metadata: - name: validation.webhook.karpenter.azure.com - labels: - {{- include "karpenter.labels" . | nindent 4 }} - {{- with .Values.additionalAnnotations }} - annotations: - {{- toYaml . | nindent 4 }} - {{- end }} -webhooks: - - name: validation.webhook.karpenter.azure.com - admissionReviewVersions: ["v1"] - clientConfig: - service: - name: {{ include "karpenter.fullname" . }} - namespace: {{ .Release.Namespace }} - port: {{ .Values.webhook.port }} - failurePolicy: Fail - sideEffects: None - rules: - - apiGroups: - - karpenter.azure.com - apiVersions: - - v1alpha2 - operations: - - CREATE - - UPDATE - resources: - - aksnodeclasses - - aksnodeclasses/status - scope: '*' -{{- end }} diff --git a/cmd/controller/main.go b/cmd/controller/main.go index b21474e0e..6de928d38 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -23,7 +23,6 @@ import ( "github.com/Azure/karpenter-provider-azure/pkg/cloudprovider" "github.com/Azure/karpenter-provider-azure/pkg/operator" - "github.com/Azure/karpenter-provider-azure/pkg/webhooks" controllers "github.com/Azure/karpenter-provider-azure/pkg/controllers" "sigs.k8s.io/karpenter/pkg/cloudprovider/metrics" @@ -62,6 +61,5 @@ func main() { aksCloudProvider, op.InstanceProvider, )...). - WithWebhooks(ctx, webhooks.NewWebhooks()...). Start(ctx) } diff --git a/pkg/apis/v1alpha2/aksnodeclass_defaults.go b/pkg/apis/v1alpha2/aksnodeclass_defaults.go deleted file mode 100644 index d11dd173c..000000000 --- a/pkg/apis/v1alpha2/aksnodeclass_defaults.go +++ /dev/null @@ -1,25 +0,0 @@ -/* -Portions Copyright (c) Microsoft Corporation. - -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. -*/ - -package v1alpha2 - -import ( - "context" -) - -// SetDefaults for the AKSNodeClass -func (in *AKSNodeClass) SetDefaults(_ context.Context) { -} diff --git a/pkg/apis/v1alpha2/aksnodeclass_util.go b/pkg/apis/v1alpha2/aksnodeclass_utils.go similarity index 87% rename from pkg/apis/v1alpha2/aksnodeclass_util.go rename to pkg/apis/v1alpha2/aksnodeclass_utils.go index 7018a908a..1b09f67f0 100644 --- a/pkg/apis/v1alpha2/aksnodeclass_util.go +++ b/pkg/apis/v1alpha2/aksnodeclass_utils.go @@ -16,10 +16,6 @@ limitations under the License. package v1alpha2 -func (in *AKSNodeClassSpec) IsEmptyImageID() bool { - return in.ImageID == nil || *in.ImageID == "" -} - func (in *AKSNodeClassSpec) GetImageVersion() string { if in.ImageVersion == nil { return "" diff --git a/pkg/apis/v1alpha2/aksnodeclass_validation.go b/pkg/apis/v1alpha2/aksnodeclass_validation.go deleted file mode 100644 index 51c1f70d4..000000000 --- a/pkg/apis/v1alpha2/aksnodeclass_validation.go +++ /dev/null @@ -1,69 +0,0 @@ -/* -Portions Copyright (c) Microsoft Corporation. - -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. -*/ - -package v1alpha2 - -import ( - "context" - "fmt" - "regexp" - - admissionregistrationv1 "k8s.io/api/admissionregistration/v1" - - "knative.dev/pkg/apis" -) - -var ( - SubscriptionShape = regexp.MustCompile(`[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}`) - ComputeGalleryImageVersionRegex = regexp.MustCompile(`(?i)/subscriptions/` + SubscriptionShape.String() + `/resourceGroups/[\w-]+/providers/Microsoft\.Compute/galleries/[\w-]+/images/[\w-]+/versions/[\d.]+`) - CommunityImageVersionRegex = regexp.MustCompile(`(?i)/CommunityGalleries/[\w-]+/images/[\w-]+/versions/[\d.]+`) -) - -func (in *AKSNodeClass) SupportedVerbs() []admissionregistrationv1.OperationType { - return []admissionregistrationv1.OperationType{ - admissionregistrationv1.Create, - admissionregistrationv1.Update, - } -} - -func IsComputeGalleryImageID(imageID string) bool { - return ComputeGalleryImageVersionRegex.MatchString(imageID) -} - -func (in *AKSNodeClass) Validate(ctx context.Context) (errs *apis.FieldError) { - //if apis.IsInUpdate(ctx) { - // original := apis.GetBaseline(ctx).(*AKSNodeClass) - // errs = in.validateImmutableFields(original) - //} - return errs.Also( - apis.ValidateObjectMetadata(in).ViaField("metadata"), - in.Spec.validate(ctx).ViaField("spec"), - ) -} - -func (in *AKSNodeClassSpec) validate(_ context.Context) (errs *apis.FieldError) { - return errs.Also( - in.validateImageID(), - ) -} - -func (in *AKSNodeClassSpec) validateImageID() (errs *apis.FieldError) { - if in.IsEmptyImageID() || ComputeGalleryImageVersionRegex.MatchString(*in.ImageID) || CommunityImageVersionRegex.MatchString(*in.ImageID) { - return nil - } - return apis.ErrInvalidValue(fmt.Sprintf( - "the provided image ID: '%s' is invalid because it doesn't match the expected format", *in.ImageID), "ImageID") -} diff --git a/pkg/cloudprovider/drift.go b/pkg/cloudprovider/drift.go index 423c0c7a5..dd8f75694 100644 --- a/pkg/cloudprovider/drift.go +++ b/pkg/cloudprovider/drift.go @@ -76,11 +76,6 @@ func (c *CloudProvider) isImageVersionDrifted( ctx context.Context, nodeClaim *corev1beta1.NodeClaim, nodeClass *v1alpha2.AKSNodeClass) (cloudprovider.DriftReason, error) { logger := logging.FromContext(ctx) - if !nodeClass.Spec.IsEmptyImageID() { - // Note: ImageID takes priority ATM - return "", nil - } - id, err := utils.GetVMName(nodeClaim.Status.ProviderID) if err != nil { // TODO (charliedmcb): Do we need to handle vm not found here before its provisioned? diff --git a/pkg/providers/imagefamily/image.go b/pkg/providers/imagefamily/image.go index 590c26956..85ba91685 100644 --- a/pkg/providers/imagefamily/image.go +++ b/pkg/providers/imagefamily/image.go @@ -64,11 +64,6 @@ func NewProvider(kubernetesInterface kubernetes.Interface, kubernetesVersionCach // Get returns Image ID for the given instance type. Images may vary due to architecture, accelerator, etc func (p *Provider) Get(ctx context.Context, nodeClass *v1alpha2.AKSNodeClass, instanceType *cloudprovider.InstanceType, imageFamily ImageFamily) (string, error) { - if !nodeClass.Spec.IsEmptyImageID() { - logging.FromContext(ctx).Debugf("Using user-provided image %s", *nodeClass.Spec.ImageID) - return *nodeClass.Spec.ImageID, nil - } - defaultImages := imageFamily.DefaultImages() for _, defaultImage := range defaultImages { if err := instanceType.Requirements.Compatible(defaultImage.Requirements, v1alpha2.AllowUndefinedLabels); err == nil { diff --git a/pkg/providers/imagefamily/image_test.go b/pkg/providers/imagefamily/image_test.go index c7d54e344..0da43af9e 100644 --- a/pkg/providers/imagefamily/image_test.go +++ b/pkg/providers/imagefamily/image_test.go @@ -17,25 +17,15 @@ limitations under the License. package imagefamily_test import ( - "context" "fmt" "testing" - "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" - "github.com/Azure/karpenter-provider-azure/pkg/apis/v1alpha2" - "github.com/Azure/karpenter-provider-azure/pkg/fake" "github.com/Azure/karpenter-provider-azure/pkg/providers/imagefamily" - "github.com/Azure/karpenter-provider-azure/pkg/test" - "github.com/samber/lo" - "sigs.k8s.io/karpenter/pkg/cloudprovider" ) -var imageProvider *imagefamily.Provider - func TestAzure(t *testing.T) { RegisterFailHandler(Fail) RunSpecs(t, "Providers/ImageProvider/Azure") @@ -47,99 +37,6 @@ const ( latestImageVersion = "1.1686127203.20217" ) -var _ = BeforeSuite(func() { - location := fake.Region - - defaultImageVersions := []*armcompute.CommunityGalleryImageVersion{ - { - Name: lo.ToPtr("1.1686127203.20215"), - Location: &location, - Type: lo.ToPtr("Microsoft.Compute/galleries/images/versions"), - Properties: &armcompute.CommunityGalleryImageVersionProperties{ - PublishedDate: lo.ToPtr(time.Now().Add(time.Minute * -10)), - }, - }, - { - Name: lo.ToPtr("1.1686127203.20213"), - Location: &location, - Type: lo.ToPtr("Microsoft.Compute/galleries/images/versions"), - Properties: &armcompute.CommunityGalleryImageVersionProperties{ - PublishedDate: lo.ToPtr(time.Now().Add(time.Minute * -20)), - }, - }, - { - Name: lo.ToPtr(latestImageVersion), - Location: &location, - Type: lo.ToPtr("Microsoft.Compute/galleries/images/versions"), - Properties: &armcompute.CommunityGalleryImageVersionProperties{ - PublishedDate: lo.ToPtr(time.Now().Add(time.Minute * -5)), - }, - }, - { - Name: lo.ToPtr(olderImageVersion), - Location: &location, - Type: lo.ToPtr("Microsoft.Compute/galleries/images/versions"), - Properties: &armcompute.CommunityGalleryImageVersionProperties{ - PublishedDate: lo.ToPtr(time.Now().Add(time.Minute * -15)), - }, - }, - { - Name: lo.ToPtr("1.1686127203.20216"), - Location: &location, - Type: lo.ToPtr("Microsoft.Compute/galleries/images/versions"), - Properties: &armcompute.CommunityGalleryImageVersionProperties{ - PublishedDate: lo.ToPtr(time.Now().Add(time.Minute * -7)), - }, - }, - } - - versionsClient := &fake.CommunityGalleryImageVersionsAPI{} - versionsClient.ImageVersions.Append(defaultImageVersions...) - imageProvider = imagefamily.NewProvider(nil, nil, versionsClient, fake.Region) -}) - -func newTestNodeClass(imageID, imageVersion string) *v1alpha2.AKSNodeClass { - nodeClass := test.AKSNodeClass() - - if imageID != "" { - nodeClass.Spec.ImageID = lo.ToPtr(imageID) - } - if imageVersion != "" { - nodeClass.Spec.ImageVersion = lo.ToPtr(imageVersion) - } - return nodeClass -} - -var _ = Describe("Image ID Resolution", func() { - var ( - nodeClassWithImageID = newTestNodeClass(testImageID, "") - nodeClassWithImageIDAndVersion = newTestNodeClass(testImageID, olderImageVersion) - nodeClassWithImageVersion = newTestNodeClass("", olderImageVersion) - ) - - DescribeTable("Resolution Of Image ID", - func(nodeClass *v1alpha2.AKSNodeClass, instanceType *cloudprovider.InstanceType, imageFamily interface{}, expectedImageID string) { - imageID, err := imageProvider.Get(context.Background(), nodeClass, instanceType, imagefamily.Ubuntu2204{}) - Expect(imageID).To(Equal(expectedImageID)) - Expect(err).To(BeNil()) - }, - Entry("Image ID is specified in the NodeClass", nodeClassWithImageID, &cloudprovider.InstanceType{}, imagefamily.Ubuntu2204{}, testImageID), - Entry("Image ID and ImageVersion are specified in the NodeClass", nodeClassWithImageIDAndVersion, &cloudprovider.InstanceType{}, imagefamily.Ubuntu2204{}, testImageID), - Entry("ImageVersion is specified in the NodeClass", nodeClassWithImageVersion, &cloudprovider.InstanceType{}, imagefamily.Ubuntu2204{}, fmt.Sprintf("/CommunityGalleries/%s/images/%s/versions/%s", imagefamily.AKSUbuntuPublicGalleryURL, imagefamily.Ubuntu2204Gen2CommunityImage, olderImageVersion)), - ) - - DescribeTable("Resolution Of Image ID", - func(communityImageName, publicGalleryURL, versionName string, expectedImageID string) { - imageID, err := imageProvider.GetImageID(communityImageName, publicGalleryURL, versionName) - Expect(imageID).To(Equal(expectedImageID)) - Expect(err).To(BeNil()) - }, - Entry("Image version is empty, should get latest", imagefamily.Ubuntu2204Gen2CommunityImage, imagefamily.AKSUbuntuPublicGalleryURL, "", fmt.Sprintf("/CommunityGalleries/%s/images/%s/versions/%s", imagefamily.AKSUbuntuPublicGalleryURL, imagefamily.Ubuntu2204Gen2CommunityImage, latestImageVersion)), - Entry("Image version is specified, should use it", imagefamily.Ubuntu2204Gen2CommunityImage, imagefamily.AKSUbuntuPublicGalleryURL, olderImageVersion, fmt.Sprintf("/CommunityGalleries/%s/images/%s/versions/%s", imagefamily.AKSUbuntuPublicGalleryURL, imagefamily.Ubuntu2204Gen2CommunityImage, olderImageVersion)), - ) - -}) - var _ = Describe("Image ID Parsing", func() { DescribeTable("Parse Image ID", func(imageID string, expectedPublicGalleryURL, expectedCommunityImageName, expectedImageVersion string, expectError bool) { diff --git a/pkg/providers/instance/instance.go b/pkg/providers/instance/instance.go index f14823b44..98e1a8154 100644 --- a/pkg/providers/instance/instance.go +++ b/pkg/providers/instance/instance.go @@ -271,11 +271,8 @@ func newVMObject( launchTemplate *launchtemplate.Template, instanceType *corecloudprovider.InstanceType) armcompute.VirtualMachine { // Build the image reference from template - imageReference := armcompute.ImageReference{} - if v1alpha2.IsComputeGalleryImageID(launchTemplate.ImageID) { - imageReference.ID = &launchTemplate.ImageID - } else { - imageReference.CommunityGalleryImageID = &launchTemplate.ImageID + imageReference := armcompute.ImageReference{ + CommunityGalleryImageID: &launchTemplate.ImageID, } vm := armcompute.VirtualMachine{ Location: to.Ptr(location), diff --git a/pkg/webhooks/webhooks.go b/pkg/webhooks/webhooks.go deleted file mode 100644 index 1864f9391..000000000 --- a/pkg/webhooks/webhooks.go +++ /dev/null @@ -1,62 +0,0 @@ -/* -Portions Copyright (c) Microsoft Corporation. - -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. -*/ - -package webhooks - -import ( - "context" - - "k8s.io/apimachinery/pkg/runtime/schema" - "knative.dev/pkg/configmap" - "knative.dev/pkg/controller" - knativeinjection "knative.dev/pkg/injection" - "knative.dev/pkg/webhook/resourcesemantics" - "knative.dev/pkg/webhook/resourcesemantics/defaulting" - "knative.dev/pkg/webhook/resourcesemantics/validation" - - "github.com/Azure/karpenter-provider-azure/pkg/apis/v1alpha2" -) - -func NewWebhooks() []knativeinjection.ControllerConstructor { - return []knativeinjection.ControllerConstructor{ - NewCRDDefaultingWebhook, - NewCRDValidationWebhook, - } -} - -func NewCRDDefaultingWebhook(ctx context.Context, _ configmap.Watcher) *controller.Impl { - return defaulting.NewAdmissionController(ctx, - "defaulting.webhook.karpenter.azure.com", - "/default/karpenter.azure.com", - Resources, - func(ctx context.Context) context.Context { return ctx }, - true, - ) -} - -func NewCRDValidationWebhook(ctx context.Context, _ configmap.Watcher) *controller.Impl { - return validation.NewAdmissionController(ctx, - "validation.webhook.karpenter.azure.com", - "/validate/karpenter.azure.com", - Resources, - func(ctx context.Context) context.Context { return ctx }, - true, - ) -} - -var Resources = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{ - v1alpha2.SchemeGroupVersion.WithKind("AKSNodeClass"): &v1alpha2.AKSNodeClass{}, -}