Skip to content

tls: fix daysUntilFirstCertExpires() return nil when expired#21428

Merged
mattklein123 merged 7 commits intoenvoyproxy:mainfrom
daixiang0:expried
May 31, 2022
Merged

tls: fix daysUntilFirstCertExpires() return nil when expired#21428
mattklein123 merged 7 commits intoenvoyproxy:mainfrom
daixiang0:expried

Conversation

@daixiang0
Copy link
Copy Markdown
Member

@daixiang0 daixiang0 commented May 25, 2022

Signed-off-by: Loong Dai loong.dai@intel.com

Add days_until_first_cert_expiring stat integration test to ensure no nil error.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Loong Dai <loong.dai@intel.com>
@mattklein123
Copy link
Copy Markdown
Member

There is no point in using an optional if you are going to return 0. Please fix the broken callsite to handle the lack of a value and add a test. Thanks!

/wait

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing and add the test. Please check CI also.

/wait

daixiang0 added 3 commits May 26, 2022 08:51
Signed-off-by: Loong Dai <loong.dai@intel.com>
Signed-off-by: Loong Dai <loong.dai@intel.com>
Signed-off-by: Loong Dai <loong.dai@intel.com>
Signed-off-by: Loong Dai <loong.dai@intel.com>
@daixiang0
Copy link
Copy Markdown
Member Author

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21428 (comment) was created by @daixiang0.

see: more, trace.

Signed-off-by: Loong Dai <loong.dai@intel.com>
@mattklein123
Copy link
Copy Markdown
Member

Please check CI

/wait

@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented May 27, 2022

@daixiang0
Copy link
Copy Markdown
Member Author

@mattklein123
Copy link
Copy Markdown
Member

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21428 (comment) was created by @mattklein123.

see: more, trace.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM with small comments. Sorry for the CI problems I think they are fixed.

/wait

Comment on lines +1 to +3
// NOLINT(namespace-envoy)
constexpr char TEST_EXPIRED__CERT_HASH[] = "FC:F7:07:14:C3:0D:B4:BE:0B:BF:23:9B:C2:09:DA:CD:54:66:"
"32:65:07:50:35:E8:D0:14:ED:D6:B1:96:A1:3C";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is unused. Delete?

Copy link
Copy Markdown
Member Author

@daixiang0 daixiang0 May 28, 2022

Choose a reason for hiding this comment

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

Now certs.sh will generate a hash file, not only here, but also in other tests, although some of the hash files are unused. Do we delete them as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK I see, that's fine, thanks.

Comment on lines +1 to +5
// NOLINT(namespace-envoy)
constexpr char TEST_SERVER_CERT_HASH[] = "DC:E2:2B:65:90:43:9A:36:1C:8E:6D:CA:42:8A:8C:37:C7:A1:77:"
"00:5B:C1:3E:33:8A:B9:2D:04:2C:B1:3F:0A";
constexpr char TEST_SERVER_CERT_NOT_BEFORE[] = "Apr 7 16:46:35 2022 GMT";
constexpr char TEST_SERVER_CERT_NOT_AFTER[] = "Apr 6 16:46:35 2024 GMT";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add comments on where this came from and how to regenerate it if needed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

All are generated by certs.h, refer to https://github.com/envoyproxy/envoy/blob/main/test/config/integration/certs/README.md, or I can update that script and add comments during generating.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 006bbc3 into envoyproxy:main May 31, 2022
@daixiang0 daixiang0 deleted the expried branch May 31, 2022 03:27
Amila-Rukshan pushed a commit to Amila-Rukshan/envoy that referenced this pull request Jun 28, 2022
…oxy#21428)

Signed-off-by: Loong Dai <loong.dai@intel.com>
Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants