resolver: migrate to EndpointSlice API #167
Conversation
WalkthroughThis change updates the Elasti Helm chart and resolver to use Kubernetes EndpointSlices instead of the deprecated Endpoints API. It introduces a dedicated ServiceAccount for the resolver, adjusts RBAC permissions to only allow listing EndpointSlices, and updates the resolver's logic and configuration to reflect these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Throttler
participant K8sHelper
participant KubernetesAPI
Throttler->>K8sHelper: CheckIfServiceEndpointSliceActive(ns, svc)
K8sHelper->>KubernetesAPI: List EndpointSlices (label: kubernetes.io/service-name=svc)
KubernetesAPI-->>K8sHelper: Return EndpointSlice(s)
K8sHelper->>Throttler: Return readiness status (true/false)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
charts/elasti/templates/deployment.yaml(1 hunks)charts/elasti/templates/resolver-additional-access-binding-rbac.yaml(1 hunks)charts/elasti/templates/resolver-additional-access-rbac.yaml(1 hunks)charts/elasti/templates/serviceaccount.yaml(1 hunks)charts/elasti/values.yaml(1 hunks)pkg/k8shelper/ops.go(2 hunks)resolver/internal/throttler/throttler.go(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/elasti/templates/resolver-additional-access-binding-rbac.yaml
[error] 13-13: too many spaces inside braces
(braces)
[error] 13-13: too many spaces inside braces
(braces)
charts/elasti/templates/resolver-additional-access-rbac.yaml
[error] 8-8: wrong indentation: expected 2 but found 0
(indentation)
charts/elasti/templates/serviceaccount.yaml
[error] 9-9: too many spaces inside braces
(braces)
[error] 14-14: too many spaces inside braces
(braces)
[error] 14-14: too many spaces inside braces
(braces)
[error] 17-17: too many spaces inside braces
(braces)
[error] 19-19: too many spaces inside braces
(braces)
🔇 Additional comments (9)
resolver/internal/throttler/throttler.go (1)
102-102: LGTM! Correctly migrates to EndpointSlice API.The method call has been properly updated from
CheckIfServiceEndpointActivetoCheckIfServiceEndpointSliceActive, aligning with the migration from the deprecated Kubernetes Endpoint API to the EndpointSlice API. The rest of the function logic remains appropriately unchanged.charts/elasti/templates/deployment.yaml (1)
137-137: LGTM! Properly assigns dedicated ServiceAccount.The resolver deployment now uses a dedicated ServiceAccount instead of the default one, which improves security posture by enabling more granular RBAC permissions. The naming convention follows Kubernetes best practices.
charts/elasti/values.yaml (1)
71-72: LGTM! Adds proper ServiceAccount configuration.The addition of the
serviceAccountsection with an emptyannotationsmap provides a clean configuration structure for the new resolver ServiceAccount, following standard Helm chart patterns.charts/elasti/templates/resolver-additional-access-binding-rbac.yaml (1)
13-14: LGTM! Correctly binds to dedicated ServiceAccount.The ClusterRoleBinding now properly references the new dedicated resolver ServiceAccount instead of the default one, ensuring the resolver component gets the appropriate permissions.
Note: The static analysis warning about "too many spaces inside braces" is a false positive - the
{{ }}syntax is correct Helm templating.charts/elasti/templates/resolver-additional-access-rbac.yaml (1)
8-10: Excellent security improvement with EndpointSlice API migration.The RBAC permissions have been significantly refined following the principle of least privilege - replacing broad permissions on multiple resources with focused "list" access to EndpointSlices in the correct
discovery.k8s.ioAPI group. This aligns perfectly with the code changes that now use EndpointSlices instead of the deprecated Endpoints API.charts/elasti/templates/serviceaccount.yaml (2)
10-19: LGTM! Good separation of concerns with dedicated ServiceAccount.The addition of a separate ServiceAccount for the resolver component follows security best practices by implementing the principle of least privilege. Each component now has its own identity with tailored RBAC permissions.
9-9: Ignore yamllint false positives for Helm template syntax.The yamllint errors about "too many spaces inside braces" are false positives. The
{{ }}syntax is standard Helm templating and the spacing follows Helm best practices for readability.Also applies to: 19-19
pkg/k8shelper/ops.go (2)
8-8: LGTM! Correct import for EndpointSlice API.The import of
discoveryv1API is appropriate for the migration from deprecated Endpoints to EndpointSlices.
39-43: LGTM! Correct API usage and label selector.The function correctly uses the DiscoveryV1 API and the proper label selector
discoveryv1.LabelServiceNameto filter EndpointSlices by service name.
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
tests/e2e/tests/08-resolver-multiple-endpoints/03-disable-scaling.yaml (1)
1-8:ScaledObjectmanifest is invalid – missing requiredspecor use a patch step instead.
ScaledObjectCRD requires a non-emptyspec(at leastscaleTargetRef& onetrigger).
Applying this manifest will fail server-side validation and abort the test run.If the goal is merely to pause an already existing ScaledObject, switch the file to a KUTTL patch step:
-apiVersion: keda.sh/v1alpha1 -kind: ScaledObject -metadata: - name: target-scaled-object - namespace: target - annotations: - autoscaling.keda.sh/paused: "true" +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: + # Pause KEDA autoscaling for the existing ScaledObject + - command: kubectl annotate scaledobject.keda.sh target-scaled-object -n target autoscaling.keda.sh/paused=true --overwrite
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
operator/internal/controller/opsDeployment.go(1 hunks)tests/e2e/Makefile(1 hunks)tests/e2e/manifest/reset.sh(1 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/01-patch-deployment.yaml(1 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/02-patch-elasti-service.yaml(1 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/03-disable-scaling.yaml(1 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/04-scale-to-1-target.yaml(1 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/05-assert-mode-proxy.yaml(1 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/06-scale-to-2-target.yaml(1 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/07-assert-mode-serve.yaml(1 hunks)tests/e2e/tests/08-resolver-multiple-endpoints/99-reset.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: in kubeelasti e2e tests, kuttl teststep scripts do not run inside the kuttl container - they run in ...
Learnt from: rethil
PR: truefoundry/KubeElasti#154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.083Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like `jq` should be documented as E2E test prerequisites rather than worrying about container image contents.
Applied to files:
tests/e2e/tests/08-resolver-multiple-endpoints/99-reset.yamltests/e2e/manifest/reset.shtests/e2e/Makefiletests/e2e/tests/08-resolver-multiple-endpoints/07-assert-mode-serve.yamltests/e2e/tests/08-resolver-multiple-endpoints/05-assert-mode-proxy.yamltests/e2e/tests/08-resolver-multiple-endpoints/00-scale-to-0-target.yamltests/e2e/tests/08-resolver-multiple-endpoints/04-scale-to-1-target.yamltests/e2e/tests/08-resolver-multiple-endpoints/01-patch-deployment.yamltests/e2e/tests/08-resolver-multiple-endpoints/06-scale-to-2-target.yaml
📚 Learning: `jq` should be added as a prerequisite for running kubeelasti e2e tests since it's used in test scri...
Learnt from: rethil
PR: truefoundry/KubeElasti#154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.083Z
Learning: `jq` should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like `tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml` for JSON parsing.
Applied to files:
tests/e2e/tests/08-resolver-multiple-endpoints/99-reset.yamltests/e2e/manifest/reset.shtests/e2e/Makefiletests/e2e/tests/08-resolver-multiple-endpoints/07-assert-mode-serve.yamltests/e2e/tests/08-resolver-multiple-endpoints/05-assert-mode-proxy.yamltests/e2e/tests/08-resolver-multiple-endpoints/00-scale-to-0-target.yamltests/e2e/tests/08-resolver-multiple-endpoints/04-scale-to-1-target.yamltests/e2e/tests/08-resolver-multiple-endpoints/01-patch-deployment.yamltests/e2e/tests/08-resolver-multiple-endpoints/02-patch-elasti-service.yamltests/e2e/tests/08-resolver-multiple-endpoints/06-scale-to-2-target.yaml
📚 Learning: the elastiservice.spec.triggers[].metadata.query field in kubeelasti has a broader attack vector for...
Learnt from: rethil
PR: truefoundry/KubeElasti#153
File: pkg/scaling/scalers/prometheus_scaler.go:46-46
Timestamp: 2025-07-21T17:07:03.263Z
Learning: The ElastiService.spec.triggers[].metadata.query field in KubeElasti has a broader attack vector for Prometheus query injection compared to the uptimeFilter parameter, as it allows arbitrary Prometheus queries to be executed.
Applied to files:
tests/e2e/tests/08-resolver-multiple-endpoints/02-patch-elasti-service.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/resolver)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/operator)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/pkg)
- GitHub Check: Run Kuttl E2E Tests
🔇 Additional comments (3)
tests/e2e/tests/08-resolver-multiple-endpoints/02-patch-elasti-service.yaml (1)
7-14: Prometheus trigger uses a literalvector(1)query – confirm this is intentional.
vector(1)always returns 1 and effectively disables threshold-based scaling. If the intent is to keep the service perpetually “up” for this scenario, leave as-is; otherwise consider a more realistic metric to avoid masking issues in future tests.tests/e2e/Makefile (1)
89-89: LGTM! Excellent simplification using Helm best practices.The replacement of the multi-step conditional installation process with
helm upgrade --install --create-namespaceis a significant improvement. This single command handles namespace creation, installation, and upgrades idempotently, eliminating the need for manual deployment existence checks and template rendering.operator/internal/controller/opsDeployment.go (1)
30-30: LGTM! Cleaner readiness check logic.The direct check on
ReadyReplicas > 0is more straightforward than the previous condition logic. This field specifically tracks ready and available replicas, making it the ideal metric for determining when to switch to serve mode.
tests/e2e/tests/08-resolver-multiple-endpoints/00-scale-to-0-target.yaml
Show resolved
Hide resolved
tests/e2e/tests/08-resolver-multiple-endpoints/04-scale-to-1-target.yaml
Show resolved
Hide resolved
tests/e2e/tests/08-resolver-multiple-endpoints/05-assert-mode-proxy.yaml
Show resolved
Hide resolved
tests/e2e/tests/08-resolver-multiple-endpoints/05-assert-mode-proxy.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/08-resolver-multiple-endpoints/06-scale-to-2-target.yaml
Show resolved
Hide resolved
tests/e2e/tests/08-resolver-multiple-endpoints/07-assert-mode-serve.yaml
Outdated
Show resolved
Hide resolved
|
Hey @ramantehlan & @shubhamrai1993, this one is ready for review. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
charts/elasti/templates/deployment.yaml(1 hunks)charts/elasti/values.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/operator)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/resolver)
- GitHub Check: test (/home/runner/work/KubeElasti/KubeElasti/pkg)
- GitHub Check: golangci-lint (/home/runner/work/KubeElasti/KubeElasti/operator)
- GitHub Check: Run Kuttl E2E Tests
🔇 Additional comments (2)
charts/elasti/templates/deployment.yaml (1)
139-142: Resolver ServiceAccount and RBAC binding confirmed
Both the ServiceAccount and its ClusterRoleBinding are present in the same release, so the Deployment will have its SA rendered and bound before rollout:
- charts/elasti/templates/serviceaccount.yaml (lines 11–17): defines
name: {{ include "elasti.fullname" . }}-resolver- charts/elasti/templates/resolver-additional-access-binding-rbac.yaml (lines 12–14): binds that SA as a subject of the ClusterRoleBinding
Helm’s default ordering guarantees ServiceAccounts and RBAC objects are created before Deployments, and there are no
helm.sh/hookannotations on these resources.charts/elasti/values.yaml (1)
90-92: Configuration addition looks goodAdding an
annotationsmap keeps the chart extensible and matches the controller’s configuration block. No further action required.
Description
Resolver is using deprecated Endpoint API (as for kubernetes 1.33), which should be replaced with EndpointSlice API (just like operator).
This PR also adds separate ServiceAccount for resolver (it was using default) and cleans up RBAC permissions for it.
Fixes #164
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor