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

Return an empty list when NetworkPolicyStats is disabled #2386

Merged

Conversation

PeterEltgroth
Copy link
Contributor

Fixes #2214. When NetworkPolicyStats is disabled an empty list is now returned rather than BadRequest.

In testing this change allowed a Sonobuoy test suite which used Flux to sync, rather than fail to sync.

Contrinuting step 5 says to update "Unreleased"; however, I did not find a section titled that in any of the changelog documents.

Signed-off-by: Peter Eltgroth peltgroth@vmware.com

Signed-off-by: Peter Eltgroth <peltgroth@vmware.com>
antoninbas
antoninbas previously approved these changes Jul 12, 2021
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

Don't worry about updating the "Unreleased" section. I need to update the CONTRIBUTING guidelines apparently :)

@antoninbas antoninbas requested a review from tnqn July 12, 2021 22:20
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.

You may need to update pkg/apiserver/registry/stats/antreaclusternetworkpolicystats/rest.go and pkg/apiserver/registry/stats/antreanetworkpolicystats/rest.go as well, otherwise the other two APIs will still return the error if Flux discovers them.

@PeterEltgroth
Copy link
Contributor Author

You may need to update pkg/apiserver/registry/stats/antreaclusternetworkpolicystats/rest.go and pkg/apiserver/registry/stats/antreanetworkpolicystats/rest.go as well, otherwise the other two APIs will still return the error if Flux discovers them.

I'll take a look today.

…est'

Signed-off-by: Peter Eltgroth <peltgroth@vmware.com>
@tnqn
Copy link
Member

tnqn commented Jul 15, 2021

@PeterEltgroth thanks for the PR. It still missed "pkg/apiserver/registry/stats/antreaclusternetworkpolicystats/rest.go". They have to be 3 stats APIs because:

  • networkpolicystats is for K8s NetworkPolicy
  • antreanetworkpolicystats is for Antrea Namespace level NetworkPolicy
  • antreaclusternetworkpolicystats is for Antrea Cluster level NetworkPolicy

…est'

Signed-off-by: Peter Eltgroth <peltgroth@vmware.com>
@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2021

Codecov Report

Merging #2386 (7e9af0b) into main (c3b14ff) will increase coverage by 17.77%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2386       +/-   ##
===========================================
+ Coverage   42.03%   59.80%   +17.77%     
===========================================
  Files         146      284      +138     
  Lines       18136    22178     +4042     
===========================================
+ Hits         7623    13264     +5641     
+ Misses       9827     7488     -2339     
- Partials      686     1426      +740     
Flag Coverage Δ
e2e-tests ∅ <ø> (?)
kind-e2e-tests 47.01% <0.00%> (?)
unit-tests 41.99% <100.00%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
...stry/stats/antreaclusternetworkpolicystats/rest.go 53.06% <100.00%> (+8.16%) ⬆️
...er/registry/stats/antreanetworkpolicystats/rest.go 52.83% <100.00%> (+7.54%) ⬆️
...piserver/registry/stats/networkpolicystats/rest.go 48.97% <100.00%> (+8.16%) ⬆️
pkg/apiserver/handlers/webhook/convert_crd.go 2.56% <0.00%> (ø)
pkg/apiserver/handlers/webhook/mutation_labels.go 24.71% <0.00%> (ø)
pkg/legacyapis/controlplane/v1beta2/conversion.go 30.00% <0.00%> (ø)
pkg/legacyapis/controlplane/register.go 88.23% <0.00%> (ø)
.../agent/apiserver/handlers/networkpolicy/handler.go 58.33% <0.00%> (ø)
...formers/externalversions/security/v1alpha1/tier.go 64.28% <0.00%> (ø)
pkg/agent/cniserver/ipam/ipam_delegator.go 48.83% <0.00%> (ø)
... and 233 more

@tnqn
Copy link
Member

tnqn commented Jul 16, 2021

/test-all

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, thanks @PeterEltgroth

@tnqn
Copy link
Member

tnqn commented Jul 16, 2021

/skip-conformance
as https://jenkins.antrea-ci.rocks/job/antrea-conformance-for-pull-request/2411/console just failed to clean up after success validation

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.

Should listing NetworkPolicyStats when the Feature Gate is disabled return an empty list instead of an error?
4 participants