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

internal/provisioner: support using ContourDeployment.Config #4459

Merged
merged 3 commits into from
Apr 12, 2022

Conversation

skriss
Copy link
Member

@skriss skriss commented Apr 11, 2022

When a GatewayClass has a parametersRef
to a ContourDeployment resource that has
Config specified, use that Config when
provisioning Gateways and their associated
ContourConfigurations.

Signed-off-by: Steve Kriss krisss@vmware.com

@codecov
Copy link

codecov bot commented Apr 11, 2022

Codecov Report

Merging #4459 (b0c7b27) into main (667dc3f) will decrease coverage by 0.00%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4459      +/-   ##
==========================================
- Coverage   76.47%   76.47%   -0.01%     
==========================================
  Files         138      138              
  Lines       12427    12433       +6     
==========================================
+ Hits         9504     9508       +4     
- Misses       2674     2676       +2     
  Partials      249      249              
Impacted Files Coverage Δ
internal/provisioner/model/model.go 86.95% <ø> (ø)
internal/provisioner/controller/gateway.go 44.50% <73.33%> (+2.58%) ⬆️
...provisioner/objects/contourconfig/contourconfig.go 86.79% <100.00%> (+1.30%) ⬆️
internal/sorter/sorter.go 97.57% <0.00%> (-1.22%) ⬇️
internal/provisioner/controller/gatewayclass.go 48.71% <0.00%> (+1.70%) ⬆️

@skriss skriss added the release-note/small A small change that needs one line of explanation in the release notes. label Apr 11, 2022
@skriss skriss force-pushed the gw-class-params-contourconfig branch from 1e7441c to 5f756e4 Compare April 11, 2022 20:10
skriss added 2 commits April 12, 2022 17:37
When a GatewayClass has a parametersRef
to a ContourDeployment resource that has
Config specified, use that Config when
provisioning Gateways and their associated
ContourConfigurations.

Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss skriss force-pushed the gw-class-params-contourconfig branch from 01e38b1 to 3c3be71 Compare April 12, 2022 17:38
@skriss skriss marked this pull request as ready for review April 12, 2022 17:39
@skriss skriss requested a review from a team as a code owner April 12, 2022 17:39
@skriss skriss requested review from tsaarni, youngnick and sunjayBhatia and removed request for a team April 12, 2022 17:39
@@ -323,3 +332,22 @@ func (r *gatewayReconciler) ensureContourDeleted(ctx context.Context, contour *m

return errs
}

func (r *gatewayReconciler) getGatewayClassParams(ctx context.Context, gatewayClass *gatewayapi_v1alpha2.GatewayClass) (*contour_api_v1alpha1.ContourDeployment, error) {
if !isContourDeploymentRef(gatewayClass.Spec.ParametersRef) {
Copy link
Member

Choose a reason for hiding this comment

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

is this case possible? since GatewayClasses won't be accepted if they have an invalid parametersref and we won't reconcile a Gateway for a GatewayClass that has not been accepted?

Copy link
Member Author

@skriss skriss Apr 12, 2022

Choose a reason for hiding this comment

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

Yeah probably not, I think this function call is effectively just handling nil-checking on the parametersref right now (since parameters are optional). Can simplify to just do that directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking about it some more, there probably is a (admittedly pretty hard-to-trigger) race condition here, if a GatewayClass has been updated from a valid ContourDeployment ref to a ref to some other resource type but hasn't yet reconciled / marked "Accepted: false", and ~simultaneously a Gateway reconcile is triggered that uses that GatewayClass. Based on that I'm thinking it's safer to keep the check here (it's just checking fields, no cache/API calls so not expensive to execute), but I'll add some comments.

Copy link
Member

Choose a reason for hiding this comment

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

ah yep that is a good point about the race, yeah worth adding some comments (and maybe a test if possible) as to what is expected in this edge case

Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss skriss merged commit 7b0b73c into projectcontour:main Apr 12, 2022
@skriss skriss deleted the gw-class-params-contourconfig branch April 12, 2022 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants