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

Remove EnableTLSConfig from antrea agent #2193

Merged
merged 1 commit into from
May 22, 2021

Conversation

zyiou
Copy link
Contributor

@zyiou zyiou commented May 19, 2021

As discussed, EnableTLSToFlowAggregator config is unnecessary for Flow Exporter. We can use tls in protocol of FlowCollectorAddr instead to enable encryption for communication between flow exporter to flow aggregator.

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2021

Codecov Report

Merging #2193 (39aa316) into main (fa54190) will increase coverage by 0.69%.
The diff coverage is 85.00%.

❗ Current head 39aa316 differs from pull request most recent head 0188cb4. Consider uploading reports for the commit 0188cb4 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2193      +/-   ##
==========================================
+ Coverage   60.92%   61.62%   +0.69%     
==========================================
  Files         273      273              
  Lines       20662    20663       +1     
==========================================
+ Hits        12589    12734     +145     
+ Misses       6752     6593     -159     
- Partials     1321     1336      +15     
Flag Coverage Δ
e2e-tests ∅ <ø> (?)
kind-e2e-tests 52.56% <80.00%> (+0.75%) ⬆️
unit-tests 41.18% <5.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
pkg/agent/flowexporter/exporter/exporter.go 76.31% <84.21%> (+1.06%) ⬆️
pkg/util/flowexport/flowexport.go 82.92% <100.00%> (ø)
pkg/controller/networkpolicy/tier.go 47.50% <0.00%> (-5.00%) ⬇️
pkg/controller/traceflow/controller.go 71.68% <0.00%> (ø)
pkg/controller/networkpolicy/status_controller.go 87.09% <0.00%> (+0.64%) ⬆️
...ntroller/networkpolicy/networkpolicy_controller.go 70.00% <0.00%> (+0.64%) ⬆️
pkg/agent/openflow/pipeline.go 71.14% <0.00%> (+1.03%) ⬆️
...gent/controller/networkpolicy/status_controller.go 75.34% <0.00%> (+2.73%) ⬆️
...agent/controller/traceflow/traceflow_controller.go 73.14% <0.00%> (+3.53%) ⬆️
... and 6 more

@zyiou zyiou force-pushed the zyiou/configTLS branch from ee1b769 to 13e1d79 Compare May 19, 2021 18:54
@@ -189,6 +189,7 @@ func setupTestWithIPFIXCollector(tb testing.TB) (*TestData, bool, bool, error) {
}
if testOptions.providerName == "kind" {
// In Kind cluster, there are issues with DNS name resolution on worker nodes.
// We will skip TLS testing for Kind cluster because the server certificate is generated with Flow aggregator's DNS name
Copy link
Member

Choose a reason for hiding this comment

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

As part of Flow Aggregator windows support #2138, we are supporting cluster IP in the certificate. Can we enable the kind test with TLS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. updated

# If no PROTO is given, we consider "tcp" as default. We support "tcp" and "udp"
# L4 transport protocols.
#flowCollectorAddr: "flow-aggregator.flow-aggregator.svc:4739:tcp"
# If no PROTO is given, we consider "tls" as default. We support "tls", "tcp" and
Copy link
Member

@srikartati srikartati May 19, 2021

Choose a reason for hiding this comment

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

We should change for windows yaml too.

@antoninbas antoninbas self-requested a review May 19, 2021 23:21
@zyiou zyiou force-pushed the zyiou/configTLS branch 2 times, most recently from 15d2585 to 39aa316 Compare May 21, 2021 06:30
srikartati
srikartati previously approved these changes May 21, 2021
Copy link
Member

@srikartati srikartati left a comment

Choose a reason for hiding this comment

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

LGTM

antoninbas
antoninbas previously approved these changes May 21, 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.

Because this is still an Alpha feature, I am ok with just removing a config parameter like this.

@zyiou
Copy link
Contributor Author

zyiou commented May 21, 2021

/test-all

@zyiou
Copy link
Contributor Author

zyiou commented May 21, 2021

/test-e2e
/test-conformance
/test-networkpolicy
/test-windows-e2e

@zyiou zyiou dismissed stale reviews from antoninbas and srikartati via 0188cb4 May 21, 2021 21:01
@zyiou zyiou force-pushed the zyiou/configTLS branch from 39aa316 to 0188cb4 Compare May 21, 2021 21:01
@zyiou
Copy link
Contributor Author

zyiou commented May 21, 2021

/test-e2e
/test-conformance
/test-networkpolicy
/test-windows-e2e

Copy link
Member

@srikartati srikartati left a comment

Choose a reason for hiding this comment

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

LGTM

@srikartati srikartati merged commit 29f4deb into antrea-io:main May 22, 2021
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.

4 participants