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

Databases: Set Kafka Connection Attributes (port, uri, private_uri) #1131

Closed
wants to merge 2 commits into from

Conversation

danaelhe
Copy link
Member

Addresses #1121

Database team is working on a ticket to address this in the API layer, but in the meantime this sets the port, uri, and private uri at the terraform layer so users can easily collect connection details for their kafka clusters.

Default port will be the public SASL port, 25073. This is consistent with the UI.

@danaelhe danaelhe requested a review from a team March 20, 2024 19:59
Comment on lines +617 to +618
// default for kafka will be Public SASL port, consistent with UI
d.Set("port", kafkaPublicSSLPort)
Copy link
Member

Choose a reason for hiding this comment

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

Are the ports guaranteed to be stable? Maybe we should grab it from the response? Something like:

const kafkaSASL = "sasl"
if database.Connection.ApplicationPorts != nil {
	if saslPort, ok := database.Connection.ApplicationPorts[kafkaSASL]; ok {
		d.Set("port", saslPort)
	}
}

func buildKafkaConnectionURI(conn *godo.DatabaseConnection, port int) string {
host := conn.Host

uri := fmt.Sprintf("%s:%d", host, port)
Copy link
Member

Choose a reason for hiding this comment

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

Lets make sure that this is consistent with what the DBaaS team will eventually return from the API. Should there be a scheme? The other engines all seem to use the format scheme://user:pass@host:port

Copy link
Member Author

Choose a reason for hiding this comment

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

Lets make sure that this is consistent with what the DBaaS team will eventually return from the API. Should there be a scheme? The other engines all seem to use the format scheme://user:pass@host:port

Converting this PR to a draft as dbaas said they might actually get to it this week or next!

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

@danaelhe danaelhe marked this pull request as draft March 21, 2024 17:33
@loosla
Copy link
Contributor

loosla commented Oct 10, 2024

Thanks for the implementation!
I’ve created a different PR since the initial code underwent some refactoring, along with changes from the Database team regarding Kafka (like the port). A new PR inspired by this one is here: #1246

@loosla loosla closed this Oct 10, 2024
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.

3 participants