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

[dev-v2.7] Add K8s September patches #967

Merged
merged 3 commits into from
Sep 28, 2022

Conversation

rayandas
Copy link
Contributor

@rayandas rayandas commented Sep 14, 2022

This PR includes K8s September patches (issue), and fix for metrics server 0.5.0 failing due to docker CIS hardening (issue)

@rayandas rayandas marked this pull request as ready for review September 14, 2022 11:25
@rayandas rayandas requested review from snasovich, kinarashah and a team September 14, 2022 11:27
@rayandas rayandas force-pushed the rayandas-sept-k8s-patch branch 2 times, most recently from e4523a2 to df2dde0 Compare September 16, 2022 04:00
Copy link
Collaborator

@snasovich snasovich left a comment

Choose a reason for hiding this comment

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

See added comment, we need a new template to properly fix metrics server issue.
Not using "Request changes" so merging is not blocked in case I'm not around to clear the reject. But these changes need to be addressed.

@@ -213,6 +213,7 @@ spec:
readOnlyRootFilesystem: true
runAsNonRoot: true
runAsUser: 1000
allowPrivilegeEscalation: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to introduce a new template as per discussions in this thread - https://github.com/rancher/kontainer-driver-metadata/pull/938/files#r953052960

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright! Few questions. The next template verion should be metrics_v0.5.1.go ? And what K8s version should support this new metrics version? The current version has ">=1.20.14-rancher2-1 <1.23.3-rancher1-1": metricsServerv050 see https://github.com/rancher/kontainer-driver-metadata/blob/dev-v2.7/rke/templates/templates.go#L160
cc: @kinarashah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kinarashah @snasovich I added a template metrics_v0.5.1.go also updated the range of K8s supported version for metric server

">=1.22.14-rancher1-1 <1.23.3-rancher1-1": metricsServerv051,
">=1.20.14-rancher2-1 <1.22.13-rancher1-1": metricsServerv050,

Please review and let me know what all changes are required.

@rayandas rayandas force-pushed the rayandas-sept-k8s-patch branch 2 times, most recently from 9aea06d to 8a383c0 Compare September 20, 2022 12:47
Copy link
Collaborator

@snasovich snasovich left a comment

Choose a reason for hiding this comment

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

Some comments to address, otherwise LGTM

@@ -0,0 +1,237 @@
package templates
Copy link
Collaborator

Choose a reason for hiding this comment

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

The chosen file name is confusing as it makes impression that it's for metrics server 0.5.1.
But in fact we've been using rancher/mirrored-metrics-server:v0.5.1 with metrics_v0.5.0.go template for a while now.
Maybe name it (and related variables) as metrics_v0.5.1-fix.go to help avoid confusion? @kinarashah , please chime in on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Will be updating it.

rke/templates/templates.go Outdated Show resolved Hide resolved
rke/templates/templates.go Outdated Show resolved Hide resolved
@rayandas rayandas force-pushed the rayandas-sept-k8s-patch branch 2 times, most recently from 58bcaef to db32064 Compare September 27, 2022 10:00
snasovich
snasovich previously approved these changes Sep 27, 2022
Copy link
Collaborator

@snasovich snasovich left a comment

Choose a reason for hiding this comment

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

LGTM, one minor comment that's may be addressed depending on timing.

">=1.8.0-rancher0 <1.20.4-rancher1-1": metricsServerv18,
">=1.23.3-rancher1-1": metricsServerv061,
">=1.22.15-rancher1-1 <1.23.3-rancher1-1": metricsServerv051fix,
">=1.20.14-rancher2-1 <=1.22.13-rancher1-1": metricsServerv050,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've just realized that all other template version ranges use exclusive top end of range, so ideally we want to be consistent and have it as below:

Suggested change
">=1.20.14-rancher2-1 <=1.22.13-rancher1-1": metricsServerv050,
">=1.20.14-rancher2-1 <1.22.15-rancher1-1": metricsServerv050,

However this is a nitpick plus I was the one who suggested inclusive top end of the range, so approving.

If you have a chance to fix it before the second approval is done, that would be great. If this is the only change, consider my approval still valid even if GitHub automatically removes it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated your suggestion.

Copy link
Member

@kinarashah kinarashah left a comment

Choose a reason for hiding this comment

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

Changes LGTM,

  • confirmed we need a new metrics-server template because latest v0.6.1 has more changes that 0.5.x don't use
  • no need for service options, version info looks right
  • default k8s is updated

@kinarashah
Copy link
Member

@rayandas If possible, cherry-pick these commits to the custom branch created for 2.6 in the same PR so it's easy to backport and review.

@rayandas
Copy link
Contributor Author

Yeah I will cherry-pick these commits to the backport PR and update.
Merging this PR.

@rayandas rayandas merged commit 07fd0a0 into rancher:dev-v2.7 Sep 28, 2022
@snasovich snasovich mentioned this pull request Oct 4, 2022
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.

3 participants