Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Include all Provider fields in the AWSNodeTemplate CRD #1951

Merged
merged 11 commits into from Jun 20, 2022

Conversation

suket22
Copy link
Contributor

@suket22 suket22 commented Jun 16, 2022

Description
As part of releasing custom UserData support, this moves all of the provider fields into ProviderRef as well. This should allow everyone to specify everything related to the AWS provider within the AWSNodeTemplate CRD.

Made a couple more changes while I was on this -

  • Got rid of ProviderRefNamespace - dunno why I did this given the CRD was cluster scoped anyway.
  • Fixed the CRD. I had set paths to awsnodetemplate which is the annotation kubebuilder is looking for to determine the plural name of the CRD. Anyone running a pre-release version of Karpenter should be unaffected as long as they update the charts. (new CRD works, and old provisioner yaml is fine).

How was this change tested?
Tested a few things on this front -

  • Made sure that the validation webhook is working on the Provider fields inside the AWSNodeTemplate CRD.
  • Made sure you can't specify provider and providerRef.
  • Validated that old behavior works (spinning up AL2 and Bottlerocket nodes)
  • Validated that new behavior works (spinning up AL2 and Bottlerocket nodes with UserData using the new CRD).

Example Manifest to try this change out -

apiVersion: karpenter.sh/v1alpha5
kind: Provisioner
metadata:
  name: default
spec:
  limits:
    resources:
      cpu: 20
  providerRef:
    name: default
  ttlSecondsAfterEmpty: 30
---
apiVersion: karpenter.k8s.aws/v1alpha1
kind: AWSNodeTemplate
metadata:
  name: default
spec:
  amiFamily: Bottlerocket
  instanceProfile: KarpenterNodeInstanceProfile-karp-cluster
  subnetSelector:
    karpenter.sh/discovery: karp-cluster
  securityGroupSelector:
    karpenter.sh/discovery: karp-cluster
  userData:  |
    [settings.kubernetes]
    kube-api-qps = 30

Does this change impact docs?
Docs will be added in a subsequent PR since this change is large enough as-is.
I'll need to add -

  1. Some examples on CRD and Provisioner Usage.
  2. Undo the changes that hide the user data documentation.
  3. Additions to the upgrade guide.

With that, a separate PR makes sense.

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

Release Note

feat: Include all Provider fields in the AWSNodeTemplate CRD

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@netlify
Copy link

netlify bot commented Jun 16, 2022

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 0d29ea8
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/62b0a6d17519890008321e9d

@suket22 suket22 marked this pull request as ready for review June 16, 2022 23:25
@suket22 suket22 requested a review from a team as a code owner June 16, 2022 23:25
@suket22 suket22 requested a review from bwagner5 June 16, 2022 23:25
pkg/cloudprovider/aws/cloudprovider.go Outdated Show resolved Hide resolved
pkg/cloudprovider/aws/cloudprovider.go Outdated Show resolved Hide resolved
pkg/cloudprovider/aws/cloudprovider.go Outdated Show resolved Hide resolved
pkg/cloudprovider/aws/cloudprovider.go Outdated Show resolved Hide resolved
pkg/cloudprovider/aws/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/aws/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/aws/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/aws/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/aws/cloudprovider.go Outdated Show resolved Hide resolved
pkg/test/environment.go Outdated Show resolved Hide resolved
suket22 and others added 3 commits June 20, 2022 09:27
Co-authored-by: Ellis Tarn <ellistarn@gmail.com>
Co-authored-by: Ellis Tarn <ellistarn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants