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

basic Gateway address support #4443

Merged
merged 3 commits into from
Apr 11, 2022
Merged

Conversation

skriss
Copy link
Member

@skriss skriss commented Apr 1, 2022

Adds provisioner support for a single
Gateway address of type IP or hostname.
The value will be used for the Envoy
service's spec.loadBalancerIP field.

Updates #4235.

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

@codecov
Copy link

codecov bot commented Apr 1, 2022

Codecov Report

Merging #4443 (023f91e) into main (b7404ee) will decrease coverage by 0.06%.
The diff coverage is 74.35%.

❗ Current head 023f91e differs from pull request most recent head 05c7206. Consider uploading reports for the commit 05c7206 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4443      +/-   ##
==========================================
- Coverage   76.07%   76.01%   -0.07%     
==========================================
  Files         138      138              
  Lines       12332    12351      +19     
==========================================
+ Hits         9382     9388       +6     
- Misses       2701     2713      +12     
- Partials      249      250       +1     
Impacted Files Coverage Δ
internal/k8s/statusaddress.go 78.57% <ø> (-1.57%) ⬇️
internal/provisioner/model/model.go 86.95% <ø> (ø)
internal/dag/gatewayapi_processor.go 95.54% <68.75%> (-1.43%) ⬇️
internal/provisioner/controller/gateway.go 41.31% <100.00%> (+2.18%) ⬆️
internal/provisioner/objects/service/service.go 73.27% <100.00%> (+0.11%) ⬆️
internal/sorter/sorter.go 96.96% <0.00%> (-1.82%) ⬇️

@skriss skriss force-pushed the gw-address-support branch from 8618c0d to 268a938 Compare April 1, 2022 21:39
Comment on lines -171 to -186
// Only set the Gateway's address if it has a condition of "Ready: true".
var ready bool
for _, cond := range o.Status.Conditions {
if cond.Type == string(gatewayapi_v1alpha2.GatewayConditionReady) && cond.Status == metav1.ConditionTrue {
ready = true
break
}
}
if !ready {
s.Logger.
WithField("name", o.Name).
WithField("namespace", o.Namespace).
Debug("Gateway is not ready, not setting address")
return
}

Copy link
Member Author

@skriss skriss Apr 1, 2022

Choose a reason for hiding this comment

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

I can't remember why I originally included this logic here, but it creates an issue in the case where an address is requested in the Gateway spec, because the Gateway is then considered "Not Ready" until that address shows up in the status, which this would block. Tests seem OK without it so hoping its removal doesn't cause issues..

@skriss skriss added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Apr 1, 2022
@skriss
Copy link
Member Author

skriss commented Apr 1, 2022

Now reading through all the threads in projectcontour/contour-operator#336 and realizing this implementation may be too simplistic in that it won't work for certain infrastructure providers. Leaving in draft while I do some more investigation..

@skriss skriss force-pushed the gw-address-support branch from fdf13e4 to 0005c41 Compare April 4, 2022 22:25
@skriss skriss marked this pull request as ready for review April 4, 2022 22:47
@skriss skriss requested a review from a team as a code owner April 4, 2022 22:47
@skriss skriss requested review from stevesloka, youngnick and sunjayBhatia and removed request for a team April 4, 2022 22:47
@skriss
Copy link
Member Author

skriss commented Apr 4, 2022

Now reading through all the threads in projectcontour/contour-operator#336 and realizing this implementation may be too simplistic in that it won't work for certain infrastructure providers. Leaving in draft while I do some more investigation..

After looking at those thread some more, I think this still makes sense as an initial implementation that will be useful for some but not all infrastructure providers. We'll also look to add support for setting annotations on the Envoy service which, when combined with this PR, should cover most use cases.

@youngnick
Copy link
Member

Agreed with the approach, having some support is better than none.

@skriss skriss force-pushed the gw-address-support branch from 0005c41 to fd9ff5c Compare April 6, 2022 19:41
@skriss
Copy link
Member Author

skriss commented Apr 6, 2022

Rebased this on #4448 to make use of the testing approach.

skriss added 3 commits April 8, 2022 18:01
Adds provisioner support for a single
Gateway address of type IP or hostname.
The value will be used for the Envoy
service's spec.loadBalancerIP field.

Updates projectcontour#4235.

Signed-off-by: Steve Kriss <krisss@vmware.com>
Sets the Gateway's Ready condition to false
with a reason of AddressNotAssigned if at
lease one address has been requested, but
no requested address has been assigned.

Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
@sunjayBhatia
Copy link
Member

needs a rebase/merge but from first look looks pretty good

@skriss skriss force-pushed the gw-address-support branch from fd9ff5c to 05c7206 Compare April 8, 2022 19:34
@skriss skriss merged commit 1e2beab into projectcontour:main Apr 11, 2022
@skriss skriss deleted the gw-address-support branch April 11, 2022 15:59
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.

3 participants