Added libs for eksctl #131
Conversation
WalkthroughAdds three new Jsonnet libraries under eks/: a fluent ClusterConfig builder, a default EKS cluster configuration object with validations and presets, and an eksctl configuration/manifest generator that composes managed/unmanaged node groups and renders YAML. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
eks/eksctl.libsonnet (1)
67-73: Same bug here: conditional ‘subnets’ keyReplicate the conditional‑object fix from managed groups to unmanaged groups.
Apply the same diff pattern as above for nodeGroups.
🧹 Nitpick comments (7)
eks/clusterConfig.libsonnet (2)
29-39: Make addon configurationValues optional and render as YAML for readabilityCurrent code assumes configurationValues exists and is an object. If missing or already a string, this errors or double‑quotes JSON. Suggest guarding and pruning nulls; also render YAML for easier diffing.
Apply:
addons: { withAddons(addons):: { addons: [ - addons[a] { - name: a, - configurationValues: std.toString(super.configurationValues), - } + local cv = if std.objectHas(addons[a], 'configurationValues') then addons[a].configurationValues else null; + std.prune(addons[a] { + name: a, + configurationValues: + if cv == null then null + else if std.type(cv) == 'string' then cv + else std.manifestYamlDoc(cv, indent_array_in_object=true, quote_keys=false), + }) for a in std.objectFields(addons) ], }, },FYI eksctl accepts configurationValues as YAML/JSON string; resolveConflicts is the correct field name. (eksctl.io)
22-27: Only include sharedNodeSecurityGroup if non‑nullwithSharedNodeSecurityGroup(null) would emit an explicit null in output if upstream sets it to null. Gate on value, not field presence.
Example where used in eksctl.libsonnet (see further comment) should also check for non‑null. (docs.aws.amazon.com)
eks/defaultConfig.libsonnet (2)
146-151: AZ vs subnets toggles: clarify usage in READMEGood defaults (hidden subnets, visible AZs). Add a short README snippet so consumers know to unhide subnets when they pass explicit subnet IDs.
150-152: propagateASGTags on managed nodegroups may not behave as expectedFor MNGs, tag propagation semantics differ from unmanaged; autoscaler scale‑from‑zero often relies on CAS permissions instead. Keep, but manage expectations.
Consider documenting CAS permission approach (eks:DescribeNodegroup) if you rely on labels/taints. (master--eksctl.netlify.app)eks/eksctl.libsonnet (3)
114-125: Only include sharedNodeSecurityGroup when non‑nullobjectHas returns true for hidden fields too; passing null would emit null in output. Check for non‑null.
Apply:
- if std.objectHas(c.vpc, 'sharedNodeSecurityGroup') then + if std.objectHas(c.vpc, 'sharedNodeSecurityGroup') && c.vpc.sharedNodeSecurityGroup != null then clusterConfig.vpc.withSharedNodeSecurityGroup(c.vpc.sharedNodeSecurityGroup) else {}Reference for field: sharedNodeSecurityGroup is valid. (docs.aws.amazon.com)
84-97: Name strategy: hard‑coded single‑AZ set looks intentional; consider config switchTreating management/data/frontend/stable as single‑AZ and others as multi‑AZ is opinionated. If you expect other names, consider a toggle per group (e.g., singleAZ: true) instead of name matching.
30-48: Single‑AZ builder assumes subnets map has AZ keysIf subnets map is missing an AZ key, this will fail. Optional: guard with a clearer error or validate earlier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eks/clusterConfig.libsonnet(1 hunks)eks/defaultConfig.libsonnet(1 hunks)eks/eksctl.libsonnet(1 hunks)
🔇 Additional comments (7)
eks/clusterConfig.libsonnet (1)
1-5: Solid base object for eksctl ClusterConfigapiVersion/kind defaults look correct for eksctl.
eks/defaultConfig.libsonnet (5)
4-8: Good required top‑level validationsHard‑errors on name/region/version/accountId are helpful to fail fast.
66-81: IRSA serviceAccount wiring looks fineRole name/policy ARNs derived from cluster name/accountId are consistent. Ensure cluster OIDC is enabled (it is, below).
(eksctl.io)
208-251: Unmanaged nodegroup defaults look saneSpot settings via instancesDistribution and AZ/subnets wiring align with eksctl docs.
(eksctl.io)
94-152: Approve — overrideBootstrapCommand NodeConfig valid with AL2023/nodeadm
eksctl accepts a NodeConfig via managedNodeGroup.overrideBootstrapCommand and, on AL2023, nodeadm merges it with the default NodeConfig; maxPods=110 for IP prefix-delegation is appropriate. Older custom-AMI guidance is superseded by the nodeadm flow.
20-34: Addons defaults align with eksctl — keep resolveConflictsFile: eks/defaultConfig.libsonnet (lines 20–34)
Verified: eksctl docs show configurationValues (YAML block or JSON string) used together with resolveConflicts: overwrite; current code matches the examples.
eks/eksctl.libsonnet (1)
5-8: Checksum stability — std.toString(config) is order-sensitive; verify or make deterministicFile: eks/eksctl.libsonnet Lines: 5-8
The jq test produced different md5s for {a:1,b:2} vs {b:2,a:1}, confirming key-order affects the checksum. If this hash is used for resource names, either make serialization deterministic (sort object fields or use a stable JSON/manifest serializer) or accept the risk of spurious renames.
| [if std.objectHas(config.managedNodeGroups[name], 'subnets') then 'subnets' else null]: | ||
| if std.type(config.managedNodeGroups[name].subnets) == 'object' then | ||
| // Transform a availabilityZone -> subnetId mapping into a list of subnets if field `subnets` is unhidden | ||
| [super.subnets[az].id for az in std.objectFields(super.subnets)] | ||
| else | ||
| config.managedNodeGroups[name].subnets, | ||
|
|
There was a problem hiding this comment.
Bug: computed field name may be null (invalid Jsonnet object key)
Using [if cond then 'subnets' else null]: ... will evaluate to null in the else branch, which is not a valid field name and will error at runtime. Use conditional objects instead.
Apply:
- [if std.objectHas(config.managedNodeGroups[name], 'subnets') then 'subnets' else null]:
- if std.type(config.managedNodeGroups[name].subnets) == 'object' then
- // Transform a availabilityZone -> subnetId mapping into a list of subnets if field `subnets` is unhidden
- [super.subnets[az].id for az in std.objectFields(super.subnets)]
- else
- config.managedNodeGroups[name].subnets,
+ // Only include `subnets` when present
+ (if std.objectHas(config.managedNodeGroups[name], 'subnets') then {
+ subnets:
+ if std.type(config.managedNodeGroups[name].subnets) == 'object' then
+ [super.subnets[az].id for az in std.objectFields(super.subnets)]
+ else
+ config.managedNodeGroups[name].subnets,
+ } else {}),Apply the same pattern in multiAZUnmanagedNodeGroup (Lines 67–73).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [if std.objectHas(config.managedNodeGroups[name], 'subnets') then 'subnets' else null]: | |
| if std.type(config.managedNodeGroups[name].subnets) == 'object' then | |
| // Transform a availabilityZone -> subnetId mapping into a list of subnets if field `subnets` is unhidden | |
| [super.subnets[az].id for az in std.objectFields(super.subnets)] | |
| else | |
| config.managedNodeGroups[name].subnets, | |
| // Only include `subnets` when present | |
| (if std.objectHas(config.managedNodeGroups[name], 'subnets') then { | |
| subnets: | |
| if std.type(config.managedNodeGroups[name].subnets) == 'object' then | |
| [super.subnets[az].id for az in std.objectFields(super.subnets)] | |
| else | |
| config.managedNodeGroups[name].subnets, | |
| } else {}), |
🤖 Prompt for AI Agents
In eks/eksctl.libsonnet around lines 53 to 59 (and also apply the same pattern
to multiAZUnmanagedNodeGroup at lines 67–73), the computed field expression "[if
std.objectHas(config.managedNodeGroups[name], 'subnets') then 'subnets' else
null]: ..." can produce a null key which is invalid in Jsonnet; change this to
produce a conditional object instead (i.e., replace the computed-field form with
an if/then that returns either an object containing the 'subnets' field mapping
to the same value/transformation or an empty object), and do the same
replacement in the multiAZUnmanagedNodeGroup block so no branch ever yields a
null key.
| local yamlManifest(cluster) = | ||
| // Setting structure of the rendered YAML manifest | ||
| std.lines([ | ||
| manifestYamlDoc({ apiVersion: cluster.apiVersion }), | ||
| manifestYamlDoc({ kind: cluster.kind }), | ||
| manifestYamlDoc({ metadata: cluster.metadata }), | ||
| manifestYamlDoc({ iam: cluster.iam }), | ||
| manifestYamlDoc({ vpc: cluster.vpc }), | ||
| manifestYamlDoc({ addons: cluster.addons }), | ||
| manifestYamlDoc({ cloudWatch: cluster.cloudWatch }), | ||
| manifestYamlDoc({ managedNodeGroups: cluster.managedNodeGroups }), | ||
| manifestYamlDoc({ nodeGroups: cluster.nodeGroups }), | ||
| ]); |
There was a problem hiding this comment.
Render a single YAML document, not multiple one‑field documents
eksctl config examples are single documents containing all sections. Emitting N separate YAML docs (one per field) risks parse failures or ignored sections.
Replace yamlManifest with:
-local yamlManifest(cluster) =
- // Setting structure of the rendered YAML manifest
- std.lines([
- manifestYamlDoc({ apiVersion: cluster.apiVersion }),
- manifestYamlDoc({ kind: cluster.kind }),
- manifestYamlDoc({ metadata: cluster.metadata }),
- manifestYamlDoc({ iam: cluster.iam }),
- manifestYamlDoc({ vpc: cluster.vpc }),
- manifestYamlDoc({ addons: cluster.addons }),
- manifestYamlDoc({ cloudWatch: cluster.cloudWatch }),
- manifestYamlDoc({ managedNodeGroups: cluster.managedNodeGroups }),
- manifestYamlDoc({ nodeGroups: cluster.nodeGroups }),
- ]);
+local yamlManifest(cluster) =
+ manifestYamlDoc(cluster);Examples in docs show a single doc. (eksctl.io)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local yamlManifest(cluster) = | |
| // Setting structure of the rendered YAML manifest | |
| std.lines([ | |
| manifestYamlDoc({ apiVersion: cluster.apiVersion }), | |
| manifestYamlDoc({ kind: cluster.kind }), | |
| manifestYamlDoc({ metadata: cluster.metadata }), | |
| manifestYamlDoc({ iam: cluster.iam }), | |
| manifestYamlDoc({ vpc: cluster.vpc }), | |
| manifestYamlDoc({ addons: cluster.addons }), | |
| manifestYamlDoc({ cloudWatch: cluster.cloudWatch }), | |
| manifestYamlDoc({ managedNodeGroups: cluster.managedNodeGroups }), | |
| manifestYamlDoc({ nodeGroups: cluster.nodeGroups }), | |
| ]); | |
| local yamlManifest(cluster) = | |
| manifestYamlDoc(cluster); |
🤖 Prompt for AI Agents
In eks/eksctl.libsonnet around lines 135-147, the current yamlManifest emits
multiple one-field YAML documents; change it to render a single YAML document by
constructing one object that contains all sections and pass that single object
to manifestYamlDoc. Replace the array of manifestYamlDoc calls with a single
manifestYamlDoc call whose argument is an object combining apiVersion, kind,
metadata, iam, vpc, addons, cloudWatch, managedNodeGroups and nodeGroups
(merging or including only defined fields as needed) so the output is one valid
eksctl config document.
No description provided.