fix(generic-app): update svc to support empty ports #456
Conversation
WalkthroughThis pull request introduces updates to the Changes
Sequence DiagramsequenceDiagram
participant Chart as Helm Chart
participant Service as Kubernetes Service
participant StatefulSet as StatefulSet
Chart->>Service: Configure Service
alt ClusterIP Service
Chart-->>Service: Set clusterIP to None
end
Chart->>StatefulSet: Configure StatefulSet
alt Service Enabled
Chart-->>StatefulSet: Set serviceName dynamically
end
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
charts/generic-app/templates/apps.yaml (1)
315-317: LGTM! Well-structured headless service configuration.The addition of
clusterIP: Nonefor ClusterIP services is correct and follows Kubernetes best practices for headless services, which is particularly useful for StatefulSet DNS-based discovery.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
charts/generic-app/Chart.yaml(1 hunks)charts/generic-app/README.md(3 hunks)charts/generic-app/templates/apps.yaml(3 hunks)charts/generic-app/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- charts/generic-app/values.yaml
🔇 Additional comments (5)
charts/generic-app/Chart.yaml (1)
17-17: Consider using patch version for bug fixes.The PR title indicates this is a bug fix (
fix: update svc to support empty ports), but the version bump from0.2.0to0.3.0suggests a minor release with new features. According to semantic versioning:
- Patch version (0.2.1): Bug fixes and patches
- Minor version (0.3.0): New backward-compatible functionality
Consider using a patch version bump (0.2.1) instead, unless this change introduces new features not mentioned in the PR title.
Run this script to check the scope of changes and verify if this warrants a minor version bump:
✅ Verification successful
Minor version bump (0.3.0) is appropriate
While the PR title mentions a fix, the change introduces new functionality by making service ports optional, which is a backward-compatible enhancement to the chart's capabilities. This aligns with semantic versioning principles where minor version bumps (0.x.0) are used for new features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the scope of changes to determine if this warrants a minor version bump # Check for new features or breaking changes in the templates echo "Checking for significant changes in templates..." rg -l "template" charts/generic-app/templates/ # Look for changes in values.yaml that might indicate new features echo "Checking for new configuration options..." rg -A 5 "ports:|service:" charts/generic-app/values.yamlLength of output: 922
charts/generic-app/README.md (2)
5-5: LGTM! Version update is consistent.The version badge has been correctly updated to match the new chart version 0.3.0.
330-331: Verify the example configuration aligns with empty ports support.The example configuration still shows explicit port definitions in the default values, which might confuse users about the new empty ports support feature.
Let's verify if there are any other examples in the codebase that need to be updated:
charts/generic-app/templates/apps.yaml (2)
Line range hint
318-331: LGTM! Proper handling of optional ports.The conditional block for ports aligns with the PR objective to support empty ports while maintaining backward compatibility for services that define ports.
350-354: LGTM! Verify StatefulSet behavior when service is disabled.The conditional serviceName configuration is correct. However, please verify that disabling the service doesn't break any StatefulSet that relies on DNS-based discovery.
Let's check for potential StatefulSet configurations that might rely on DNS discovery:
✅ Verification successful
Verification complete - conditional serviceName is correct and safe
The current implementation is the right approach. When service is disabled, it implies DNS-based discovery isn't needed for that StatefulSet deployment, so omitting serviceName is the correct behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for StatefulSet configurations that might need DNS discovery # Look for patterns indicating DNS usage in container specs # Search for DNS-related environment variables or command arguments rg -l '(?i)(hostname|dns|service|discovery)' --type yaml # Search for StatefulSet configurations with potential service dependencies ast-grep --pattern 'kind: StatefulSet $$$ spec { $$$ template { $$$ spec { $$$ containers { $$$ env { $$$ valueFrom { fieldRef { fieldPath: "metadata.name" } } $$$ } $$$ } $$$ } $$$ } $$$ }'Length of output: 6337
| @@ -367,7 +367,7 @@ We do not recommend that you upgrade the application by overriding `image.tag`. | |||
| | apps.example-app.nodeSelector | Node selector | object | `{"kubernetes.io/arch":"amd64","kubernetes.io/os":"linux"}` | | |||
| | apps.example-app.podSecurityContext | Pod security context | object | `{"fsGroup":2000}` | | |||
| | apps.example-app.replicaCount | Number of pod replicas to run | int | `1` | | |||
| | apps.example-app.service | Default service configuration | object | `{"annotations":{"prometheus.io/scrape":"true"},"enabled":true,"ports":{"http":8080,"metrics":9090},"type":"ClusterIP"}` | | |||
| | apps.example-app.service | Default service configuration | object | `{"annotations":{"prometheus.io/scrape":"true"},"enabled":true,"type":"ClusterIP"}` | | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update service configuration documentation for empty ports.
The service configuration example still shows explicit port definitions, which doesn't reflect the new empty ports support feature mentioned in the PR title.
Consider updating the service configuration example to demonstrate both cases:
- With explicit ports
- Without ports (new feature)
| apps.example-app.service | Default service configuration | object | `{"annotations":{"prometheus.io/scrape":"true"},"enabled":true,"type":"ClusterIP"}` |
+
+Note: The service configuration now supports empty ports. You can either:
+- Specify explicit ports:
+ ```yaml
+ service:
+ enabled: true
+ type: ClusterIP
+ ports:
+ http: 8080
+ metrics: 9090
+ ```
+- Or omit ports entirely:
+ ```yaml
+ service:
+ enabled: true
+ type: ClusterIP
+ ```
| {{- if eq $service.type "ClusterIP" }} | ||
| clusterIP: None | ||
| {{- end }} |
There was a problem hiding this comment.
this spec must not be on a regular clusterIP type service. What I think you wanted here was type = "Headless" which will then have "clusterIP: None" (would be a new type, don't think it has been contemplated so far)
| @@ -342,7 +347,11 @@ spec: | |||
| {{- include "app.selectorLabels" $ | nindent 6 }} | |||
| lname: {{ $lName }} | |||
| {{- if $appIsStatefulSet }} | |||
| {{- with .service }} | |||
| {{- if .enabled }} | |||
| serviceName: {{ $fullComponentName }} | |||
There was a problem hiding this comment.
serviceName is mandatory in a StatefulSet I think, so the user needs to have a headless service to put here. The app supports defining multiple services, and the user when creating a headless one is also quite likely to create a regular clusterIP service. Do we want to force him to have to make this specific one the headless one?
Also defaults to $fullComponentName, presumably the non headless one will then need to have a different name. These things are tricky, try to find some sound defaults + a behavior that is not very tricky for the user (or reasonably documented... or the app just failing when a certain assumed condition isn't verified with a clear error message about the constraints... or anything else you may have as an idea).
There was a problem hiding this comment.
You can have a statefulset without a service hence way I left it as an option - I've tested that. In general however having a headless service is better for the consistent pod identity.
There was a problem hiding this comment.
I didn't say you need to have a Service to have a StatefulSet, I said you need to have serviceName in a StatefulSet, which then implies a headless service must exist - otherwise what are you filling this field with?:
serviceName (string), required
serviceName is the name of the service that governs this StatefulSet. This service must exist before the StatefulSet, and is responsible for the network identity of the set. Pods get DNS/hostnames that follow the pattern: pod-specific-string.serviceName.default.svc.cluster.local where "pod-specific-string" is managed by the StatefulSet controller.
it says required
Summary by CodeRabbit
New Features
Documentation
0.3.0Chores