more gh comments

pkg/apis/settings/settings.go

@@ -108,7 +108,8 @@ func ToContext(ctx context.Context, s *Settings) context.Context {
 func FromContext(ctx context.Context) *Settings {
 	data := ctx.Value(ContextKey)
 	if data == nil {
-		panic("settings not in context")
+		// This is developer error if this happens, so we should panic
+		panic("settings doesn't exist in context")
 	}
 	return data.(*Settings)
 }

pkg/operator/options/options.go

@@ -70,10 +70,6 @@ func (o *Options) Parse(fs *flag.FlagSet, args ...string) error {
 		return fmt.Errorf("parsing flags, %w", err)
 	}
 
-	if err := o.Validate(); err != nil {
-		return fmt.Errorf("validating options, %w", err)
-	}
-
 	// Check if each option has been set. This is a little brute force and better options might exist,
 	// but this only needs to be here for one version
 	o.setFlags = map[string]bool{}
@@ -87,6 +83,10 @@ func (o *Options) Parse(fs *flag.FlagSet, args ...string) error {
 		o.setFlags[f.Name] = ok || cliFlags.Has(f.Name)
 	})
 
+	if err := o.Validate(); err != nil {
+		return fmt.Errorf("validating options, %w", err)
+	}
+
 	return nil
 }
 

pkg/operator/options/options_validation.go

@@ -19,49 +19,49 @@ import (
 	"net/url"
 	"time"
 
-	"knative.dev/pkg/apis"
+	"go.uber.org/multierr"
 )
 
-func (o Options) Validate() (errs *apis.FieldError) {
-	return errs.Also(
+func (o Options) Validate() error {
+	return multierr.Combine(
 		o.validateEndpoint(),
 		o.validateClusterName(),
 		o.validateVMMemoryOverheadPercent(),
 		o.validateAssumeRoleDuration(),
-	).ViaField("aws")
+	)
 }
 
-func (o Options) validateAssumeRoleDuration() (errs *apis.FieldError) {
+func (o Options) validateAssumeRoleDuration() error {
 	if o.AssumeRoleDuration < time.Minute*15 {
-		return errs.Also(apis.ErrInvalidValue("assume-role-duration cannot be less than 15 Minutes", "assume-role-duration"))
+		return fmt.Errorf("assume-role-duration cannot be less than 15 minutes")
 	}
 	return nil
 }
 
-func (o Options) validateClusterName() (errs *apis.FieldError) {
+func (o Options) validateClusterName() error {
 	// TODO: Remove with karpenter-global-settings
-	if !o.setFlags["aws-cluster-name"] {
+	if !o.setFlags["cluster-name"] {
 		return nil
 	}
 	if o.ClusterName == "" {
-		return errs.Also(apis.ErrMissingField("cluster-name is required", "cluster-name"))
+		return fmt.Errorf("cluster-name is required")
 	}
 	return nil
 }
 
-func (o Options) validateEndpoint() (errs *apis.FieldError) {
+func (o Options) validateEndpoint() error {
 	endpoint, err := url.Parse(o.ClusterEndpoint)
 	// url.Parse() will accept a lot of input without error; make
 	// sure it's a real URL
 	if err != nil || !endpoint.IsAbs() || endpoint.Hostname() == "" {
-		return errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%q not a valid cluster-endpoint URL", o.ClusterEndpoint), "cluster-endpoint"))
+		return fmt.Errorf("%q not a valid cluster-endpoint URL", o.ClusterEndpoint)
 	}
 	return nil
 }
 
-func (o Options) validateVMMemoryOverheadPercent() (errs *apis.FieldError) {
+func (o Options) validateVMMemoryOverheadPercent() error {
 	if o.VMMemoryOverheadPercent < 0 {
-		return errs.Also(apis.ErrInvalidValue("cannot be negative", "vmMemoryOverheadPercent"))
+		return fmt.Errorf("vm-memory-overhead-percent may not be negative")
 	}
 	return nil
 }

test/pkg/environment/aws/environment.go

@@ -55,6 +55,8 @@ type Environment struct {
 	TimeStreamAPI timestreamwriteiface.TimestreamWriteAPI
 
 	SQSProvider *interruption.SQSProvider
+
+	InterruptionQueueName string
 }
 
 func NewEnvironment(t *testing.T) *Environment {
@@ -81,6 +83,8 @@ func NewEnvironment(t *testing.T) *Environment {
 		EKSAPI:        eks.New(session),
 		SQSProvider:   interruption.NewSQSProvider(sqs.New(session)),
 		TimeStreamAPI: GetTimeStreamAPI(session),
+
+		InterruptionQueueName: lo.Must(os.LookupEnv("INTERRUPTION_QUEUE_NAME")),
 	}
 }
 
@@ -91,15 +95,3 @@ func GetTimeStreamAPI(session *session.Session) timestreamwriteiface.TimestreamW
 	}
 	return &NoOpTimeStreamAPI{}
 }
-
-func GetClusterName() string {
-	return lo.Must(os.LookupEnv("CLUSTER_NAME"))
-}
-
-func GetClusterEndpoint() string {
-	return lo.Must(os.LookupEnv("CLUSTER_ENDPOINT"))
-}
-
-func GetInterruptionQueueName() string {
-	return lo.Must(os.LookupEnv("INTERRUPTION_QUEUE_NAME"))
-}

test/pkg/environment/common/environment.go

@@ -58,6 +58,9 @@ type Environment struct {
 	Monitor    *Monitor
 
 	StartingNodeCount int
+
+	ClusterName     string
+	ClusterEndpoint string
 }
 
 func NewEnvironment(t *testing.T) *Environment {
@@ -76,12 +79,14 @@ func NewEnvironment(t *testing.T) *Environment {
 	gomega.SetDefaultEventuallyTimeout(5 * time.Minute)
 	gomega.SetDefaultEventuallyPollingInterval(1 * time.Second)
 	return &Environment{
-		Context:    ctx,
-		cancel:     cancel,
-		Config:     config,
-		Client:     client,
-		KubeClient: kubernetes.NewForConfigOrDie(config),
-		Monitor:    NewMonitor(ctx, client),
+		Context:         ctx,
+		cancel:          cancel,
+		Config:          config,
+		Client:          client,
+		KubeClient:      kubernetes.NewForConfigOrDie(config),
+		Monitor:         NewMonitor(ctx, client),
+		ClusterName:     lo.Must(os.LookupEnv("CLUSTER_NAME")),
+		ClusterEndpoint: lo.Must(os.LookupEnv("CLUSTER_ENDPOINT")),
 	}
 }
 

test/suites/chaos/suite_test.go

@@ -41,7 +41,6 @@ import (
 	"github.com/aws/karpenter/pkg/apis/v1alpha1"
 	awstest "github.com/aws/karpenter/pkg/test"
 	"github.com/aws/karpenter/test/pkg/debug"
-	"github.com/aws/karpenter/test/pkg/environment/aws"
 	"github.com/aws/karpenter/test/pkg/environment/common"
 )
 
@@ -69,8 +68,8 @@ var _ = Describe("Chaos", func() {
 			defer cancel()
 
 			provider := awstest.AWSNodeTemplate(v1alpha1.AWSNodeTemplateSpec{AWS: v1alpha1.AWS{
-				SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": aws.GetClusterName()},
-				SubnetSelector:        map[string]string{"karpenter.sh/discovery": aws.GetClusterName()},
+				SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": env.ClusterName},
+				SubnetSelector:        map[string]string{"karpenter.sh/discovery": env.ClusterName},
 			}})
 			provisioner := test.Provisioner(test.ProvisionerOptions{
 				Requirements: []v1.NodeSelectorRequirement{
@@ -114,8 +113,8 @@ var _ = Describe("Chaos", func() {
 			defer cancel()
 
 			provider := awstest.AWSNodeTemplate(v1alpha1.AWSNodeTemplateSpec{AWS: v1alpha1.AWS{
-				SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": aws.GetClusterName()},
-				SubnetSelector:        map[string]string{"karpenter.sh/discovery": aws.GetClusterName()},
+				SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": env.ClusterName},
+				SubnetSelector:        map[string]string{"karpenter.sh/discovery": env.ClusterName},
 			}})
 			provisioner := test.Provisioner(test.ProvisionerOptions{
 				Requirements: []v1.NodeSelectorRequirement{

test/suites/consolidation/suite_test.go

@@ -59,8 +59,8 @@ var _ = AfterEach(func() { env.AfterEach() })
 var _ = Describe("Consolidation", func() {
 	It("should consolidate nodes (delete)", Label(debug.NoWatch), Label(debug.NoEvents), func() {
 		provider := awstest.AWSNodeTemplate(v1alpha1.AWSNodeTemplateSpec{AWS: v1alpha1.AWS{
-			SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": environmentaws.GetClusterName()},
-			SubnetSelector:        map[string]string{"karpenter.sh/discovery": environmentaws.GetClusterName()},
+			SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": env.ClusterName},
+			SubnetSelector:        map[string]string{"karpenter.sh/discovery": env.ClusterName},
 		}})
 		provisioner := test.Provisioner(test.ProvisionerOptions{
 			Requirements: []v1.NodeSelectorRequirement{
@@ -125,8 +125,8 @@ var _ = Describe("Consolidation", func() {
 	})
 	It("should consolidate on-demand nodes (replace)", func() {
 		provider := awstest.AWSNodeTemplate(v1alpha1.AWSNodeTemplateSpec{AWS: v1alpha1.AWS{
-			SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": environmentaws.GetClusterName()},
-			SubnetSelector:        map[string]string{"karpenter.sh/discovery": environmentaws.GetClusterName()},
+			SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": env.ClusterName},
+			SubnetSelector:        map[string]string{"karpenter.sh/discovery": env.ClusterName},
 		}})
 		provisioner := test.Provisioner(test.ProvisionerOptions{
 			Requirements: []v1.NodeSelectorRequirement{
@@ -241,8 +241,8 @@ var _ = Describe("Consolidation", func() {
 	})
 	It("should consolidate on-demand nodes to spot (replace)", func() {
 		provider := awstest.AWSNodeTemplate(v1alpha1.AWSNodeTemplateSpec{AWS: v1alpha1.AWS{
-			SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": environmentaws.GetClusterName()},
-			SubnetSelector:        map[string]string{"karpenter.sh/discovery": environmentaws.GetClusterName()},
+			SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": env.ClusterName},
+			SubnetSelector:        map[string]string{"karpenter.sh/discovery": env.ClusterName},
 		}})
 		provisioner := test.Provisioner(test.ProvisionerOptions{
 			Requirements: []v1.NodeSelectorRequirement{

test/suites/drift/suite_test.go

@@ -72,8 +72,8 @@ var _ = Describe("Drift", Label("AWS"), func() {
 	BeforeEach(func() {
 		customAMI = env.GetCustomAMI("/aws/service/eks/optimized-ami/%s/amazon-linux-2/recommended/image_id", 1)
 		nodeTemplate = awstest.AWSNodeTemplate(v1alpha1.AWSNodeTemplateSpec{AWS: v1alpha1.AWS{
-			SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": aws.GetClusterName()},
-			SubnetSelector:        map[string]string{"karpenter.sh/discovery": aws.GetClusterName()},
+			SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": env.ClusterName},
+			SubnetSelector:        map[string]string{"karpenter.sh/discovery": env.ClusterName},
 		}})
 		provisioner = test.Provisioner(test.ProvisionerOptions{
 			Requirements: []v1.NodeSelectorRequirement{{Key: v1alpha5.LabelCapacityType, Operator: v1.NodeSelectorOpIn, Values: []string{v1alpha5.CapacityTypeOnDemand}}},
@@ -100,7 +100,7 @@ var _ = Describe("Drift", Label("AWS"), func() {
 		oldCustomAMI := *parameter.Parameter.Value
 		nodeTemplate.Spec.AMIFamily = &v1alpha1.AMIFamilyCustom
 		nodeTemplate.Spec.AMISelector = map[string]string{"aws-ids": oldCustomAMI}
-		nodeTemplate.Spec.UserData = awssdk.String(fmt.Sprintf("#!/bin/bash\n/etc/eks/bootstrap.sh '%s'", aws.GetClusterName()))
+		nodeTemplate.Spec.UserData = awssdk.String(fmt.Sprintf("#!/bin/bash\n/etc/eks/bootstrap.sh '%s'", env.ClusterName))
 
 		env.ExpectCreated(pod, nodeTemplate, provisioner)
 		env.EventuallyExpectHealthy(pod)
@@ -133,7 +133,7 @@ var _ = Describe("Drift", Label("AWS"), func() {
 		oldCustomAMI := *parameter.Parameter.Value
 		nodeTemplate.Spec.AMIFamily = &v1alpha1.AMIFamilyCustom
 		nodeTemplate.Spec.AMISelector = map[string]string{"aws-ids": oldCustomAMI}
-		nodeTemplate.Spec.UserData = awssdk.String(fmt.Sprintf("#!/bin/bash\n/etc/eks/bootstrap.sh '%s'", aws.GetClusterName()))
+		nodeTemplate.Spec.UserData = awssdk.String(fmt.Sprintf("#!/bin/bash\n/etc/eks/bootstrap.sh '%s'", env.ClusterName))
 
 		env.ExpectCreated(pod, nodeTemplate, provisioner)
 		env.EventuallyExpectHealthy(pod)
@@ -150,7 +150,7 @@ var _ = Describe("Drift", Label("AWS"), func() {
 	})
 	It("should deprovision nodes that have drifted due to securitygroup", func() {
 		By("getting the cluster vpc id")
-		output, err := env.EKSAPI.DescribeCluster(&eks.DescribeClusterInput{Name: awssdk.String(aws.GetClusterName())})
+		output, err := env.EKSAPI.DescribeCluster(&eks.DescribeClusterInput{Name: awssdk.String(env.ClusterName)})
 		Expect(err).To(BeNil())
 
 		By("creating new security group")
@@ -164,11 +164,11 @@ var _ = Describe("Drift", Label("AWS"), func() {
 					Tags: []*ec2.Tag{
 						{
 							Key:   awssdk.String("karpenter.sh/discovery"),
-							Value: awssdk.String(aws.GetClusterName()),
+							Value: awssdk.String(env.ClusterName),
 						},
 						{
 							Key:   awssdk.String(test.DiscoveryLabel),
-							Value: awssdk.String(aws.GetClusterName()),
+							Value: awssdk.String(env.ClusterName),
 						},
 						{
 							Key:   awssdk.String("creation-date"),
@@ -184,7 +184,7 @@ var _ = Describe("Drift", Label("AWS"), func() {
 		var securitygroups []aws.SecurityGroup
 		var testSecurityGroup aws.SecurityGroup
 		Eventually(func(g Gomega) {
-			securitygroups = env.GetSecurityGroups(map[string]string{"karpenter.sh/discovery": aws.GetClusterName()})
+			securitygroups = env.GetSecurityGroups(map[string]string{"karpenter.sh/discovery": env.ClusterName})
 			testSecurityGroup, _ = lo.Find(securitygroups, func(sg aws.SecurityGroup) bool {
 				return awssdk.StringValue(sg.GroupName) == "security-group-drift"
 			})
@@ -221,7 +221,7 @@ var _ = Describe("Drift", Label("AWS"), func() {
 		env.EventuallyExpectNotFound(pod, machine, node)
 	})
 	It("should deprovision nodes that have drifted due to subnets", func() {
-		subnets := env.GetSubnetNameAndIds(map[string]string{"karpenter.sh/discovery": aws.GetClusterName()})
+		subnets := env.GetSubnetNameAndIds(map[string]string{"karpenter.sh/discovery": env.ClusterName})
 		Expect(len(subnets)).To(BeNumerically(">", 1))
 
 		nodeTemplate.Spec.SubnetSelector = map[string]string{"aws-ids": subnets[0].ID}
@@ -299,7 +299,7 @@ var _ = Describe("Drift", Label("AWS"), func() {
 	)
 	DescribeTable("AWSNodeTemplate Drift", func(fieldName string, nodeTemplateSpec v1alpha1.AWSNodeTemplateSpec) {
 		if fieldName == "InstanceProfile" {
-			nodeTemplateSpec.AWS.InstanceProfile = awssdk.String(fmt.Sprintf("KarpenterNodeInstanceProfile-Drift-%s", aws.GetClusterName()))
+			nodeTemplateSpec.AWS.InstanceProfile = awssdk.String(fmt.Sprintf("KarpenterNodeInstanceProfile-Drift-%s", env.ClusterName))
 			ExpectInstanceProfileCreated(nodeTemplateSpec.AWS.InstanceProfile)
 		}
 
@@ -475,7 +475,7 @@ func ExpectInstanceProfileCreated(instanceProfileName *string) {
 		Tags: []*iam.Tag{
 			{
 				Key:   awssdk.String(test.DiscoveryLabel),
-				Value: awssdk.String(aws.GetClusterName()),
+				Value: awssdk.String(env.ClusterName),
 			},
 		},
 	}
@@ -484,7 +484,7 @@ func ExpectInstanceProfileCreated(instanceProfileName *string) {
 	Expect(ignoreAlreadyExists(err)).ToNot(HaveOccurred())
 	addInstanceProfile := &iam.AddRoleToInstanceProfileInput{
 		InstanceProfileName: instanceProfileName,
-		RoleName:            awssdk.String(fmt.Sprintf("KarpenterNodeRole-%s", aws.GetClusterName())),
+		RoleName:            awssdk.String(fmt.Sprintf("KarpenterNodeRole-%s", env.ClusterName)),
 	}
 	_, err = env.IAMAPI.AddRoleToInstanceProfile(addInstanceProfile)
 	Expect(ignoreAlreadyContainsRole(err)).ToNot(HaveOccurred())

test/suites/expiration/expiration_test.go

@@ -66,8 +66,8 @@ var _ = Describe("Expiration", func() {
 	var provisioner *v1alpha5.Provisioner
 	BeforeEach(func() {
 		nodeTemplate = awstest.AWSNodeTemplate(v1alpha1.AWSNodeTemplateSpec{AWS: v1alpha1.AWS{
-			SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": aws.GetClusterName()},
-			SubnetSelector:        map[string]string{"karpenter.sh/discovery": aws.GetClusterName()},
+			SecurityGroupSelector: map[string]string{"karpenter.sh/discovery": env.ClusterName},
+			SubnetSelector:        map[string]string{"karpenter.sh/discovery": env.ClusterName},
 		}})
 		provisioner = test.Provisioner(test.ProvisionerOptions{
 			ProviderRef:            &v1alpha5.MachineTemplateRef{Name: nodeTemplate.Name},