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 doc for workload identity #3770

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

sonasingh46
Copy link
Contributor

What type of PR is this?
This PR documents workload identity .

What this PR does / why we need it:
This is a workload identity documentation PR.

  • cherry-pick candidate

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

None

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Jul 27, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 27, 2023
@sonasingh46 sonasingh46 changed the title add doc for workload identity [WIP]add doc for workload identity Jul 27, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 27, 2023
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.34% 🎉

Comparison is base (e812eae) 55.57% compared to head (7432707) 55.91%.
Report is 69 commits behind head on main.

❗ Current head 7432707 differs from pull request most recent head 16e3c90. Consider uploading reports for the commit 16e3c90 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3770      +/-   ##
==========================================
+ Coverage   55.57%   55.91%   +0.34%     
==========================================
  Files         190      190              
  Lines       19523    19499      -24     
==========================================
+ Hits        10850    10903      +53     
+ Misses       8064     7981      -83     
- Partials      609      615       +6     

see 36 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon 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 adding this @sonasingh46! Do you think we should add a section for workload identity in the existing doc https://capz.sigs.k8s.io/topics/multitenancy.html#identity-types and maybe link to this doc for setup or just move everything to that page?

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 2, 2023
@sonasingh46
Copy link
Contributor Author

Do you think we should add a section for workload identity in the existing doc https://capz.sigs.k8s.io/topics/multitenancy.html#identity-types and maybe link to this doc for setup or just move everything to that page?

@CecileRobertMichon -- Yes, this is a good idea. @willie-yao is helping me with the docs verification to just get another eye and once the doc is verified and ready, I will move it as you said.

@sonasingh46 sonasingh46 changed the title [WIP]add doc for workload identity add doc for workload identity Aug 2, 2023
@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 Aug 2, 2023
@sonasingh46
Copy link
Contributor Author

@willie-yao -- I have made changes to the doc to add some missing part and that it becomes more easy to follow.
/assign @willie-yao

Copy link
Contributor

@willie-yao willie-yao left a comment

Choose a reason for hiding this comment

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

Thanks so much for the update @sonasingh46! Just a few suggestions from my end while following each step.

docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

This is great! I just made some small grammar comments when first reading it. Next I'll try out the instructions.

docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
Copy link
Contributor

@willie-yao willie-yao left a comment

Choose a reason for hiding this comment

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

Left a few more comments on issues I ran across while going through the doc. Otherwise it is fully working for me!

docs/book/src/topics/workload-identity.md Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Show resolved Hide resolved
@mboersma
Copy link
Contributor

mboersma commented Aug 18, 2023

I wanted to go through the docs like a user would, looking at CAPZ Book web pages. But I ran the local server and couldn't find the doc:

% brew install mdbook
% make -C docs/book serve
% # open localhost:3000 in a browser

I know it's linked from the "multitenancy" topic, but his doc probably needs a ToC entry in the SUMMARY.md file, near here:

- [Windows](./topics/windows.md)

@sonasingh46
Copy link
Contributor Author

/assign @CecileRobertMichon

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2023
@willie-yao
Copy link
Contributor

/lgtm

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

LGTM label has been added.

Git tree hash: 4155453da89472b6aff42786ddd55ba368587d8f

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2023
docs/book/src/topics/multitenancy.md Outdated Show resolved Hide resolved
docs/book/src/topics/multitenancy.md Outdated Show resolved Hide resolved

## Identity Types

### Workload Identity
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Workload Identity
### Workload Identity (Recommended)

### Workload Identity

Follow this [link](./workload-identity.md) for workload identity docs.

### Service Principal With Client Password
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Service Principal With Client Password
### AAD Pod Identity using Service Principal With Client Password (Deprecated)

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't add comments on the lines below but should be the same for other sections

### AAD Pod Identity using Service Principal With Certificate (Deprecated)
### AAD Pod Identity using User-Assigned Managed Identity (Deprecated)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but for the following:

AAD Pod Identity using User-Assigned Managed Identity (Deprecated)

I don't think User assigned managed identity as anything to do with AAD pod identity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

docs/book/src/topics/multitenancy.md Outdated Show resolved Hide resolved
docs/book/src/topics/multitenancy.md Outdated Show resolved Hide resolved
docs/book/src/topics/workload-identity.md Outdated Show resolved Hide resolved
@CecileRobertMichon
Copy link
Contributor

/cc @mboersma

@@ -49,7 +69,7 @@ data:
clientSecret: <client-secret-of-SP-identity>
```

### Service Principal With Certificate
### AAD Pod Identity using Service Principal With Certificate
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### AAD Pod Identity using Service Principal With Certificate
### AAD Pod Identity using Service Principal With Certificate (Deprecated)

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold

please squash!

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 5, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b8a3ea27991efd9f1e4728c239ea9f2aa58e7883

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2023
Copy link
Contributor

@willie-yao willie-yao left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks for all your hard work on this feature!! 🚀

Signed-off-by: Ashutosh Kumar <[email protected]>
@sonasingh46
Copy link
Contributor Author

@CecileRobertMichon @willie-yao -- Have squashed the commit. Sorry about the back and forth.

@CecileRobertMichon
Copy link
Contributor

/hold cancel

thank you for your patience with all my comments @sonasingh46

@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 Sep 6, 2023
@k8s-ci-robot k8s-ci-robot merged commit 803f6dd into kubernetes-sigs:main Sep 6, 2023
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. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants