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.

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

Makes karmada-metrics-adapter use the correct certificate when deployed via karmadactl #5827

Open
wants to merge 1 commit into
base: release-1.11
Choose a base branch
from

Conversation

KhalilSantana
Copy link
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

Currently, karmadactl blundles its own manifest for the metrics-adapter component, which is lacking the TLS flags. Thus it generates a local self-signed certificate, which karmada-apiserver doesn't trust, therefore breaking the metrics endpoint (kubectl top node for example).

Which issue(s) this PR fixes:
Fixes #5805

Special notes for your reviewer:

Note: this is my first PR to this repo. I'd also point out that this commit should be cherry-picked into master aswell, so it is fixed for all releases going forward.

Does this PR introduce a user-facing change?:

Fix: karmada-metrics-adapter use the correct certificate when deployed via karmadactl

@karmada-bot karmada-bot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. labels Nov 16, 2024
@karmada-bot
Copy link
Collaborator

Welcome @KhalilSantana! It looks like this is your first PR to karmada-io/karmada 🎉

@karmada-bot karmada-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 16, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 30.98%. Comparing base (80841d1) to head (ed7b9eb).
Report is 2 commits behind head on release-1.11.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@               Coverage Diff                @@
##           release-1.11    #5827      +/-   ##
================================================
- Coverage         31.01%   30.98%   -0.04%     
================================================
  Files               643      644       +1     
  Lines             54205    54259      +54     
================================================
- Hits              16814    16811       -3     
- Misses            36361    36417      +56     
- Partials           1030     1031       +1     
Flag Coverage Δ
unittests 30.98% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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


🚨 Try these New Features:

@zhzhuang-zju
Copy link
Contributor

@KhalilSantana The CI of this PR failed due to it wasn't signed off, usually please use git commit -s -m 'your message ' or git commit -m ' Signed-off-by: AuthorName [email protected] \n ' to pass DCO (detail guideline can refer to: https://probot.github.io/apps/dco/).

@RainbowMango
Copy link
Member

/assign @XiShanYongYe-Chang
and the author
/assign @chaunceyjiang

@chaunceyjiang
Copy link
Member

Should he push to the master branch first, then cherry-pick to the release-1.11 branch?

@XiShanYongYe-Chang
Copy link
Member

XiShanYongYe-Chang commented Nov 18, 2024

Should he push to the master branch first, then cherry-pick to the release-1.11 branch?

Yes, you are right! Hi @KhalilSantana, can you help push it to the master branch and sign off refer to https://github.com/karmada-io/karmada/pull/5827/checks?check_run_id=33089738444, and then we maybe can cherry-pick to the release-1.11 branch and other previous branches.

@chaosi-zju
Copy link
Member

chaosi-zju commented Nov 18, 2024

This argument is necessary, otherwise, the metrics-adapter will generate its own certificates, which may lack certain SAN domains (e.g: karmada-metrics-adapter.karmada-system.svc.cluster.local), causing validation failures with the client's CA certificates.

@chaosi-zju
Copy link
Member

I added this argument in local_up install method (#4026) and karmada-operator method (#4063)

Due to a previous oversight, the necessary changes in karmadactl were not recognized.

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from chaunceyjiang. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@KhalilSantana
Copy link
Contributor Author

KhalilSantana commented Nov 18, 2024

Should he push to the master branch first, then cherry-pick to the release-1.11 branch?

Yes, you are right! Hi @KhalilSantana, can you help push it to the master branch and sign off refer to https://github.com/karmada-io/karmada/pull/5827/checks?check_run_id=33089738444, and then we maybe can cherry-pick to the release-1.11 branch and other previous branches.

Hi, so I've signed-off on the commit. But I have a question do I need to create a new PR for the changes in master? I've made the changes in my repos' master branch now, then cherry picked them into my release-1.11 branch, but I can't select the source branch of this PR to be master after creating the PR (see screenshot bellow). I can only select the target branch, which I'm unsure whether that will cause any issues.

image

So looks like I need two PRs: one for the master branch (to be created) and this PR. Is this the case, or am I not seeing an easier path?

@XiShanYongYe-Chang
Copy link
Member

Hi @KhalilSantana You need to submit a PR that is modified to the master branch. After the PR is integrated, you can cherry-pick the PR to the release-1.10, release-1.11, and release-1.12 branches.

@karmada-bot karmada-bot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants