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

Bump to contour 1.26 and use gateway-provisioner #555

Merged
merged 9 commits into from
Oct 12, 2023

Conversation

ReToCode
Copy link
Member

@ReToCode ReToCode commented Oct 9, 2023

Changes

  • Bump to contour 1.26
  • Drop installation using contour-operator
  • Use gateway-provisioner to deploy gateways
  • Enable tests that now work: websocket,websocket/split,host-rewrite

/kind enhancement

Fixes #495
Fixes #536

Release Note

net-gateway-api now tests with contour version 1.26 which supports websockets and host-rewrites

@knative-prow knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 9, 2023
envoy:
containerPorts:
- name: http
portNumber: 8081
Copy link
Member Author

Choose a reason for hiding this comment

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

@dprotaso @nak3 how was this used? Seems like the gateway below just used port 80?

Not sure if we can define this using the gateway-provisioner. If this is needed, we probably need to use helm to have more configuration flags.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@nak3 nak3 Oct 10, 2023

Choose a reason for hiding this comment

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

IIRC, the 8081 port is used for prober. The current implementation lists endpoint IP which is 8081, and probe against it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, is this still needed? Tests seem to pass without it.

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #555 (a6a2ef4) into main (df07871) will not change coverage.
Report is 2 commits behind head on main.
The diff coverage is n/a.

❗ Current head a6a2ef4 differs from pull request most recent head b237fba. Consider uploading reports for the commit b237fba to get more accurate results

@@           Coverage Diff           @@
##             main     #555   +/-   ##
=======================================
  Coverage   73.08%   73.08%           
=======================================
  Files           9        9           
  Lines         691      691           
=======================================
  Hits          505      505           
  Misses        157      157           
  Partials       29       29           

@ReToCode
Copy link
Member Author

message: 'service "ingress-conformance-host-rewrite-ragyzvdm" is invalid: serving-tests/ingress-conformance-host-rewrite-ragyzvdm
is an ExternalName service, these are not currently enabled. See the config.enableExternalNameService
config file setting, service "ingress-conformance-host-rewrite-ragyzvdm" is
invalid: serving-tests/ingress-conformance-host-rewrite-ragyzvdm is an ExternalName
service, these are not currently enabled. See the config.enableExternalNameService
config file setting'

@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 10, 2023
@ReToCode
Copy link
Member Author

I realised that the websocket tests now also work, so also enabling them.

/assign @dprotaso
/assign @nak3
/assign @KauzClay

@nak3
Copy link
Contributor

nak3 commented Oct 11, 2023

Deploying the changed manifest seems deploy envoy-knative-local-gateway svc with LoadBalancer not ClusterIP. Is that alright for internal envoy?

$ kubectl get svc -n contour-internal 
NAME                            TYPE           CLUSTER-IP      EXTERNAL-IP   PORT(S)        AGE
contour-knative-local-gateway   ClusterIP      10.96.50.182    <none>        8001/TCP       19m
envoy-knative-local-gateway     LoadBalancer   10.96.204.218   <pending>     80:30715/TCP   19m

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2023
@ReToCode
Copy link
Member Author

Good point, seems to be the default. I added configuration for ClusterIP.

@ReToCode
Copy link
Member Author

/retest

@ReToCode
Copy link
Member Author

/retest

1 similar comment
@ReToCode
Copy link
Member Author

/retest

@nak3
Copy link
Contributor

nak3 commented Oct 12, 2023

LGTM
@dprotaso @KauzClay will you or VMware team take a look?

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2023
@knative-prow
Copy link

knative-prow bot commented Oct 12, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, ReToCode

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot merged commit c9bb4b2 into knative-extensions:main Oct 12, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/enhancement lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump Contour to v1.26 Do not use contour-operator
4 participants