diff --git a/pkg/operator/options/options.go b/pkg/operator/options/options.go index 960fa080e4d6..2d836d798aa9 100644 --- a/pkg/operator/options/options.go +++ b/pkg/operator/options/options.go @@ -47,23 +47,22 @@ type Options struct { InterruptionQueueName string ReservedENIs int - isolatedVPC string setFlags map[string]bool } -func (o *Options) AddFlags(fs *flag.FlagSet) { +func (o *Options) AddFlags(fs *coreoptions.FlagSet) { fs.StringVar(&o.AssumeRoleARN, "assume-role-arn", env.WithDefaultString("ASSUME_ROLE_ARN", ""), "Role to assume for calling AWS services.") fs.DurationVar(&o.AssumeRoleDuration, "assume-role-duration", env.WithDefaultDuration("ASSUME_ROLE_DURATION", 15*time.Minute), "Duration of assumed credentials in minutes. Default value is 15 minutes. Not used unless aws.assumeRole set.") fs.StringVar(&o.ClusterCABundle, "cluster-ca-bundle", env.WithDefaultString("CLUSTER_CA_BUNDLE", ""), "Cluster CA bundle for nodes to use for TLS connections with the API server. If not set, this is taken from the controller's TLS configuration.") fs.StringVar(&o.ClusterName, "cluster-name", env.WithDefaultString("CLUSTER_NAME", ""), "[REQUIRED] The kubernetes cluster name for resource discovery.") fs.StringVar(&o.ClusterEndpoint, "cluster-endpoint", env.WithDefaultString("CLUSTER_ENDPOINT", ""), "The external kubernetes cluster endpoint for new nodes to connect with. If not specified, will discover the cluster endpoint using DescribeCluster API.") - fs.StringVar(&o.isolatedVPC, "isolated-vpc", env.WithDefaultString("ISOLATED_VPC", "false"), "If true, then assume we can't reach AWS services which don't have a VPC endpoint. This also has the effect of disabling look-ups to the AWS pricing endpoint.") + fs.BoolVarWithEnv(&o.IsolatedVPC, "isolated-vpc", "ISOLATED_VPC", false, "If true, then assume we can't reach AWS services which don't have a VPC endpoint. This also has the effect of disabling look-ups to the AWS pricing endpoint.") fs.Float64Var(&o.VMMemoryOverheadPercent, "vm-memory-overhead-percent", env.WithDefaultFloat64("VM_MEMORY_OVERHEAD_PERCENT", 0.075), "The VM memory overhead as a percent that will be subtracted from the total memory for all instance types.") fs.StringVar(&o.InterruptionQueueName, "interruption-queue-name", env.WithDefaultString("INTERRUPTION_QUEUE_NAME", ""), "Interruption queue is disabled if not specified. Enabling interruption handling may require additional permissions on the controller service account. Additional permissions are outlined in the docs.") fs.IntVar(&o.ReservedENIs, "reserved-enis", env.WithDefaultInt("RESERVED_ENIS", 0), "Reserved ENIs are not included in the calculations for max-pods or kube-reserved. This is most often used in the VPC CNI custom networking setup https://docs.aws.amazon.com/eks/latest/userguide/cni-custom-network.html.") } -func (o *Options) Parse(fs *flag.FlagSet, args ...string) error { +func (o *Options) Parse(fs *coreoptions.FlagSet, args ...string) error { if err := fs.Parse(args); err != nil { if errors.Is(err, flag.ErrHelp) { os.Exit(0) @@ -88,9 +87,6 @@ func (o *Options) Parse(fs *flag.FlagSet, args ...string) error { return fmt.Errorf("validating options, %w", err) } - // Parse options which can't be directly parsed by flag - o.IsolatedVPC = (o.isolatedVPC == "true") - return nil } diff --git a/pkg/operator/options/options_validation.go b/pkg/operator/options/options_validation.go index 976543d12d75..984058d8b8e2 100644 --- a/pkg/operator/options/options_validation.go +++ b/pkg/operator/options/options_validation.go @@ -28,7 +28,6 @@ func (o Options) Validate() error { o.validateVMMemoryOverheadPercent(), o.validateAssumeRoleDuration(), o.validateReservedENIs(), - o.validateIsolateVPC(), ) } @@ -39,13 +38,6 @@ func (o Options) validateAssumeRoleDuration() error { return nil } -func (o Options) validateIsolateVPC() error { - if o.isolatedVPC != "true" && o.isolatedVPC != "false" { - return fmt.Errorf("%q is not a valid value for isolated-vpc, options are true or false", o.isolatedVPC) - } - return nil -} - func (o Options) validateEndpoint() error { if o.ClusterEndpoint == "" { return nil diff --git a/pkg/operator/options/suite_test.go b/pkg/operator/options/suite_test.go index b69131348cc0..267f77f0cbd2 100644 --- a/pkg/operator/options/suite_test.go +++ b/pkg/operator/options/suite_test.go @@ -18,14 +18,19 @@ import ( "context" "flag" "fmt" + "os" "testing" "time" - "github.com/aws/karpenter/pkg/apis/settings" - "github.com/aws/karpenter/pkg/operator/options" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/samber/lo" . "knative.dev/pkg/logging/testing" + + coreoptions "github.com/aws/karpenter-core/pkg/operator/options" + "github.com/aws/karpenter/pkg/apis/settings" + "github.com/aws/karpenter/pkg/operator/options" + "github.com/aws/karpenter/pkg/test" ) var ctx context.Context @@ -37,10 +42,34 @@ func TestAPIs(t *testing.T) { } var _ = Describe("Options", func() { - var fs *flag.FlagSet + var envState map[string]string + var environmentVariables = []string{ + "ASSUME_ROLE_ARN", + "ASSUME_ROLE_DURATION", + "CLUSTER_CA_BUNDLE", + "CLUSTER_NAME", + "CLUSTER_ENDPOINT", + "ISOLATED_VPC", + "VM_MOMORY_OVERHEAD_PERCENT", + "INTERRUPTION_QUEUE_NAME", + "RESERVED_ENIS", + } + + var fs *coreoptions.FlagSet var opts *options.Options + BeforeEach(func() { - fs = flag.NewFlagSet("karpenter", flag.ContinueOnError) + envState = map[string]string{} + for _, ev := range environmentVariables { + val, ok := os.LookupEnv(ev) + if ok { + envState[ev] = val + } + os.Unsetenv(ev) + } + fs = &coreoptions.FlagSet{ + FlagSet: flag.NewFlagSet("karpenter", flag.ContinueOnError), + } opts = &options.Options{} opts.AddFlags(fs) @@ -50,15 +79,90 @@ var _ = Describe("Options", func() { Expect(err).To(BeNil()) }) + AfterEach(func() { + for _, ev := range environmentVariables { + os.Unsetenv(ev) + } + for ev, val := range envState { + os.Setenv(ev, val) + } + }) + Context("Merging", func() { - It("shouldn't overwrite cli flags / env vars", func() { + It("shouldn't overwrite options when all are set", func() { + err := opts.Parse( + fs, + "--assume-role-arn", "options-cluster-role", + "--assume-role-duration", "20m", + "--cluster-ca-bundle", "options-bundle", + "--cluster-name", "options-cluster", + "--cluster-endpoint", "https://options-cluster", + "--isolated-vpc", + "--vm-memory-overhead-percent", "0.1", + "--interruption-queue-name", "options-cluster", + "--reserved-enis", "10", + ) + Expect(err).ToNot(HaveOccurred()) + ctx = settings.ToContext(ctx, &settings.Settings{ + AssumeRoleARN: "settings-cluster-role", + AssumeRoleDuration: time.Minute * 22, + ClusterCABundle: "settings-bundle", + ClusterName: "settings-cluster", + ClusterEndpoint: "https://settings-cluster", + IsolatedVPC: true, + VMMemoryOverheadPercent: 0.05, + InterruptionQueueName: "settings-cluster", + ReservedENIs: 8, + }) + opts.MergeSettings(ctx) + expectOptionsEqual(opts, test.Options(test.OptionsFields{ + AssumeRoleARN: lo.ToPtr("options-cluster-role"), + AssumeRoleDuration: lo.ToPtr(20 * time.Minute), + ClusterCABundle: lo.ToPtr("options-bundle"), + ClusterName: lo.ToPtr("options-cluster"), + ClusterEndpoint: lo.ToPtr("https://options-cluster"), + IsolatedVPC: lo.ToPtr(true), + VMMemoryOverheadPercent: lo.ToPtr[float64](0.1), + InterruptionQueueName: lo.ToPtr("options-cluster"), + ReservedENIs: lo.ToPtr(10), + })) + + }) + It("should overwrite options when none are set", func() { + err := opts.Parse(fs) + Expect(err).ToNot(HaveOccurred()) + ctx = settings.ToContext(ctx, &settings.Settings{ + AssumeRoleARN: "settings-cluster-role", + AssumeRoleDuration: time.Minute * 22, + ClusterCABundle: "settings-bundle", + ClusterName: "settings-cluster", + ClusterEndpoint: "https://settings-cluster", + IsolatedVPC: true, + VMMemoryOverheadPercent: 0.05, + InterruptionQueueName: "settings-cluster", + ReservedENIs: 8, + }) + opts.MergeSettings(ctx) + expectOptionsEqual(opts, test.Options(test.OptionsFields{ + AssumeRoleARN: lo.ToPtr("settings-cluster-role"), + AssumeRoleDuration: lo.ToPtr(22 * time.Minute), + ClusterCABundle: lo.ToPtr("settings-bundle"), + ClusterName: lo.ToPtr("settings-cluster"), + ClusterEndpoint: lo.ToPtr("https://settings-cluster"), + IsolatedVPC: lo.ToPtr(true), + VMMemoryOverheadPercent: lo.ToPtr[float64](0.05), + InterruptionQueueName: lo.ToPtr("settings-cluster"), + ReservedENIs: lo.ToPtr(8), + })) + + }) + It("should correctly merge options and settings when mixed", func() { err := opts.Parse( fs, "--assume-role-arn", "options-cluster-role", "--cluster-ca-bundle", "options-bundle", "--cluster-name", "options-cluster", "--cluster-endpoint", "https://options-cluster", - "--isolated-vpc", "false", "--interruption-queue-name", "options-cluster", ) Expect(err).ToNot(HaveOccurred()) @@ -74,17 +178,46 @@ var _ = Describe("Options", func() { ReservedENIs: 10, }) opts.MergeSettings(ctx) - Expect(opts.AssumeRoleARN).To(Equal("options-cluster-role")) - Expect(opts.AssumeRoleDuration).To(Equal(time.Minute * 20)) - Expect(opts.ClusterCABundle).To(Equal("options-bundle")) - Expect(opts.ClusterName).To(Equal("options-cluster")) - Expect(opts.ClusterEndpoint).To(Equal("https://options-cluster")) - Expect(opts.IsolatedVPC).To(BeFalse()) - Expect(opts.VMMemoryOverheadPercent).To(Equal(0.1)) - Expect(opts.InterruptionQueueName).To(Equal("options-cluster")) - Expect(opts.ReservedENIs).To(Equal(10)) + expectOptionsEqual(opts, test.Options(test.OptionsFields{ + AssumeRoleARN: lo.ToPtr("options-cluster-role"), + AssumeRoleDuration: lo.ToPtr(20 * time.Minute), + ClusterCABundle: lo.ToPtr("options-bundle"), + ClusterName: lo.ToPtr("options-cluster"), + ClusterEndpoint: lo.ToPtr("https://options-cluster"), + IsolatedVPC: lo.ToPtr(true), + VMMemoryOverheadPercent: lo.ToPtr[float64](0.1), + InterruptionQueueName: lo.ToPtr("options-cluster"), + ReservedENIs: lo.ToPtr(10), + })) }) + It("should correctly fallback to env vars when CLI flags aren't set", func() { + os.Setenv("ASSUME_ROLE_ARN", "env-role") + os.Setenv("ASSUME_ROLE_DURATION", "20m") + os.Setenv("CLUSTER_CA_BUNDLE", "env-bundle") + os.Setenv("CLUSTER_NAME", "env-cluster") + os.Setenv("CLUSTER_ENDPOINT", "https://env-cluster") + os.Setenv("ISOLATED_VPC", "true") + os.Setenv("VM_MEMORY_OVERHEAD_PERCENT", "0.1") + os.Setenv("INTERRUPTION_QUEUE_NAME", "env-cluster") + os.Setenv("RESERVED_ENIS", "10") + fs = &coreoptions.FlagSet{ + FlagSet: flag.NewFlagSet("karpenter", flag.ContinueOnError), + } + opts.AddFlags(fs) + opts.Parse(fs) + expectOptionsEqual(opts, test.Options(test.OptionsFields{ + AssumeRoleARN: lo.ToPtr("env-role"), + AssumeRoleDuration: lo.ToPtr(20 * time.Minute), + ClusterCABundle: lo.ToPtr("env-bundle"), + ClusterName: lo.ToPtr("env-cluster"), + ClusterEndpoint: lo.ToPtr("https://env-cluster"), + IsolatedVPC: lo.ToPtr(true), + VMMemoryOverheadPercent: lo.ToPtr[float64](0.1), + InterruptionQueueName: lo.ToPtr("env-cluster"), + ReservedENIs: lo.ToPtr(10), + })) + }) }) Context("Validation", func() { @@ -96,7 +229,7 @@ var _ = Describe("Options", func() { Expect(func() { opts.MergeSettings(ctx) fmt.Printf("%#v", opts) - }).To(Panic()) + }).To(Panic()) }) It("should fail when assume role duration is less than 15 minutes", func() { err := opts.Parse(fs, "--assume-role-duration", "1s") @@ -116,3 +249,16 @@ var _ = Describe("Options", func() { }) }) }) + +func expectOptionsEqual(optsA *options.Options, optsB *options.Options) { + GinkgoHelper() + Expect(optsA.AssumeRoleARN).To(Equal(optsB.AssumeRoleARN)) + Expect(optsA.AssumeRoleDuration).To(Equal(optsB.AssumeRoleDuration)) + Expect(optsA.ClusterCABundle).To(Equal(optsB.ClusterCABundle)) + Expect(optsA.ClusterName).To(Equal(optsB.ClusterName)) + Expect(optsA.ClusterEndpoint).To(Equal(optsB.ClusterEndpoint)) + Expect(optsA.IsolatedVPC).To(Equal(optsB.IsolatedVPC)) + Expect(optsA.VMMemoryOverheadPercent).To(Equal(optsB.VMMemoryOverheadPercent)) + Expect(optsA.InterruptionQueueName).To(Equal(optsB.InterruptionQueueName)) + Expect(optsA.ReservedENIs).To(Equal(optsB.ReservedENIs)) +}