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 missing ldap_ca_cert #1201

Merged

Conversation

winkingturtle-vmw
Copy link
Contributor

WHAT is this change about?

Fix nfs ldap opsfile to include ldap_ca_cert

What customer problem is being addressed? Use customer persona to define the problem e.g. Alana is unable to...

Current opsfile is broken

Please provide any contextual information.

Has a cf-deployment including this change passed cf-acceptance-tests?

  • YES
  • NO (There is not cats test that covers this opsfile)

Does this PR introduce a breaking change? Please take a moment to read through the examples before answering the question.

  • YES - please choose the category from below. Feel free to provide additional details.
  • NO

How should this change be described in cf-deployment release notes?

As an operator I can deploy nfs with ldap and I can then cf push my app

Does this PR introduce a new BOSH release into the base cf-deployment.yml manifest or any ops-files?

  • YES - please specify
  • NO

Does this PR make a change to an experimental or GA'd feature/component?

  • experimental feature/component
  • GA'd feature/component

Please provide Acceptance Criteria for this change?

Using this opsfile will work.

What is the level of urgency for publishing this change?

  • Urgent - unblocks current or future work
  • Slightly Less than Urgent

Tag your pair, your PM, and/or team!

@@ -20,6 +20,9 @@
- type: replace
path: /instance_groups/name=diego-cell/jobs/name=nfsv3driver/properties/nfsv3driver/allowed-in-source?
value: ""
- type: replace
path: /instance_groups/name=diego-cell/jobs/name=nfsv3driver/properties/nfsv3driver/ldap_ca_cert?
value: ((ldap_test_server_ssl.ca))
Copy link
Contributor

@jochenehret jochenehret Sep 9, 2024

Choose a reason for hiding this comment

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

The certificate ldap_test_server_ssl is defined in enable-nfs-test-ldapserver.yml, but that ops file is only for test purposes. Your intention is to make the ldap's SSL CA certificate configurable, right? Should we use another name for the CA (without "test")? You should also enhance the vars-enable-nfs-ldap.yml file to include the CA. See e.g. vars-use-trusted-ca-cert-for-apps.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jochenehret I have added an example value for the CA and updated the PR.

I don't have a strong opinions about the naming convention, since you can't use enable-nfs-ldap.yml without enabled-nfs-test-ldapserver.yml. Even if your ldap server is external to the foundation, you'd have to have it deployed with a cert.

Copy link
Contributor

@jochenehret jochenehret Sep 9, 2024

Choose a reason for hiding this comment

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

You should be able to use enable-nfs-ldap.yml without enable-nfs-test-ldapserver.yml. The latter ops file deploys a test ldap server and also generates a test SSL certificate. For a productive setup, you just need enable-nfs-ldap.yml and provide the CA for the LDAP's SSL certificate (or no CA certificate if the CA is already known to the machine's trust store).

@jochenehret jochenehret requested review from a team September 9, 2024 13:49
nfs-ldap-fqdn: fqdn
ldap_test_server_ssl:
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably just name it ldap_ca_cert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@jochenehret jochenehret merged commit 23c495b into cloudfoundry:develop Sep 13, 2024
1 check passed
@winkingturtle-vmw winkingturtle-vmw deleted the fix-nfs-ldap-opsfile branch September 13, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants