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

forbid calico mgmt cluster creation unless allowCalico env is set to true #4138

Merged
merged 1 commit into from
Dec 14, 2022
Merged

forbid calico mgmt cluster creation unless allowCalico env is set to true #4138

merged 1 commit into from
Dec 14, 2022

Conversation

xiujuanx
Copy link
Contributor

@xiujuanx xiujuanx commented Dec 12, 2022 •
edited
Loading

What this PR does / why we need it

We should forbid calico mgmt cluster creation, unless the user has set _ALLOW_CALICO_ON_MANAGEMENT_CLUSTER param to be true in advance.

Which issue(s) this PR fixes

Fixes #4137

Describe testing done for PR

Release note

The creation of calico management-cluster is forbidden.

Additional information

Special notes for your reviewer

@xiujuanx xiujuanx requested review from a team as code owners December 12, 2022 03:51
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4138/20221212040050/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.

@xiujuanx xiujuanx requested a review from wjun December 12, 2022 04:08
@xiujuanx xiujuanx force-pushed the validate-calico-env branch from dfdf5b8 to 08c8255 Compare December 12, 2022 06:33
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4138/20221212064349/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.

@xiujuanx xiujuanx force-pushed the validate-calico-env branch from 08c8255 to c48e7e6 Compare December 12, 2022 09:13
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4138/20221212092225/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 Dec 12, 2022 •
edited
Loading

Codecov Report

Merging #4138 (5b595b8) into main (25c4bd6) will decrease coverage by 0.49%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4138      +/-   ##
==========================================
- Coverage   48.66%   48.17%   -0.50%     
==========================================
  Files         446      469      +23     
  Lines       44279    45748    +1469     
==========================================
+ Hits        21549    22039     +490     
- Misses      20689    21631     +942     
- Partials     2041     2078      +37
Impacted Files Coverage Δ
tkg/tkgctl/init.go 47.41% <100.00%> (+47.41%) ⬆️
packageclients/pkg/packageclient/package_update.go 83.57% <0.00%> (-1.43%) ⬇️
...in/cluster/set_machinehealthcheck_control_plane.go 21.21% <0.00%> (ø)
...i/plugin/cluster/delete_machinehealthcheck_node.go 16.66% <0.00%> (ø)
cmd/cli/plugin/cluster/upgrade.go 58.94% <0.00%> (ø)
cmd/cli/plugin/cluster/get_machinehealthcheck.go 11.42% <0.00%> (ø)
cmd/cli/plugin/cluster/delete.go 12.50% <0.00%> (ø)
cmd/cli/plugin/cluster/kubeconfig_get.go 8.82% <0.00%> (ø)
...md/cli/plugin/cluster/delete_machinehealthcheck.go 19.23% <0.00%> (ø)
cmd/cli/plugin/cluster/credentials_update.go 8.73% <0.00%> (ø)
... and 21 more

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

12345lcr
12345lcr reviewed Dec 12, 2022
View reviewed changes
tkg/client/validate.go Outdated
// _ALLOW_CALICO_ON_MANAGEMENT_CLUSTER must be true when cniType is calico
if cniType == "calico" {
allowCalicoType := os.Getenv("_ALLOW_CALICO_ON_MANAGEMENT_CLUSTER")
if allowCalicoType != "true" {
Copy link
Contributor

@12345lcr 12345lcr Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Please double check the func ConfigureAndValidateCNIType. I think this will block workload cluster and pacific cluster creation using calico as well

Copy link
Contributor

@12345lcr 12345lcr Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As you can see all the calico related e2e tests in pr pipelines are failing due to not able to create calico workload clusters

@xiujuanx xiujuanx force-pushed the validate-calico-env branch from c48e7e6 to e972925 Compare December 13, 2022 03:35
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4138/20221213034355/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.

@xiujuanx xiujuanx force-pushed the validate-calico-env branch from e972925 to c0e25b4 Compare December 13, 2022 07:50
@xiujuanx xiujuanx requested a review from 12345lcr December 13, 2022 07:52
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4138/20221213080138/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.

@xiujuanx xiujuanx force-pushed the validate-calico-env branch from c0e25b4 to 79f7be8 Compare December 13, 2022 08:05
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4138/20221213081424/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.

12345lcr
12345lcr approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@12345lcr 12345lcr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes LGTM. Let's wait for the pr pipeline results

@xiujuanx xiujuanx force-pushed the validate-calico-env branch from 79f7be8 to 5b595b8 Compare December 13, 2022 08:55
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4138/20221213090542/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.

wjun
wjun approved these changes Dec 14, 2022
View reviewed changes
Copy link
Contributor

@wjun wjun left a comment

Choose a reason for hiding this comment

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

LGTM

@wjun wjun added the ok-to-merge PRs should be labelled with this before merging label Dec 14, 2022
@xiujuanx xiujuanx merged commit e365490 into main Dec 14, 2022
@xiujuanx xiujuanx deleted the validate-calico-env branch December 14, 2022 05:32
wenqiq pushed a commit to wenqiq/tanzu-framework that referenced this pull request Dec 20, 2022
@xiujuanx @wenqiq
forbid calico mgmt creation (vmware-tanzu#4138)
a8e642a
@rajathagasthya rajathagasthya mentioned this pull request Dec 21, 2022
Closed
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@wjun wjun wjun approved these changes

@12345lcr 12345lcr 12345lcr approved these changes

@DanielXiao DanielXiao Awaiting requested review from DanielXiao

Assignees
No one assigned
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Forbid Calico management cluster creation
4 participants
@xiujuanx @wjun @12345lcr @vmwclabot