cloudfoundry / cloudfoundry/silk-release
Bug: Loggregator certificate properties are now required after the removal of 'use_v2_api'
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 14
- Forks
- 36
- Avg merge
- 10h 14m
- Merged PRs (30d)
- 2
Description
Current behavior
Background
The following commits aim to remove support for the V1 Loggregator API:
- https://github.com/cloudfoundry/silk-release/commit/e1805919a49e41c531bff148d213818aa1e66064
- https://github.com/cloudfoundry/diego-logging-client/commit/99f67ee4ef1ba98f06f52bf33a54c41211ef1265
However, by doing so, these changes now also require the actual initialization of the newV2IngressClient() in the diego-logging-client code.
Where as before, if the loggregator.* properties were missing/undefined on the vxlan-policy-agent job, the diego-logging-client would simply initialize a noopIngressClient.
In our case, the latter was the case. We did not have any of the loggregator.* properties defined on the vxlan-policy-agent job, which had worked fine up until now. Or at least, it didn't cause any jobs to fail.
However, during our last deployment now, and without any errors from the templating engine in BOSH about missing required properties, the deployment failed to start the Diego Cell instances with the following error:
cfnetworking: initializing ingress client: failed to load keypair: tls: failed to find any PEM data in certificate input
This was especially painful to discover during deployment (and not templating), since this is one of our larger Cloud Foundry deployments, which takes a few hours to deploy...
Root Cause
Since the above mentioned commits now force the full initialization of the newV2IngressClient(), which in turn requires all of the TLS properties, the following templating using if_p() is misleading/incorrect:
<% if_p("loggregator.ca_cert") do |value| %><% if_p("loggregator.cert") do |value| %><% if_p("loggregator.key") do |value| %>
Because these are now effectively required properties, without any default values in the spec file.
Desired behavior
Describe how the problem should be fixed
Change the loggregator.* properties to be required by switching from if_p() to p() in the above mentioned templates.
This will ensure to surface the required properties to the deployment/manifest operator, notifyng them that this release now requires TLS certificates for the Loggregator client, should those properties not be defined on the job definition in the deployment.
Does this require a new bosh release?
No.
Does it require configuration changes in cf-deployment?
No.
Do we need to have a special release note?
No. People who are already affected by this bug, have probably fixed it already. This is a pure quality-of-life change, to surface these requirements earlier during the templating phase, instead of failing during deployment phase.
Do we need to update repo documentation?
No. The Loggregator Client integration does not seem to be explicitly mentioned in any of the docs/* files. The spec file for the vxlan-policy-agent already indicates that these are required properties, by not providing any default: values (as is natural for certificate properties).
Affected Version
3.111.0
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Locate the vxlan-policy-agent templates containing the three loggregator.ca_cert, loggregator.cert, and loggregator.key checks, then compare them with the job's spec properties. Change the templating so missing certificate properties are reported during BOSH manifest generation, and verify the deployment no longer reaches initialization with absent TLS data.
Written by the indexing model from the issue text.
Assessment
- Domain
- infrastructure, release
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 68/100