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

Simplify install #466

Merged
merged 3 commits into from
Oct 18, 2021
Merged

Simplify install #466

merged 3 commits into from
Oct 18, 2021

Conversation

mxyng
Copy link
Collaborator

@mxyng mxyng commented Oct 15, 2021

  • Follow Kubernetes recommended labels as defined here
  • Make use of standard helper functions
    • Mostly for name and labels but this practice will make subchart easier
  • Remove some unused RBAC related things from the engine
    • An engine Role shouldn't be necessary anymore since the engine is no longer being accessed through the proxy
    • Remove most rules from engine ClusterRole which has been briefly tested so it's possible some are still needed
      • user|group impersonate
      • pods list
      • services get
      • ingresses list
      • namespaces list

Implement #465

rules:
- apiGroups: [""]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice cleanup

I do think we need:

  • impersonate – users & groups
  • list – pods

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we still need services and pods too:

services, err := clientset.CoreV1().Services(namespace).List(context.TODO(), metav1.ListOptions{
    LabelSelector: "app.kubernetes.io/instance=infra-engine",
})
componentPods, err := clientset.CoreV1().Pods("kube-system").List(context.TODO(), metav1.ListOptions{
    LabelSelector: "component=kube-controller-manager",
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep! Add list - services to that list

Copy link
Collaborator Author

@mxyng mxyng Oct 15, 2021

Choose a reason for hiding this comment

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

services list is still part of the role; only services get was removed. Will add back users|groups impersonate and pods list

@jmorganca
Copy link
Contributor

This is great stuff! What would the new README.md look like? Should we include docs in this update?

rules:
- apiGroups: [""]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we still need services and pods too:

services, err := clientset.CoreV1().Services(namespace).List(context.TODO(), metav1.ListOptions{
    LabelSelector: "app.kubernetes.io/instance=infra-engine",
})
componentPods, err := clientset.CoreV1().Pods("kube-system").List(context.TODO(), metav1.ListOptions{
    LabelSelector: "component=kube-controller-manager",
})

@@ -1,8 +1,9 @@
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: infra-registry-secret-reader
namespace: {{ .Release.Namespace }}
name: {{ include "registry.fullname" . }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be more descriptive in the name of this role still?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that depends on if we expect to add more roles for the registry. IMO keeping it simple and consistent produces the best experience for the user, i.e. if they want to know what permission infra-registry has, they can get the role without first looking up what the role is called.

@mxyng
Copy link
Collaborator Author

mxyng commented Oct 15, 2021

This is great stuff! What would the new README.md look like? Should we include docs in this update?

The omni chart is still a work in progress and I've split it off into a separate PR so this shouldn't require any README.md changes

@mxyng mxyng merged commit 43abfd4 into main Oct 18, 2021
@mxyng mxyng deleted the simplify-install branch October 18, 2021 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants