Add Ambassador addon to kops

[concaf]

This commit adds Ambassador (https://getambassador.io/) addon to
Kops.

Ambassador is installed via 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.

CC: @brucehorn @inercia

[k8s-ci-robot]

Welcome @concaf!

It looks like this is your first PR to kubernetes/kops 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kops has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @concaf. Thanks for your PR.

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

[olemarkus]

Hi,

Thanks for contributing this. I am curious about how you see the value of installing a single ambassador instance as an addon versus using helm or other install methods.
I support any addon that needs specific configuration of kops, and adds validation logic for that (take nodelocaldns or any CNI for example). But this addon just install basic resources without any configuration options or other forms of kops-specific logic.

[johngmyers]

This is a bit overly permissive. Could it be tightened up?

[concaf]

We could tighten it based on the current resources that the operator installs however the main point of concern is that since this operator uses Helm to install the later Ambassador version and keeps it up to date over time, these permissions will go stale and cause unexpected errors for our users - so we've kept the permissions relaxed intentionally.
There are other operators (specifically which perform upgrades) in the wild which do the same - one example would be https://github.com/fluxcd/helm-operator/blob/157d422bc11f127b1d9a5eba18fc8882c4b9cde9/deploy/rbac.yaml#L10-L21. WDYT?

[mikesplain]

I agree with John, this is pretty permissive. That said, I get that an operator can require widely scoped permissions.

I guess my biggest concern is to whom these permissions are granted. Currently it's a service account in the default namespace. Personally, I consider default a dangerous place for an SA that has global permissions. Maybe moving it to kube-system or another namespace we know exists would reduce some risk here as well? I know we don't want to avoid deviating from your official docs but just throwing out a few ideas.

At minimum, maybe we could document the permissiveness in the readme?

[johngmyers]

I've been mulling my position on this. I'm having a hard time supporting such carte blanche permissions. They're not even scoped to the namespace that Ambassador would be installed in.

It seems not unreasonable to me that an addition to the functionality of a subsystem, requiring it to have new types of resources, would also require an upgrade to the operator that manages it. I'm presuming the operator provides some functionality to the subsystem beyond just calling Helm and this would be likely to need to be upgraded as well.

[concaf]

@mikesplain @johngmyers I've made a couple of changes as suggested. I've moved operator and Ambassador's installation to kube-system and documented scope of the permissions here - https://github.com/kubernetes/kops/pull/9115/files#diff-b45153696dcf1520ef38065366461babR16-R19. PTAL 👍

[johngmyers]

/ok-to-test

[brucehorn]

Ole, thanks for the comment. We had a good discussion about this at the office hours last week. We understand the Helm approach (we use Helm ourselves), but we think that addons have some advantages over just using Helm. When looking for ways to provide new functionality such as ingress to kops, web search lands you right in the project at one of the addon directories, which is good--we want people to land in a useful place. Addons provide a good UX for the end users, with a nice integration process. One of the things that we discussed at the office hours was the fact that some of the addons seem not to have been maintained recently--we addressed that by committing to keeping our Datawire addons up to date, and there will be an effort to recruit maintainers for the existing addons. Finally we committed to follow whatever new directions the kops team takes us to provide extension functionality in the future.

[concaf]

@johngmyers @olemarkus gentle nudge ;) Is there anything I can do to take this forward?

[concaf]

/test pull-kops-e2e-kubernetes-aws

[mikesplain]

Thanks so much for your contribution! As a current tester of ambassador and glad to see us suggesting it to users. Hopefully we can talk through a few points and get this out soon!

[mikesplain]

I agree with John, this is pretty permissive. That said, I get that an operator can require widely scoped permissions.

I guess my biggest concern is to whom these permissions are granted. Currently it's a service account in the default namespace. Personally, I consider default a dangerous place for an SA that has global permissions. Maybe moving it to kube-system or another namespace we know exists would reduce some risk here as well? I know we don't want to avoid deviating from your official docs but just throwing out a few ideas.

At minimum, maybe we could document the permissiveness in the readme?

[concaf]

@mikesplain @johngmyers sorry to keep this PR hanging, I got busy with some other stuff.
As suggested (and as I've mentioned in other review comments), I've documented the permissions scope in the docs and moved the installation from default namespace to kube-system. Let me know if you want further tweaking on this 👍

[inercia]

/assign @rdrgmnzs

[concaf]

Hey @rdrgmnzs @johngmyers @olemarkus @mikesplain, gentle nudge ;) Anything I can do to take this forward?

[olemarkus]

I am a bit sceptical to installing this to kube-system. default seems to be the better place even though it is not ideal either.

[concaf]

@olemarkus I changed the namespace to kube-system per suggestions by @mikesplain, see - #9115 (comment)

Maybe moving it to kube-system or another namespace we know exists would reduce some risk here as well?

Would you rather have it in default, or should I create another namespace called ambassador and install it there?

[olemarkus]

/assign @mikesplain
I think Mike is better able to answer this than me.

[mikesplain]

First of all, thanks for sticking with us on this one @concaf.

@olemarkus brings up a good point. Historically pretty much everything Kops managed has lived in kube-system but that doesn't meant it's a good practice. That said, it looks like at least ingress-nginx utilizes it's own namespace so we should probably do that instead. I apologize for the advice, I wasn't sure what the right answer was at the time and was trying to pose it as suggestion.

Can we move it to an ambassador namespace? Either ambassador or ambassador-operator? I assume the first makes the most sense but I leave that up to you. Baring that, consider this LGTMed and ready for approval on my end. Feel free to ping me when that change is done and I'll approve asap!

[concaf]

@mikesplain yeah, makes sense. Default installation instructions for Ambassador deploy it to the ambassador namespace as well.
I've pushed a commit that make this change. PTAL, thanks! :)

[concaf]

@mikesplain gentle nudge ;)

[mikesplain]

Ahh yes thanks @concaf! This looks good, do you mind squashing into a single commit? I'll approve ASAP once that's done. Thank you so much!

[concaf]

@mikesplain sure thing. I've rebased against master and folded everything in one commit. PTAL :) Thanks for sticking and helping with this.

[mikesplain]

Thanks @concaf for the contribution!

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: concaf, mikesplain

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:

• OWNERS [mikesplain]

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