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

Bump ClusterGroup to crd/v1alpha3 and deprecate single ipBlock #2008

Merged
merged 1 commit into from
May 19, 2021

Conversation

Dyanngg
Copy link
Contributor

@Dyanngg Dyanngg commented Mar 29, 2021

This PR upgrades ClusterGroup CRD related resources to crd/v1alpha3 to deprecate ipBlock in crd/v1alpha2 ClusterGroup.

The CRD upgrade process follows https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/. Namely, there is a conversion webhook deployed to convert crd/v1alpha2 ClusterGroup to crd/v1alpha3 and vice versa. crd/v1alpha2 ClusterGroup will continue to be served until deprecated.

On Antrea controller startup it will use the apiextension clienset to sync CA cert between Antrea apiserver and conversion webhook config of the ClusterGroup CRD. Conversion endpoint will be /convert/clustergroup.

@lgtm-com
Copy link

lgtm-com bot commented Mar 29, 2021

This pull request introduces 6 alerts when merging 6e2ce92 into 85456c9 - view on LGTM.com

new alerts:

  • 6 for Reflected cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Mar 29, 2021

This pull request introduces 6 alerts when merging 41425b0 into 85456c9 - view on LGTM.com

new alerts:

  • 6 for Reflected cross-site scripting

@codecov-io
Copy link

codecov-io commented Mar 29, 2021

Codecov Report

Merging #2008 (65b1957) into main (5752f96) will decrease coverage by 6.42%.
The diff coverage is 12.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2008      +/-   ##
==========================================
- Coverage   61.57%   55.15%   -6.43%     
==========================================
  Files         265      268       +3     
  Lines       19770    19949     +179     
==========================================
- Hits        12174    11003    -1171     
- Misses       6333     7782    +1449     
+ Partials     1263     1164      -99     
Flag Coverage Δ
kind-e2e-tests 41.43% <8.17%> (-11.44%) ⬇️
unit-tests 41.31% <9.94%> (-0.40%) ⬇️

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

Impacted Files Coverage Δ
pkg/apiserver/apiserver.go 78.22% <0.00%> (-11.21%) ⬇️
pkg/controller/grouping/convert.go 0.00% <0.00%> (ø)
...ntroller/networkpolicy/networkpolicy_controller.go 69.13% <ø> (-15.44%) ⬇️
pkg/controller/networkpolicy/status_controller.go 48.10% <0.00%> (-40.72%) ⬇️
pkg/apiserver/handlers/webhook/convert_crd.go 2.56% <2.56%> (ø)
pkg/apiserver/certificate/cacert_controller.go 35.76% <23.80%> (-23.20%) ⬇️
pkg/k8s/client.go 49.01% <25.00%> (-0.99%) ⬇️
pkg/apis/crd/v1alpha3/register.go 75.00% <75.00%> (ø)
pkg/controller/networkpolicy/clustergroup.go 66.35% <90.90%> (-16.22%) ⬇️
pkg/apiserver/certificate/certificate.go 76.71% <100.00%> (+6.84%) ⬆️
... and 71 more

@Dyanngg Dyanngg changed the title Bump ClusterGroup to core/v1alpha3 and support ipBlocks slice [WIP] Bump ClusterGroup to core/v1alpha3 and support ipBlocks slice Mar 30, 2021
@lgtm-com
Copy link

lgtm-com bot commented Apr 6, 2021

This pull request introduces 6 alerts when merging 322b9a2 into 3b294ea - view on LGTM.com

new alerts:

  • 6 for Reflected cross-site scripting

@Dyanngg Dyanngg changed the title [WIP] Bump ClusterGroup to core/v1alpha3 and support ipBlocks slice Bump ClusterGroup to crd/v1alpha3 to deprecate single ipBlock Apr 6, 2021
@lgtm-com
Copy link

lgtm-com bot commented Apr 6, 2021

This pull request introduces 6 alerts when merging 65b1957 into 5752f96 - view on LGTM.com

new alerts:

  • 6 for Reflected cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Apr 16, 2021

This pull request introduces 6 alerts when merging dcea255 into 1890da1 - view on LGTM.com

new alerts:

  • 6 for Reflected cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Apr 16, 2021

This pull request introduces 6 alerts when merging a11ce47 into 1890da1 - view on LGTM.com

new alerts:

  • 6 for Reflected cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Apr 16, 2021

This pull request introduces 6 alerts when merging 1ca667c into 1890da1 - view on LGTM.com

new alerts:

  • 6 for Reflected cross-site scripting

@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2021

Codecov Report

Merging #2008 (771477a) into main (771477a) will not change coverage.
The diff coverage is n/a.

❗ Current head 771477a differs from pull request most recent head 7364b7f. Consider uploading reports for the commit 7364b7f to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2008   +/-   ##
=======================================
  Coverage   41.33%   41.33%           
=======================================
  Files         132      132           
  Lines       16539    16539           
=======================================
  Hits         6837     6837           
  Misses       9115     9115           
  Partials      587      587           
Flag Coverage Δ
unit-tests 41.33% <0.00%> (ø)

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

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

Reviewed 37 files so far, will continue..

build/yamls/base/controller-rbac.yml Outdated Show resolved Hide resolved
cmd/antrea-controller/controller.go Outdated Show resolved Hide resolved
pkg/apiserver/handlers/webhook/convert_crd.go Outdated Show resolved Hide resolved
@antoninbas antoninbas added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API. label Apr 21, 2021
@antoninbas antoninbas added this to the Antrea v1.1 release milestone Apr 21, 2021
@lgtm-com
Copy link

lgtm-com bot commented Apr 21, 2021

This pull request introduces 6 alerts when merging 69887bd into 9bb7179 - view on LGTM.com

new alerts:

  • 6 for Reflected cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Apr 21, 2021

This pull request introduces 1 alert when merging deb023b into 422d92d - view on LGTM.com

new alerts:

  • 1 for Reflected cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented May 11, 2021

This pull request introduces 1 alert when merging ec478e6 into d6e1d7e - view on LGTM.com

new alerts:

  • 1 for Reflected cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented May 11, 2021

This pull request introduces 1 alert when merging b5eab04 into 03fe6ee - view on LGTM.com

new alerts:

  • 1 for Reflected cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented May 11, 2021

This pull request introduces 1 alert when merging 4b7086e into ce7c844 - view on LGTM.com

new alerts:

  • 1 for Reflected cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented May 11, 2021

This pull request introduces 1 alert when merging ed0bded into ce7c844 - view on LGTM.com

new alerts:

  • 1 for Reflected cross-site scripting

@Dyanngg
Copy link
Contributor Author

Dyanngg commented May 11, 2021

/test-all

tnqn
tnqn previously approved these changes May 13, 2021
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM, however conflicts need to be resolved

@Dyanngg Dyanngg changed the title Bump ClusterGroup to crd/v1alpha3 to deprecate single ipBlock Bump ClusterGroup to crd/v1alpha3 and deprecate single ipBlock May 14, 2021
@Dyanngg
Copy link
Contributor Author

Dyanngg commented May 14, 2021

LGTM, however conflicts need to be resolved

Rebased onto the main branch

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2021

This pull request introduces 1 alert when merging 7364b7f into 771477a - view on LGTM.com

new alerts:

  • 1 for Reflected cross-site scripting

@@ -0,0 +1,69 @@
// Copyright 2021 Antrea Authors
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I just noticed this location of this file. I fell it might be a little confusing for developers that the package contains unrelated two topics: one is the calculation of the group members, one is the convertion of ClusterGroup API. What do you think about pkg/apiserver/handlers/webhook/ for this file?

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 moved it to pkg/controller/networkpolicy/convert.go to follow what the validation and mutation webhook's pattern. I'll leave it out of pkg/apiserver/handlers/webhook/ so that this pkg can focus on the webhook infra and not the actual conversion/mutation/validation logic

pkg/controller/grouping/convert.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented May 14, 2021

This pull request introduces 1 alert when merging fa4d34d into 3134f7e - view on LGTM.com

new alerts:

  • 1 for Reflected cross-site scripting

Add apiextensions-apiserver clientset and convert function for two versions of CRDs

Add convert function for the two versions of CRDs

Add controller logic to handle ipBlocks

Fix issues in webhook registration

Fix conversion logic

Add E2E test and fix UT

Address comment

Signed-off-by: Yang Ding <[email protected]>
@Dyanngg
Copy link
Contributor Author

Dyanngg commented May 14, 2021

/test-all

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2021

This pull request introduces 1 alert when merging ca34206 into 3134f7e - view on LGTM.com

new alerts:

  • 1 for Reflected cross-site scripting

@Dyanngg Dyanngg requested a review from tnqn May 14, 2021 20:52
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@Dyanngg
Copy link
Contributor Author

Dyanngg commented May 17, 2021

/test-networkpolicy

1 similar comment
@Dyanngg
Copy link
Contributor Author

Dyanngg commented May 18, 2021

/test-networkpolicy

@Dyanngg Dyanngg merged commit a9c7744 into antrea-io:main May 19, 2021
@Dyanngg Dyanngg deleted the cg-ipb-slice branch May 19, 2021 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants