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.

Sign up for GitHub

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

Jump to bottom

Support self-hosted instances #674

Merged
merged 8 commits into from
Dec 19, 2024
Merged

Support self-hosted instances #674

merged 8 commits into from
Dec 19, 2024

Conversation

bobbyiliev
Copy link
Contributor

Now users can configure the provider like this:

# Self-hosted configuration
provider "materialize" {
  host     = "localhost"      # Required for self-hosted mode
  port     = 6875             # Optional, defaults to 6875
  database = "materialize"    # Optional, defaults to materialize
  username = "materialize"    # Optional, defaults to materialize
  password = ""               # Optional
  sslmode  = "disable"        # Optional, defaults to require
}

# SaaS configuration (unchanged)
provider "materialize" {
  password       = "materialize_password"
  default_region = "aws/us-east-1"
}

alex-hunt-materialize
alex-hunt-materialize reviewed Nov 27, 2024
View reviewed changes
Copy link
Contributor

@alex-hunt-materialize alex-hunt-materialize left a comment

Choose a reason for hiding this comment

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

Code mostly looks good to me. Just one question and nitpick.

pkg/provider/provider.go
Type: schema.TypeInt,
Optional: true,
Description: "The Materialize port. Can also come from the `MZ_PORT` environment variable.",
DefaultFunc: schema.EnvDefaultFunc("MZ_PORT", 6875),
Copy link
Contributor

Choose a reason for hiding this comment

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

For self-hosted, do we want to point at the external SQL address, or the internal one? The external one doesn't allow login as mz_system, and other users have limited permissions.

Also, nitpick, it might be good to name this in a way that is clearly a SQL port, as we also have HTTP(S) ports.

@bobbyiliev bobbyiliev force-pushed the support-for-self-hosted-instances branch 3 times, most recently from 1ad036f to eb7d6b7 Compare November 28, 2024 16:12
bobbyiliev
bobbyiliev commented Nov 28, 2024
View reviewed changes
integration/self_hosted/cluster.tf
@@ -0,0 +1,66 @@
resource "materialize_cluster" "cluster" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The large number of line changes in this PR is due to all of the files in the integration/self_hosted directory. Those are just standard tests. If reviewing this PR, focus on the changes in the pkg directory.

@bobbyiliev bobbyiliev force-pushed the support-for-self-hosted-instances branch from 727585e to 08b1efa Compare November 28, 2024 16:41
@bobbyiliev bobbyiliev marked this pull request as ready for review November 28, 2024 16:41
@bobbyiliev bobbyiliev requested a review from a team as a code owner November 28, 2024 16:41
@bobbyiliev bobbyiliev requested review from SangJunBak and removed request for a team November 28, 2024 16:41
bobbyiliev
bobbyiliev commented Nov 28, 2024
View reviewed changes
pkg/provider/provider.go
Comment on lines 176 to +184
func providerConfigure(ctx context.Context, d *schema.ResourceData, version string) (interface{}, diag.Diagnostics) {
// Check for self-hosted configuration
if host := d.Get("host").(string); host != "" {
log.Printf("[DEBUG] Configuring self-hosted provider")
return configureSelfHosted(ctx, d, version)
}
log.Printf("[DEBUG] Configuring SaaS provider")
return configureSaaS(ctx, d, version)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where we determine if we should use the self-hosted authentication flow or the SaaS authentication flow.

@bobbyiliev bobbyiliev force-pushed the support-for-self-hosted-instances branch from 08b1efa to b5e676d Compare November 29, 2024 10:55
@bobbyiliev bobbyiliev changed the title [WIP] Support self-hosted instances Support self-hosted instances Dec 7, 2024
@bobbyiliev bobbyiliev force-pushed the support-for-self-hosted-instances branch from b5e676d to f56bf5e Compare December 18, 2024 09:49
alex-hunt-materialize
alex-hunt-materialize reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@alex-hunt-materialize alex-hunt-materialize left a comment

Choose a reason for hiding this comment

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

Sorry for the back and forth on the port and user. 6875 and materialize was wrong initially, but I think correct now.

CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/workflows/integration-self-hosted.yml Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
examples/provider/provider.tf Outdated Show resolved Hide resolved
integration/self_hosted/main.tf Show resolved Hide resolved
integration/self_hosted/rbac.tf Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
@bobbyiliev bobbyiliev force-pushed the support-for-self-hosted-instances branch 2 times, most recently from 9e167b1 to 4add410 Compare December 18, 2024 13:25
alex-hunt-materialize
alex-hunt-materialize reviewed Dec 18, 2024
View reviewed changes
integration/self_hosted/main.tf
provider "materialize" {
host = "materialized"
port = 6877
database = "materialize"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only for the internal integration tests which are using the Docker emulator. It allows me to test some resources that the materialize user does not have the rights to, like setting system parameters for example:

resource "materialize_system_parameter" "default_cluster" {
  name  = "max_tables"
  value = "2000"
}

This will not be exposed to the end users and the orchestratord.

@bobbyiliev bobbyiliev force-pushed the support-for-self-hosted-instances branch from 87a8b5a to 4add410 Compare December 19, 2024 14:27
@bobbyiliev bobbyiliev force-pushed the support-for-self-hosted-instances branch from 4add410 to 1ed1729 Compare December 19, 2024 15:56
@bobbyiliev bobbyiliev merged commit 267b2b1 into main Dec 19, 2024
6 checks passed
@bobbyiliev bobbyiliev deleted the support-for-self-hosted-instances branch December 19, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-hunt-materialize alex-hunt-materialize alex-hunt-materialize approved these changes

@SangJunBak SangJunBak Awaiting requested review from SangJunBak SangJunBak is a code owner automatically assigned from MaterializeInc/console

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bobbyiliev @alex-hunt-materialize