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

Update Venafi username/password authentication section #1618

Merged

Conversation

ilyesAj
Copy link
Contributor

@ilyesAj ilyesAj commented Jan 3, 2025

update username/password section

Signed-off-by: ilyes Ajroud <[email protected]>
@cert-manager-prow cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 3, 2025
Copy link

netlify bot commented Jan 3, 2025

Deploy Preview for cert-manager ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit c58cb87
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager/deploys/678a2ea7e2e1160008c0128b
😎 Deploy Preview https://deploy-preview-1618--cert-manager.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: ilyes Ajroud <[email protected]>
@cert-manager-prow cert-manager-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 3, 2025
@cert-manager-prow cert-manager-prow bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Jan 6, 2025
@ilyesAj ilyesAj force-pushed the feat/update_venafi_documentation branch from a894fa5 to 01cc876 Compare January 6, 2025 13:32
@cert-manager-prow cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jan 6, 2025
Signed-off-by: ilyes Ajroud <[email protected]>
@ilyesAj ilyesAj changed the title Update venafi username/password section Update venafi username/password authentification section Jan 6, 2025
Copy link
Member

@hawksight hawksight left a comment

Choose a reason for hiding this comment

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

Mostly just to correct that no refresh_token is used with cert-manager OS.

Perhaps add some info for users to scope their client integration appropriately as that could help.

Otherwise, thank you for updating the docs, it's better. I have a few more things but they are not in reference to your changes, but previous content, so after this I will endeavour to address them in a follow up.

content/docs/configuration/venafi.md Outdated Show resolved Hide resolved
content/docs/configuration/venafi.md Show resolved Hide resolved
content/docs/configuration/venafi.md Show resolved Hide resolved
@@ -186,19 +186,20 @@ $ kubectl create secret generic \

### Username / Password Authentication

> ⚠️ When you supply a Venafi TPP username and password,
Copy link
Member

Choose a reason for hiding this comment

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

I believe we should now be fine to remove this warning as API keys are old and these docs only affect current and future. So previous version users can still adhear to the warning. Please shout out if anyone things we need to keep this warning on API Keys for TPP.

Signed-off-by: ilyes Ajroud <[email protected]>
@ilyesAj ilyesAj requested a review from hawksight January 12, 2025 13:52
Copy link
Member

@hawksight hawksight left a comment

Choose a reason for hiding this comment

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

One minor comment to remove reference to revoke in some text. Just don't want to give anyone false hope that cert-manager does any sort of revocation. It's probably just a remnant of a misunderstanding of the TPP scopes requried.

Otherwise it looks better and handles the use case, so thank you for taking the time to correct these docs. I'll approve on here and see what / commands I need to approve.

Co-authored-by: Peter Fiddes <[email protected]>
Signed-off-by: ilyes Ajroud <[email protected]>
@hawksight
Copy link
Member

/lgtm

@hawksight
Copy link
Member

/approve

Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks.

There are a few typos to be fixed.

image

Signed-off-by: ilyes Ajroud <[email protected]>
@cert-manager-prow cert-manager-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2025
@ilyesAj
Copy link
Contributor Author

ilyesAj commented Jan 15, 2025

Thanks.

There are a few typos to be fixed.

image

thanks , fixed

@ilyesAj ilyesAj requested a review from wallrj January 15, 2025 22:11
@wallrj wallrj changed the title Update venafi username/password authentification section Update Venafi username/password authentication section Jan 16, 2025
@wallrj
Copy link
Member

wallrj commented Jan 16, 2025

/test pull-cert-manager-website-verify

@cert-manager-prow
Copy link
Contributor

@wallrj: No presubmit jobs available for cert-manager/website@master

In response to this:

/test pull-cert-manager-website-verify

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.

Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.
I left a few suggestions and spotted a couple of typos.

If you don't have time to address those remarks, then I'll happy to merge this and then I can follow up with another PR to bring the rest of this document up to date.

Create an application integration with name and ID `cert-manager`.
Set the "Base Access Settings" to `certificate: manage,revoke`.
Create an application integration with name and ID `cert-manager.io`.
Set the "Base Access Settings" to `certificate: manage`.
Copy link
Member

Choose a reason for hiding this comment

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

I can't remember whether recent versions of TPP include the cert-manager.io API integration by default.
@hawksight do you know?
If so, we can add a note to explain that this step might be optional with recent versions of TPP.

Optional: Here, or in a followup PR we should make sure we're using accurate terminology.

For example, "application integration" should be "API integration".
And "Base Access Settings" should be "Scope".

image image

Copy link
Member

Choose a reason for hiding this comment

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

It is my understanding the cert-manager.io API integration is standard in recent versions at least yes. We can add that in a follow up PR I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wallrj agrees with @hawksight , we can open a followup PR on this .

Venafi has updated its terminology starting from the recent version, and we need to ensure alignment with it. However, I currently do not have the ability to instantiate Venafi test environment to check it.

content/docs/configuration/venafi.md Outdated Show resolved Hide resolved
content/docs/configuration/venafi.md Show resolved Hide resolved
content/docs/configuration/venafi.md Outdated Show resolved Hide resolved
content/docs/configuration/venafi.md Outdated Show resolved Hide resolved
Signed-off-by: ilyes Ajroud <[email protected]>
@ilyesAj ilyesAj requested a review from wallrj January 17, 2025 10:28
@hawksight
Copy link
Member

/lgtm

@cert-manager-prow cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2025
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks

/approve
/lgtm

@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hawksight, wallrj

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

@cert-manager-prow cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2025
@cert-manager-prow cert-manager-prow bot merged commit 1243d17 into cert-manager:master Jan 18, 2025
7 checks passed
@ilyesAj ilyesAj deleted the feat/update_venafi_documentation branch January 18, 2025 11:55
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. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants