Skip to content

Conversation

@jmprusi
Copy link
Contributor

@jmprusi jmprusi commented Nov 29, 2019

Proposed Changes

  • Adds a new installation guide for Knative with Kourier

Related to: knative/serving#5982

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 29, 2019
@knative-prow-robot
Copy link
Contributor

Hi @jmprusi. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow-robot knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 29, 2019
@RichieEscarez RichieEscarez added this to the Backlog milestone Dec 2, 2019
@samodell
Copy link
Contributor

/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 11, 2019
@samodell samodell self-assigned this Dec 11, 2019
```
kubectl apply \
--filename https://raw.githubusercontent.com/3scale/kourier/master/deploy/kourier-knative.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this reference a tag of Kourier that was tested with this release of Knative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We created a "floating tag": https://github.com/3scale/kourier/blob/v0.3/deploy/kourier-knative.yaml

We will override that tag pointing to the latest v0.3.x release. We follow semver, so any breaking change will trigger an increase of the major version, so it should work. wdyt?

Copy link
Contributor

@samodell samodell 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 adding this guide! A few suggestions before we can get it merged in.


Let's do a core install of Knative Serving with the released yaml templates:

1. To install Knative, first install the CRDs by running the following `kubectl apply`
Copy link
Contributor

Choose a reason for hiding this comment

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

We just got automatic numbering to render properly on the website! Lists that are indented at the same level will auto-number for you:

1. one

1. two

1. three

becomes

  1. one

  2. two

  3. three

Copy link
Contributor

Choose a reason for hiding this comment

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

(so the fix here is just to start every numbered list item with "1.")

will appear as `<pending>`. You must wait for this IP address to be assigned
before DNS may be set up.
Get this external IP address with:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Get this external IP address with:
To get the external IP address, use the following command:

kubectl edit cm config-domain --namespace knative-serving
```
Given the external IP above, change the content to:
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to add a line break after "to:"; all code blocks need to have white space before and after.

kubectl apply -f helloworld-go.yaml
```

2. Send a request
Copy link
Contributor

Choose a reason for hiding this comment

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

Above comment about list numbering applies here as well.


[Install with Gloo](./Knative-with-Gloo.md): Gloo functions as a lightweight gateway for Knative. Choose this option if you don't require a service mesh in your cluster and want a lightweight alternative to Istio. Gloo supports all documented Knative features, as well as extensions to Serving such as Eventing and Monitoring.

## Installing Knative with Kourier
Copy link
Contributor

Choose a reason for hiding this comment

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

We're doing the headings in alphabetical order; could you put Kourier after the "Installing Knative with Istio" heading?

Get this external IP address with:
```
$ kubectl get svc kourier -n knative-serving
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$ kubectl get svc kourier -n knative-serving
kubectl get svc kourier -n knative-serving

Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with the rest of the Knative docs, omit the command line prompt.

Copy link
Contributor

@RichieEscarez RichieEscarez left a comment

Choose a reason for hiding this comment

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

Can we fix the filename to use only lowercase? The use of capitals is non-standard and breaks local testing (unlike our hosting server on knative.dev, the localhost does not automatically change all filenames to lowercase and therefore all install guides show as 404 when running the localbuild.sh script).

Change: Knative-with-Kourier.md

To: knative-with-kourier.md

For example, this is what the hosted URL will be (therefore the filename should match and also be lowercase):
https://knative.dev/docs/install/knative-with-kourier/

@jmprusi
Copy link
Contributor Author

jmprusi commented Jan 13, 2020

Thanks for the review @samodell @RichieEscarez.

Sorry for the delay I've been on some much-needed vacations. Updated the PR with all the review comments fixed :)

```
kubectl apply \
--filename https://raw.githubusercontent.com/3scale/kourier/master/deploy/kourier-knative.yaml
Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

diff between the two pages.

25c25
<     - name: http2
---
>     - name: http
33,47d32
< apiVersion: v1
< kind: Service
< metadata:
<   name: kourier-tls
<   namespace: kourier-system
< spec:
<   ports:
<     - name: http2
<       port: 443
<       protocol: TCP
<       targetPort: 8443
<   selector:
<     app: 3scale-kourier-gateway
<   type: LoadBalancer
< ---
87,93c72,73
<             httpGet:
<               httpHeaders:
<                 - name: Host
<                   value: internalkourier
<               path: /__internalkouriersnapshot
<               port: 8081
<               scheme: HTTP
---
>             exec:
>               command: ['ash','-c','(printf "GET /__internalkouriersnapshot HTTP/1.1\r\nHost: internalkourier\r\n\r\n"; sleep 1) | nc -n localhost 8081 | grep "HTTP/1.1 200 OK"']
129c109
<         - image: quay.io/3scale/kourier:v0.3.5
---
>         - image: quay.io/3scale/kourier:v0.3.4
202c182,197
<   type: ClusterIP
---
>   type: LoadBalancer
> ---
> apiVersion: v1
> kind: Service
> metadata:
>   name: kourier-external
>   namespace: kourier-system
> spec:
>   ports:
>     - name: http2
>       port: 80
>       protocol: TCP
>       targetPort: 8080
>   selector:
>     app: 3scale-kourier-gateway
>   type: LoadBalancer
216c211
<   type: ClusterIP
---
>   type: LoadBalancer
224a220,225
>     admin:
>       access_log_path: /tmp/test
>       address:
>         socket_address:
>           address: 0.0.0.0
>           port_value: 19000
239,267d239
<       listeners:
<         - name: stats_listener
<           address:
<             socket_address:
<               address: 0.0.0.0
<               port_value: 9000
<           filter_chains:
<             - filters:
<                 - name: envoy.http_connection_manager
<                   config:
<                     stat_prefix: stats_server
<                     route_config:
<                       virtual_hosts:
<                         - name: admin_interface
<                           domains:
<                             - "*"
<                           routes:
<                             - match:
<                                 safe_regex:
<                                   google_re2: {}
<                                   regex: '/(certs|stats(/prometheus)?|server_info|clusters|listeners|ready)?'
<                                 headers:
<                                   - name: ':method'
<                                     exact_match: GET
<                               route:
<                                 cluster: service_stats
<                     http_filters:
<                       - name: envoy.router
<                         config: {}
269,271c241
<         - name: service_stats
<           connect_timeout: 0.250s
<           type: static
---
>         - connect_timeout: 0.2s
273c243
<             cluster_name: service_stats
---
>             cluster_name: xds_cluster
275,285c245,250
<               lb_endpoints:
<                 endpoint:
<                   address:
<                     pipe:
<                       path: /tmp/envoy.admin
<         - name: xds_cluster
<           connect_timeout: 1s
<           hosts:
<             - socket_address:
<                 address: "kourier-control"
<                 port_value: 18000
---
>               - lb_endpoints:
>                 - endpoint:
>                     address:
>                       socket_address:
>                         address: kourier-control
>                         port_value: 18000
286a252,255
>           upstream_connection_options:
>             tcp_keepalive: {}
>           lb_policy: ROUND_ROBIN
>           name: xds_cluster
288,292d256
<     admin:
<       access_log_path: "/dev/stdout"
<       address:
<         pipe:
<           path: /tmp/envoy.admin

Copy link

Choose a reason for hiding this comment

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

Ah - I see, your version is later. @samodell may wish to inquire of davidor@ who create the one in knative/serving.

@samodell samodell removed triage/needs-eng-input Engineering input is requested needs edits labels Jan 14, 2020
@samodell
Copy link
Contributor

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmprusi, samodell

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-robot knative-prow-robot merged commit aa19f77 into knative:master Jan 22, 2020
samodell pushed a commit to samodell/docs that referenced this pull request Jan 23, 2020
* Adds a new guide describing how to install knative with kourier

* Links the knative with kourier installation guide in the main README file

* Use kourier-system instead of knative-serving when getting kourier IP

* Install Kourier using the included yaml in third_party
knative-prow-robot pushed a commit that referenced this pull request Jan 23, 2020
* Kourier Installation guide for Knative Serving (#2006)

* Adds a new guide describing how to install knative with kourier

* Links the knative with kourier installation guide in the main README file

* Use kourier-system instead of knative-serving when getting kourier IP

* Install Kourier using the included yaml in third_party

* Update the Auto TLS instruction (#2083)

* initial version of auto tls instruction

* Update auto-tls doc to mention namespace cert feature and http01 challenge feature

* address comments

* fix format

* address comments

* Format cmd for easy reading&copy (#2120)

* Updating to LATEST for Kafka Channel/Source (#2121)

* Updating to LATEST for Kafka Channel/Source

* Switching the values to 1 replica, but 3 partitions

Co-authored-by: Joaquim Moreno Prusi <jmprusi@keepalive.io>
Co-authored-by: Zhimin Xiang <zhiminx@google.com>
Co-authored-by: 赤月 <974226358@qq.com>
Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Indicates the PR's author has signed the CLA. kind/install lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

7 participants