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

Add disabled-features flag to ContourDeployment for provisioner #6152

Merged
merged 7 commits into from
Feb 9, 2024

Conversation

lubronzhan
Copy link
Contributor

@lubronzhan lubronzhan commented Feb 2, 2024

Fix #5276

Based on previous art from @padlar #5331.

This PR introduce support for disabled-features in provisioner so that any Contour instances associated with this ContourDeployment with disabled-features flag will stop reconciling CRDs defined inside.

Right now supported values are: grpcroutes;tlsroutes;extensionservices;backendtlspolicies

TODO:

  • Add e2e test

@lubronzhan lubronzhan requested a review from a team as a code owner February 2, 2024 02:41
@lubronzhan lubronzhan requested review from tsaarni and skriss and removed request for a team February 2, 2024 02:41
@sunjayBhatia sunjayBhatia requested review from a team, izturn and clayton-gonsalves and removed request for a team February 2, 2024 02:41
@lubronzhan lubronzhan added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Feb 3, 2024
@lubronzhan lubronzhan force-pushed the topic/lubron/fix-5276 branch 2 times, most recently from aaa0f7a to e8412c1 Compare February 3, 2024 05:13
Copy link

codecov bot commented Feb 3, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (0809407) 79.35% compared to head (fcabf6f) 79.42%.
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6152      +/-   ##
==========================================
+ Coverage   79.35%   79.42%   +0.06%     
==========================================
  Files         141      141              
  Lines       16107    16133      +26     
==========================================
+ Hits        12782    12813      +31     
+ Misses       3014     3009       -5     
  Partials      311      311              
Files Coverage Δ
internal/provisioner/controller/gateway.go 57.98% <100.00%> (+0.17%) ⬆️
internal/provisioner/model/model.go 100.00% <100.00%> (ø)
...ernal/provisioner/objects/deployment/deployment.go 89.90% <100.00%> (+0.09%) ⬆️
...ovisioner/objects/rbac/clusterrole/cluster_role.go 64.28% <100.00%> (ø)
internal/provisioner/objects/rbac/role/role.go 63.79% <100.00%> (ø)
internal/provisioner/objects/rbac/util/util.go 100.00% <100.00%> (ø)
internal/provisioner/objects/rbac/rbac.go 10.41% <0.00%> (ø)

... and 1 file with indirect coverage changes

@lubronzhan lubronzhan changed the title [WIP]Add support for disabled-features in provisioner [WIP]Add disabled-features flag to ContourDeployment for provisioner Feb 3, 2024
@lubronzhan lubronzhan force-pushed the topic/lubron/fix-5276 branch 5 times, most recently from bfb7cd1 to 116b712 Compare February 3, 2024 06:48
@lubronzhan lubronzhan changed the title [WIP]Add disabled-features flag to ContourDeployment for provisioner Add disabled-features flag to ContourDeployment for provisioner Feb 3, 2024
@lubronzhan lubronzhan force-pushed the topic/lubron/fix-5276 branch 2 times, most recently from 47fee53 to 6cb8be4 Compare February 5, 2024 18:54
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

just a few comments for now, will do another pass shortly

internal/provisioner/model/model.go Outdated Show resolved Hide resolved
test/e2e/provisioner/provisioner_test.go Show resolved Hide resolved
internal/provisioner/objects/deployment/deployment_test.go Outdated Show resolved Hide resolved
Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>
Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>
Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>
Signed-off-by: lubronzhan <lubronzhan@gmail.com>
Signed-off-by: lubronzhan <lubronzhan@gmail.com>
Signed-off-by: lubronzhan <lubronzhan@gmail.com>
@lubronzhan lubronzhan force-pushed the topic/lubron/fix-5276 branch 2 times, most recently from d1f3b4e to ff86ae5 Compare February 8, 2024 08:07
Signed-off-by: lubronzhan <lubronzhan@gmail.com>
@lubronzhan
Copy link
Contributor Author

lubronzhan commented Feb 8, 2024

The upgrade test has many context deadline exceeded issue, is that related to something known issue

2024/02/08 08:34:22.321719 error making request #79: Get "http://172.18.255.200/": context deadline exceeded (Client.Timeout exceeded while awaiting headers)

Didn't find the button to rerun the test

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

I'll wait to merge in case anyone else wants a look but LGTM

Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

Just one docs nit, otherwise LGTM

@sunjayBhatia I'd be fine getting this in for the upcoming release, WDYT?

apis/projectcontour/v1alpha1/contourdeployment.go Outdated Show resolved Hide resolved
@sunjayBhatia
Copy link
Member

sunjayBhatia commented Feb 9, 2024

The upgrade test has many context deadline exceeded issue, is that related to something known issue

2024/02/08 08:34:22.321719 error making request #79: Get "http://172.18.255.200/": context deadline exceeded (Client.Timeout exceeded while awaiting headers)

Didn't find the button to rerun the test

yeah, I dont think we made an issue for it but we've seen this a lot I believe since we haven't properly coordinated the Envoy/Contour Service/Deployment redeploy order in the provisioner

Just one docs nit, otherwise LGTM

@sunjayBhatia I'd be fine getting this in for the upcoming release, WDYT?

yep sounds good to me 👍🏽

@skriss
Copy link
Member

skriss commented Feb 9, 2024

The upgrade test has many context deadline exceeded issue, is that related to something known issue

2024/02/08 08:34:22.321719 error making request #79: Get "http://172.18.255.200/": context deadline exceeded (Client.Timeout exceeded while awaiting headers)

Didn't find the button to rerun the test

yeah, I dont think we made an issue for it but we've seen this a lot I believe since we haven't properly coordinated the Envoy/Contour Service/Deployment redeploy order in the provisioner

ref. #5375, got closed as stale but still valid to do

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>
@skriss skriss merged commit 39a7d20 into projectcontour:main Feb 9, 2024
26 checks passed
@lubronzhan lubronzhan deleted the topic/lubron/fix-5276 branch February 9, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor A minor change that needs about a paragraph of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gateway Provisioner: Add ability to opt-out of reconciling optional CRDs
3 participants