Skip to content
This repository was archived by the owner on Dec 3, 2019. It is now read-only.

No longer auto-installing maistra on openshift [SRVKS-196]#52

Merged
markusthoemmes merged 2 commits into
TP1from
srvks-196
Aug 14, 2019
Merged

No longer auto-installing maistra on openshift [SRVKS-196]#52
markusthoemmes merged 2 commits into
TP1from
srvks-196

Conversation

@jcrossley3
Copy link
Copy Markdown
Contributor

I removed the SCC stuff, too, which I think was only required for maistra, but I need @nak3 to confirm

I removed the SCC stuff, too, which I think was only required for
maistra, but need @kenjiro to confirm
@nak3
Copy link
Copy Markdown
Contributor

nak3 commented Aug 13, 2019

Please let me confirm. The SCC stuff is necessary for not only Maistra but also any Istio env when it uses side-car injection due to requirement of NET_ADMIN. And Knative's autoscaler and activator still uses the side-car injection as knative/serving#966.

So, do you think that we should let users configure the SCC by themselves?

@markusthoemmes
Copy link
Copy Markdown
Contributor

@nak3 SCC is not necessary as of Maistra TP12 (when using CNI). It's part of their multi-tenancy story to not need these cluster roles. The sidecars no longer need NET_ADMIN access as they don't setup the network through iptables anymore.

@nak3
Copy link
Copy Markdown
Contributor

nak3 commented Aug 14, 2019

Thank you for the clarification. Then, I think that we do not need the privileged SCC.

But I think that the anyuid SCC was added by @savitaashture. So can you also confirm it? @savitaashture

list := &unstructured.UnstructuredList{}
list.SetGroupVersionKind(istio)
if err := r.client.List(context.TODO(), nil, list); err != nil {
msg := fmt.Sprintf("Istio not detected")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the interest of usability: Should we call out "ServiceMesh not detected" here? I know that technically we only need Istio, but the user of Openshift Serverless might think "hey, what the hell is Istio? I only see something like ServiceMesh referenced in the docs". Maybe we could also write something like "Istio not detected, ServiceMesh is likely not installed" to satisfy both ends?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call. Done.

@markusthoemmes
Copy link
Copy Markdown
Contributor

@nak3 thanks for being on the watch 😂 The anyuid thing was needed for custom gateway and was indeed added by @savitaashture. Seems like we no longer need that in newer releases though as I haven't heard any complaints for this setup lately.

Copy link
Copy Markdown
Contributor

@markusthoemmes markusthoemmes left a comment

Choose a reason for hiding this comment

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

LGTM modulo the one comment I left. Thanks for throwing this together quickly!

@markusthoemmes markusthoemmes merged commit 44f19e4 into TP1 Aug 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants