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

Fix NetworkPolicy resources dump for Agent's supportbundle #3083

Conversation

antoninbas
Copy link
Contributor

There was an obvious bug where all NetworkPolicy resources
(NetworkPolicies, AddressGroups, AppliedToGroups) were dumped to the
same file, and the file itself ("agentinfo") was subsequently
overwritten with a dump of the appropriate AgentInfo CRD as expected.

This is fixed by dumping these resources to their respective appropriate
files: networkpolicies, addressgroups, appliedtogroups.

In addition, we:

  • switch from JSON to YAML for these resources (along with the AgentInfo
    CRD) for consistency with the files included in the Controller's
    supportbundle.
  • define a custom MarshalYAML function for the IPAddress type. IPAddress
    is typedef'ed to a byte slice, which means that by default it is
    marshalled as an array of individual byte values, making the files
    difficult to read.

Fixes #3082

Signed-off-by: Antonin Bas abas@vmware.com

There was an obvious bug where all NetworkPolicy resources
(NetworkPolicies, AddressGroups, AppliedToGroups) were dumped to the
same file, and the file itself ("agentinfo") was subsequently
overwritten with a dump of the appropriate AgentInfo CRD as expected.

This is fixed by dumping these resources to their respective appropriate
files: networkpolicies, addressgroups, appliedtogroups.

In addition, we:
* switch from JSON to YAML for these resources (along with the AgentInfo
  CRD) for consistency with the files included in the Controller's
  supportbundle.
* define a custom MarshalYAML function for the IPAddress type. IPAddress
  is typedef'ed to a byte slice, which means that by default it is
  marshalled as an array of individual byte values, making the files
  difficult to read.

Fixes antrea-io#3082

Signed-off-by: Antonin Bas <abas@vmware.com>
@codecov-commenter
Copy link

codecov-commenter commented Dec 3, 2021

Codecov Report

Merging #3083 (4fd59cc) into main (b7da020) will decrease coverage by 7.74%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3083      +/-   ##
==========================================
- Coverage   59.79%   52.05%   -7.75%     
==========================================
  Files         292      454     +162     
  Lines       24732    51687   +26955     
==========================================
+ Hits        14788    26904   +12116     
- Misses       8317    22494   +14177     
- Partials     1627     2289     +662     
Flag Coverage Δ
e2e-tests 52.50% <55.55%> (?)
integration-tests 31.60% <ø> (?)
kind-e2e-tests 48.20% <64.28%> (+1.37%) ⬆️
unit-tests 40.14% <0.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
pkg/apis/controlplane/v1beta2/marshal.go 0.00% <0.00%> (ø)
pkg/support/dump.go 55.42% <62.50%> (-2.30%) ⬇️
pkg/agent/cniserver/pod_configuration_linux.go 26.31% <0.00%> (-40.36%) ⬇️
pkg/agent/flowexporter/connections/conntrack.go 45.71% <0.00%> (-30.48%) ⬇️
pkg/controller/networkpolicy/endpoint_querier.go 61.46% <0.00%> (-29.97%) ⬇️
.../agent/flowexporter/priorityqueue/priorityqueue.go 63.29% <0.00%> (-29.31%) ⬇️
pkg/ipam/ipallocator/allocator.go 56.33% <0.00%> (-27.88%) ⬇️
pkg/agent/cniserver/ipam/antrea_ipam_controller.go 40.47% <0.00%> (-24.91%) ⬇️
pkg/ipam/poolallocator/allocator.go 37.80% <0.00%> (-24.62%) ⬇️
.../registry/networkpolicy/clustergroupmember/rest.go 64.28% <0.00%> (-23.95%) ⬇️
... and 439 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.

LGTM

)

func (a IPAddress) MarshalYAML() (interface{}, error) {
return net.IP(a).String(), nil
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, I'm wondering if we should change IP field to string like K8s objects, to make the response readable even when troubleshooting with curl or kubectl get addressgroup directly.
We wanted to save some space when making it []byte, however, it seems we forgot to call To4 for IPv4 address so it actually uses more space (16bytes vs 15bytes) since the very begining.
@jianjuns @antoninbas

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have clues string or (4) bytes wont make big difference we can switch to string. I feel typically network control plane will optimize though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

either way works for me. If we switch to strings, it could maybe get a bit long for some IPv6 addresses?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, 39 bytes at most. We don't need to make a decision right now as it needs a new API version. We could consider this when it's necessary to have a new version.

@antoninbas
Copy link
Contributor Author

/test-all

@antoninbas
Copy link
Contributor Author

/test-all
/test-integration

1 similar comment
@antoninbas
Copy link
Contributor Author

/test-all
/test-integration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/backport Indicates a PR that requires backports.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supportbundle for each Agent is missing NetworkPolicy resources
4 participants