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

chore: adjust manifest limits #891

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

thisthat
Copy link
Member

@thisthat thisthat commented Feb 21, 2023

This PR

- Removes kube-rbac-proxy since it goes against our Observability first approach (see their docs)

  • adjust limits based on monitoring while running the performance tests
    Screenshot 2023-02-21 at 17 18 14
    Screenshot 2023-02-21 at 17 19 02

@thisthat thisthat changed the title chore: adjust manifest for kube-rbac-proxy chore: adjust manifest limits Feb 21, 2023
@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #891 (2822553) into main (7d2621b) will decrease coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 2822553 differs from pull request most recent head 894270d. Consider uploading reports for the commit 894270d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #891      +/-   ##
==========================================
- Coverage   58.55%   58.52%   -0.03%     
==========================================
  Files          97       97              
  Lines        7552     7552              
==========================================
- Hits         4422     4420       -2     
- Misses       2938     2939       +1     
- Partials      192      193       +1     
Impacted Files Coverage Δ
...ptnworkloadinstance/reconcile_prepostevaluation.go 81.81% <0.00%> (-9.10%) ⬇️
...lers/lifecycle/keptnworkloadinstance/controller.go 81.61% <0.00%> (+0.44%) ⬆️
Flag Coverage Δ
component-tests 41.65% <ø> (+0.04%) ⬆️
keptn-lifecycle-operator 54.67% <ø> (ø)
klt-cert-manager 67.50% <ø> (ø)
scheduler 21.17% <ø> (ø)

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

@thisthat thisthat marked this pull request as ready for review February 21, 2023 16:11
@mowies
Copy link
Member

mowies commented Feb 22, 2023

@thisthat can you tell me more about the failing probes? I'd rather leave them in and fix them or change them than completely remove them

@thisthat
Copy link
Member Author

thisthat commented Feb 22, 2023

Hey @mowies, probes to kube-rbac-proxy print the following log: 2023/02/22 07:19:43 http: TLS handshake error from 10.128.16.85:38312: EOF. This is printed with an error level and could cause alerts on your observability platform.
On the K8s side, everything works fine since K8s skips cert checks, but the proxy prints this error log nevertheless.
I briefly looked into the code of the proxy and it looks like there is no health/ready probe endpoint for that container.

@mowies
Copy link
Member

mowies commented Feb 22, 2023

Then we could still use more low level probes that don't rely on certificates instead of removing them altogether.

@netlify
Copy link

netlify bot commented Feb 22, 2023

Deploy Preview for keptn-lifecyle-testing ready!

Name Link
🔨 Latest commit 2822553
🔍 Latest deploy log https://app.netlify.com/sites/keptn-lifecyle-testing/deploys/63f71db886fcfd000825eee4
😎 Deploy Preview https://deploy-preview-891--keptn-lifecyle-testing.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@thisthat
Copy link
Member Author

thisthat commented Feb 22, 2023

@mowies @RealAnna and I discussed this topic and the benefits of kube-rbac-proxy to our project.
Given our Observability first approach, this container goes against our principle, making it harder to scrape the container metrics. So, I would like to remove it.

@mowies
Copy link
Member

mowies commented Feb 22, 2023

@thisthat let's create a separate issue for the removal :)

@thisthat
Copy link
Member Author

You're right @mowies , I've created #901

mowies
mowies previously approved these changes Feb 23, 2023
bacherfl
bacherfl previously approved these changes Feb 24, 2023
@thisthat thisthat dismissed stale reviews from bacherfl and mowies via da98e6b February 27, 2023 06:19
@thisthat thisthat force-pushed the noissue/adjust-proxy-manifest branch from 2822553 to da98e6b Compare February 27, 2023 06:19
@thisthat thisthat force-pushed the noissue/adjust-proxy-manifest branch from da98e6b to 894270d Compare February 27, 2023 06:20
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@thschue thschue left a comment

Choose a reason for hiding this comment

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

lgtm

@thschue thschue merged commit 32ce1b0 into keptn:main Feb 28, 2023
@keptn-bot keptn-bot mentioned this pull request Mar 1, 2023
@keptn-bot keptn-bot mentioned this pull request Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants