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

Add Istio permissions to cluster role #2248

Conversation

haines
Copy link
Contributor

@haines haines commented Aug 25, 2021

Description

Add the permissions shown here to the cluster role created by the Helm chart, so that it can work with Istio.

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 25, 2021
@haines haines force-pushed the helm-chart-cluster-role-permissions branch from aceda25 to bca8174 Compare August 25, 2021 14:17
Copy link

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Bitnami's chart has this config for Istio as well as ones for other CRs

@tariq1890
Copy link
Contributor

yes this would be a great addition to the repo. Requesting your attention on this @Raffo

@haines haines force-pushed the helm-chart-cluster-role-permissions branch 2 times, most recently from 233949a to ae120c6 Compare October 6, 2021 07:54
@haines
Copy link
Contributor Author

haines commented Oct 6, 2021

@njuettner could you please take a look?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2021
@haines haines force-pushed the helm-chart-cluster-role-permissions branch from ae120c6 to 08943af Compare October 7, 2021 05:49
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2021
@sekka1
Copy link

sekka1 commented Oct 8, 2021

I need this as well. Can a maintainer review this?

@sekka1
Copy link

sekka1 commented Oct 8, 2021

/assign @Raffo

@haines
Copy link
Contributor Author

haines commented Oct 20, 2021

@njuettner could you please take a look?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2021
@haines haines force-pushed the helm-chart-cluster-role-permissions branch from 08943af to 7a1cd02 Compare October 26, 2021 07:49
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2021
@haines
Copy link
Contributor Author

haines commented Oct 26, 2021

/assign @stevehipwell

@haines
Copy link
Contributor Author

haines commented Oct 26, 2021

/unassign @Raffo

Copy link
Contributor

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

@haines this is a great bug fix. I'll be using it myself once it's been merged, you've probably saved me having to figure out why this chart wouldn't work with Istio (I only have it in non-Istio clusters).

I think that these RBAC rules should be conditionally created, and as we already have the information in the sources value this would be my preference. Please see suggestions.

charts/external-dns/templates/clusterrole.yaml Outdated Show resolved Hide resolved
charts/external-dns/Chart.yaml Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 26, 2021
@haines haines force-pushed the helm-chart-cluster-role-permissions branch from 7eb93cb to f1225e0 Compare October 26, 2021 09:50
@haines
Copy link
Contributor Author

haines commented Oct 26, 2021

Thanks for the suggestions @stevehipwell! I've applied them now.

I particularly like the conditional creation of RBAC rules - no need to grant unused permissions. Should this also be applied to the other sources that already have permissions in the cluster role (services, ingresses, pods, and nodes)? I can do this as part of this PR, or create a follow-up if you would prefer.

@haines haines requested a review from stevehipwell October 26, 2021 09:54
@stevehipwell
Copy link
Contributor

/assign @Raffo

@haines haines force-pushed the helm-chart-cluster-role-permissions branch from f1225e0 to e46c48f Compare November 2, 2021 14:36
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 2, 2021
@haines
Copy link
Contributor Author

haines commented Nov 2, 2021

@stevehipwell now that you're an approver in the chart owners file, are you able to merge this?

@haines haines requested a review from stevehipwell November 2, 2021 14:37
@stevehipwell
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: agilgur5, haines, stevehipwell

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

@stevehipwell
Copy link
Contributor

@Raffo could you do the lgtm when this finishes CI?

@Raffo
Copy link
Contributor

Raffo commented Nov 3, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2021
@k8s-ci-robot k8s-ci-robot merged commit ab7a965 into kubernetes-sigs:master Nov 3, 2021
@haines haines deleted the helm-chart-cluster-role-permissions branch November 3, 2021 11:37
@stevehipwell
Copy link
Contributor

@Raffo the chart release action failed again due to gh-pages being protected. What's the protected status of the gh-pages branch currently set to? If possible could you paste images here? I'll add a comment below with the manual changes to make to gh-pages once the protection issue has been sorted out.

@stevehipwell
Copy link
Contributor

@Raffo the item to be appended to entries.external-dns is below and the new value for generated should be "2021-11-03T11:40:00.848784158Z".

  - annotations:
      artifacthub.io/changes: |
        - kind: changed
          description: "Update image to v0.10.1"
    apiVersion: v2
    appVersion: 0.10.1
    description: ExternalDNS synchronizes exposed Kubernetes Services and Ingresses with
      DNS providers.
    home: https://github.com/kubernetes-sigs/external-dns/
    icon: https://github.com/kubernetes-sigs/external-dns/raw/master/img/external-dns.png
    keywords:
    - kubernetes
    - external-dns
    - dns
    maintainers:
    - email: steve.hipwell@gmail.com
      name: stevehipwell
    name: external-dns
    sources:
    - https://github.com/kubernetes-sigs/external-dns/
    type: application
    urls:
    - https://github.com/kubernetes-sigs/external-dns/releases/download/external-dns-helm-chart-1.4.1/external-dns-1.4.1.tgz
    version: 1.4.1

@Raffo
Copy link
Contributor

Raffo commented Nov 3, 2021

@stevehipwell

image

I removed that check multiple times and left literally no protection. I believe there is some job that sets it, likely the jobs owned by the kubernetes-sigs org. Can you try reaching out in the CNCF slack to see if we can get that removed for that particular branch?

I will do the manual edit in the meantime.

@stevehipwell
Copy link
Contributor

@Raffo do you have a suggestion for channel?

@Raffo
Copy link
Contributor

Raffo commented Nov 3, 2021

@stevehipwell I would try github-management or kubernetes-contributor, unsure if there are better channels.

@stevehipwell
Copy link
Contributor

@Raffo kubernetes/test-infra#24222 should fix the issue, but the protection might need manually removing after it's been merged.

@stevehipwell
Copy link
Contributor

@Raffo I don't think you added the release entry to index.yaml and just modified the timestamp. Could you add the entry and bump the timestamp and see if that publishes the chart?

@Raffo
Copy link
Contributor

Raffo commented Nov 3, 2021

@stevehipwell the entry you posted seems the same to one that's already there?

@stevehipwell
Copy link
Contributor

@Raffo it's similar but version and urls are different. I forgot to ask @haines to update the annotations for this PR, but it looks like he's picked up on this for his latest PR.

@Raffo
Copy link
Contributor

Raffo commented Nov 3, 2021

@stevehipwell oh sorry, I didn't see the diff, I can add it then.

@stevehipwell
Copy link
Contributor

@Raffo no worries, hopefully this will be the last time!

@Raffo
Copy link
Contributor

Raffo commented Nov 3, 2021

Done!

@stevehipwell
Copy link
Contributor

@Raffo I can confirm that the chart is now published.

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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants