[Windows] Check if importing certificate is needed #1963

lzhecheng · 2021-03-17T09:41:32Z

Added a new option "ImportCertificate" to decide if certificate is needed.
Default is true
Simplified certificate addition with command "Import-Certificate"

antoninbas

Do the installation instructions in https://github.com/vmware-tanzu/antrea/blob/main/docs/windows.md need to be updated?

hack/windows/Install-OVS.ps1

codecov-io · 2021-03-18T04:10:58Z

Codecov Report

Merging #1963 (8480199) into main (e80ab3b) will decrease coverage by 4.65%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1963      +/-   ##
==========================================
- Coverage   64.35%   59.69%   -4.66%     
==========================================
  Files         193      197       +4     
  Lines       16967    17217     +250     
==========================================
- Hits        10919    10278     -641     
- Misses       4899     5837     +938     
+ Partials     1149     1102      -47

Flag	Coverage Δ
kind-e2e-tests	`45.39% <ø> (-9.82%)`	⬇️
unit-tests	`41.89% <ø> (+0.24%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/apis/controlplane/helper.go	`25.00% <0.00%> (-75.00%)`	⬇️
pkg/apis/controlplane/v1beta2/helper.go	`25.00% <0.00%> (-75.00%)`	⬇️
pkg/controller/networkpolicy/mutate.go	`0.00% <0.00%> (-71.77%)`	⬇️
pkg/controller/networkpolicy/validate.go	`0.00% <0.00%> (-71.57%)`	⬇️
pkg/k8s/name.go	`33.33% <0.00%> (-66.67%)`	⬇️
pkg/controller/networkpolicy/store/group.go	`6.66% <0.00%> (-65.34%)`	⬇️
pkg/agent/controller/networkpolicy/packetin.go	`9.09% <0.00%> (-57.07%)`	⬇️
pkg/apiserver/handlers/webhook/mutation_crd.go	`0.00% <0.00%> (-52.64%)`	⬇️
pkg/apiserver/handlers/webhook/validation_crd.go	`0.00% <0.00%> (-52.64%)`	⬇️
pkg/controller/networkpolicy/tier.go	`0.00% <0.00%> (-52.50%)`	⬇️
... and 46 more

antoninbas · 2021-03-18T20:32:46Z

docs/windows.md

-script to install the OVS driver and register userspace binaries as services.
+script `Install-OVS.ps1` to install the OVS driver and register userspace binaries
+as services. You can set `-ImportCertificate` option of `Install-OVS.ps1` to $false
+if you have a signed OVS package.


if you have a signed OVS package.

does that imply signed by a valid certificated authority? Does it make sense to provide -ImportCertificate $false, and still enable test-signing mode as described below?

Maybe we should separate this information into 2 different sections. "Test-only installation" and "Production installation" (for folks with their own signed OVS package).

Indeed.
I didn't separate into 2 sections since they share many. But I still mentioned the difference (test-only and production) and I think it is enough for users to distinguish. PTAL.

antoninbas

some nits, I think it is better now

docs/windows.md

antoninbas · 2021-03-19T20:51:30Z

docs/windows.md

-script to install the OVS driver and register userspace binaries as services.
+script `Install-OVS.ps1` to install the OVS driver and register userspace binaries
+as services. If you want to use your own signed OVS package for production, you can
+run `Install-OVS.ps1` like this: `Install-OVS.ps1 -ImportCertificate $false -Local -LocalFile <PathToOVSPackage>`


nit: for the full command use a code block with a powershell code specifier

docs/windows.md

jianjuns

LGTM.

lzhecheng · 2021-03-23T01:35:53Z

This change includes a doc and install-ovs script. The script is verified by job "jenkins-windows-ovs" and the build is successful.
/skip-all

lzhecheng · 2021-03-23T06:39:56Z

@antoninbas could you please approve again? I didn't realize there was a "docs and spelling" mistake that "Fenced code blocks should be surrounded by blank lines"

/skip-all

tnqn · 2021-03-23T09:33:26Z

"s/check If importing/check if importing" in PR title
"s/if importing certificate is needed/check if importing certificate is needed" in commit message title

* Added a new option "ImportCertificate" to decide if certificate is needed. Default is true * Simplified certificate addition with command "Import-Certificate"

lzhecheng · 2021-03-23T09:55:26Z

"s/check If importing/check if importing" in PR title
"s/if importing certificate is needed/check if importing certificate is needed" in commit message title

Updated. /skip-all

tnqn

LGTM

* Added a new option "ImportCertificate" to decide if certificate is needed. Default is true * Simplified certificate addition with command "Import-Certificate"

vmwclabot added the cla-not-required label Mar 17, 2021

antoninbas reviewed Mar 17, 2021

View reviewed changes

hack/windows/Install-OVS.ps1 Outdated Show resolved Hide resolved

hack/windows/Install-OVS.ps1 Show resolved Hide resolved

lzhecheng force-pushed the win-install-ovs branch from ef06f7d to 0479dde Compare March 18, 2021 03:10

lzhecheng requested a review from antoninbas March 18, 2021 06:00

antoninbas reviewed Mar 18, 2021

View reviewed changes

lzhecheng force-pushed the win-install-ovs branch from 0479dde to e2768e7 Compare March 19, 2021 04:24

antoninbas reviewed Mar 19, 2021

View reviewed changes

jianjuns reviewed Mar 19, 2021

View reviewed changes

docs/windows.md Outdated Show resolved Hide resolved

lzhecheng force-pushed the win-install-ovs branch from e2768e7 to c4e06d3 Compare March 22, 2021 01:36

jianjuns reviewed Mar 22, 2021

View reviewed changes

antoninbas previously approved these changes Mar 23, 2021

View reviewed changes

lzhecheng dismissed antoninbas’s stale review via b3b7bc4 March 23, 2021 05:24

lzhecheng force-pushed the win-install-ovs branch from c4e06d3 to b3b7bc4 Compare March 23, 2021 05:24

lzhecheng requested a review from antoninbas March 23, 2021 06:40

[Windows] Check if importing certificate is needed

8480199

* Added a new option "ImportCertificate" to decide if certificate is needed. Default is true * Simplified certificate addition with command "Import-Certificate"

lzhecheng changed the title ~~[Windows] check If importing certificate is needed~~ [Windows] Check if importing certificate is needed Mar 23, 2021

lzhecheng force-pushed the win-install-ovs branch from b3b7bc4 to 8480199 Compare March 23, 2021 09:54

antoninbas approved these changes Mar 23, 2021

View reviewed changes

tnqn approved these changes Mar 24, 2021

View reviewed changes

lzhecheng merged commit 9b8440d into antrea-io:main Mar 25, 2021

lzhecheng deleted the win-install-ovs branch March 25, 2021 00:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Check if importing certificate is needed #1963

[Windows] Check if importing certificate is needed #1963

lzhecheng commented Mar 17, 2021

antoninbas left a comment

codecov-io commented Mar 18, 2021 •

edited

Loading

antoninbas Mar 18, 2021

lzhecheng Mar 19, 2021

antoninbas left a comment

antoninbas Mar 19, 2021

lzhecheng Mar 22, 2021

jianjuns left a comment

lzhecheng commented Mar 23, 2021

lzhecheng commented Mar 23, 2021

tnqn commented Mar 23, 2021

lzhecheng commented Mar 23, 2021

tnqn left a comment

[Windows] Check if importing certificate is needed #1963

[Windows] Check if importing certificate is needed #1963

Conversation

lzhecheng commented Mar 17, 2021

antoninbas left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 18, 2021 • edited Loading

Codecov Report

antoninbas Mar 18, 2021

Choose a reason for hiding this comment

lzhecheng Mar 19, 2021

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas Mar 19, 2021

Choose a reason for hiding this comment

lzhecheng Mar 22, 2021

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

lzhecheng commented Mar 23, 2021

lzhecheng commented Mar 23, 2021

tnqn commented Mar 23, 2021

lzhecheng commented Mar 23, 2021

tnqn left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 18, 2021 •

edited

Loading