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

Deprecated Syslog Binding URL API removal #3527

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chombium
Copy link
Contributor

@chombium chombium commented Nov 29, 2023

Code clean up after the deprecation of the /v4 API for the Syslog Binding URL.

With the introduction of mTLS for Syslog Drains we have changed the internal structure of the bindings. In order to enable updating without downtime of the cloud controller, the syslog binding cache and the syslog agent, we have introduced two endpoints for reading the syslog binding in the old and new format. This PR cleans up the code of the legacy syslog binding API endpoint.

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change:

  • An explanation of the use cases your change solves

  • Links to any other associated PRs

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

I have done extensive manual testing on a full fledged Cloud Foundry deployment.

@cloudfoundry/wg-app-runtime-platform-logging-and-metrics-approvers can someone please take a look at this? I've also cleaned up the usage of the old URL in the Syslog Binding Cache and the Syslog Agent in the following PR.

Copy link
Member

@mkocher mkocher left a comment

Choose a reason for hiding this comment

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

Nice to get some old code deleted. We looked into this and think this will be a safe change to make in our environments.

@@ -12,280 +12,6 @@ module VCAP::CloudController
let!(:binding_with_drain1) { ServiceBinding.make(syslog_drain_url: 'fish,finger', app: app_obj, service_instance: instance1) }
let!(:binding_with_drain2) { ServiceBinding.make(syslog_drain_url: 'foobar', app: app_obj, service_instance: instance2) }

describe 'GET /internal/v4/syslog_drain_urls' do
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this PR removed the test coverage for the shared hostname_from_app_name method. It would be good to add the test cases to the v5 specs.

@mkocher
Copy link
Member

mkocher commented Dec 4, 2023

We took another look and found one internal consumer of this internal API. We're going to work on moving it to v5 to not hold up this cleanup.

@chombium
Copy link
Contributor Author

Hi @mkocher, I've updated all the tests which were included in the v4 API to work with v5 API, so that we have all of the tests which we had before.

When I only run the whole RSpec.describe SyslogDrainUrlsInternalController all the tests are successful, but when I run bundle exec rake some of the tests fail and the PR validation fails. I will have to check what's going on :-/

@chombium
Copy link
Contributor Author

I've added few more examples for the test by mistake. Now I've cleaned up the spec and everything should be fine now.

Code clean up after the deprecation of the `/v4` API for the Syslog
Binding URL.
@chombium
Copy link
Contributor Author

@ctlong, @mkocher As we have merge the api cleanup in the loggregator-agent-release, now we should take a look and merge this PR as well. I have just rebased the state with the latest ccng main branch, everything is ready for merging.

@ctlong
Copy link
Member

ctlong commented Jan 18, 2024

@chombium we'd like to hold off on merging this PR for a little while (~1-2 months). It turns out that we have an internal agent that uses the old v4 endpoint that we need to migrate to the v5 endpoint.

Thanks for your diligence in cleaning up this cruft!

@chombium
Copy link
Contributor Author

chombium commented Jan 18, 2024

@ctlong thanks for the clarification. I thought that as its counterpart in Loggregator agent release was merged, we can merge this as well. We're not in a hurry, take your time to do the needful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants