Add Addon resource to EKS provider #1153
Conversation
rquitales
force-pushed
the
rquitales/managed-addons
branch
from
588b2e1
to
eb094f9
Compare
rquitales
force-pushed
the
rquitales/managed-addons
branch
from
eb094f9
to
5334057
Compare
rquitales
force-pushed
the
rquitales/managed-addons
branch
from
5334057
to
036f1cc
Compare
rquitales
force-pushed
the
rquitales/managed-addons
branch
from
036f1cc
to
5238e1a
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. New resources:
|
|
Tests are failing due to orphaned AWS resources. Will need to run the cleanup script before re-running the test suite. |
provider/cmd/pulumi-gen-eks/main.go
Outdated
| }, | ||
| "configurationValues": { | ||
| TypeSpec: schema.TypeSpec{Type: "string"}, | ||
| Description: "Custom configuration values for addons with single JSON string. This JSON string value must match the JSON schema derived from describe-addon-configuration.", |
There was a problem hiding this comment.
Why don’t we change this to a map and serialize it for users?
There was a problem hiding this comment.
Updated this to be a map and added additional integration tests for this.
| constructor(name: string, args: AddonOptions, opts?: pulumi.CustomResourceOptions) { | ||
| const cluster = args.cluster; | ||
|
|
||
| super("eks:index:Addon", name, args, { |
There was a problem hiding this comment.
What’s the reason for making addons their own component vs making that an input to the eks cluster component?
This one is mostly wrapping the aws classic addon resource right now. Including it in the EKS component would make discoverability easier
There was a problem hiding this comment.
I thought some more about this and actually prefer this approach. The cluster resource is already quite overloaded with parameters and the addon is very self-contained.
rquitales
force-pushed
the
rquitales/managed-addons
branch
from
5238e1a
to
799fb41
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. New resources:
|
|
Quick question about these addons. We have some related code in pulumi-aws: pulumi/pulumi-aws#3009 Does this change look good in light of this code? Does it interact with defaultAddonsToRemove or should it? Thanks. |
| name, | ||
| { | ||
| ...args, | ||
| clusterName: cluster.core.cluster.name, |
There was a problem hiding this comment.
So, question, genuinely curious what's the benefit of wrapping aws.eks.Addon in a component Addon?
Can users use a vanilla aws.eks.Addon right now already without this helper?
Perhaps we have some more functionality we expect to land in this wrapper?
Is this automatically setting parent? Or removing deprecated properties? Or is it automatic JSON.stringify?
Can we bake any of this directly in AWS Addon or a bad idea?
There was a problem hiding this comment.
The main reason to expose aws.eks.Addon within this EKS provider is for visibility that users can use AWS managed addons with EKS. This also means that users won't need to have fragmented code that uses resources from both the AWS or EKS providers. Initially, the ask was for us to provide wrappers over the CoreDNS and kube-proxy addons. We decided against that since it would be a maintenance burden to keep up with any schema changes that newer versions of these addons might incur, while also maintaining support of the older addon versions.
This PR already introduces some additional functionality, such as automatic JSON.stringify as well.
| clusterName: cluster.core.cluster.name, | ||
| configurationValues: JSON.stringify(args.configurationValues), | ||
| }, | ||
| { parent: this, provider: opts?.provider }, |
There was a problem hiding this comment.
What if we have provders: {"aws": awsProv}? I think actually this might work already and provider:* might be redundant as it will inherit via parent: this, we were investigating resetting AWS versions underneath the awsx provider and https://github.com/pulumi/pulumi-awsx?tab=readme-ov-file#custom-aws-provider-versions it works fine while all the provider does is setting {parent: this}.
There was a problem hiding this comment.
Good to know! I was mostly following the existing pattern we have for other resources that we wrap. Since the outcome is the same, I'll keep it as is for now to maintain consistency in the codebase.
There was a problem hiding this comment.
The PR Looks great, thank you, learned a few TypeScript tricks here.
One higher level question I have is why we decided to wrap aws.eks.Addon in the first place, perhaps we can document internally for posterity, it looks like some thought was put into it though. I'd just appreciate reading into it more to inform future decisions of this sort.
rquitales
force-pushed
the
rquitales/managed-addons
branch
from
bfece1b
to
ce9f701
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. New resources:
|
Proposed Changes
In late 2020, AWS introduced EKS managed addons, which allow AWS to handle the management of cluster addons as part of the EKS lifecycle. This pull request integrates the upstream
aws.eks.addonresource into our EKS provider, enhancing visibility and management of this resource.By incorporating this resource, users will be able to manage addons such as coredns and kube-proxy, along with other supported addons. We chose not to create specific Pulumi resources for each addon, as doing so and exposing schema configurations as resource properties could be fragile with future schema changes. Additionally, user requests to create CoreDNS and kube-proxy resources were proposed before AWS managed addons were available, when self-management was necessary to keep these components up to date. VpcCni is another example of an addon that we also intend to transition to AWS management.
Testing:
corednsas an addonRelated issues (optional)
Closes: #587
Closes: #253