Skip to content

OSDOCS-1856: Adding docs for automatically syncing LDAP groups #36040

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 14, 2021
Merged

OSDOCS-1856: Adding docs for automatically syncing LDAP groups #36040

merged 1 commit into from
Oct 14, 2021

Conversation

bergerhoffer
Copy link
Contributor

@bergerhoffer bergerhoffer commented Sep 2, 2021 •
edited
Loading

@bergerhoffer bergerhoffer added this to the Future Release milestone Sep 2, 2021
@openshift-ci openshift-ci bot 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. labels Sep 2, 2021
@netlify
Copy link

netlify bot commented Sep 2, 2021 •
edited
Loading

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: 44cacb7

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/616706f96aa137000836685e

😎 Browse the preview: https://deploy-preview-36040--osdocs.netlify.app

bergerhoffer
bergerhoffer commented Sep 8, 2021
View reviewed changes
bergerhoffer
bergerhoffer commented Sep 8, 2021
View reviewed changes
bergerhoffer
bergerhoffer commented Sep 8, 2021
View reviewed changes
bergerhoffer
bergerhoffer commented Sep 8, 2021
View reviewed changes
bergerhoffer
bergerhoffer commented Sep 8, 2021
View reviewed changes
bergerhoffer
bergerhoffer commented Sep 8, 2021
View reviewed changes
bergerhoffer
bergerhoffer commented Sep 8, 2021
View reviewed changes
modules/ldap-auto-syncing.adoc Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stlaz Prior to this, we said that you had to set up the LDAP IDP. If you look at our docs on it [1] the names are ldap-secret for the secret, and ca-config-map. Any thoughts on if we should use those names, or are they too generic?

[1] https://docs.openshift.com/container-platform/4.8/authentication/identity_providers/configuring-ldap-identity-provider.html#identity-provider-creating-ldap-secret_configuring-ldap-identity-provider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stlaz Did you have any thoughts on this question here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can refer to the secret used in the IdP configuration, specific names are probably not necessary in the description.

bergerhoffer
bergerhoffer commented Sep 9, 2021
View reviewed changes
bergerhoffer
bergerhoffer commented Sep 9, 2021
View reviewed changes
bergerhoffer
bergerhoffer commented Sep 9, 2021
View reviewed changes
stlaz
stlaz reviewed Sep 10, 2021
View reviewed changes
@bergerhoffer bergerhoffer force-pushed the OSDOCS-1856 branch 2 times, most recently from b788450 to 2742912 Compare September 14, 2021 23:51
soltysh
soltysh reviewed Sep 15, 2021
View reviewed changes
@bergerhoffer
Copy link
Contributor Author

Note to self: change batch/v1 to batch/v1beta1 for the 4.6 and 4.7 backports

@bergerhoffer bergerhoffer force-pushed the OSDOCS-1856 branch 2 times, most recently from 96fd7f0 to 5399203 Compare September 16, 2021 03:37
bergerhoffer
bergerhoffer commented Sep 16, 2021
View reviewed changes
bergerhoffer
bergerhoffer commented Sep 16, 2021
View reviewed changes
@bobfuru bobfuru self-assigned this Sep 30, 2021
@bobfuru bobfuru added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Sep 30, 2021
bobfuru
bobfuru reviewed Sep 30, 2021
View reviewed changes
Copy link
Contributor

@bobfuru bobfuru left a comment

Choose a reason for hiding this comment

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

A couple of very minor comments but this LGTM!

@bergerhoffer
Copy link
Contributor Author

Note to self again: change batch/v1 to batch/v1beta1 for the 4.6 and 4.7 backports.

yaoli-redhat
yaoli-redhat reviewed Oct 8, 2021
View reviewed changes
modules/ldap-auto-syncing.adoc Outdated
Copy link

@yaoli-redhat yaoli-redhat Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

is the ca filed required when insecure is false? Will 'insecure is true' be supported?

Copy link
Contributor

Choose a reason for hiding this comment

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

you may want to see the already existing docs: https://docs.openshift.com/container-platform/4.8/authentication/ldap-syncing.html#sync-ldap-v1-ldapsyncconfig

yaoli-redhat
yaoli-redhat reviewed Oct 8, 2021
View reviewed changes
modules/ldap-auto-syncing.adoc Outdated
Copy link

Choose a reason for hiding this comment

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

should the url be ldaps rather than ldap?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this being ldaps:// actually, but ldap:// should still work

@bergerhoffer
Copy link
Contributor Author

@stlaz Can you take a look at @yaoli-redhat's comments? I don't know the answer to them.

@bergerhoffer
Copy link
Contributor Author

@yaoli-redhat Per @stlaz's answer and discussion in slack, I updated ldap:// to ldaps://. And he said that insecure:false was the default, so it should be fine to keep, so I left that alone.

Can you take another look with that update and let me know how this looks? Thanks!

@yaoli-redhat
Copy link

@bergerhoffer thanks for the update and lgtm.

@bergerhoffer
Copy link
Contributor Author

Thanks @yaoli-redhat!

@bergerhoffer
Copy link
Contributor Author

/cherrypick enterprise-4.9

@openshift-cherrypick-robot
Copy link

@bergerhoffer: once the present PR merges, I will cherry-pick it on top of enterprise-4.9 in a new PR and assign it to you.

In response to this:

/cherrypick enterprise-4.9

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/test-infra repository.

@bergerhoffer
Copy link
Contributor Author

/cherrypick enterprise-4.8

@openshift-cherrypick-robot
Copy link

@bergerhoffer: once the present PR merges, I will cherry-pick it on top of enterprise-4.8 in a new PR and assign it to you.

In response to this:

/cherrypick enterprise-4.8

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/test-infra repository.

@bergerhoffer
Copy link
Contributor Author

4.6 and 4.7 backports to be done manually so that I can change batch/v1 to batch/v1beta1 for these versions.

@bergerhoffer bergerhoffer merged commit 96f414f into openshift:main Oct 14, 2021
@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Oct 14, 2021
Merged
@openshift-cherrypick-robot
Copy link

@bergerhoffer: new pull request created: #37520

In response to this:

/cherrypick enterprise-4.9

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/test-infra repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Oct 14, 2021
Merged
@openshift-cherrypick-robot
Copy link

@bergerhoffer: new pull request created: #37521

In response to this:

/cherrypick enterprise-4.8

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/test-infra repository.

This was referenced Oct 14, 2021
Merged
Merged
@bergerhoffer bergerhoffer deleted the OSDOCS-1856 branch August 3, 2022 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

5 more reviewers

@jboxman jboxman jboxman left review comments

@soltysh soltysh soltysh left review comments

@stlaz stlaz stlaz left review comments

@bobfuru bobfuru bobfuru left review comments

@yaoli-redhat yaoli-redhat yaoli-redhat left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@bobfuru bobfuru

Labels

branch/enterprise-4.6 branch/enterprise-4.7 branch/enterprise-4.8 branch/enterprise-4.9 peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

OCP 4.9 GA

Development

Successfully merging this pull request may close these issues.

8 participants

@bergerhoffer @yaoli-redhat @openshift-cherrypick-robot @jboxman @soltysh @stlaz @bobfuru @vikram-redhat