fix: Minor corrections to the cert-manager certificate being used for the Agent in docs #673
Conversation
gnunn1
requested review from
chetan-rns,
jannfis,
jgwest and
mikeshng
as code owners
|
Warning Rate limit exceeded@gnunn1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 47 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 (2)
WalkthroughDocumentation updated: principal and agent certificate and secret names changed to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
gnunn1
force-pushed
the
main
branch
2 times, most recently
from
2bda5df to
be12f46
Compare
|
Please don't merge this yet, I need to look at this one more time with a clear head. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/configuration/agent/pki-certificates.md(5 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/configuration/agent/pki-certificates.md
237-237: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: Build and push image
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
docs/configuration/agent/pki-certificates.md (3)
227-227: Unresolved: Malformed server URL still present (critical issue from previous review).The kubectl secret creation command still contains the malformed server URL syntax
https://server=argocd-agent-resource-proxy...This duplicates theserver=literal and is not valid. The fix from the previous review was not applied.Apply this diff to fix the syntax:
-kubectl create secret generic <cluster-name>-cluster -n argocd --from-literal=name=<cluster-name> --from-literal=server=https://server=argocd-agent-resource-proxy.argocd.svc.cluster.local:9090?agentName=<cluster-name> --from-file=config=./config +kubectl create secret generic <cluster-name>-cluster -n argocd --from-literal=name=<cluster-name> --from-literal=server=https://argocd-agent-resource-proxy.argocd.svc.cluster.local:9090?agentName=<cluster-name> --from-file=config=./config
237-239: Unresolved critical issues in labeling commands (from previous review).Two unresolved issues remain in the kubectl label section:
Missing language specifier (line 237): The fenced code block lacks a
bashlanguage identifier for proper syntax highlighting.Incomplete second label command (line 239): The second
kubectl labelcommand is missing the resource type and name specification. Both labels should be applied to the same secret resource.Apply this diff to resolve both issues:
-``` +```bash kubectl label secret <cluster-name>-cluster argocd.argoproj.io/secret-type=cluster -kubectl label argocd-agent.argoproj-labs.io/agent-name=<cluster-name> +kubectl label secret <cluster-name>-cluster argocd-agent.argoproj-labs.io/agent-name=<cluster-name>
276-276: Unresolved: Usingoc neatinstead ofkubectl neat(major issue from previous review).Two commands still use
oc neatinstead of the standardkubectl neatplugin syntax. This should be corrected for consistency with the kubectl-based documentation and environment.Apply this diff to both commands:
-kubectl get secret <cluster-name>-agent -o yaml -n argocd | oc neat > <cluster-name>-agent.yaml +kubectl get secret <cluster-name>-agent -o yaml -n argocd | kubectl neat > <cluster-name>-agent.yaml-kubectl get secret argocd-agent-ca -o yaml -n argocd | yq 'del(.data.["tls.key"])' -y | oc neat > argocd-agent-ca.yaml +kubectl get secret argocd-agent-ca -o yaml -n argocd | yq 'del(.data.["tls.key"])' -y | kubectl neat > argocd-agent-ca.yamlAlso applies to: 290-290
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/configuration/agent/pki-certificates.md(5 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/configuration/agent/pki-certificates.md
237-237: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: Build and push image
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
docs/configuration/agent/pki-certificates.md (1)
185-185: Good: Certificate naming conventions updated per PR objectives.The changes correctly implement the PR objectives:
- Line 185: Certificate secret appended with
-principalsuffix as required- Line 189: CommonName updated from hardcoded value to
<cluster-name>placeholder- Line 261 & 265: Agent certificate naming and CommonName also updated consistently
Also applies to: 189-189, 261-261, 265-265
| ``` | ||
| yq -i '.type = "Opaque"' ./argocd-agent-ca.yaml -y | ||
| yq -i '.metadata.name = "argocd-agent-client-tls"' <path-to-secret>/managed-cluster-agent.yaml -y | ||
| yq -i '.metadata.name = "argocd-agent-client-tls"' <path-to-secret>/<cluster-name>-agent.yaml -y |
There was a problem hiding this comment.
Minor: Path placeholder inconsistency.
Line 298 uses <path-to-secret>/ as a placeholder for the directory path, but this is inconsistent with line 276, which shows the file is output to the current directory with ./. For clarity and consistency, this should either specify the full path or reference the current directory convention used above.
Apply this diff to improve clarity:
-yq -i '.metadata.name = "argocd-agent-client-tls"' <path-to-secret>/<cluster-name>-agent.yaml -y
+yq -i '.metadata.name = "argocd-agent-client-tls"' ./<cluster-name>-agent.yaml -y📝 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.
| yq -i '.metadata.name = "argocd-agent-client-tls"' <path-to-secret>/<cluster-name>-agent.yaml -y | |
| yq -i '.metadata.name = "argocd-agent-client-tls"' ./<cluster-name>-agent.yaml -y |
🤖 Prompt for AI Agents
docs/configuration/agent/pki-certificates.md around line 298: the yq example
uses the placeholder "<path-to-secret>/" which is inconsistent with the earlier
examples that write files to the current directory using "./"; update the
placeholder to "./" (or otherwise match the convention used at line 276) so the
command reads consistently and clearly indicating the file in the current
directory.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/configuration/agent/pki-certificates.md (1)
289-289: Replaceoc neatwithkubectl neatfor consistency.This line still uses
oc neatinstead ofkubectl neat. For consistency with the corrected command at line 275 and to align with standard kubectl tooling, this should be updated.Apply this diff:
-kubectl get secret argocd-agent-ca -o yaml -n argocd | yq 'del(.data.["tls.key"])' -y | oc neat > argocd-agent-ca.yaml +kubectl get secret argocd-agent-ca -o yaml -n argocd | yq 'del(.data.["tls.key"])' -y | kubectl neat > argocd-agent-ca.yaml
♻️ Duplicate comments (1)
docs/configuration/agent/pki-certificates.md (1)
297-297: Align path placeholder with established convention.This line uses
<path-to-secret>/while earlier examples (lines 275-276, 296) use explicit paths or./. For consistency, update this placeholder to match the established convention.Apply this diff:
-yq -i '.metadata.name = "argocd-agent-client-tls"' <path-to-secret>/<cluster-name>-agent.yaml -y +yq -i '.metadata.name = "argocd-agent-client-tls"' ./<cluster-name>-agent.yaml -y
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/configuration/agent/pki-certificates.md(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T10:37:28.008Z
Learnt from: chetan-rns
Repo: argoproj-labs/argocd-agent PR: 632
File: test/data/pre-sync/pre-sync-job.yaml:2-17
Timestamp: 2025-10-31T10:37:28.008Z
Learning: In the argocd-agent repository, Kubernetes manifests under test/data/ directories are test fixtures for e2e testing and do not require production-grade security hardening such as securityContext settings for runAsNonRoot or allowPrivilegeEscalation.
Applied to files:
docs/configuration/agent/pki-certificates.md
🪛 markdownlint-cli2 (0.18.1)
docs/configuration/agent/pki-certificates.md
236-236: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: Build and push image
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
docs/configuration/agent/pki-certificates.md (4)
185-185: Principal certificate naming changes look good.The updates to suffix the secret name with
-principaland use<cluster-name>for the commonName align well with the PR objectives and improve consistency.Also applies to: 189-189
227-227: Server URL fix and explanatory note are good.The malformed URL syntax has been corrected, and the new note clarifies why a unique server URL is essential to avoid cache collisions. This improves both correctness and documentation clarity.
Also applies to: 229-233
237-238: Kubectl label commands are now complete and correct.Both label commands now properly specify the secret resource and name, addressing the past review concern. The commands are consistent and properly formatted.
275-275: Agent certificate export command is correct.The command now properly uses
kubectl neatinstead ofoc neat, ensuring consistency with kubectl-based tooling. The secret name is also correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/configuration/agent/pki-certificates.md (2)
289-290: Replaceoc neatwithkubectl neatfor consistency.Line 289 uses
oc neatwhile line 275 useskubectl neat. For consistency with the standard kubectl plugin syntax established at line 275, replace the OpenShift CLI reference with the kubectl plugin.Apply this diff:
-kubectl get secret argocd-agent-ca -o yaml -n argocd | yq 'del(.data.["tls.key"])' -y | oc neat > argocd-agent-ca.yaml +kubectl get secret argocd-agent-ca -o yaml -n argocd | yq 'del(.data.["tls.key"])' -y | kubectl neat > argocd-agent-ca.yaml
295-298: Use consistent path convention for file references.Line 275 exports the agent certificate to
<cluster-name>-agent.yaml(current directory), but line 297 references it as<path-to-secret>/<cluster-name>-agent.yaml. For consistency, use the current directory convention (./<cluster-name>-agent.yaml) established at line 275.Apply this diff:
yq -i '.type = "Opaque"' argocd-agent-ca.yaml -y -yq -i '.metadata.name = "argocd-agent-client-tls"' <path-to-secret>/<cluster-name>-agent.yaml -y +yq -i '.metadata.name = "argocd-agent-client-tls"' ./<cluster-name>-agent.yaml -y
🧹 Nitpick comments (1)
docs/configuration/agent/pki-certificates.md (1)
302-305: Use consistent path prefix in apply commands.Line 303 uses
./prefix for the path but line 304 omits it. For consistency, both should use./to indicate files in the current directory.Apply this diff:
```bash kubectl apply -f ./argocd-agent-ca.yaml -kubectl apply -f <cluster-name>-agent.yaml +kubectl apply -f ./<cluster-name>-agent.yaml</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 1248462980dfd548f844908550990d3cce33dab3 and 2f67dd29ee848a526aa412142f0cbb04e89e778a. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `docs/configuration/agent/pki-certificates.md` (8 hunks) * `docs/configuration/principal/pki-certificates.md` (4 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * docs/configuration/principal/pki-certificates.md </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (1)</summary> <details> <summary>📚 Learning: 2025-10-31T10:37:28.008Z</summary>Learnt from: chetan-rns
Repo: argoproj-labs/argocd-agent PR: 632
File: test/data/pre-sync/pre-sync-job.yaml:2-17
Timestamp: 2025-10-31T10:37:28.008Z
Learning: In the argocd-agent repository, Kubernetes manifests under test/data/ directories are test fixtures for e2e testing and do not require production-grade security hardening such as securityContext settings for runAsNonRoot or allowPrivilegeEscalation.**Applied to files:** - `docs/configuration/agent/pki-certificates.md` </details> </details> </details> <details> <summary>⏰ 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). (2)</summary> * GitHub Check: Build and push image * GitHub Check: Analyze (go) </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>docs/configuration/agent/pki-certificates.md (1)</summary><blockquote> `178-206`: **Good: Principal certificate naming and configuration aligned.** The changes properly suffix the certificate's secret name with `-principal` and use `<cluster-name>` as the commonName instead of a hardcoded value. The environment variable extraction at lines 202–206 correctly references the new secret name. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…anaged-cluster Signed-off-by: Gerald Nunn <[email protected]>
Signed-off-by: Gerald Nunn <[email protected]>
Signed-off-by: Gerald Nunn <[email protected]>
… to managed-cluster Signed-off-by: Gerald Nunn <[email protected]>
Signed-off-by: Gerald Nunn <[email protected]>
Signed-off-by: Gerald Nunn <[email protected]>
Signed-off-by: Gerald Nunn <[email protected]>
|
I think this is good to merge now, there may need to be some future tweaks based on discussions about issuing CA crt (tls.crt) and key (tls.key) from the CA (ca.crt) itself if those changes come to fruition. (#676 (comment)) |
1 participant
What does this PR do / why we need it:
Fixes a couple of minor issues with the Agent instructions for cert-manager, specifically:
-principalin the certificate since later instructions rely on thatmanaged-clusterfor the commonName, this should be the cluster-nameWhich issue(s) this PR fixes:
None
How to test changes / Special notes to the reviewer:
Follow instructions for using cert-manager for PKI
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.