Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

prevents metric-server from taking ownership of kube-system namespace. #3596

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

adduarte
Copy link

@adduarte adduarte commented Oct 7, 2022

fixes: 3595

What this PR does / why we need it

prevents metric-server app on administrator cluster from taking ownership of kube-system namespace.
The configuration for the metrics-server which is deployed to the administration server uses the default
("createNamespace: true") when it is to be deployed on the "kube-system" namespace.
Using "createNamespace: true" causes kapp-controller to take ownership of the namespace and assign it to the metrics-server app.
The result is that if a admin user makes any changes to the "kube-system" namespace, they will be reverted back when metric-server app is reconciled by kapp-controller. It also adds kapp.k14s.io annotations and labels to the namespace which can interfere with control-plane operations. For example, when the metric-server app is deleted, the system will try to delete kube-system namespace which is not allowed, resulting in a problem.

Which issue(s) this PR fixes

Fixes # 3595

Describe testing done for PR

Tested by editing the ${CLUSTERNAME}-vc-metrics-server-addon

Release note

prevent metric-server app from taking ownership of kube-system namespace in administrator cluster
labeled kube-system namespace and verified labels are not removed when the metrics-server app is reconciled. 

Additional information

Special notes for your reviewer

@adduarte adduarte requested review from a team as code owners October 7, 2022 20:54
@github-actions
Copy link

github-actions bot commented Oct 7, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3596/20221007210401/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Merging #3596 (e91b028) into main (2f9044a) will decrease coverage by 0.90%.
The diff coverage is 3.33%.

@@            Coverage Diff             @@
##             main    #3596      +/-   ##
==========================================
- Coverage   46.93%   46.03%   -0.91%     
==========================================
  Files         402      427      +25     
  Lines       40203    41754    +1551     
==========================================
+ Hits        18869    19221     +352     
- Misses      19582    20765    +1183     
- Partials     1752     1768      +16     
Impacted Files Coverage Δ
cli/runtime/config/clientconfig.go 53.17% <0.00%> (ø)
cmd/cli/plugin/cluster/osimage.go 100.00% <ø> (ø)
cmd/cli/plugin/cluster/osimage_oracle.go 3.20% <0.00%> (ø)
cli/core/pkg/config/edition.go 57.14% <100.00%> (ø)
...in/cluster/set_machinehealthcheck_control_plane.go 21.21% <0.00%> (ø)
cmd/cli/plugin/cluster/get_machinehealthcheck.go 11.42% <0.00%> (ø)
cmd/cli/plugin/cluster/create.go 42.22% <0.00%> (ø)
cmd/cli/plugin/cluster/set_node_pool.go 14.63% <0.00%> (ø)
...md/cli/plugin/cluster/delete_machinehealthcheck.go 19.23% <0.00%> (ø)
...cluster/delete_machinehealthcheck_control_plane.go 16.66% <0.00%> (ø)
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@randomvariable
Copy link
Contributor

We should merge this one. Can we rebase it @adduarte ?

@adduarte adduarte closed this Nov 7, 2022
@adduarte adduarte reopened this Nov 8, 2022
@adduarte adduarte force-pushed the topic/adduarte/fix_metric_server_namespace_ownership branch from 75c65e6 to e91b028 Compare November 10, 2022 05:57
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3596/20221110060737/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@adduarte adduarte added the ok-to-merge PRs should be labelled with this before merging label Nov 15, 2022
@maralavi
Copy link
Contributor

maralavi commented Nov 15, 2022

@adduarte Could you please just update this statement in the description?

they will be reverted back when metric-server app is reconciled by addons-manager--> they will be reverted back when metric-server app is reconciled by kapp-controller

@adduarte adduarte merged commit 6d64f81 into main Nov 15, 2022
@adduarte adduarte deleted the topic/adduarte/fix_metric_server_namespace_ownership branch November 15, 2022 19:05
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants