refactor: rework options flow to have a more consistent pattern #390

comtalyst · 2024-06-07T18:02:29Z

Fixes #

Description

How was this change tested?

Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: #
No

Release Note

… middle layers to eliminate confusion

comtalyst · 2024-06-07T18:16:27Z

pkg/operator/operator.go

 	)
 	imageResolver := imagefamily.New(
 		operator.GetClient(),
 		imageProvider,
 	)
 	launchTemplateProvider := launchtemplate.NewProvider(
 		ctx,
+		opts,


This should be the highlight worth discussing.

Prior to this, in addition to passing the options (and azConfig) directly that you can see in the changes preview, some of the providers did call options.FromContext(ctx) again. I find this inconsistent and would prefer to stick to one side—either pass all required parameters here, or just pass in ctx/opts.

The current code chooses passing in opts. This will make the providers depends on the knowledge of options (no change), while sparing operator from having to know which provider needs what. This is particularly helpful in launchTemplateProvider, which is have a lot of demands for fields in options.

With that said, each provider have different demands in options fields, but ideally I want to keep the dependencies/pattern consistent for all providers.

Alternatively, we could stay explicit on the interface and pass all variables that we currently getting from options.FromContext(ctx) through here instead. The interface will be be big for high-demanding providers, but we will be able to cut their dependence on options.

Another alternative is to add some layering in options to categorize them by uses, appearing at "sub-structs".
But this will force options to contain knowledge about the exact uses of their fields, which is going to add additional complexity, given some fields are shared with multiple uses.

An extension to that is to have additional layer between options (which will mostly take envs in raw) and this layer to map those to its provider uses. But that would be no difference that just creating structs here, which will only be better than explicit passing by extensibility and small reusability, at the cost of additional complexity.

But all these options are not really making use of options.FromContext(ctx) pattern established by karpenter-core and AWS provider. If we want to keep that, another options is to just pass in ctx to all places and let options.FromContext(ctx) retrieve the options whenever it is needed. This will assume that all instances here are singletons or at least have no differences between each, which is true(?), for now.

If you ask me, I just don't think it is a better option than others. Opinions in offline discussions I had seems to be against this pattern.
Although it is probably the best in terms of consistency.

comtalyst · 2024-06-07T18:37:26Z

pkg/operator/options/options.go

-	ClusterEndpoint                string // => APIServerName in bootstrap, except needs to be w/o https/port
-	VMMemoryOverheadPercent        float64
-	ClusterID                      string
+	// Target cluster information; might be use for both bootstrapping and ARM authentications


The idea behind this is I try to categorize this by user-facing behavior (e.g., target cluster for the operations to occur, what will be on provisioned nodes) rather than implementation details.

Some should be renamed. But let's put that in a different PR

comtalyst · 2024-06-07T18:39:19Z

pkg/operator/options/options_defaulting.go

+func (o *Options) Default(ctx context.Context) error {
+	var err error
+
+	var authManager *auth.AuthManager


This is a flaw in the implementation, as authManager is created in both here and operator. Given core, there is nothing much we can do(?) if we want to follow this pattern.

comtalyst · 2024-06-07T18:40:57Z

pkg/operator/options/options.go

+	ClusterName     string
+	ClusterEndpoint string // => APIServerName in bootstrap, except needs to be w/o https/port
+	ClusterID       string
+	APIServerName   string


This and VnetGUID (that I just added) are the options that are not available to be configurable directly by the customer, but directly depends on other options through defaulting. Open to suggestions if this should be elsewhere.

comtalyst · 2024-06-07T18:43:16Z

pkg/controllers/nodeclaim/inplaceupdate/controller.go

@@ -44,13 +44,17 @@ import (
 type Controller struct {
 	kubeClient       client.Client
 	instanceProvider *instance.Provider
+
+	// Will be used to calculate the goal state
+	opts *options.Options


Another instance of "not sure if options is overkill", see the comments in operator.go for similar discussions. But if you ask me I think this is appropriate given the nature of the controllers.

charliedmcb

Just did a quick scan. I like the direction of streamlining options. However, would recommend that we keep the naming generic since the flow doesn't necessarily have to just be used for workload identities.

charliedmcb · 2024-06-07T20:28:17Z

karpenter-values-template.yaml

      value: ""
    - name: AZURE_NODE_RESOURCE_GROUP
      value: ${AZURE_RESOURCE_GROUP_MC}
+    - name: ARM_AUTH_METHOD
+      value: "workload-identity"


nit: the reason its called "from environment" is because its actually using a more generic/general auth option that finds the creds in the environment. This is the flow that works for workload-identity. However, I think the naming should probably keep the more generic term since that is what its actually doing. Unless you have more tied to this.

So, by theory, there could be "from environment" auth option that is not workload identity, right?
If that's the case then I think keeping "from-environment" would be a better choice, given we doesn't seem to have ties to workload identity (right?).

pkg/auth/authmanager.go

Bryce-Soghigian · 2024-06-10T23:46:18Z

pkg/auth/authmanager.go

+// NewCredential provides a token credential
+func (am AuthManager) NewCredential() (azcore.TokenCredential, error) {
+	if am.authMethod == AuthMethodWorkloadIdentity {
+		klog.V(2).Infoln("cred: using workload identity for new credential")
+		return azidentity.NewDefaultAzureCredential(nil)
+	}
+
+	if am.authMethod == AuthMethodSysMSI {
+		klog.V(2).Infoln("cred: using system assigned MSI for new credential")
+		msiCred, err := azidentity.NewManagedIdentityCredential(nil)
+		if err != nil {
+			return nil, err
+		}
+		return msiCred, nil
+	}
+
+	return nil, fmt.Errorf("cred: unsupported auth method: %s", am.authMethod)
+}


Does this belong in the shared SDK? For my token fix i imagine other ppl may want the same fix. Uing azure sdk for go extensions may be a good idea for our auth so we can share that logic too. Should we move this there?

Bryce-Soghigian · 2024-06-10T23:47:49Z

pkg/controllers/controllers.go

 	controllers := []controller.Controller{
 		nodeclaimgarbagecollection.NewController(kubeClient, cloudProvider),
-		inplaceupdate.NewController(kubeClient, instanceProvider),
+		inplaceupdate.NewController(kubeClient, instanceProvider, opts),


why not just options.FromContext(ctx)? or for that matter why not just pass in ctx so that future updates to the controller can access everything from the context object? Are we refactoring options out of ctx?

(discussed offline)
Although both share the same principle, options.FromContext(ctx) seems to be the pattern that is generally discouraged by all opinions we have discussed with, except karpenter-core.
For my own reasoning, this would allow us to separate options from ctx, which might wanted to be used for other purposes. We could still argue that options still contain things that might not be used, but it still allow extensibility and hide the knowledge of "what this component requires" from the creation interface. So I would say it is something in between ctx and explicitly passing parameters.

Anyway, it is not a strong opinion.

Bryce-Soghigian · 2024-06-10T23:52:20Z

pkg/operator/options/options_defaulting.go

+)
+
+// Default sets the default values for the options, but only for those too complicated to be in env default (e.g., depends on other envs)
+func (o *Options) Default(ctx context.Context) error {


If we are going to go this far, can't we just resolve a lot of the configuration from having clustername and rg?

Why make users pass in things like "NetworkDataplane" or "NetworkPluginMode"? Why not just use the AKS SDK to dynamically resolve that configuration for them?

For Self hosted at least that might make things easier, if the values aren't updated consistently between the two models on restart for managed that might be a problem.

Not a bad idea. Now that we establish this framework, we could consider simplifying the integration interface and get obvious fields by ourselves. A trade-off for this is Karpenter will have more responsibility of "getting required info of a cluster" which could at least lead to having more traffic at the initialization.

I think the changes for this, if we decided to, should be in a separate PR at least. But we can start discussing here if we have strong ideas.

Bryce-Soghigian · 2024-06-10T23:53:51Z

pkg/operator/options/options_defaulting.go

+	}
+
+	if o.ClusterID, err = getAKSClusterID(o.APIServerName); err != nil {
+		return fmt.Errorf("failed to get ClusterID: %w", err)


should we make these errors typed? Makes this part easier to test, rachel left this feedback in another of my prs but you touch this part a lot more so lmk what you think about changing all of these to typed errors

I think it would be a nice to have, but not a necessity at the moment.

Bryce-Soghigian · 2024-06-10T23:54:37Z

pkg/operator/options/options_defaulting.go

+// getAKSClusterID returns cluster ID based on the DNS prefix of the cluster.
+// The logic comes from AgentBaker and other places, originally from aks-engine
+// with the additional assumption of DNS prefix being the first 33 chars of FQDN
+func getAKSClusterID(apiServerFQDN string) (string, error) {
+	dnsPrefix := apiServerFQDN[:33]
+	h := fnv.New64a()
+	h.Write([]byte(dnsPrefix))
+	r := rand.New(rand.NewSource(int64(h.Sum64()))) //nolint:gosec
+	return fmt.Sprintf("%08d", r.Uint32())[:8], nil
+}
+


General question, does this belong in agentbaker? We stole it from agentbaker will self contained have this in the pkg/agent/common or wahtever we can just import rather than redefining it?

Good point. I will dive deeper into this along with the contract interface rework. Hate

Bryce-Soghigian · 2024-06-10T23:55:59Z

pkg/operator/options/options_defaulting.go

+func contains(slice []string, target string) bool {
+	for _, element := range slice {
+		if target == element {
+			return true
+		}
+	}
+	return false


kubernetes helpers for slices should have this already

Bryce-Soghigian · 2024-06-10T23:56:58Z

pkg/operator/options/options_validation.go

 	}
+
+	if o.NodeResourceGroup == "" {
+		return fmt.Errorf("missing field, node-resource-group")


same comment about shared error type. we really only care that

its missing

what is missing

Bryce-Soghigian · 2024-06-10T23:59:08Z

pkg/providers/imagefamily/bootstrap/aksbootstrap.go

@@ -40,7 +40,7 @@ type AKS struct {
 	Arch                           string
 	TenantID                       string
 	SubscriptionID                 string
-	UserAssignedIdentityID         string
+	KubeletIdentityClientID        string


great name change!

Do we worry about these values being different from those in azure json? So people may grep for UserAssignedIdentityID since its called that everywhere else?

(discussed offline)

It is better this way given that the UserAssignedIdentityID that is known by azure.json is in fact KubeletIdentityClientID that is known by the cluster. This confusion is the prime motivation for this entire work.

Currently, I choose to keep this variable name until we feed it to the bootstrap module. There could be even more opportunities around that area to make it even more clear...

Now that we have this knowledge, we should be aware when coming across their uses in those "everywhere else." Maybe we can catch some big bugs.

Bryce-Soghigian · 2024-06-11T00:00:52Z

pkg/providers/imagefamily/image.go

 	return &Provider{
 		kubernetesVersionCache: kubernetesVersionCache,
 		imageCache:             cache.New(imageExpirationInterval, imageCacheCleaningInterval),
-		location:               location,
+		location:               opts.Location,


why not just put options in the Provider struct itself and we can access location from that? I also will need some values from options in some of these providers in this pr https://github.com/Azure/karpenter-provider-azure/pull/365/files so that pattern would make access a bit easier.

(discussed offline)

I am still hesitate to make these providers depends on options entirely.
Right now it does module-wise, but not the struct, as the only reference to options is on creation function (NewProvider). Maybe the better name to this is NewProviderFromOptions.

But again, not a strong opinion. Wondering what do others think? Should we be more extreme on one side (e.g., using opts/ctx everywhere)?

Bryce-Soghigian · 2024-06-11T00:02:15Z

pkg/providers/instancetype/instancetype.go

 	memory := resources.Quantity(fmt.Sprintf("%dGi", int64(memoryGiB(sku))))
 	// Account for VM overhead in calculation
 	memory.Sub(resource.MustParse(fmt.Sprintf("%dMi", int64(math.Ceil(
-		float64(memory.Value())*options.FromContext(ctx).VMMemoryOverheadPercent/1024/1024)))))


does this come from core?

Not core but AWS provider: https://github.com/aws/karpenter-provider-aws/blob/531d7e00ad69bed8fa74de79abdaf8d61b476910/pkg/providers/instancetype/types.go#L224.

But, yes, this will introduce a difference between the two.
In general, I think our changes it will be for the better (at least, intention-wise). But the trade-off is consistency.

In this case, I am not completely sure, but leaning towards keeping current change.
Current changes mean consistency with this pattern within our provider (e.g., providers).
Restoring to original means consistency with the AWS provider. But regardless whether we keep other changes or restore all, inconsistency remains within our provider, unless we become more extreme with the use of ctx.

Bryce-Soghigian · 2024-06-11T00:03:44Z

pkg/providers/launchtemplate/launchtemplate.go

-	// TODO(bsoghigian): this should be refactored to lo.Ternary(nodeClass.Spec.VnetSubnetID != nil, lo.FromPtr(nodeClass.Spec.VnetSubnetID), os.Getenv("AZURE_SUBNET_ID")) when we add VnetSubnetID to the nodeclass
-	vnetSubnetComponents, err := utils.GetVnetSubnetIDComponents(options.FromContext(ctx).SubnetID)
+func (p *Provider) getVnetInfoLabels(_ *v1alpha2.AKSNodeClass) (map[string]string, error) {
+	// TODO(bsoghigian): this should be refactored to lo.Ternary(nodeClass.Spec.SubnetID != nil, lo.FromPtr(nodeClass.Spec.SubnetID), p.opts.SubnetID) when we add VnetSubnetID to the nodeclass


are you sure we should differ from the AKS Agentpool api in what we call SubnetID? We discussed this somewhere on the other PR and i thought we agreed on calling it VnetSubnetID

I named it SubnetID from the start but got feedback we should change it to be VNETSubnetID

I think this change is a mistake from mass text replace. My bad.

But there will be similar changes (not in this PR) that I intend to rename all SubnetID to VnetSubnetID. We can discuss more later along with the rest of the renames.

pkg/providers/loadbalancer/loadbalancer.go

Bryce-Soghigian · 2024-06-11T00:04:59Z

pkg/providers/pricing/pricing.go

@@ -64,7 +65,9 @@ func NewAPI() client.PricingAPI {
 	return client.New()
 }

-func NewProvider(ctx context.Context, pricing client.PricingAPI, region string, startAsync <-chan struct{}) *Provider {
+func NewProvider(ctx context.Context, opts *options.Options, pricing client.PricingAPI, startAsync <-chan struct{}) *Provider {
+	region := opts.Location


why not just use opts.Location in the map lookup?

It is being used in other places (at least line 76/79) as well. But I don't think any choice matters.

comtalyst added 7 commits June 6, 2024 17:06

refactor: rework aged authentication config flow

c4dfa88

refactor: rename userAssignedIdentityID to kubeletIdentityClientID in…

a914946

… middle layers to eliminate confusion

chore: not supporting legacy envs

69f83e2

chore: rename "credential-from-environment" to "workload-identity"

133ffec

chore: make presubmit

70c2e8e

refactor: rename AuthMethod to ArmAuthMethod for clarity

c3615b8

refactor: rework options/env flow

3a18375

comtalyst commented Jun 7, 2024

View reviewed changes

charliedmcb reviewed Jun 7, 2024

View reviewed changes

comtalyst added 2 commits June 7, 2024 13:41

fix: incorrect if

4b4b3c4

chore: move node identities defaulting away for now

02a046c

Bryce-Soghigian reviewed Jun 10, 2024

View reviewed changes

Bryce-Soghigian reviewed Jun 11, 2024

View reviewed changes

pkg/providers/loadbalancer/loadbalancer.go Show resolved Hide resolved

Bryce-Soghigian reviewed Jun 11, 2024

View reviewed changes

refactor: rework options flow to have a more consistent pattern #390

Are you sure you want to change the base?

refactor: rework options flow to have a more consistent pattern #390

Conversation

comtalyst commented Jun 7, 2024

comtalyst Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comtalyst Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comtalyst Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

charliedmcb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bryce-Soghigian Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comtalyst Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comtalyst Jun 7, 2024 •

edited

Loading

comtalyst Jun 7, 2024 •

edited

Loading

comtalyst Jun 7, 2024 •

edited

Loading

Bryce-Soghigian Jun 10, 2024 •

edited

Loading

comtalyst Jun 11, 2024 •

edited

Loading