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

Docs for using Ambassador as Ingress Controller #1585

Merged

Conversation

inercia
Copy link
Contributor

@inercia inercia commented May 13, 2020

This PR documents how to install Ambassador as an Ingress Controller.

Ambassador is installed via the Ambassador Operator which is the recommended way of installing Ambassador (it makes sure users always have the latest version of Ambassador installed and takes care of updates as well).

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 13, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @inercia. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 13, 2020
@k8s-ci-robot k8s-ci-robot requested review from amwat and aojea May 13, 2020 19:15
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 13, 2020
@aojea
Copy link
Contributor

aojea commented May 13, 2020

/ok-to-test
/lgtm
Thanks @inercia

@k8s-ci-robot k8s-ci-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 May 13, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2020
@aojea
Copy link
Contributor

aojea commented May 13, 2020

/assign @BenTheElder

@aojea
Copy link
Contributor

aojea commented May 13, 2020

/hold
@inercia the current ingresses are adding some specific configuration to pin the ingress to the control plane node, so the port forwarding defined in the kind-config works.
Is this docs addressing it?

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2020
Copy link
Contributor

@amwat amwat left a comment

Choose a reason for hiding this comment

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

FYI: https://deploy-preview-1585--k8s-kind.netlify.app/docs/user/ingress/#ambassador

+1 to @aojea 's comment about ensuring the node-selector=ingress-ready part.

/hold
I tried to follow the basic instructions (single node) but the ingress didn't seem to work?
can you confirm whether or not it's compatible with the using ingress section.

@inercia inercia force-pushed the inercia/ambassador_ingress branch from a2c0770 to d11189c Compare May 14, 2020 14:44
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2020
@inercia
Copy link
Contributor Author

inercia commented May 14, 2020

Hi @aojea! Thanks for spending some time reviewing this. I didn't realize about the ingress-ready: "true" label. I have modified the code for adding a nodeSelector with that label, so it should happen automatically.

@amwat You are right, it was not working because I forgot to note that Ingress resources must include an annotation kubernetes.io/ingress.class: "ambassador" for being recognized by Ambassador. Otherwise, they will be ignored. I have updated the docs accordingly.

@aojea
Copy link
Contributor

aojea commented May 14, 2020

It's not working for me :(

Also, I think that for this section we should give more detailed steps, most users need more guidance on how to annotate the ingress resource

I forgot to note that Ingress resources must include an annotation kubernetes.io/ingress.class: "ambassador"

Copy link
Contributor

@amwat amwat left a comment

Choose a reason for hiding this comment

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

You will also need to add tolerations for node-role.kubernetes.io/master similar to that shown in the contour section.

Also, +1 to adding the annotation instruction explicitly as a step.
maybe in the using ingress section add a line, something like

If you are using Ambassador also run:
kubectl annotate ingress example-ingress kubernetes.io/ingress.class=ambassador

site/content/docs/user/ingress.md Show resolved Hide resolved
@inercia inercia force-pushed the inercia/ambassador_ingress branch from d11189c to 1a7146c Compare May 15, 2020 14:41
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 15, 2020
@inercia
Copy link
Contributor Author

inercia commented May 15, 2020

It's not working for me :(

What version of Kubernetes are you running? I have discovered it wasn't working with 1.18 (not completely sure the reason), but it should be working fine now. I have also added a kubectl wait in the instruction for being sure Ambassador is installed before going further. I would appreciate it if you could try the new code.

Also, I think that for this section we should give more detailed steps, most users need more guidance on how to annotate the ingress resource

Yes, in fact, Ingress Controllers should use the ingress.class annotation for differentiating who serves an Ingress, but most controllers assume that, if the annotation is not there, they should take over that Ingress (see Nginx and contour). Ambassador does the opposite (this behavior has not been standardized). I have added a note on how to add the annotation.

@inercia inercia force-pushed the inercia/ambassador_ingress branch from 1a7146c to 7b56667 Compare May 15, 2020 14:44
@aojea
Copy link
Contributor

aojea commented May 15, 2020

What version of Kubernetes are you running?

KIND is installing 1.18 by default now,

@inercia inercia force-pushed the inercia/ambassador_ingress branch from 7b56667 to c9d1d1b Compare May 19, 2020 12:16
@inercia
Copy link
Contributor Author

inercia commented May 19, 2020

You will also need to add tolerations for node-role.kubernetes.io/master similar to that shown in the contour section.

@amwat I have added the toleration for running in masters. The new Ambassador deployment will have:

       tolerations:
       - effect: NoSchedule
         key: node-role.kubernetes.io/master
         operator: Equal

@aojea I have added a kubectl patch command for adding the annotation in the Ingress example. I have also added a kubectl wait for waiting for Ambassador to be ready, so it should be more reliable now...

@aojea
Copy link
Contributor

aojea commented May 19, 2020

KIND is installing 1.18 by default now,

no luck yet running the example with 1.18.2
pods are running, ingress patched but the curl to localhost/foo hangs :(

@concaf
Copy link

concaf commented May 20, 2020

@aojea I just tried deploying Ambassador with Kind following the instructions and it seems to work for me.
I've tested this locally on Fedora 30, Kube 1.18, Docker 19.

I tried it again on a fresh VM on GCE (Ubuntu 16.04) with nothing installed except docker, kind and kubectl, and it works there as well. I have created a short screencast of the same for reference - https://asciinema.org/a/5ju0SBnNiP9s6NAhB6a0Sv4TT - PTAL.

@aojea
Copy link
Contributor

aojea commented May 20, 2020

@aojea I just tried deploying Ambassador with Kind following the instructions and it seems to work for me.
I've tested this locally on Fedora 30, Kube 1.18, Docker 19.

I tried it again on a fresh VM on GCE (Ubuntu 16.04) with nothing installed except docker, kind and kubectl, and it works there as well. I have created a short screencast of the same for reference - https://asciinema.org/a/5ju0SBnNiP9s6NAhB6a0Sv4TT - PTAL.

awesome, I think that my problem yesterday was that quay was down 🤔 .

quay.io/datawire/ambassador-operator:v1.2.3

Anyway, thanks for the detailed walkthrough, and sorry for being a bit picky, but it is in the best for everybody that things work out of the box, or that will cause a bad experience to users.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2020
@inercia inercia force-pushed the inercia/ambassador_ingress branch from c9d1d1b to 4ed89b9 Compare May 20, 2020 08:31
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2020
@inercia inercia force-pushed the inercia/ambassador_ingress branch from 4ed89b9 to c1c2d6a Compare May 20, 2020 08:31
@inercia
Copy link
Contributor Author

inercia commented May 20, 2020

Apologies for the push -f: just removed some spurious blanks lines...

@aojea
Copy link
Contributor

aojea commented May 20, 2020

/lgtm
/hold cancel
👍

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 20, 2020
Copy link
Contributor

@amwat amwat 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!
Tested locally and is working great for me.
/lgtm

site/content/docs/user/ingress.md Outdated Show resolved Hide resolved
This commit adds support for installing Ambassador
(https://getambassador.io/) as an Ingress Controller.

Ambassador is installed via the Ambassador Operator which
is the recommended way of installed Ambassador (it makes sure
users always have the latest version of Ambassador installed
and takes care of the update schedule as well).
@inercia inercia force-pushed the inercia/ambassador_ingress branch from c1c2d6a to 04603cf Compare May 20, 2020 14:08
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2020
@inercia
Copy link
Contributor Author

inercia commented May 20, 2020

/retest

@BenTheElder
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, inercia

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 20, 2020
@aojea
Copy link
Contributor

aojea commented May 20, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2020
@k8s-ci-robot k8s-ci-robot merged commit 1413744 into kubernetes-sigs:master May 20, 2020
@inercia
Copy link
Contributor Author

inercia commented May 20, 2020

Thanks @aojea, @BenTheElder, @amwat, and @concaf for your patience and your time. :-)

@inercia inercia deleted the inercia/ambassador_ingress branch May 20, 2020 15:27
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants