Discuss defaulting behavior

[ellistarn]

Tell us about your request

I created a simple provisioner:

  spec:
    provider:
      securityGroupSelector:
        kubernetes.io/cluster/etarn: '*'
      subnetSelector:
        kubernetes.io/cluster/etarn: '*'
    ttlSecondsAfterEmpty: 360

Here's what I got:

  spec:
    kubeletConfiguration: {}
    provider:
      amiFamily: AL2
      apiVersion: extensions.karpenter.sh/v1alpha1
      kind: AWS
      securityGroupSelector:
        kubernetes.io/cluster/etarn: '*'
      subnetSelector:
        kubernetes.io/cluster/etarn: '*'
    requirements:
    - key: kubernetes.io/arch
      operator: In
      values:
      - amd64
    - key: karpenter.sh/capacity-type
      operator: In
      values:
      - on-demand
    ttlSecondsAfterEmpty: 360

Here's what I'm not sure about:

• amiFamily: AL2should probably default in memory, rather than defaulting into the spec. This allows the user to be unaware of Karpenter's default, should it change over time. By not specifying AMIFamily, I'm really saying that I don't care.
• kubeletConfiguration: {} should be omitted, since we use omitempty. It's not clear why this is persisted.
• apiVersion: extensions.karpenter.sh/v1alpha1, kind: AWS. This feels really crufty. It would be great if we modeled this with a separate vendor specific CRD. This is a breaking change and a good candidate for v1beta1 Laundry List #1327

Attachments
If you think you might have additional information that you'd like to include via an attachment, please do - we'll take a look. (Remember to remove any personally-identifiable information.)

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

[njtran]

amiFamily: AL2should probably default in memory, rather than defaulting into the spec. This allows the user to be unaware of Karpenter's default, should it change over time. By not specifying AMIFamily, I'm really saying that I don't care.

I like having this in Provisioner spec and not memory so that it's a more deterministic behavior and more easily accessible. I could see a use-case for not wanting to have this in the spec for configuration bloat, but since it's a defaulted option, I don't think this puts extra strain on initial configuration. Additionally, if the user wants, this would make it more apparent that they can configure this. WDYT?

[ellistarn]

There's a difference in semantic. In one case, the user says "I don't care", and Karpenter defaults to something forever. In the other case, the user days "I don't care", and Karpenter defaults to something that can vary with the install version. We're faced with the challenge of picking the best semantic for the situation. I'm not sure I'm convinced either way yet.

I agree that there are definitely side effects of "discoverability", but I think that should be a cli/ui decision, and not impact the semantic mentioned above.

[stevehipwell]

@ellistarn I think that as a consumer if I don't specify something I want Karpenter to make the decision for me, and I want Karpenter to keep doing this; I don't want Karpenter stuck using what seemed like the right choice when the resource was created.

There are techniques in K8s that would allow both dynamicly changing values and the current version to be in the resource (field ownership) but it would add overhead and IMHO make it much harder to understand (you'd need to set kubectl args and cross reference fields when viewing).

[ellistarn]

Ack! We've also been preferring in-memory defaults, which achieves what you're talking about.

[stevehipwell]

Ack! We've also been preferring in-memory defaults, which achieves what you're talking about.

And is idiomatic k8s behaviour.

[ellistarn]

Thanks @bwagner5 for #1497

[tzneal]

I recently discovered that if you use terraform to create the Karpenter provisioner, you need to provide all of our default values to terraform or it will fail to apply cleanly:

╷
│ Error: Provider produced inconsistent result after apply
│
│ When applying changes to kubernetes_manifest.provisioner_default, provider "provider[\"registry.terraform.io/hashicorp/kubernetes\"]" produced an unexpected new value:
│ .object.spec.requirements: new element 1 has appeared.
│
│ This is a bug in the provider, which should be reported in the provider's own issue tracker.

This particular error is because of the architecture defaulting, but a similar error occurs due to the kind and apiVersion on the provider.

[stevehipwell]

@tzneal how did you add the provisioner with TF?

[tzneal]

I put up a PR documenting it at: #1551

Also, I never used TF before this morning, so I would appreciate anyone that's used it before reviewing.

[stevehipwell]

What versions of TF, TF K8s provider and K8s are you using?

[tzneal]

TF: Terraform v1.1.7

module "eks" {
  source          = "terraform-aws-modules/eks/aws"
  version         = "<18"

  cluster_version = "1.21"

I just went through the "getting started with terraform" docs on karpenter.sh and added in the provisioner at the end.

[stevehipwell]

@tzneal have you followed the docs for computed fields? This is expected behaviour.

That said I'd personally try the TF kubectl provider with server_side_apply = true. I think it's coded to respect field management so ignore fields managed by another manager and if not I don't think it' checks the resultant state after apply. I do a significant amount of Terraform against Kubernetes and I won't use the official kubernetes_manifest due to it's significant limitations, the kubectl provider isn't perfect but it's much more predictable.

[tzneal]

Maybe I was using computed_fields incorrectly, but it didn't appear to work unless I provided some default value in my specification. I interpreted this from the docs: When an field is defined as "computed" Terraform will allow the final value stored in state after apply as returned by the API to be different than what the user requested. to mean that modified values are ok, but new values are not.

I got started down this path due to a question in Slack from someone trying to use kubernetes_manifest.

[stevehipwell]

Did you try adding all the dynamic fields (modified & created) to computed_fields?

[tzneal]

Moving discussion to the #1551