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

📖 : Proposal : Discontinue Kube RBAC Proxy in Default Kubebuilder Scaffolding #3860

Merged

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Apr 10, 2024

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 10, 2024
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2024
@camilamacedo86 camilamacedo86 changed the title Proposal : Discontinue Kube RBAC Proxy in Default Kubebuilder Scaffolding :doc: Proposal : Discontinue Kube RBAC Proxy in Default Kubebuilder Scaffolding Apr 10, 2024
@camilamacedo86 camilamacedo86 changed the title :doc: Proposal : Discontinue Kube RBAC Proxy in Default Kubebuilder Scaffolding 📖 : Proposal : Discontinue Kube RBAC Proxy in Default Kubebuilder Scaffolding Apr 10, 2024
@camilamacedo86 camilamacedo86 force-pushed the doc-proposal-rbac-proxy branch 2 times, most recently from 9836b4f to 2e3a7de Compare April 11, 2024 07:27
@camilamacedo86 camilamacedo86 marked this pull request as ready for review April 15, 2024 18:28
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 15, 2024
@camilamacedo86
Copy link
Member Author

/hold

To allow approvals and more people review this one

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 16, 2024
Comment on lines 47 to 49
> However, if we be able to combine the cert-manager and the new feature provided
> by controller-runtime we can achieve the same or a superior level of protection
> without relay in any extra third-party dependency.

Choose a reason for hiding this comment

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

link to where this is documented?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @joejulian,

Thank you for your review and time.
Could you please let us know what documentation are you looking for here?
What exactly clarifications do you need in the OpenQuestion answered.

Choose a reason for hiding this comment

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

Someone who was previously relying on kube-rbac-proxy wants to now follow the new recommended method of using cert-manager and the new mtls feature (if I'm understanding this correctly), where do they find the documentation for that? Could we link that here?

Copy link
Member Author

@camilamacedo86 camilamacedo86 Apr 18, 2024

Choose a reason for hiding this comment

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

That is the proposal for change
We are proposing it all get done in phases
So, the first phase does not use cert-manager
See: #3853

Then, in a follow up of this PR we need to add a config in the kustomize
that will allow we patch the serviceMonitor and certs such as we do now for webhooks
See that we have in the config/default/ a patch for cert-manager + webhooks:
See

# [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER' prefix.
# Uncomment the following replacements to add the cert-manager CA injection annotations
#replacements:
# - source: # Add cert-manager annotation to ValidatingWebhookConfiguration, MutatingWebhookConfiguration and CRDs
# kind: Certificate
# group: cert-manager.io
# version: v1
# name: serving-cert # this name should match the one in certificate.yaml
# fieldPath: .metadata.namespace # namespace of the certificate CR
# targets:
# - select:
# kind: ValidatingWebhookConfiguration
# fieldPaths:
# - .metadata.annotations.[cert-manager.io/inject-ca-from]
# options:
# delimiter: '/'
# index: 0
# create: true
# - select:
# kind: MutatingWebhookConfiguration
# fieldPaths:
# - .metadata.annotations.[cert-manager.io/inject-ca-from]
# options:
# delimiter: '/'
# index: 0
# create: true
# - select:
# kind: CustomResourceDefinition
# fieldPaths:
# - .metadata.annotations.[cert-manager.io/inject-ca-from]
# options:
# delimiter: '/'
# index: 0
# create: true
# - source:
# kind: Certificate
# group: cert-manager.io
# version: v1
# name: serving-cert # this name should match the one in certificate.yaml
# fieldPath: .metadata.name
# targets:
# - select:
# kind: ValidatingWebhookConfiguration
# fieldPaths:
# - .metadata.annotations.[cert-manager.io/inject-ca-from]
# options:
# delimiter: '/'
# index: 1
# create: true
# - select:
# kind: MutatingWebhookConfiguration
# fieldPaths:
# - .metadata.annotations.[cert-manager.io/inject-ca-from]
# options:
# delimiter: '/'
# index: 1
# create: true
# - select:
# kind: CustomResourceDefinition
# fieldPaths:
# - .metadata.annotations.[cert-manager.io/inject-ca-from]
# options:
# delimiter: '/'
# index: 1
# create: true
# - source: # Add cert-manager annotation to the webhook Service
# kind: Service
# version: v1
# name: webhook-service
# fieldPath: .metadata.name # namespace of the service
# targets:
# - select:
# kind: Certificate
# group: cert-manager.io
# version: v1
# fieldPaths:
# - .spec.dnsNames.0
# - .spec.dnsNames.1
# options:
# delimiter: '.'
# index: 0
# create: true
# - source:
# kind: Service
# version: v1
# name: webhook-service
# fieldPath: .metadata.namespace # namespace of the service
# targets:
# - select:
# kind: Certificate
# group: cert-manager.io
# version: v1
# fieldPaths:
# - .spec.dnsNames.0
# - .spec.dnsNames.1
# options:
# delimiter: '.'
# index: 1
# create: true
and https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/config/default/manager_webhook_patch.yaml

In the follow up we will need to do something similar
We will need to have something like

# [METRICS WITH HTTPS] To enable the ServiceMonitor using HTTPS uncomment the following line
# Note that for that works you also need ensure that you enable cert-manager in your project
- path: metrics_https_patch.yaml

That should patch https://github.com/kubernetes-sigs/kubebuilder/pull/3853/files#diff-8545904ae77cfcef7da403b4fe25b822439217abe9307d5d31b29690140d0fc6

As part of the task we will need also to update the docs to clarify how that works and etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @joejulian

It was also added in the proposal.
See that we are adding examples either how the YAML would end up.

Thank you for raise it out.

Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

This all LGTM. Thanks for putting this together @camilamacedo86 !

@camilamacedo86
Copy link
Member Author

Hi @joejulian

Have we your LGTM as well now that all suggestions are applied?
Or have you any objection of the proposal itself?

Copy link

@joejulian joejulian left a comment

Choose a reason for hiding this comment

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

Yes, thanks, lgtm fwiw.

Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

I'm yet to get familiar with K8s network policies and go over the changes with controller-runtime. However, on reading the proposal, the proposed option seems better than the alternatives and the transition plan seems good.

Thanks @camilamacedo86!
/lgtm
/approve

### Phase 4: When kube-rbac-proxy be accepted under the umbrella

Once kube-rbac-proxy is included in the Kubernetes umbrella,
Kubebuilder maintainers can support its integration through a [plugin](https://kubebuilder.io/plugins/plugins).
Copy link
Member

Choose a reason for hiding this comment

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

Can we also have the link to the issue that tracks the inclusion of kube-rbac-proxy to k8s-apimachinery umbrella ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, everettraven, joejulian, varshaprasad96

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 [camilamacedo86,varshaprasad96]

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

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2024
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@camilamacedo86 camilamacedo86 force-pushed the doc-proposal-rbac-proxy branch 2 times, most recently from ef6f984 to 26847a9 Compare April 23, 2024 06:24
Copy link
Member Author

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

It seems that we have enough approvals and lgtms and no objections at all to the proposal.

It was either communicate in the channels and mailing list.
Sadly, as you can see we either have not so many options since we cannot keep maintained and ensure that build and promotion of kube-rbac-proxy images

Therefore, it seems good enough to fly now.

PS.: We can create many follow up PR to change/improve and etc this one as we see fit as well.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2024
@camilamacedo86 camilamacedo86 force-pushed the doc-proposal-rbac-proxy branch 2 times, most recently from 1f25c7c to f395295 Compare April 23, 2024 06:40
@camilamacedo86 camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2024
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2024
@camilamacedo86 camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2024
@k8s-ci-robot k8s-ci-robot merged commit 97a4f72 into kubernetes-sigs:master Apr 23, 2024
18 checks passed
@camilamacedo86 camilamacedo86 deleted the doc-proposal-rbac-proxy branch April 23, 2024 13:11
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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-blocker size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants