Add tests for additional SSL directives and key algorithms. #276 #469

arsenalzp · 2023-09-09T19:00:57Z

Proposed changes

Fix #276

Add tests for additional SSL directives and public key algorithms.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING document
I have run make install-tools and have attached any dependency changes to this pull request
If applicable, I have added tests that prove my fix is effective or that my feature works
If applicable, I have checked that any relevant tests pass after adding my changes
[] If applicable, I have updated any relevant documentation (README.md)
[] If applicable, I have tested my cross-platform changes on Ubuntu 22, Redhat 8, SUSE 15 and FreeBSD 13

…ginx#276) Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

netlify · 2023-09-09T19:01:01Z

✅ Deploy Preview for agent-public-docs canceled.

Name	Link
🔨 Latest commit	`81a4699`
🔍 Latest deploy log	https://app.netlify.com/sites/agent-public-docs/deploys/6509d16eb6bd980008449041

oliveromahony · 2023-09-13T09:47:52Z

Thanks for contributing, if you run make deps it should update all the appropriate files for you

sdk/config_helpers.go

arsenalzp · 2023-09-14T15:55:55Z

Thanks for contributing, if you run make deps it should update all the appropriate files for you

Hello,
it is failing with the following errors:

go: finding module for package github.com/nginx/agent/sdk/v2/proto
go: finding module for package github.com/nginx/agent/sdk/v2/proto/events
go: finding module for package github.com/nginx/agent/sdk/v2/proto/common
go: github.com/nginx/agent/v2/src/core imports
        github.com/nginx/agent/sdk/v2/proto: module github.com/nginx/agent/sdk/v2@latest found (v2.29.0, replaced by ./sdk), but does not contain package github.com/nginx/agent/sdk/v2/proto
go: github.com/nginx/agent/v2/src/extensions imports
        github.com/nginx/agent/sdk/v2/proto/events: module github.com/nginx/agent/sdk/v2@latest found (v2.29.0, replaced by ./sdk), but does not contain package github.com/nginx/agent/sdk/v2/proto/events
go: github.com/nginx/agent/v2 imports
        github.com/nginx/agent/sdk/v2/agent/events imports
        github.com/nginx/agent/sdk/v2/proto/common: module github.com/nginx/agent/sdk/v2@latest found (v2.29.0, replaced by ./sdk), but does not contain package github.com/nginx/agent/sdk/v2/proto/common
make: *** [Makefile:103: deps] Error 1

go version is 1.21.1

aphralG · 2023-09-14T16:04:52Z

Thanks for contributing, if you run make deps it should update all the appropriate files for you

Hello, it is failing with the following errors:

go: finding module for package github.com/nginx/agent/sdk/v2/proto
go: finding module for package github.com/nginx/agent/sdk/v2/proto/events
go: finding module for package github.com/nginx/agent/sdk/v2/proto/common
go: github.com/nginx/agent/v2/src/core imports
        github.com/nginx/agent/sdk/v2/proto: module github.com/nginx/agent/sdk/v2@latest found (v2.29.0, replaced by ./sdk), but does not contain package github.com/nginx/agent/sdk/v2/proto
go: github.com/nginx/agent/v2/src/extensions imports
        github.com/nginx/agent/sdk/v2/proto/events: module github.com/nginx/agent/sdk/v2@latest found (v2.29.0, replaced by ./sdk), but does not contain package github.com/nginx/agent/sdk/v2/proto/events
go: github.com/nginx/agent/v2 imports
        github.com/nginx/agent/sdk/v2/agent/events imports
        github.com/nginx/agent/sdk/v2/proto/common: module github.com/nginx/agent/sdk/v2@latest found (v2.29.0, replaced by ./sdk), but does not contain package github.com/nginx/agent/sdk/v2/proto/common
make: *** [Makefile:103: deps] Error 1

go version is 1.21.1

Hi,
Could you try running go install github.com/pseudomuto/protoc-gen-doc/cmd/protoc-gen-doc@v1.5.1 then try run make deps again. Thank you

arsenalzp · 2023-09-14T16:11:56Z

Hi, Could you try running go install github.com/pseudomuto/protoc-gen-doc/cmd/protoc-gen-doc@v1.5.1 then try run make deps again. Thank you

go install successfully finished:

$ go install github.com/pseudomuto/protoc-gen-doc/cmd/protoc-gen-doc@v1.5.1
go: downloading github.com/golang/protobuf v1.5.2
go: downloading github.com/Masterminds/sprig v2.15.0+incompatible
go: downloading google.golang.org/genproto v0.0.0-20210917145530-b395a37504d4
go: downloading github.com/envoyproxy/protoc-gen-validate v0.3.0-java
go: downloading github.com/mwitkow/go-proto-validators v0.0.0-20180403085117-0950a7990007
go: downloading google.golang.org/protobuf v1.27.1
go: downloading github.com/gogo/protobuf v1.1.1
go: downloading github.com/google/uuid v1.1.2
go: downloading github.com/Masterminds/semver v1.4.2
go: downloading github.com/huandu/xstrings v1.0.0
go: downloading github.com/imdario/mergo v0.3.4
go: downloading golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9
go: downloading github.com/aokoli/goutils v1.0.1
$

However, make deps fell down into the same errors...

aphralG · 2023-09-14T16:49:53Z

Hi, Could you try running go install github.com/pseudomuto/protoc-gen-doc/cmd/protoc-gen-doc@v1.5.1 then try run make deps again. Thank you

go install successfully finished:

$ go install github.com/pseudomuto/protoc-gen-doc/cmd/protoc-gen-doc@v1.5.1
go: downloading github.com/golang/protobuf v1.5.2
go: downloading github.com/Masterminds/sprig v2.15.0+incompatible
go: downloading google.golang.org/genproto v0.0.0-20210917145530-b395a37504d4
go: downloading github.com/envoyproxy/protoc-gen-validate v0.3.0-java
go: downloading github.com/mwitkow/go-proto-validators v0.0.0-20180403085117-0950a7990007
go: downloading google.golang.org/protobuf v1.27.1
go: downloading github.com/gogo/protobuf v1.1.1
go: downloading github.com/google/uuid v1.1.2
go: downloading github.com/Masterminds/semver v1.4.2
go: downloading github.com/huandu/xstrings v1.0.0
go: downloading github.com/imdario/mergo v0.3.4
go: downloading golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9
go: downloading github.com/aokoli/goutils v1.0.1
$

However, make deps fell down into the same errors...

We think there is an issue with go version 1.21.1, we are looking into it at the moment. Until we resolve the issue, you can make your changes and make sure the unit tests pass, then we can run make deps on the branch for you. Thank you

arsenalzp · 2023-09-14T20:07:54Z

I found the issue and fixed it.

go compiler was yielding about missing package proto
it was missed due to protobuf file compilation error
protobuf compilation fell down into errors due to missed protoc-gen-doc and protoc-gen-gogo binaries
$GOBIN was set to $GOPATH/bin
protoc-gen-doc and protoc-gen-gogo were installed bu running go install

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

…nxConfigTest

arsenalzp · 2023-09-15T07:27:45Z

Clause ssl_certificate_key was removed from the code as was mentioned earlier.
Could you please review code again?

Dean-Coakley

Looks good to me. Thanks very much for your work @arsenalzp !

Dean-Coakley

Actually vendor dirs need updating and these are some unit test failures that need to be resolved.

Then it should be good to go.

test/integration/vendor/github.com/nginx/agent/sdk/v2/config_helpers.go

arsenalzp · 2023-09-15T12:56:21Z

I'm trying to investigate root cause why unit test fails; I tried to run make unit-test locally, no fails, unit-test of sdk module, single test of TestGetNginxConfig and TestSslDirectives passes successfully.

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

arsenalzp · 2023-09-15T15:30:53Z

Actually vendor dirs need updating and these are some unit test failures that need to be resolved.

Then it should be good to go.

Necessary changes were made.

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

arsenalzp · 2023-09-15T16:41:50Z

I've just found that some test files use scripts/tls/gen_cert.sh without algorithm name parameter, it leads to certificate generation failure. Necessary changes were made to gen_cert.sh file.
Could you please be so kind to review changes again?

arsenalzp · 2023-09-15T22:13:03Z

It is obviously that the issue is located in generateCertificates() function during gen_cert.sh script running: exit status 2 means there's a permissions problem or a missing keyword in a command or script.
Unit tests pass locally.
Colleagues, I need your help with debugging, because I'm unable to trigger unit test run in CI.

scripts/tls/gen_cert.sh

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

scripts/tls/gen_cert.sh

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

codecov-commenter · 2023-09-20T09:35:55Z

Codecov Report

Patch coverage has no change and project coverage change: -0.14% ⚠️

Comparison is base (556f51e) 66.44% compared to head (81a4699) 66.31%.
Report is 1 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #469      +/-   ##
==========================================
- Coverage   66.44%   66.31%   -0.14%     
==========================================
  Files         118      118              
  Lines       13390    13390              
==========================================
- Hits         8897     8879      -18     
- Misses       3903     3921      +18     
  Partials      590      590

see 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

arsenalzp · 2023-09-20T18:00:27Z

All unit and integration tests were passed! Thank all of you for your assistance!

Add tests for additional SSL directives and public key algorithms. (n…

79d0d07

…ginx#276) Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

github-actions bot added the chore Pull requests for routine tasks label Sep 9, 2023

dhurley requested review from oliveromahony, dhurley, Dean-Coakley, craigell and aphralG September 12, 2023 08:48

oliveromahony reviewed Sep 13, 2023

View reviewed changes

sdk/config_helpers.go Outdated Show resolved Hide resolved

arsenalzp and others added 3 commits September 14, 2023 22:31

Merge branch 'nginx:main' into nginxConfigTest

5bd7525

remove ssl_certificate_key directive from playload

a6df342

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

Merge branch 'nginxConfigTest' of github.com:arsenalzp/agent into ngi…

4419391

…nxConfigTest

github-actions bot added the dependencies label Sep 14, 2023

Dean-Coakley approved these changes Sep 15, 2023

View reviewed changes

Dean-Coakley suggested changes Sep 15, 2023

View reviewed changes

test/integration/vendor/github.com/nginx/agent/sdk/v2/config_helpers.go Outdated Show resolved Hide resolved

remove ssl_certificate_key from tests in vendor files

22d192b

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

add default parameter for algorithm name

bbbee7a

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

dhurley reviewed Sep 18, 2023

View reviewed changes

scripts/tls/gen_cert.sh Outdated Show resolved Hide resolved

fix dsa parameter file location

6b5788a

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

dhurley reviewed Sep 19, 2023

View reviewed changes

scripts/tls/gen_cert.sh Outdated Show resolved Hide resolved

fix gen_cert script to comply with POSIX

81a4699

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

Dean-Coakley approved these changes Sep 20, 2023

View reviewed changes

dhurley approved these changes Sep 21, 2023

View reviewed changes

dhurley merged commit d1f6013 into nginx:main Sep 21, 2023
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for additional SSL directives and key algorithms. #276 #469

Add tests for additional SSL directives and key algorithms. #276 #469

arsenalzp commented Sep 9, 2023

netlify bot commented Sep 9, 2023 •

edited

oliveromahony commented Sep 13, 2023

arsenalzp commented Sep 14, 2023

aphralG commented Sep 14, 2023 •

edited

arsenalzp commented Sep 14, 2023

aphralG commented Sep 14, 2023

arsenalzp commented Sep 14, 2023 •

edited

arsenalzp commented Sep 15, 2023

Dean-Coakley left a comment

Dean-Coakley left a comment •

edited

arsenalzp commented Sep 15, 2023

arsenalzp commented Sep 15, 2023 •

edited

arsenalzp commented Sep 15, 2023

arsenalzp commented Sep 15, 2023 •

edited

codecov-commenter commented Sep 20, 2023

arsenalzp commented Sep 20, 2023

Add tests for additional SSL directives and key algorithms. #276 #469

Add tests for additional SSL directives and key algorithms. #276 #469

Conversation

arsenalzp commented Sep 9, 2023

Proposed changes

Checklist

netlify bot commented Sep 9, 2023 • edited

✅ Deploy Preview for agent-public-docs canceled.

oliveromahony commented Sep 13, 2023

arsenalzp commented Sep 14, 2023

aphralG commented Sep 14, 2023 • edited

arsenalzp commented Sep 14, 2023

aphralG commented Sep 14, 2023

arsenalzp commented Sep 14, 2023 • edited

arsenalzp commented Sep 15, 2023

Dean-Coakley left a comment

Choose a reason for hiding this comment

Dean-Coakley left a comment • edited

Choose a reason for hiding this comment

arsenalzp commented Sep 15, 2023

arsenalzp commented Sep 15, 2023 • edited

arsenalzp commented Sep 15, 2023

arsenalzp commented Sep 15, 2023 • edited

codecov-commenter commented Sep 20, 2023

Codecov Report

arsenalzp commented Sep 20, 2023

netlify bot commented Sep 9, 2023 •

edited

aphralG commented Sep 14, 2023 •

edited

arsenalzp commented Sep 14, 2023 •

edited

Dean-Coakley left a comment •

edited

arsenalzp commented Sep 15, 2023 •

edited

arsenalzp commented Sep 15, 2023 •

edited