[APK] Template secrets for custom secret #1334
Conversation
sgayangi
requested review from
CrowleyRajapakse,
ashera96 and
npamudika
as code owners
|
Warning Rate limit exceeded@sgayangi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughAdds Helm-configurable TLS secret/file names, extends RateLimitPolicy with count/time-unit fields, validates/mapping time units in synchronizer, and propagates BurstControl into RateLimitPolicy CR create/update flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant EventHub as EventHub (policy payload)
participant Fetcher as RateLimitFetcher
participant Validator as TimeUnit Validator
participant Types as Policy Structs
participant K8sClient as K8s Client
participant K8sAPI as Kubernetes API (RateLimitPolicy CR)
EventHub->>+Fetcher: deliver RateLimitPolicy event
Fetcher->>+Validator: extract time unit (DefaultLimit / RateLimitTimeUnit)
Validator-->>-Fetcher: mapped unit or unsupported
alt supported
Fetcher->>Types: populate RateLimitCount & RateLimitTimeUnit
Fetcher->>+K8sClient: create/update RateLimitPolicy CR (include BurstControl.RequestsPerUnit, Unit)
K8sClient->>+K8sAPI: apply CR (create/update)
K8sAPI-->>-K8sClient: result
K8sClient-->>-Fetcher: ack / log
else unsupported
Fetcher-->>-EventHub: log and skip policy
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
helm-charts/templates/deployment.yaml(2 hunks)helm-charts/values.yaml(1 hunks)
🔇 Additional comments (2)
helm-charts/templates/deployment.yaml (1)
73-79: LGTM! Template expressions correctly reference configurable certificate filenames.The volumeMount subPaths now reference
.Values.configs.tls.*with appropriate defaults that maintain backward compatibility. The template syntax is correct and aligns with the values.yaml configuration.helm-charts/values.yaml (1)
59-64: This configuration is working as designed and does not require changes.The deployment template uses
{{ .Values.configs.tls.secretName | default "apk-agent-server-cert"}}, which correctly references the secretName from values.yaml ("keys-certs") with a fallback to "apk-agent-server-cert" for backward compatibility when the value is not provided. This is standard Helm pattern and there is no inconsistency between the files.Likely an incorrect or invalid review comment.
| - name: apk-agent-certificates | ||
| secret: | ||
| secretName: apk-agent-server-cert | ||
| secretName: {{ .Values.configs.tls.secretName | default "apk-agent-server-cert"}} |
There was a problem hiding this comment.
Resolve the secretName inconsistency.
The default value "apk-agent-server-cert" here doesn't match the value "keys-certs" set in values.yaml (line 61). When values.yaml is used, the secret name will be "keys-certs", but if someone deploys without specifying this value, it will fall back to "apk-agent-server-cert". This mismatch can lead to:
- Deployment failures if the expected secret doesn't exist
- Confusion about which secret name to create
Consider one of these approaches:
Option 1: Match the values.yaml default
- secretName: {{ .Values.configs.tls.secretName | default "apk-agent-server-cert"}}
+ secretName: {{ .Values.configs.tls.secretName | default "keys-certs"}}Option 2: Keep backward compatibility (and update values.yaml line 61 to match)
# In values.yaml, change line 61 to:
secretName: "apk-agent-server-cert"🤖 Prompt for AI Agents
In helm-charts/templates/deployment.yaml around line 109, the template default
secretName "apk-agent-server-cert" is inconsistent with values.yaml which sets
secretName "keys-certs"; update one side so both match. Either change the
template default to "keys-certs" to align with values.yaml, or change
values.yaml (line 61) to "apk-agent-server-cert" to preserve the template’s
current default; ensure the chosen value is documented and tested so deployments
using no override use the expected secret name.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apim-apk-agent/internal/k8sClient/k8s_client.go(2 hunks)apim-apk-agent/internal/synchronizer/ratelimit_policy_fetcher.go(2 hunks)apim-apk-agent/pkg/eventhub/types/types.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apim-apk-agent/internal/synchronizer/ratelimit_policy_fetcher.go (3)
apim-apk-agent/pkg/eventhub/types/types.go (1)
DefaultLimit(201-211)import-export-cli/integration/apim/throttlePolicyDTO.go (1)
RequestCount(118-122)apim-apk-agent/internal/loggers/logger.go (1)
LoggerSynchronizer(51-51)
apim-apk-agent/internal/k8sClient/k8s_client.go (2)
apim-apk-agent/pkg/eventhub/types/types.go (1)
Subscription(21-36)apim-apk-agent/internal/eventhub/marshaller.go (1)
Subscription(69-81)
🔇 Additional comments (4)
apim-apk-agent/internal/k8sClient/k8s_client.go (1)
479-482: LGTM: BurstControl initialization.The BurstControl is properly initialized during CR creation with values from the policy.
apim-apk-agent/internal/synchronizer/ratelimit_policy_fetcher.go (2)
144-163: LGTM: Explicit time unit validation with switch statements.The switch statements properly normalize time units and handle unsupported values by logging errors and skipping invalid policies. This prevents invalid time unit values from being propagated to Kubernetes CRs.
The second switch (lines 155-163) validates
RateLimitTimeUnitbefore it's used to populateBurstControlin the k8s client.
333-352: LGTM: Consistent time unit validation for subscription policies.The time unit validation logic is consistent with the implementation in
FetchRateLimitPoliciesOnEvent, ensuring uniform handling across both policy types.apim-apk-agent/pkg/eventhub/types/types.go (1)
187-188: The review comment references the incorrect struct. Theuint32casts at lines 480 and 498 ink8s_client.gooperate oneventhubTypes.SubscriptionPolicy.RateLimitCount(which isint32, defined at line 122 intypes.go), not onRateLimitPolicy.RateLimitCount(which isint, defined at line 187).The function
DeploySubscriptionRateLimitPolicyCRtakes aSubscriptionPolicyparameter, notRateLimitPolicy. While both structs exist and both have aRateLimitCountfield, the casts in this function apply to theSubscriptionPolicyvariant.Note: The underlying concern about casting without validation is valid—both
int32andintnegative values would produce large wraparound values when cast touint32—but it applies to the wrong struct definition.Likely an incorrect or invalid review comment.
sgayangi
force-pushed
the
apk-helm-chart-fix
branch
from
9358ec8 to
2977a6c
Compare
sgayangi
force-pushed
the
apk-helm-chart-fix
branch
from
2977a6c to
51633b6
Compare
2 participants
When cert-manager is disabled, can use custom secrets
Summary by CodeRabbit
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.