Skip to content

Adding read perms for pods and services to DNS01 ClusterRole#7823

Open
ali-hamza-noor wants to merge 3 commits intocert-manager:masterfrom
ali-hamza-noor:test/http-challenges
Open

Adding read perms for pods and services to DNS01 ClusterRole#7823
ali-hamza-noor wants to merge 3 commits intocert-manager:masterfrom
ali-hamza-noor:test/http-challenges

Conversation

@ali-hamza-noor
Copy link
Copy Markdown
Contributor

@ali-hamza-noor ali-hamza-noor commented Jun 20, 2025 •
edited by SgtCoDFish
Loading

Pull Request Motivation

The DNS01 role needs the read perms as there was a bug (#7827) reported whenever the users were disabling the HTTP01 role

TODO @ali-hamza-noor : add reference to #7666

Kind

/kind bug

Release Note

Fix for disabling the HTTP01 role

@ali-hamza-noor
testing http challenges role
27a9655 
Signed-off-by: alnoor <alihamzanoor99@gmail.com>
@cert-manager-prow
Copy link
Copy Markdown
Contributor

cert-manager-prow bot commented Jun 20, 2025

@ali-hamza-noor: The label(s) kind/release, kind/note cannot be applied, because the repository doesn't have them.

Details

In response to this:

Pull Request Motivation

Kind

/kind

Release Note

NONE

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-sigs/prow repository.

@cert-manager-prow cert-manager-prow bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Jun 20, 2025
@cert-manager-prow
Copy link
Copy Markdown
Contributor

cert-manager-prow bot commented Jun 20, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign erikgb for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details 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

@cert-manager-prow cert-manager-prow bot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. area/deploy Indicates a PR modifies deployment configuration size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 20, 2025
@ali-hamza-noor
Copy link
Copy Markdown
Contributor Author

ali-hamza-noor commented Jun 20, 2025

/ok-to-test

@ali-hamza-noor ali-hamza-noor changed the title testing http challenges role WIP: testing http challenges role Jun 20, 2025
@cert-manager-prow cert-manager-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 20, 2025
@ali-hamza-noor
add perms to list and get pods and services
2728e17 
Signed-off-by: alihamzanoor <alihamzanoor99@gmail.com>
@ali-hamza-noor
fix: Add the read functionality for pods and services
d068684 
Signed-off-by: alnoor <alihamzanoor99@gmail.com>
@cert-manager-prow cert-manager-prow bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 24, 2025
@ali-hamza-noor ali-hamza-noor changed the title WIP: testing http challenges role Adding read perms for pods and services to DNS01 ClusterRole Jun 24, 2025
@cert-manager-prow cert-manager-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2025
inteon
inteon reviewed Jul 2, 2025
View reviewed changes
deploy/charts/cert-manager/templates/rbac.yaml
verbs: ["create", "patch"]
- apiGroups: [ "" ]
resources: [ "pods", "services" ]
verbs: [ "get", "list", "watch"]
Copy link
Copy Markdown
Member

@inteon inteon Jul 2, 2025

Choose a reason for hiding this comment

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

I think there are some other permissions missing still:

  - apiGroups: [""]
    resources: ["pods", "services"]
    verbs: ["get", "list", "watch"]
  - apiGroups: ["networking.k8s.io"]
    resources: ["ingresses"]
    verbs: ["get", "list", "watch"]
  - apiGroups: [ "gateway.networking.k8s.io" ]
    resources: [ "httproutes" ]
    verbs: ["get", "list", "watch"]

Anyway, I think it would be better to revert this in v1.18 and include a fixed version in v1.19 instead.
Ideally, we could eg. stop these informers.

Copy link
Copy Markdown
Contributor Author

@ali-hamza-noor ali-hamza-noor Jul 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah sure. With regards to additional perms for ingresses and httproutes, I think we agreed on not needing those in one of the stand-ups before. Let's talk more on this in the stand-up. I do like the idea of stopping informers.

@inteon inteon mentioned this pull request Jul 2, 2025
Merged
@wallrj
Copy link
Copy Markdown
Member

wallrj commented Jul 2, 2025

@ali-hamza-noor We discussed this at stand up this morning and I agree with @inteon that we should revert this feature and try again in 1.19.

Let's try and figure out a better UX for this feature; something like global.disableHTTP01 and figure out how to disable the Pod informer. That would have an added benefit of saving some memory in the controller because it would no longer need to cache all the Pods in the cluster.

@cert-manager-prow cert-manager-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2025
@cert-manager-prow
Copy link
Copy Markdown
Contributor

cert-manager-prow bot commented Jul 2, 2025

PR needs rebase.

Details

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-sigs/prow repository.

@cert-manager-prow
Copy link
Copy Markdown
Contributor

cert-manager-prow bot commented Dec 30, 2025

@ali-hamza-noor: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cert-manager-master-e2e-v1-34-upgrade d068684 link true /test pull-cert-manager-master-e2e-v1-34-upgrade
pull-cert-manager-master-e2e-v1-35-upgrade d068684 link true /test pull-cert-manager-master-e2e-v1-35-upgrade

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@inteon inteon inteon left review comments

Assignees

No one assigned

Labels

area/deploy Indicates a PR modifies deployment configuration dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ali-hamza-noor @wallrj @inteon