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

Provide tls block for tls options in amqp(), http(), riemann() dest. drivers #1715

Merged
merged 4 commits into from
Oct 30, 2017

Conversation

mitzkia
Copy link
Collaborator

@mitzkia mitzkia commented Oct 13, 2017

Three destination drivers: amqp(), http(), riemann() allows to use tls options inside the driver configuration.
This patch-set provides to use these tls options also in a general tls() block as it is used in network type drivers.
The patch-set did not brake the original usage for actual drivers.
Example new type of configuration for tls options in riemann destination:

destination d_riemann {
	riemann(
		server("127.0.0.1") 
		port(5672) 
		tls(
			ca_file("ca") 
			key_file("key")
		)
	);
};

* Config example:

destination  d_amqp {
	amqp(
		host("127.0.0.1")
		port(5672)
		username("test")
		password("test")
		tls(
			ca_file("ca-file")
			key_file("key-file")
		)
	);
};

Signed-off-by: Andras Mitzki <mitzkia@gmail.com>
Signed-off-by: Máté Farkas <mate.farkas@balabit.com>
* Example config
destination  d_http {
	http(
		url("http://127.0.0.1:8080")
		tls(
			ca_file("ca-file")
			key_file("key-file")
		)
	);
};



Signed-off-by: Andras Mitzki <mitzkia@gmail.com>
* Example config
destination d_riemann {
	riemann(
		server("127.0.0.1")
		port(5672)
		tls(
			ca_file("ca")
			key_file("key")
		)
	);
};


Signed-off-by: Andras Mitzki <mitzkia@gmail.com>
@mitzkia mitzkia force-pushed the provide_tls_block_for_tls_options branch from b8dbbe9 to 2303e15 Compare October 13, 2017 07:46
@mitzkia mitzkia changed the title Provide tls block for tls options Provide tls block for tls options in amqp(), http(), riemann() dest. drivers Oct 13, 2017
@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

1 similar comment
@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

Copy link
Collaborator

@Kokan Kokan left a comment

Choose a reason for hiding this comment

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

Would it be possible to mark the original configuration options as deprecated in 3.13 ?
Possible a warning during startup.

@mitzkia
Copy link
Collaborator Author

mitzkia commented Oct 13, 2017

Thanks, I am ok with the deprecation of original usages. I will create a new patch for that.

Copy link
Collaborator

@bazsi bazsi left a comment

Choose a reason for hiding this comment

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

I really like the change, thank you. I've made one note which could make the riemann grammar simpler, but that's not a show-stopper.

{
CHECK_ERROR(riemann_dd_set_connection_type(last_driver, $3), @3,
"Unknown Riemann connection type: %s", $3);
free($3);
}
| KW_TYPE '(' LL_STRING riemann_type_options ')'
| KW_TYPE '(' LL_STRING riemann_tls_options ')'
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

btw, there's a rule called "string" that maps either LL_IDENTIFIER or LL_STRING, so the two rules above can be collapsed into a single one.

…STRING

Signed-off-by: Andras Mitzki <mitzkia@gmail.com>
@kira-syslogng
Copy link
Contributor

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

@mitzkia
Copy link
Collaborator Author

mitzkia commented Oct 30, 2017

@Kokan as I saw we can only add KWS_OBSOLETE flag to directly options (not to "option groups").
Let see in modules/riemann/riemann-parser.c:

  { "cacert",                   KW_CA_FILE, KWS_OBSOLETE, "The cacert() option is deprecated in favour of ca-file()" },
  { "cert",                     KW_CERT_FILE, KWS_OBSOLETE, "The cert() option is deprecated in favour of cert-file()" },

With this method I think we can not "mark" the original usage as obsolete.
If I mark one of the options as obsolete it will be obsolete also in a new way of configuration.

Copy link
Collaborator

@Kokan Kokan left a comment

Choose a reason for hiding this comment

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

It would have been nice to mark groups, but the PR is good regardless of this. Possible in the documentation could be marked as obsolete.

@Kokan Kokan merged commit 7839eca into syslog-ng:master Oct 30, 2017
@Kokan Kokan removed the in progress label Oct 30, 2017
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.

None yet

4 participants