Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: x509 error when adding cluster with different cadata (Fixes #21326) #21327

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: x509 error when adding cluster with different cadata (Fixes #21326) #21327

wants to merge 3 commits into from
+34 −13

Conversation

aminarefzadeh
Copy link
Contributor

@aminarefzadeh aminarefzadeh commented Dec 29, 2024 •
edited
Loading

Fixes #21326

Describe the bug

We want to add a new cluster using ArgoCD cli with --cluster-endpoint kube-public option. The problem is, Argo uses the endpoint specified in kube-public config, but it does not use the certificate_authority_data associated with this endpoint, instead it uses the ca_data in kube config, And this will produce x509 error when adding a new cluster.

To Reproduce

Adding a cluster using Argocd cli with --cluster-endpoint kube-public option. When certificate_authority_data is different in kubeconfig and kube-public.

root@runner:/# argocd cluster add --name neda-dog --core --system-namespace argocd --cluster-endpoint kube-public --upsert -y neda-dog
INFO[0000] ServiceAccount "argocd-manager" already exists in namespace "argocd"
INFO[0000] ClusterRole "argocd-manager-role" updated
INFO[0000] ClusterRoleBinding "argocd-manager-role-binding" updated
ERRO[0001] finished unary call with code Unknown         error="Get \"https://api.k8s-dog.***.cloud:443/version?timeout=32s\": tls: failed to verify certificate: x509: certificate signed by unknown authority" grpc.code=Unknown grpc.method=Create grpc.service=cluster.ClusterService grpc.start_time="2024-12-29T17:13:01Z" grpc.time_ms=95.35 span.kind=server system=grpc
FATA[0001] rpc error: code = Unknown desc = Get "https://api.k8s-dog.***.cloud:443/version?timeout=32s": tls: failed to verify certificate: x509: certificate signed by unknown authority

Version

root@runner:/# argocd version --core
argocd: v2.13.2+dc43124
  BuildDate: 2024-12-11T19:01:33Z
  GitCommit: dc43124058130db9a747d141d86d7c2f4aac7bf9
  GitTreeState: clean
  GoVersion: go1.22.9
  Compiler: gc
  Platform: linux/amd64
argocd-server: v2.13.2+dc43124
  BuildDate: 2024-12-11T19:01:33Z
  GitCommit: dc43124058130db9a747d141d86d7c2f4aac7bf9
  GitTreeState: clean
  GoVersion: go1.22.9
  Compiler: gc
  Platform: linux/amd64
  Kustomize Version: v5.5.0 2024-10-09T13:10:16Z
  Helm Version: v3.16.3+gcfd0749
  Kubectl Version: v0.31.0
  Jsonnet Version: v0.20.0

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@aminarefzadeh aminarefzadeh requested a review from a team as a code owner December 29, 2024 18:40
@bunnyshell Bunnyshell
Copy link

bunnyshell bot commented Dec 29, 2024 •
edited
Loading

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🔵 /bns:start to start the environment
  • 🚀 /bns:deploy to redeploy the environment
  • ❌ /bns:delete to remove the environment

@codecov Codecov
Copy link

codecov bot commented Dec 29, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 55.23%. Comparing base (8126508) to head (995035d).
Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
cmd/argocd/commands/cluster.go 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #21327      +/-   ##
==========================================
+ Coverage   55.19%   55.23%   +0.03%     
==========================================
  Files         337      337              
  Lines       57058    57059       +1     
==========================================
+ Hits        31496    31518      +22     
+ Misses      22863    22845      -18     
+ Partials     2699     2696       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aminarefzadeh aminarefzadeh requested a review from a team as a code owner December 29, 2024 20:15
andrii-korotkov-verkada
andrii-korotkov-verkada reviewed Jan 1, 2025
View reviewed changes
Copy link
Contributor

@andrii-korotkov-verkada andrii-korotkov-verkada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, move adding user to a different PR and resolve the merge conflict.

USERS.md Outdated Show resolved Hide resolved
@aminarefzadeh aminarefzadeh force-pushed the fix-setting-ca-data-from-kube-public-configuration branch from 094d7ef to 2e6763c Compare January 1, 2025 19:49
@aminarefzadeh
Copy link
Contributor Author

aminarefzadeh commented Jan 2, 2025 •
edited
Loading

Looks good overall, move adding user to a different PR and resolve the merge conflict.

Hi Andrii, Can you tell me what is the status of this PR? If there is any problem, please tell me so I can resolve it.

@andrii-korotkov-verkada

nitishfy
nitishfy reviewed Jan 3, 2025
View reviewed changes
Copy link
Member

@nitishfy nitishfy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me though. However, I'd suggest adding a description for this PR on what error were you typically getting earlier/or consider raising an issue for the same.

@aminarefzadeh
Copy link
Contributor Author

The changes look good to me though. However, I'd suggest adding a description for this PR on what error were you typically getting earlier/or consider raising an issue for the same.

Thanks for your feedback. I have updated the description of this PR and described the bug we are trying to fix.
If the description is vague or lacks information, please inform me so I could provide more information and screen shots for this bug.

@aminarefzadeh
Copy link
Contributor Author

I have also built these changes and tested them in my local environment, and it seems like the problem has been fixed.

nitishfy
nitishfy reviewed Jan 5, 2025
View reviewed changes
cmd/util/cluster.go Outdated Show resolved Hide resolved
nitishfy
nitishfy reviewed Jan 5, 2025
View reviewed changes
cmd/util/cluster.go Outdated Show resolved Hide resolved
nitishfy
nitishfy suggested changes Jan 5, 2025
View reviewed changes
Copy link
Member

@nitishfy nitishfy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor comments, overall LGTM!

@aminarefzadeh
Apply suggestions
995035d 
Signed-off-by: Amin Arefzadeh <[email protected]>
@aminarefzadeh aminarefzadeh force-pushed the fix-setting-ca-data-from-kube-public-configuration branch from 1676745 to 995035d Compare January 5, 2025 12:44
@aminarefzadeh
Copy link
Contributor Author

Left some minor comments, overall LGTM!

Thanks for the suggestions, I have applied them in the latest commit.

@aminarefzadeh
Copy link
Contributor Author

Hello again @nitishfy. Do you happen to have any update on this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrii-korotkov-verkada andrii-korotkov-verkada andrii-korotkov-verkada left review comments

@nitishfy nitishfy nitishfy requested changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

x509: certificate signed by unknown authority error when adding cluster with kube-public option using cli
3 participants
@aminarefzadeh @nitishfy @andrii-korotkov-verkada